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.

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

Removes all After the Deadline references from Jetpack #11772

Merged
merged 8 commits into from
Apr 25, 2019

Conversation

rodrigoi
Copy link
Contributor

@rodrigoi rodrigoi commented Mar 29, 2019

Fixes Automattic/wp-calypso#31865
This complements Automattic/wp-calypso#31880

Closes #9128
Closes #5640
Closes #3999
Closes #3506

Changes proposed in this Pull Request:

This PR removes all references for After the Deadline from Jetpack. This includes module, settings and the classic editor block.

Before After
Writing Settings Screen Shot 2019-03-29 at 19 14 34 Screen Shot 2019-03-29 at 19 19 52
Classic Editor Block Screen Shot 2019-03-29 at 19 13 47 Screen Shot 2019-03-29 at 19 21 22

Testing instructions:

On an site with an active Jetpack connection:

  • Go to Jetpack > Settings > Writing and look for the Composing card.

  • Verify that the remaining options work as expected.

  • Edit or Create a Post or Page with the Block Editor.

  • Switch to the Code Editor view.

  • Add text to the code editor without adding any tags to activate the Classic Editor Block.

  • Switch back to Visual Editor.

  • Click on the Classic Editor Block and verify that all options work as expected.

Proposed changelog entry for your changes:

  • After the Deadline proofreading service module removed

@rodrigoi rodrigoi self-assigned this Mar 29, 2019
@rodrigoi rodrigoi requested a review from a team as a code owner March 29, 2019 22:07
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello rodrigoi! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D26213-code before merging this PR. Thank you!

@Automattic Automattic deleted a comment from jetpackbot Mar 29, 2019
@rodrigoi rodrigoi added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Mar 30, 2019
@rodrigoi rodrigoi requested a review from a team March 30, 2019 00:09
@mmtr
Copy link
Member

mmtr commented Mar 30, 2019

Are the changes on the yarn.lock file intended?

mmtr
mmtr previously approved these changes Mar 30, 2019
Copy link
Member

@mmtr mmtr left a comment

Choose a reason for hiding this comment

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

This is working for me as expected.

Note that this also removes the "Proofreading Writing" on posts created with the Classic editor (after activate the Classic editor plugin).

Before After
Screen Shot 2019-03-30 at 11 20 20 Screen Shot 2019-03-30 at 11 20 51
  • Switch to the Code Editor view.
  • Add text to the code editor without adding any tags to activate the Classic Editor Block.
  • Switch back to Visual Editor.

You can also insert a classic block directly without switching modes:

Screen Shot 2019-03-30 at 11 28 13

@simison
Copy link
Member

simison commented Mar 31, 2019

Note this convo about removing/deprecating PHP files: #11713 (review)

We might want to keep them around for a couple Jetpack versions.

@kraftbj
Copy link
Contributor

kraftbj commented Apr 1, 2019

Yup, at least want to keep modules/after-the-deadline.php. The issue as best I'm able to deduce is how we include module files (e.g. autoload any *.php file from the modules dir) is what is problematic since opcache will store what those files are in memory when the actual files are then deleted. https://github.com/Automattic/jetpack/blob/master/modules/wpcc.php for an example.

To help me understand the decision to drop the feature, can you drop the shortlink anchor/link to the p2 discussion. I want to make sure our Happiness team is looped in and we have plenty of time to update documentation and prepare predefs for any questions about this.

kraftbj
kraftbj previously requested changes Apr 1, 2019
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

modules/after-the-deadline.php needs to stay as a deprecated module file.

@kraftbj kraftbj added this to the 7.3 milestone Apr 1, 2019
@kraftbj kraftbj added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] AtD [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Apr 1, 2019
@rodrigoi
Copy link
Contributor Author

rodrigoi commented Apr 1, 2019

@kraftbj @simison thanks for taking a look at this! So, to correct procedure would be to mark the module as deprecated and remove the menu item from the classic editor plugin? Or can I remove the tinymce plugin files?

I'll probably close this PR and open a new one with the new changes.

@kraftbj
Copy link
Contributor

kraftbj commented Apr 1, 2019

I think you can delete everything as you did, just leave the modules/after-the-deadline.php file with a stub, exactly like https://github.com/Automattic/jetpack/blob/master/modules/wpcc.php did.

@jetpackbot
Copy link

jetpackbot commented Apr 2, 2019

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 29, 2019.
Scheduled code freeze: April 22, 2019

Generated by 🚫 dangerJS against 6e49ccd

@kwight
Copy link
Contributor

kwight commented Apr 3, 2019

To help me understand the decision to drop the feature, can you drop the shortlink anchor/link to the p2 discussion.

@kraftbj I believe it stemmed from Automattic/wp-calypso#28550 (comment)

@kwight
Copy link
Contributor

kwight commented Apr 3, 2019

After a fresh yarn build upon switching branches, it all works (or, uh, is missing) as expected 👍

@gwwar
Copy link
Contributor

gwwar commented Apr 4, 2019

@kraftbj Besides getting a clean CI run, was there any other feedback to address, or are we mostly okay here?

jeherve
jeherve previously requested changes Apr 5, 2019
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 looks good and tests well. I would only recommend that you remove the yarn.lock changes from this PR, since they are not related to the module changes.

@jeherve jeherve dismissed kraftbj’s stale review April 5, 2019 09:24

Feedback addressed

@rodrigoi rodrigoi force-pushed the update/remove-after-the-deadline branch from 31658cd to ffd8ad7 Compare April 5, 2019 15:19
@rodrigoi rodrigoi added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Apr 24, 2019
@rodrigoi
Copy link
Contributor Author

@simison rebased, retested and ready for review. The companion diff for WordPress.com is un-fusionable, so I'll force the test status on the MC tool.

@rodrigoi rodrigoi dismissed jeherve’s stale review April 24, 2019 18:26

yarn.lock file removed from the diff.

@kraftbj
Copy link
Contributor

kraftbj commented Apr 24, 2019

All this looks good—are we going to clean up any user meta AtD options? AtD_options, AtD_check_when, AtD_guess_lang, AtD_ignored_phrases?

Since cycling through every user could be too taxing, just something like filtering when an user is updated to check/remove that meta?

@rodrigoi
Copy link
Contributor Author

@kraftbj I don't have a strong plan for cleaning user meta, because I'm not very familiar with the sync process and where is the best place to do it. But it's something for another (smaller) PR.

@kraftbj
Copy link
Contributor

kraftbj commented Apr 24, 2019

It's unrelated to sync. It would be on the local Jetpack site.

@kraftbj kraftbj self-assigned this Apr 24, 2019
@kraftbj
Copy link
Contributor

kraftbj commented Apr 24, 2019

I'd like to test my changes outside of my local environment, but running out of time for today. I'll circle back to this tomorrow.

@kraftbj
Copy link
Contributor

kraftbj commented Apr 25, 2019

Before and after with an existing user with AtD preferences then updates the user after this patch:
2019-04-25 at 09 37

@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 25, 2019
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 in my end. 👍 🚢 💀

@rodrigoi rodrigoi merged commit 926490b into master Apr 25, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 25, 2019
@rodrigoi rodrigoi deleted the update/remove-after-the-deadline branch April 25, 2019 19:48
kraftbj added a commit that referenced this pull request Apr 30, 2019
anomiex added a commit that referenced this pull request May 10, 2022
Did a pass over _inc/client/components looking for things that weren't
used anywhere, and found a bunch:

* components/data/query-connect-url - Last use removed in #8014
* components/data/query-connection-status - Last use removed in 62e9ab0
* components/data/query-modules - Last use removed in bfc40ad
* components/data/query-plugin-updates - Last use removed in #17003
* components/data/query-site-products - Last use removed in #21594
* components/form/* - Didn't check for last use, too many bits. But it
  looks like the `formsy-react` package many of these depended on wasn't
  even installed since #8208.
* components/inline-expand - Last use removed in #6550
* components/jetpack-dialogue - Last use removed in #16518
* components/jetpack-logo - Last use removed in #20148
* components/jetpack-termination-dialog - Last use removed in #21048
* components/module-settings/index.jsx - Last use removed in #10644
* components/module-settings/inline-module-toggle.jsx - Last use removed
  in #12118
* components/screen-reader-text - Last use removed in #18843
* components/settings - Last use removed in 26315e1, I think
* components/tags-input - Last use removed in #11772

Then there were a few more that were only used from some of the above:

* components/data/query-connected-plugins
* components/module-settings/form-components.jsx
* components/multiple-choice-question
* components/setting-toggle
anomiex added a commit that referenced this pull request May 10, 2022
Did a pass over _inc/client/components looking for things that weren't
used anywhere, and found a bunch:

* components/data/query-connect-url - Last use removed in #8014
* components/data/query-connection-status - Last use removed in 62e9ab0
* components/data/query-modules - Last use removed in bfc40ad
* components/data/query-plugin-updates - Last use removed in #17003
* components/data/query-site-products - Last use removed in #21594
* components/form/* - Didn't check for last use, too many bits. But it
  looks like the `formsy-react` package many of these depended on wasn't
  even installed since #8208.
* components/inline-expand - Last use removed in #6550
* components/jetpack-dialogue - Last use removed in #16518
* components/jetpack-logo - Last use removed in #20148
* components/jetpack-termination-dialog - Last use removed in #21048
* components/module-settings/index.jsx - Last use removed in #10644
* components/module-settings/inline-module-toggle.jsx - Last use removed
  in #12118
* components/screen-reader-text - Last use removed in #18843
* components/settings - Last use removed in 26315e1, I think
* components/tags-input - Last use removed in #11772

Then there were a few more that were only used from some of the above:

* components/data/query-connected-plugins
* components/module-settings/form-components.jsx
* components/multiple-choice-question
* components/setting-toggle

Co-authored-by: Brandon Kraft <public@brandonkraft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] AtD Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
10 participants