Skip to content
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

Post by Email: Refactoring and Unit Tests #15139

Merged
merged 7 commits into from
Apr 2, 2020
Merged

Post by Email: Refactoring and Unit Tests #15139

merged 7 commits into from
Apr 2, 2020

Conversation

sergeymitr
Copy link
Contributor

@sergeymitr sergeymitr commented Mar 26, 2020 •
edited
Loading

Description

Post by Email module has two UI widgets:

  • "Post by email" section on the "Jetpack Settings -> Writing" page
  • "Post by Email" section in the user profile
    These two interfaces utilize different API endpoints, and that leads to duplicated code.

The goal of this PR is to minimize the code duplication, deprecate the older WP-AJAX endpoints in favor of the REST API, improve the test coverage of the module, and refactor the module to improve the code quality.

Nonetheless, the PR does not modify the module behavior in any way, and all deprecated functionality is still available (although it does throw "doing it wrong" warnings).

No issue was created.

Changes proposed in this Pull Request:

  • Deprecating the WP-AJAX endpoints (jetpack_post_by_email_enable, jetpack_post_by_email_regenerate, jetpack_post_by_email_disable).
  • Switching the "User Profile -> Post by Email" UI from WP-AJAX to REST API.
  • Rewriting the jQuery-based JavaScript to VanillaJS.
  • Moving REST requests proxying from Jetpack_Core_API_Data to the post-by-email module (as suggested by the @todo tag).
  • Adding integration tests for the module's REST API.

Testing instructions:

  1. Run unit tests (yarn docker:phpunit).
  2. Load the "Jetpack Settings -> Writing" and scroll down to the "Post by email" section: /wp-admin/admin.php?page=jetpack#/writing
  3. Enable module. If the module wasn't previously activated and the email field is empty, click on the "Create address" button. The email address should appear.
  4. Disable module. The email field should be grayed out and disabled.
  5. Enable module again. The email address should show up in the input field, and "Regenerate address" button will appear. Click on that button and make sure that email address is replaced with a new one.

post-by-email-1

  1. Open the "User Profile" page and scroll down to the "Post by Email" section: /wp-admin/profile.php
  2. Make sure the email in the form matches the one on step 4.
  3. Click "Regenerate Address". The email address should be replaced with a new one.
  4. Click "Disable Post By Email". The form should be replaced with the "Enable Post By Email" button.
  5. Click _"Enable Post By Email". The form should reappear.

post-by-email2

Proposed changelog entry for your changes:

  • Post by Email module: WP-AJAX endpoints are deprecated in favor of the REST API.

@sergeymitr
Refactor the Post by Email module
1537bb4 
* Deprecate the WP-AJAX endpoints (`jetpack_post_by_email_enable`, `jetpack_post_by_email_regenerate`, `jetpack_post_by_email_disable`)
* Switch the "User Profile -> Post by Email" UI from WP-AJAX to REST API
* Moving REST requests proxying from `Jetpack_Core_API_Data` to the `post-by-email` module (as suggested by the @todo tag)
@sergeymitr
Integration tests for Post by Email REST API.
949cb01 
Currently class `WP_Test_Post_By_Email_API` tests one API method: `post_by_email_address => create`.
In the future tests for `regenerate` and `delete` will be added.

REST API authentication mocking code is mostly copied form `WP_Test_Jetpack_REST_API_Authentication`.
Test API requests are not being sent to the Jetpack API, instead the Jetpack API request is verified, and the response is mocked. This allows us to test the functionality internally.
@sergeymitr sergeymitr self-assigned this Mar 26, 2020
@jetpackbot
Copy link

jetpackbot commented Mar 26, 2020 •
edited
Loading

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: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against 867af1b

@sergeymitr
Post by Email: Unit tests for “regenerate” and “delete” API endpoints
ddc47ea 
The commit adds two unit tests for the “post-by-email” module:
- `jetpack.regeneratePostByEmailAddress`
- `jetpack.deletePostByEmailAddress`
I also made some minor refactoring in the test class `WP_Test_Post_By_Email_API`.
@sergeymitr sergeymitr force-pushed the fix/post-by-email-refactoring branch from d894bab to ddc47ea Compare March 27, 2020 00:22
@sergeymitr
Post by Email: Switching to XMLHttpRequest.
84a2f3b 
Rewriting the jQuery AJAX request on VanillaJS.
@sergeymitr sergeymitr force-pushed the fix/post-by-email-refactoring branch from c084200 to 84a2f3b Compare March 27, 2020 00:36
@sergeymitr sergeymitr marked this pull request as ready for review March 27, 2020 02:25
@sergeymitr sergeymitr requested a review from a team as a code owner March 27, 2020 02:25
@sergeymitr sergeymitr added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Mar 27, 2020
@bisko bisko mentioned this pull request Mar 31, 2020
Merged
@kraftbj kraftbj added this to the 8.5 milestone Apr 1, 2020
@kraftbj kraftbj added [Pri] Normal [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Apr 1, 2020
@kraftbj kraftbj self-requested a review April 1, 2020 20:28
@kraftbj
Copy link
Contributor

kraftbj commented Apr 1, 2020

While we were in the file, I brought it up to PHPCS compliance.

kraftbj
kraftbj previously approved these changes Apr 1, 2020
View reviewed changes
@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Apr 1, 2020
@kraftbj
Copy link
Contributor

kraftbj commented Apr 1, 2020

@sergeymitr Could you give a glance/test for acb25a9? It tests fine for me, but want to be sure. If you're happy, merge when you're ready.

@sergeymitr
Post by Email: corrected paths to JS and CSS files, adjusted style at…
867af1b 
…tribute.

- Files `post-by-email.js` and `post-by-email.css` were enqueued incorrectly. Since the class was moved into the module directory, they got additional `post-by-email/` in the path, fixed now.
- Escaping `style` attributes in the "User Profile" form leads to quotes being converted to HTML entities. I refactored this tiny piece of code to make the attribute escaping work.
@sergeymitr
Copy link
Contributor Author

@kraftbj
Thanks for the refactoring, that totally makes sense.
I fixed a couple of minor issues, please review.

@jeherve jeherve removed the [Status] Ready to Merge Go ahead, you can push that green button! label Apr 2, 2020
@jeherve jeherve added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Apr 2, 2020
@kraftbj kraftbj merged commit d57e5c9 into master Apr 2, 2020
@kraftbj kraftbj deleted the fix/post-by-email-refactoring branch April 2, 2020 19:25
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Apr 2, 2020
@sergeymitr sergeymitr added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Apr 2, 2020
@sergeymitr sergeymitr mentioned this pull request Apr 6, 2020
Closed
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kraftbj kraftbj kraftbj approved these changes

Assignees

@sergeymitr sergeymitr

Labels
[Feature] Post By Email [Pri] Normal [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
8.5
Development

Successfully merging this pull request may close these issues.
5 participants
@sergeymitr @jetpackbot @kraftbj @jeherve @matticbot