Skip to content

Subscriptions: Fix "deactivating module unsets Follow email option" #14325

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
dereksmart merged 1 commit into from
Jan 20, 2020
Merged

Subscriptions: Fix "deactivating module unsets Follow email option" #14325

dereksmart merged 1 commit into from
Jan 20, 2020

Conversation

@htdat
Copy link
Member

@htdat htdat commented Jan 11, 2020

Fixes #14302

Changes proposed in this Pull Request:

As explained here #14302 (comment), this PR does these things:

  1. When activating Subscriptions module, if social_notifications_subscribe option does not exist. Set it to off. Otherwise, does nothing.

  2. Remove the current add_action filter when deactivating Subscriptions module.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Fix an issue in a current feature.

Testing instructions:

  1. Start from a brand new Jetpack site, with the Subscriptions module on.
  2. Go to WP Admin > Settings > Discussion
  3. Turn on "Email me whenever Someone follows my blog"
  4. Go to WP Admin > Jetpack > Settings > Discussion and deactivate the Subscriptions module.
  5. Once you've received confirmation, reactivate it.
  6. Go to WP Admin Settings > Discussion
  7. Make sure that "Email me whenever Someone follows my blog" is STILL on.

Proposed changelog entry for your changes:

  • Subscriptions: Maintain the setting Email me whenever Someone follows my blog when activating and deactivating this module.

@htdat htdat added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Subscriptions All subscription-related things such as paid and unpaid, user management, and newsletter settings. [Status] Needs Review This PR is ready for review. labels Jan 11, 2020
@htdat htdat requested a review from a team January 11, 2020 12:43
@jetpackbot
Copy link
Collaborator

jetpackbot commented Jan 11, 2020

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: January 14, 2020.
Scheduled code freeze: January 7, 2020

Generated by 🚫 dangerJS against 9b07a28

@htdat htdat self-assigned this Jan 13, 2020
@jeherve jeherve added this to the 8.2 milestone Jan 13, 2020
jeherve
jeherve approved these changes Jan 13, 2020
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well for me.

Was there a specific reason why you chose to delete the option before?

@htdat
Copy link
Member Author

htdat commented Jan 15, 2020

Was there a specific reason why you chose to delete the option before?

I tried to follow the same way social_notifications_like option is handled
https://github.com/Automattic/jetpack/blob/master/modules/likes.php#L98-L119

I thought it's kinda a standard to delete an option belonging to a specific module when the module is deactivated. Well, this "standard" made sense to me when I prepared for #14239

@dereksmart
Copy link
Contributor

dereksmart commented Jan 20, 2020

I thought it's kinda a standard to delete an option belonging to a specific module when the module is deactivated.

Not so much on a module basis, but it's a good practice to clear more stuff on complete deletion of the Jetpack plugin. Not sure how much of that we're currently doing, but that would be the preferred time to clear IMO

@dereksmart dereksmart merged commit 20c767c into master Jan 20, 2020
@dereksmart dereksmart deleted the fix/14302 branch January 20, 2020 14:17
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review This PR is ready for review. labels Jan 20, 2020
jeherve added a commit that referenced this pull request Jan 27, 2020
@jeherve
Changelog: add #14325
b47cc89
jeherve added a commit that referenced this pull request Jan 28, 2020
@jeherve
Changelog: 8.2 release (#14332)
0589a49 
* [not verified] Remove empty readme section

* Initial changelog for 8.2

* Changelog: add #14220

* Changelog: add #14252

* Changelog: add #14291

* Changelog: add #14309

* Changelog: add #14304

* Changelog: add general connection log.

* Changelog: add #14275

* Changelog: add #14313

* Changelog: add #14213

* Changelog: add #14357

* Add sync testing instructions

* Add 8.1.1 changelog back

See eeaafab and 61757eb

* Changelog: add #14371

* Changelog: add #14386

* Changelog: add #14471

* Changelog: add #14325

* Changelog: add #14194

* Changelog: add #14340

* Changelog: add #14418

* Changelog: add #14417

* Changelog: add #14075

* Changelog: add #14467

* Changelog: add #14307

* Changelog: add #14326
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

Assignees

@htdat htdat

Labels

[Feature] Subscriptions All subscription-related things such as paid and unpaid, user management, and newsletter settings. [Type] Bug When a feature is broken and / or not performing as intended

Projects

None yet

Milestone

8.2

Development

Successfully merging this pull request may close these issues.

Subscriptions: deactivating module unsets Follow email option

6 participants

@htdat @jetpackbot @dereksmart @jeherve @matticbot