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

Fix promotions filtering by tab #44884

Merged
merged 5 commits into from Feb 22, 2024
Merged

Fix promotions filtering by tab #44884

merged 5 commits into from Feb 22, 2024

Conversation

andfinally
Copy link
Contributor

@andfinally andfinally commented Feb 22, 2024

Submission Review Guidelines:

Changes proposed in this Pull Request:

  • Fix the condition in the Promotions component which checks for tab parameter to decide whether or not to show a notice on a given page. When the pages array of a promotion specifies a page with no tab property, the code is currently showing the promotion even on pages with tab parameters.
  • Using reference to WC_Admin_Marketplace_Promotions::TRANSIENT_NAME when outputting JS data for promotions, instead of transient string.
  • Better initialisation of wc global object in the inline script.
  • Changes from adding a property to the wc global object to creating a new global object wcMarketplace, as wc was not being created on some external test sites.

Closes 19573-gh-Automattic/woocommerce.com.
Related: #44840
Related: #44655

How to test the changes in this Pull Request:

  1. Check out this branch.
  2. Change the URL in fetch_marketplace_promotions on this line to the URL here: 339ad-pb/. This is a gist providing dummy data. If you can't access that URL and you have a GitHub account, create a gist of your own, paste in this content, and use the raw URL of that gist:
[
	{
		"date_from_gmt": "2024-02-14 00:00",
		"date_to_gmt": "2024-03-31 22:59",
		"format": "notice",
		"style": "info",
		"pages": [
			{ "page": "wc-admin", "path": "/extensions" },
			{ "page": "wc-admin", "path": "/extensions", "tab": "extensions" },
			{ "page": "wc-admin", "path": "/extensions", "tab": "themes" }
		],
		"content": {
			"en_US": "<b>Limited time sale</b> up to 40% off. Sale ends March 29 at 2pm UTC."
		},
		"icon": "percent",
		"is_dismissible": true
	},
	{
		"date_from_gmt": "2024-02-14 00:00",
		"date_to_gmt": "2024-03-31 22:59",
		"format": "notice",
		"style": "success",
		"pages": [{ "page": "wc-admin", "path": "/extensions", "tab": "extensions" }],
		"content": {
			"en_US": "<b>Limited time sale</b> up to 40% off. Sale ends March 29 at 2pm UTC.",
			"fr_FR": "<b>Vente \u00e0 dur\u00e9e limit\u00e9e</b> jusqu'\u00e0 40 % de r\u00e9duction. La vente se termine le 29 mars \u00e0 14h UTC."
		}
	},
	{
		"date_from_gmt": "2024-02-14 00:00",
		"date_to_gmt": "2024-02-20 22:59",
		"format": "notice",
		"style": "success",
		"pages": [{ "page": "wc-admin", "path": "/extensions", "tab": "extensions" }],
		"content": {
			"en_US": "Test item, should be filtered out"
		}
	},
	{
		"date_from_gmt": "2024-02-13 00:00",
		"date_to_gmt": "2024-03-31 22:59",
		"format": "menu_bubble",
		"content": { "en_US": "Sale" },
		"menu_item_id": "woocommerce-marketplace"
	},
	{
		"date_from_gmt": "2024-02-01 00:00",
		"date_to_gmt": "2024-02-01 22:59",
		"format": "menu_bubble",
		"content": { "en_US": "Test item, should be filtered out" },
		"menu_item_id": "woocommerce-marketplace"
	}
]
  1. If you're doing repeated tests, and you're using wp-env, you can flush the transient the promotions class creates by doing wp-env run cli wp transient delete --all.
  2. To bypass the transient, you can comment out these lines in fetch_marketplace_promotions:
if ( false !== $promotions ) {
	return;
}
  1. The promotions class runs a scheduled action to fetch data. So it's convenient to install WP Crontrol in your environment. Then, to fetch the data again, find the scheduled action woocommerce_marketplace_fetch_promotions and click "Run now" to run it.
  2. You should see a badge on the Extensions menu item, and these notices:

Extensions Discover tab

image

 

Extensions Browse tab

image

 

Extensions Themes tab

image

 

Extensions My Subscriptions tab – no notice

image

 

  1. Other WooCommerce Admin pages should load and work as usual, with no new JS errors.

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 Feb 22, 2024
Copy link
Contributor

github-actions bot commented Feb 22, 2024

Test Results Summary

Commit SHA: 0d6ef62

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 38s
E2E Tests33600803449m 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.

@andfinally andfinally requested review from corsonr, a team and mcliwanow and removed request for a team February 22, 2024 12:52
@andfinally andfinally added the needs: developer feedback Issues that need feedback from one of the WooCommerce Core developers. label Feb 22, 2024
Copy link
Contributor

github-actions bot commented Feb 22, 2024

Hi @corsonr, @mcliwanow,

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

Copy link
Member

@corsonr corsonr left a comment

Choose a reason for hiding this comment

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

All good, I can confirm that the tabs act like a filter as intended to.

I'm wondering if this part:

@media only screen and (min-width: 600px) {
	.woocommerce-marketplace__notice {
		margin-bottom: 40px;
	}
}

Shouldn't be removed at all in plugins/woocommerce-admin/client/marketplace/components/notice/notice.scss since we agreed on 32px

@corsonr
Copy link
Member

corsonr commented Feb 22, 2024

Also, it's not related to that PR but it would be good maybe fixing the focus on the close icon:

image

@andfinally
Copy link
Contributor Author

Thanks @corsonr – the design had 32px on mobile, and 40px on bigger screens, that's why we have the media query.

Using reference to WC_Admin_Marketplace_Promotions::TRANSIENT_NAME when outputting JS data for promotions, instead of transient string.
Better initialisation of wc global object in the inline script.
@andfinally andfinally removed the needs: developer feedback Issues that need feedback from one of the WooCommerce Core developers. label Feb 22, 2024
@andfinally andfinally merged commit 24a216f into trunk Feb 22, 2024
39 checks passed
@andfinally andfinally deleted the update/promotions-show branch February 22, 2024 16:35
@github-actions github-actions bot added this to the 8.7.0 milestone Feb 22, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Feb 22, 2024
@nigeljamesstevenson nigeljamesstevenson 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 Feb 22, 2024
Konamiman pushed a commit that referenced this pull request Mar 13, 2024
* Fixing filtering by tabs in `Promotions` component.
Using reference to WC_Admin_Marketplace_Promotions::TRANSIENT_NAME when outputting JS data for promotions, instead of transient string.
Better initialisation of wc global object in the inline script.

* Changelog.

* Using global var for marketplace.

* More tentative reference to global object.

* icon TS error
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