-
Notifications
You must be signed in to change notification settings - Fork 808
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
Update/dont sync set object terms action for blacklisted taxonomies #41597
Update/dont sync set object terms action for blacklisted taxonomies #41597
Conversation
…are not being processed
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
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. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryNo summary data is avilable for parent commit c02cec5, cannot calculate coverage changes. 😴 If that commit is a feature branch rather than a trunk commit, this is expected. Otherwise, you might try re-running the Tests / Publish coverage data job on this PR once the corresponding job on the trunk commit has succeeded. |
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.
Thanks for the cleanup, Juanma!
I left an inline comment around a typo in _deprecated_function
ans since I missed that while reviewing #41433 I was wondering if we could fix those ones too.
*/ | ||
public function add_term_relationships( $args ) { | ||
_deprecated_function( __METHOD__, 'next-version' ); |
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.
_deprecated_function( __METHOD__, 'next-version' ); | |
_deprecated_function( __METHOD__, '$$next-version$$' ); |
I think this is how we are supposed to use next-version. Btw missed that with #41433 could we fix it real quick?
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.
LGTM 🚢
Fixes https://github.com/Automattic/vulcan/issues/668
Full sync for post actions is not using the terms since D37014-code, we should not fetch it and send it.
Proposed changes:
add_term_relationships
is not correct anymore and also used the same name as other modules the same we did in Full sync: Set chunk size of Woo modules dynamically #41433. Ideallybuild_full_sync_action_array
should be moved to the parent class (module)Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: