WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 years ago

#33932 reviewing enhancement

Filters for Plugin/Theme Update Email Notifications

Reported by: ronalfy Owned by: ocean90
Milestone: Future Release Priority: normal
Severity: normal Version: 3.7
Component: Upgrade/Install Keywords: has-patch needs-refresh needs-unit-tests
Focuses: Cc:

Description

I've had several requests to enable core update emails, but disable emails for plugins/themes.

If this idea is approved, I would love to be the one to tackle this as I have yet to contribute to core.

Goal is to keep send_core_update_notification_email intact, but allow filters for both plugin/theme background update notifications.

Apparent use-case is a user wants core emails, but not plugin/theme emails. One filter rules them all currently from what I can see in the code-base.

Example use-case in the wild: https://wordpress.org/support/topic/for-on-the-wishlist?replies=8

Attachments (11)

patch.diff (2.9 KB) - added by ronalfy 3 years ago.
Patch to add theme, plugin, translation files.
patch.2.diff (1.5 KB) - added by ronalfy 3 years ago.
Simplified filter.
patch.3.diff (1.2 KB) - added by ronalfy 3 years ago.
One Filter to Rule Them All
patch.4.diff (1.6 KB) - added by ronalfy 3 years ago.
Continue instead of return. Empty body check.
patch.5.diff (3.7 KB) - added by ronalfy 2 years ago.
Filter to disable e-mails for plugins, themes, and translations
patch.6.diff (3.8 KB) - added by ronalfy 2 years ago.
Filter to disable e-mails for plugins, themes, and translations
patch.7.diff (2.1 KB) - added by ronalfy 2 years ago.
Filter to disable e-mails for plugins, themes, and translations
33932.diff (9.9 KB) - added by welcher 2 years ago.
Adding unit tests
33932.patch (11.7 KB) - added by ronalfy 2 years ago.
Add filter and unit tests for automatic update emails
33932.2.patch (3.7 KB) - added by ronalfy 2 years ago.
Filter to disable e-mails for plugins, themes, and translations
33932.3.patch (3.9 KB) - added by ronalfy 2 years ago.
Filter to disable e-mails for plugins, themes, and translations

Download all attachments as: .zip

Change History (51)

@ronalfy
3 years ago

Patch to add theme, plugin, translation files.

@ronalfy
3 years ago

Simplified filter.

#1 @ronalfy
3 years ago

Patch 2 simplifies the filter calls and moves everything to one filter since we're already passing the type.

#2 @ronalfy
3 years ago

  • Keywords has-patch added

Bumping for patch. Sorry.

This ticket was mentioned in Slack in #core by ronaldhuereca. View the logs.


3 years ago

@ronalfy
3 years ago

One Filter to Rule Them All

#4 @ronalfy
3 years ago

Patch 3 solves the issue since we're already passing type for context. No reason to have 3 conditionals.

Last edited 3 years ago by ronalfy (previous) (diff)

#5 @johnbillion
3 years ago

  • Component changed from General to Upgrade/Install
  • Focuses docs removed
  • Keywords reporter-feedback added
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from 4.3 to 3.7

Thanks for the patch, Ronald!

This filter makes sense. Here's some feedback on your patch:

  • If the filter returns false for one or more of the update types, it prevents the email from being sent for all subsequent update types too. The return statement after the filter should probably be a continue. Then below the foreach loop you could return if the $body variable is empty.
  • The diff should be generated from the root of your repo so other people can apply the patch without having to specify which file is to be patched.
  • There's a typo in the docblock: translators instead of translations.
  • Passing $this as the third parameter to the filter would be a good idea so the filter receives as much context as possible.
Last edited 3 years ago by johnbillion (previous) (diff)

#6 @ronalfy
3 years ago

Thank you! I'll revisit this tomorow and revise my patch. Thank you SO much for the feedback.

Kinda curious why 4.3 was moved back to 3.7. Please pardon my noob-ness.

@ronalfy
3 years ago

Continue instead of return. Empty body check.

#7 @ronalfy
3 years ago

@johnbillion,

Added the requested changes and worked with my internal testing.

Anything I missed? Ping me on slack (ronaldhuereca). May need some mentoring here if patch is submitted incorrectly.

#8 @ronalfy
3 years ago

  • Keywords dev-feedback 2nd-opinion needs-testing added; reporter-feedback removed

#9 @johnbillion
3 years ago

  • Owner set to johnbillion
  • Resolution set to fixed
  • Status changed from new to closed

In 34543:

Introduce a send_update_notification_email which controls whether an update notification email is sent for background updates. This filter allows control over each of the update types (plugin, theme, translation) and compliments the automatic_updates_send_debug_email and send_core_update_notification_email filters.

Fixes #33932
Props ronalfy

#10 @netweb
3 years ago

  • Milestone changed from Future Release to 4.4

#11 @ronalfy
3 years ago

Just an FYI. I was instructed to create a new ticket.

There is a bug for the committed patch when I tested against WordPress 4.4 Beta 1. Here is the new ticket.

#34434

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

#12 follow-up: @ocean90
3 years ago

  • Keywords needs-patch revert added; has-patch dev-feedback 2nd-opinion needs-testing removed

[34543] is incomplete because there is a second loop for the "UPDATE LOG" part. This and the bug mentioned in #34434 sounds like we should revert [34543] for 4.4 and give it another try in 4.5.

#13 in reply to: ↑ 12 @ronalfy
3 years ago

Replying to ocean90:

[34543] is incomplete because there is a second loop for the "UPDATE LOG" part. This and the bug mentioned in #34434 sounds like we should revert [34543] for 4.4 and give it another try in 4.5.

Agreed on the revert unless patch for #34434 can be committed. Will post a new patch when 4.5 is in the pipeline.

#14 @ocean90
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This ticket was mentioned in Slack in #core by ocean90. View the logs.


3 years ago

#16 @ocean90
3 years ago

In 35619:

Upgrade: Revert [34543] because of incomplete and incorrect functionality.

See #33932.
Fixes #34434.

#17 @ocean90
3 years ago

  • Keywords revert removed
  • Milestone changed from 4.4 to Future Release

#18 follow-up: @johnbillion
2 years ago

  • Milestone changed from Future Release to 4.5

@ronalfy Do you want to tackle this again?

#19 in reply to: ↑ 18 @ronalfy
2 years ago

Replying to johnbillion:

@ronalfy Do you want to tackle this again?

Sure! I can do that. I'll try again this weekend.

@ronalfy
2 years ago

Filter to disable e-mails for plugins, themes, and translations

#20 @ronalfy
2 years ago

@johnbillion Added Patch 5. I've tested this with background updates for themes, plugins, core, and various combinations of each to ensure the patch is correct.

Can you please look over and offer any suggestions?

Thanks!

PS: I may have to redo this since there seems to be a bug (see #34878) with automatic updates at the moment.

Last edited 2 years ago by ronalfy (previous) (diff)

This ticket was mentioned in Slack in #core by jorbin. View the logs.


2 years ago

#22 @jorbin
2 years ago

  • Milestone changed from 4.5 to Future Release

There hasn't been any recent activity on this ticket and the beta 1 deadline for 4.5 is tomorrow, so punting.

@ronalfy
2 years ago

Filter to disable e-mails for plugins, themes, and translations

#23 @kidsguide
2 years ago

I have tested patch.6.diff and all works great. Each filter did what it was suppose to do.

Last edited 2 years ago by kidsguide (previous) (diff)

#24 @ronalfy
2 years ago

  • Keywords has-patch added; needs-patch removed

@ocean90 @adamsilverstein would you be willing to consider this patch for WordPress 4.6?

A common use case is if a user has automatic updates for plugins/themes/translations. We have a fairly popular plugin (60k) installs where users are constantly complaining of the massive influx of notification e-mails with the only option to turn them off using a all-is-on, all-is-off filter.

I've thoroughly tested this with plugin updates, translation updates, theme updates, and core minor/update updates.

@ronalfy
2 years ago

Filter to disable e-mails for plugins, themes, and translations

#25 @ronalfy
2 years ago

Added patch based on recs from @adamsilverstein to modify class var directly instead of creating new var.

Tested the same as above.

#26 @welcher
2 years ago

  • Keywords needs-unit-tests added

This ticket was mentioned in Slack in #core by ronaldhuereca. View the logs.


2 years ago

@welcher
2 years ago

Adding unit tests

#28 @welcher
2 years ago

I added a rather large patch that contains unit test. The reason for the size is that there seemed to be no tests related to the send_debug_email method or the upgrader component ( at least that I could find ) so I needed to add them.

The tests added here have a baseline test for the method and some tests for the filter. There is also a fix in place here that addresses #36609 which was discovered while writing the tests for this patch.

Any feedback or direction is welcome.

#29 @ronalfy
2 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#30 @ocean90
2 years ago

  • Milestone changed from Future Release to 4.6

#31 @ronalfy
2 years ago

@welcher found a bug in the patch. I think we need a similar path to where I essentially clone the class var instead of modifying it directly. https://core.trac.wordpress.org/attachment/ticket/33932/patch.6.diff

Modifying it directly can cause the action automatic_updates_complete to not pass the proper data.

Last edited 2 years ago by ronalfy (previous) (diff)

@ronalfy
2 years ago

Add filter and unit tests for automatic update emails

#32 @ronalfy
2 years ago

Added patch to fix bug in @welcher's patch when patch7 was added.

We need to basically clone the class var because the action automatic_updates_complete needs the full dataset.

Modifying the class variable will make this hook unreliable, so I chose to create a method variable instead to eliminate this conflict.

Last edited 2 years ago by ronalfy (previous) (diff)

#33 @ocean90
2 years ago

  • Keywords needs-refresh added

Some feedback about 33932.patch:

  • The doc block for send_update_notification_email needs to be aligned with a whitespace before the *.
  • The documentation should start with "Filter whether to".
  • It seems like the current documentation was copied from the send_core_update_notification_email filter. The new filter should have its own documentation because the current one is wrong.
  • It's also worth to mention, that the suggested filter lives in send_debug_email, naming it send_update_notification_email feels wrong.
  • Changes to tests/phpunit/tests/mail.php should be part of a separate ticket. It seems like [37307] fixed parts of this already.
  • In test_send_update_notification_email_filter_no_emails() the comment for send_update_notification_email should be just // Disable all notifications.
  • In test_send_update_notification_email_filter_remove_single_type() the comment for send_update_notification_email is wrong (copy/paste error).
  • In test_send_update_notification_email_filter_remove_single_type() the variable variable $$type looks odd. The alignment should be improved too.

@ronalfy
2 years ago

Filter to disable e-mails for plugins, themes, and translations

#34 @ronalfy
2 years ago

  • Keywords needs-unit-tests added; has-unit-tests removed

Thanks for the feedback @ocean90. I changed the filter name to have better context. It's now send_debug_notification_email.

Based on the feedback on the unit tests, I removed them and just included the filter in this patch.

@welcher would you be up to opening a separate ticket and applying your unit tests based on the above feedback?

THANKS!

This ticket was mentioned in Slack in #core by ronaldhuereca. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by ronaldhuereca. View the logs.


2 years ago

#37 @ocean90
2 years ago

  • Owner changed from johnbillion to ocean90
  • Status changed from reopened to reviewing

@ronalfy
2 years ago

Filter to disable e-mails for plugins, themes, and translations

#38 @ronalfy
2 years ago

@ocean90 applied new patch. If committed, recommend @kidsguide for props as he helped with testing.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


2 years ago

#40 @chriscct7
2 years ago

  • Milestone changed from 4.6 to Future Release
Note: See TracTickets for help on using tickets.