Skip to content

Conversation

@htdat
Copy link
Member

@htdat htdat commented Dec 15, 2019

Fixes #709

When /wp-admin/options-discussion.php is fully loaded, the option looks like this:

Screen Shot on 2019-12-15 at 21:22:50

Changes proposed in this Pull Request:

Basically, I followed the same approach for another similar option Someone likes one of my posts in "Likes" module https://github.com/Automattic/jetpack/search?q=social_notifications_like&type=Code

  • Handle social_notifications_subscribe option when "Subscriptions" module is activated/deactivated.
  • Add Someone follows my blog in WP Admin -> Settings -> Discussions.
  • Use jQuery to align up this option in the Email me whenever section.
  • Validate its value to only on/off.
  • Add this option to \Jetpack_Core_Json_Api_Endpoints class.

I do not add tests as social_notifications_subscribe is covered already. Can a dev help to double check this?

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

Testing instructions:

  • Disable Subscriptions module => the option should not display in "Discussions" settings
  • Enable Subscriptions => the checkbox of this option should not be selected by default.
  • Enable this option, try to subscribe with a new email => the Jetpack connection owner gets an email notification about the new email.

Proposed changelog entry for your changes:

  • Add Email me whenever: Someone follows my blog option to wp-admin settings.

@htdat htdat requested a review from a team December 15, 2019 14:15
@matticbot matticbot added the [Status] Needs Tracks Review Added/removed/modified a tracks event. label Dec 15, 2019
@htdat htdat added [Status] Needs Review This PR is ready for review. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Status] Needs Tracks Review Added/removed/modified a tracks event. labels Dec 15, 2019
@jetpackbot
Copy link
Collaborator

jetpackbot commented Dec 15, 2019

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 2386e8d

@jeherve jeherve added this to the 8.1 milestone Dec 16, 2019
@jeherve jeherve added the [Feature] Subscriptions All subscription-related things such as paid and unpaid, user management, and newsletter settings. label Dec 16, 2019
Copy link
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

This change works well for me, but I have several comments. I don't really like how we're copying the same code that's used in the Likes module, but I guess that's the quickest way to fix the issue that we're aiming at.
Thanks for working on this!

@zinigor zinigor added [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Dec 25, 2019
@htdat htdat added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Dec 26, 2019
@htdat
Copy link
Member Author

htdat commented Dec 26, 2019

@zinigor Thanks for your thorough review. I've added a new commit to address your suggestions.

I don't really like how we're copying the same code that's used in the Likes module, but I guess that's the quickest way to fix the issue that we're aiming at.

Well, me too, I do not like this approach much. But since we have moved new options to Calypso or Jetpack setting pages, it's not efficient to refactor related code.

Copy link
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Thanks for fixing everything! I have added the changes that linter required, please don't skip checks when you commit.

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Dec 27, 2019
@zinigor zinigor merged commit fd0d69e into master Dec 30, 2019
@zinigor zinigor deleted the fix/709 branch December 30, 2019 17:16
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Dec 30, 2019
zinigor added a commit that referenced this pull request Dec 30, 2019
Also added some new entries to the testing file.
zinigor added a commit that referenced this pull request Dec 30, 2019
* Changelog: 8.1 additions

* Changelog: add #13858

* Changelog: add #13963

* Changelog: add #14174

* Changelog: add #14178

* Changelog: add #14175

* Changelog: add #14192

* Changelog: add #14196

* Changelog: add #14182

* Changelog: add #14218

* Changelog: add #14214

* Changelog: add #13757

* Changelog: add #14190

* Changelog: add #14131

* Changelog: add #14101

* Changelog: add #14203

* Changelog: add #14211

* Changelog: add #14224

* Changelog: add #14230

* Changelog: add #14241

* Changelog: add #14249

* Changelog: add #14264

* Changelog: add #14263

* Changelog: add #14256

* Changelog: add #10189

* Changelog: add #14240

* Changelog: add #14239

Also added some new entries to the testing file.

Co-authored-by: Igor Zinovyev <[email protected]>
zinigor added a commit that referenced this pull request Dec 30, 2019
* Changelog: 8.1 additions

* Changelog: add #13858

* Changelog: add #13963

* Changelog: add #14174

* Changelog: add #14178

* Changelog: add #14175

* Changelog: add #14192

* Changelog: add #14196

* Changelog: add #14182

* Changelog: add #14218

* Changelog: add #14214

* Changelog: add #13757

* Changelog: add #14190

* Changelog: add #14131

* Changelog: add #14101

* Changelog: add #14203

* Changelog: add #14211

* Changelog: add #14224

* Changelog: add #14230

* Changelog: add #14241

* Changelog: add #14249

* Changelog: add #14264

* Changelog: add #14263

* Changelog: add #14256

* Changelog: add #10189

* Changelog: add #14240

* Changelog: add #14239

Also added some new entries to the testing file.

Co-authored-by: Igor Zinovyev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Subscriptions All subscription-related things such as paid and unpaid, user management, and newsletter settings. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Send email to site owner when the site gets a new email subscriber

6 participants