-
Notifications
You must be signed in to change notification settings - Fork 799
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
Jetpack Plans: introduce new package #23503
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd want to bring in the changes from recent PRs, like #23189
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's something up with the changelog in the Jetpack plugin, and I'm not sure we're making changes to Jetpack the plugin. Do you think we should start using this in the same PR so there's not duplicate code? Git would be able to see that this is a code move too, and keep the history of the file.
Are you suggesting removing the files from Jetpack plugin folder and updating the references in this PR itself? @pablinos |
Yes, that's right. I suppose it depends on how much bigger it makes the PR, but I think moving the files would probably be preferrable to copying and then deleting in a separate PR. @jeherve might have some thoughts on that. |
I'm happy with both, personally; we can split this into 2 PRs if you'd like, but if we do it may help if you have some comments here on GitHub to highlight where you've diverted from the original files in Jetpack; that should make reviewing easier, by helping the reviewer quickly spot the changes you've made on top of copying the files. |
I think it might be better to split it into two PRs as we need to make reference changes across some 30 files which will increase the PR size. For this one, we should just focus on publishing the package first. |
Fair enough, we probably don't need the Jetpack plugin changelog then.
Have we diverted from the original files or is this just a copy and paste? If we end up with two divergent copies of the code, then we'll want to follow up with updating references in the Jetpack plugin as quickly as possible. |
Great news! One last step: head over to your WordPress.com diff, D77046-code, and deploy it. Thank you! |
r242972-wpcom |
This PR creates a new package with functionality to fetch Plans data from WPCOM. We have restored an existing branch and rebased it. Old PR link - #22650
For now, it copies the classes from the Jetpack plugin. Usage in the Jetpack plugin will be replaced in subsequent PRs as this must be synced with changes in WPCOM as well.
We need this to move the work on the standalone Publicize plugin.
Changes proposed in this Pull Request:
Jetpack product discussion
#23488 (comment)
p1647598368441399/1647597121.990859-slack-CBG1CP4EN
Does this pull request change what data or activity we track or use?
No
Testing instructions: