Add the Package_Version_Tracker class #20365

kbrown9 · 2021-07-14T19:41:33Z

Add the new Package_Version_Tracker class, which will obtain the package versions using the package versions using the jetpack_package_versions filter.

Changes proposed in this Pull Request:

Add the new Package_Version_Tracker class. This class obtains the package versions and determines whether the package versions have changed. If the versions have changed, it updates an option and sends the updated versions to WPCOM.
Add the jetpack_package_versions filter, which the tracker class will use to get the package versions.
Use the jetpack_package_versions filter in the Connection, Sync, and Backup packages to send the package versions to the tracker class.

Jetpack product discussion

p9dueE-3b1-p2

Does this pull request change what data or activity we track or use?

no

Testing instructions:

Checkout this branch. Activate and connect Jetpack.
Check that the jetpack_package_versions option is populated with the Backup, Connection, and Sync versions: wp option get jetpack_package_versions
Using your wpcom sandbox, check that your site's jetpack_package_versions option is correct.
You can test that package versions are updated by changing the package version values in the Package_Version class in the Connection, Sync, and Backup packages.

github-actions · 2021-07-14T19:44:38Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

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.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

…er class. Add the new Package_Version_Tracker class, which will obtain the package versions using the package versions using the jetpack_package_versions filter.

…ion_filter

projects/packages/connection/src/class-package-version-tracker.php

- Only set up the version tracker filter when connected. - Refactor the Tracker class to use static methods. There's not reason to create an object. - Refactor the maybe_update_package_versions method to simplify and clarify the logic. - Refactor the update_package_versions_option method to send the updated values to WPCOM first, then update the local option only when the request was successful. - Improve the tests.

fgiannar

Hi Kim,

Great work here, thank you!

I tested it using the provided instructions and works as expected!

Left two minor comments :)

fgiannar · 2021-08-05T07:55:04Z

projects/packages/connection/src/class-manager.php

@@ -94,6 +94,7 @@ public static function configure() {

 		if ( $manager->is_connected() ) {
 			add_filter( 'xmlrpc_methods', array( $manager, 'public_xmlrpc_methods' ) );
+			add_filter( 'plugins_loaded', array( new Package_Version_Tracker(), 'maybe_update_package_versions' ) );


Suggested change

add_filter( 'plugins_loaded', array( new Package_Version_Tracker(), 'maybe_update_package_versions' ) );

add_filter( 'plugins_loaded', __NAMESPACE__ . '\Package_Version_Tracker::maybe_update_package_versions' );

(since we made the method static).

Btw, while in this context the static method makes sense I'd personally prefer the non-static approach as it makes unit testing so much easier (eg the previous mockable approach was beautiful) and in case we want to add more functionality to this class it won't restrict us to make every method we add static.
However, this is just a personal preference - no need to change your logic!

I was on the fence about the static methods. Since you have a preference, I changed them back to non-static. I agree that the non-static approach could make future changes significantly easier to test.

I also brought back the mocked Package_Version_Tracker::update_package_versions_option method in test_maybe_update_package_versions, and added a new test, test_maybe_update_package_versions_success, which tests the case when the HTTP request to WPCOM is successful.

projects/packages/connection/src/class-package-version-tracker.php

* The Package_Version_Tracker methods are no longer static. * Unit tests: * Return of the mocks. * Add a new test, test_maybe_update_package_versions_success, which intercepts the http request and returns a success code.

….php Co-authored-by: Foteini Giannaropoulou <[email protected]>

…ion_filter

fgiannar

Great work, Kim! Let's 🚢 !

kbrown9 added [Package] Sync [Package] Connection [Package] Backup labels Jul 14, 2021

kbrown9 self-assigned this Jul 14, 2021

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jul 14, 2021

kbrown9 added the [Status] In Progress label Jul 14, 2021

kbrown9 force-pushed the update/package_version_filter branch from d3d1932 to cc490b6 Compare July 30, 2021 05:14

kbrown9 added the [Tests] Needs Tests Some Unit Tests would be really useful to include with this PR. label Jul 30, 2021

Add the jetpack_package_versions filter and the Package_Version_Track…

8424af8

…er class. Add the new Package_Version_Tracker class, which will obtain the package versions using the package versions using the jetpack_package_versions filter.

kbrown9 force-pushed the update/package_version_filter branch from cc490b6 to 8424af8 Compare July 30, 2021 05:19

kbrown9 added 4 commits August 3, 2021 00:44

Add unit tests.

f7f4d0e

Merge remote-tracking branch 'origin/master' into update/package_vers…

6edc194

…ion_filter

Update versions.

8f75e30

Better handling of situation where versions are not strings.

17ecd1e

kbrown9 removed [Status] In Progress [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Tests] Needs Tests Some Unit Tests would be really useful to include with this PR. labels Aug 4, 2021

Merge remote-tracking branch 'origin/master' into update/package_vers…

99bb5e1

…ion_filter

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Aug 4, 2021

kbrown9 added [Status] Needs Review To request a review from fellow Jetpack developers. 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 Aug 4, 2021

kbrown9 added this to the jetpack/10.1 milestone Aug 4, 2021

kbrown9 marked this pull request as ready for review August 4, 2021 02:52

kbrown9 requested a review from fgiannar August 4, 2021 02:54

fgiannar reviewed Aug 4, 2021

View reviewed changes

projects/packages/connection/src/class-package-version-tracker.php Outdated Show resolved Hide resolved

fgiannar added [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 fellow Jetpack developers. Label will be renamed soon. labels Aug 4, 2021

kbrown9 commented Aug 4, 2021

View reviewed changes

projects/packages/connection/src/class-package-version-tracker.php Outdated Show resolved Hide resolved

fgiannar reviewed Aug 5, 2021

View reviewed changes

kbrown9 and others added 7 commits August 10, 2021 00:43

Respond to review comments:

5fcd1f7

* The Package_Version_Tracker methods are no longer static. * Unit tests: * Return of the mocks. * Add a new test, test_maybe_update_package_versions_success, which intercepts the http request and returns a success code.

Update projects/packages/connection/src/class-package-version-tracker…

b468619

….php Co-authored-by: Foteini Giannaropoulou <[email protected]>

Update sync package version.

d8a2d56

Merge remote-tracking branch 'origin/master' into update/package_vers…

243f34f

…ion_filter

Merge remote-tracking branch 'origin/master' into update/package_vers…

b577ee0

…ion_filter

Fix unit test description.

4222bd8

Merge remote-tracking branch 'origin/master' into update/package_vers…

ef1d9ea

…ion_filter

kbrown9 added [Status] Needs Review To request a review from fellow Jetpack developers. 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 Aug 11, 2021

fgiannar approved these changes Aug 11, 2021

View reviewed changes

fgiannar added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Aug 11, 2021

kbrown9 merged commit 8f7b5b4 into master Aug 11, 2021

kbrown9 deleted the update/package_version_filter branch August 11, 2021 13:28

github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Aug 11, 2021

jeherve mentioned this pull request Sep 23, 2021

Package version tracker: conflicts with third-party plugins #21165

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the Package_Version_Tracker class #20365

Add the Package_Version_Tracker class #20365

kbrown9 commented Jul 14, 2021 •

edited

Loading

github-actions bot commented Jul 14, 2021 •

edited

Loading

fgiannar left a comment

fgiannar Aug 5, 2021

kbrown9 Aug 10, 2021

fgiannar left a comment

	add_filter( 'plugins_loaded', array( new Package_Version_Tracker(), 'maybe_update_package_versions' ) );
	add_filter( 'plugins_loaded', __NAMESPACE__ . '\Package_Version_Tracker::maybe_update_package_versions' );

Add the Package_Version_Tracker class #20365

Add the Package_Version_Tracker class #20365

Conversation

kbrown9 commented Jul 14, 2021 • edited Loading

Changes proposed in this Pull Request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Jul 14, 2021 • edited Loading

fgiannar left a comment

Choose a reason for hiding this comment

fgiannar Aug 5, 2021

Choose a reason for hiding this comment

kbrown9 Aug 10, 2021

Choose a reason for hiding this comment

fgiannar left a comment

Choose a reason for hiding this comment

kbrown9 commented Jul 14, 2021 •

edited

Loading

github-actions bot commented Jul 14, 2021 •

edited

Loading