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

Add notice about webhooks using the legacy REST API #40866

Merged
merged 8 commits into from Oct 24, 2023

Conversation

Konamiman
Copy link
Contributor

@Konamiman Konamiman commented Oct 19, 2023

Changes proposed in this Pull Request:

This pull request is an extension of #40535. It:

  1. Adds a new WebhookUtil::get_legacy_webhooks_count method. It counts the existing webhooks that are configured to use the Legacy REST API classes to generate their payloads.
  2. Adds a new api_version optional argument to WC_Webhook_Data_Store::search_webhooks.
  3. Adds a new "Legacy" section to the webhooks settings page, it lists the same webhooks that are counted by get_legacy_webhooks_count.
  4. Adds a new warning notice that is shown if at least one of these legacy webhooks exist and the separate Legacy REST API extension doesn't exist:

image

Note that if legacy webhooks exist and the Legacy REST API is enabled, two notices will appear. This is on purpose, it's better to be "overinformative" in this case and the notices can be dismissed anyway.

How to test the changes in this Pull Request:

  1. If you tested Add notices about the removal of the Legacy API in WooCommerce 9.0 #40535 and have the dummy Legacy REST API plugin installed, disable it.
  2. Make sure that no webhooks exist that are configured with "Legacy API v3" in the "API Version" field (WooCommerce - Settings - Advanced - Webhooks, you need to edit a webhook to see the API version it's using).
  3. Run the following to simulate a WooCommerce update from the point of view of the admin notices: wp eval 'WC_Admin_Notices::reset_admin_notices();'
  4. Verify that no new notices appear when reloading any admin page.
  5. Create a couple of new webhooks (or edit existing ones) so that they have "Legacy API v3" in the "API Version" field.
  6. Repeat step 3.
  7. Reload the webhooks list page. Note how the "The WooCommerce webhooks with Legacy REST API payload will be unsupported soon" notice has appeared and that there's a new "Legacy" section.
  8. Go to the new "Legacy" section, verify that it lists the appropriate webhooks.
  9. Delete the webhooks, or edit them to use a different API version.
  10. Reload any admin page, the notice should have disappeared.
  11. Repeat step 5, then step 3, the notice should appear again upon reload.
  12. Create a dummy Legacy REST API plugin (see the testing instructions of Add notices about the removal of the Legacy API in WooCommerce 9.0 #40535, step 11) unless you have done that already. Activate the plugin.
  13. Repeat step 3, reload, the notice should have disappeared.
  14. Try also manually dismissing the notice (to revert that: wp db query "delete from wp_usermeta where meta_key='dismissed_legacy_webhooks_unsupported_in_woo_90'").

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Comment

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Oct 19, 2023
@github-actions
Copy link
Contributor

Hi @coreymckrill,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2023

Test Results Summary

Commit SHA: 7949d74

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 57s
E2E Tests215004021920m 7s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@Konamiman Konamiman changed the title Add a new WebhookUtil::get_legacy_webhooks_count method Add a new warning notice for webhooks using the legacy REST API Oct 19, 2023
@Konamiman Konamiman changed the title Add a new warning notice for webhooks using the legacy REST API Add a warning notice for webhooks using the legacy REST API Oct 19, 2023
@Konamiman Konamiman changed the title Add a warning notice for webhooks using the legacy REST API Add notice about webhooks using the legacy REST API Oct 19, 2023
Copy link
Collaborator

@coreymckrill coreymckrill left a comment

Choose a reason for hiding this comment

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

Works as described in the excellent testing instructions. A couple of wording suggestions, and one small concern about caching.

Co-authored-by: Corey McKrill <916023+coreymckrill@users.noreply.github.com>
Copy link
Collaborator

@coreymckrill coreymckrill left a comment

Choose a reason for hiding this comment

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

👍 Confirmed that the legacy count is accurate with Docket Cache enabled. Everything else looks good as well.

@coreymckrill coreymckrill added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Oct 20, 2023
@coreymckrill
Copy link
Collaborator

@Konamiman I'm not sure why some of the checks are failing, you might want to do a rebase if this was branched before #40572 got merged...

@alopezari alopezari added status: analysis complete Indicates if a PR has been analysed by Solaris needs: analysis Indicates if the PR requires a PR testing scrub session. and removed needs: analysis Indicates if the PR requires a PR testing scrub session. status: analysis complete Indicates if a PR has been analysed by Solaris labels Oct 23, 2023
@Konamiman Konamiman merged commit 481e799 into trunk Oct 24, 2023
19 of 20 checks passed
@Konamiman Konamiman deleted the add-legacy-webhooks-notice branch October 24, 2023 06:51
@github-actions github-actions bot added this to the 8.3.0 milestone Oct 24, 2023
@alopezari alopezari added needs: internal testing Indicates if the PR requires further testing conducted by Solaris status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Oct 24, 2023
allie500 pushed a commit that referenced this pull request Nov 1, 2023
Co-authored-by: Corey McKrill <916023+coreymckrill@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: internal testing Indicates if the PR requires further testing conducted by Solaris plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants