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

Specify args for wp.data resolution in marketing page to support WP 5.9 #37198

Merged
merged 3 commits into from Mar 14, 2023

Conversation

ecgan
Copy link
Member

@ecgan ecgan commented Mar 13, 2023

All Submissions:

Changes proposed in this Pull Request:

Closes #37176.

In this PR, we specify empty array [] as the second argument for calls to invalidateResolution in marketing page to make it work with WordPress 5.9.

To make things consistent, we also specify empty array [] as the second argument for calls to hasFinishedResolution.

How to test the changes in this Pull Request:

Functionality test:

Pre-conditions:

  • Use WordPress 5.9.
  • Enable WooCommerce Multichannel Marketing experience in WooCommerce Settings > Advanced > Features page: /wp-admin/admin.php?page=wc-settings&tab=advanced&section=features
  1. Go to WooCommerce Marketing page: /wp-admin/admin.php?page=wc-admin&path=%2Fmarketing
  2. Install and activate a channel that registers itself as a marketing channel, e.g. Google Listings and Ads.
  3. You should see the channel appears as a registered channel in the Channels card (i.e. it should not disappear and be gone).

Code check

  1. Search for invalidateResolution under the plugins/woocommerce-admin/client/marketing folder. Make sure all the calls to the function has the second argument specified.
  2. Search for hasFinishedResolution under the plugins/woocommerce-admin/client/marketing folder. Make sure all the calls to the function has the second argument specified.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?
  • Have you included testing instructions?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

… args.

This is to make things work with WP 5.9.
This is to make things consistent with invalidateResolution to make things work with WP 5.9.
@ecgan ecgan added the focus: marketing Marketing page in WooCommerce Admin, i.e. `/wp-admin/admin.php?page=wc-admin&path=%2Fmarketing`. label Mar 13, 2023
@ecgan ecgan requested review from a team March 13, 2023 19:53
@ecgan ecgan self-assigned this Mar 13, 2023
@github-actions github-actions bot added focus: react admin [team:Ghidorah] plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Mar 13, 2023
@ecgan
Copy link
Member Author

ecgan commented Mar 13, 2023

@eason9487 , here's the fix to the recent issue #37176 that you brought up in code review on my PR #37044. Could you review this please? 🙏

@github-actions
Copy link
Contributor

Test Results Summary

Commit SHA: 580e3a8

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202611m 0s
E2E Tests189006019514m 0s

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.

@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

Merging #37198 (580e3a8) into trunk (0cf5677) will increase coverage by 0.0%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             trunk   #37198   +/-   ##
========================================
  Coverage     46.7%    46.7%           
  Complexity   17191    17191           
========================================
  Files          429      429           
  Lines        64845    64845           
========================================
+ Hits         30275    30277    +2     
+ Misses       34570    34568    -2     
Impacted Files Coverage Δ
...ugins/woocommerce/includes/class-wc-post-types.php 2.8% <ø> (ø)

... and 1 file with indirect coverage changes

Copy link
Member

@eason9487 eason9487 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. LGTM.

@ecgan ecgan merged commit 98dcb9b into trunk Mar 14, 2023
31 checks passed
@ecgan ecgan deleted the feature/37176-specify-wp-data-resolution-args branch March 14, 2023 23:30
@github-actions github-actions bot added this to the 7.6.0 milestone Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus: marketing Marketing page in WooCommerce Admin, i.e. `/wp-admin/admin.php?page=wc-admin&path=%2Fmarketing`. plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify empty array [] as invalidateResolution args to support WP 5.9
2 participants