Add inbox WP admin link support for remote inbox notifications #33237

louwie17 · 2022-05-27T13:48:15Z

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Make use of admin_url by default for action urls, and only use wc_admin_url when the url starts with &path.

This would mean that this might break on older versions of WC, so whomever adds a new note will have to add this condition (other suggestions are welcome):

{
"type": "plugin_version",
"plugin": "woocommerce",
"operator": ">=",
"version": "6.5"
}

Closes #32129 .

How to test the changes in this Pull Request:

Load this branch and have WooCommerce Admin Test Helper installed, but on this branch: Use \WC_Install class as Install class has been removed woocommerce-admin-test-helper#48 (as there is a bug, if you could review those changes at the same time that would be fantastic :) )
Create a mu-plugin if you don't have one already and add this filter to it:

function add_datasource_remote_inbox_notifications( $data_sources, $id ) {
	if ( "remote_inbox_notifications" === $id ) {
		$data_sources[] = "https://gist.githubusercontent.com/louwie17/bd6f9fb6e0dec763f3dd9ed204924689/raw/a89ef9651aa0ab6cf3c636a4ea4a348f3c0b6734/inbox-notifications.json";
	}
	return $data_sources;
}
add_filter( 'data_source_poller_data_sources', 'add_datasource_remote_inbox_notifications', 10, 2 );

This add's this gist to the remote inbox notifications that will add 3 new notes.
3. Go to Tools > WCA Test Helper > Tools and trigger the wc_admin_daily_job
4. Go to WooCommerce > Home and 4 new notes should be present (Navigate to plugins, Where did this note come from?, Navigate to add products, and Navigate to onboarding )
5. Click on the action of each of them, and they should all direct correctly to it's designated location (as implied by the title and description). The Navigate to add products and the Navigate to plugins are the notes that contains the Admin url, see line 24 and 147 in the Gist ).

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 successfully run tests with your changes locally?
Have you created a changelog file for each project being changed, ie pnpm nx changelog <project>?

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.

louwie17 · 2022-05-27T13:58:26Z

plugins/woocommerce/src/Admin/RemoteInboxNotifications/SpecRunner.php

@@ -101,7 +101,10 @@ private static function get_url( $action ) {
 		}

 		if ( isset( $action->url_is_admin_query ) && $action->url_is_admin_query ) {
-			return wc_admin_url( $action->url );
+			if ( strpos( $action->url, '&path' ) === 0 ) {
+				return wc_admin_url( $action->url );


I decided to go this route, as &path seemed to be the only examples I could find of when we were linking to WCA pages.
As the url_is_admin_query entails, we likely want to phase out that support and always provide a WP Admin link even for WCA pages, but that is just a thought.

This seems like it will catch most known cases. Just for the sake of discussion, is there any chance we can reuse @joelclimbsthings's PR ( https://github.com/woocommerce/woocommerce/pull/33124/files ) to detect and navigate to links in the client?

As the url_is_admin_query entails, we likely want to phase out that support and always provide a WP Admin link even for WCA pages, but that is just a thought.

Completely agree with this. It probably would have been wise in retrospect to never allow this specific level of partial URLs only for WCA.

Just for the sake of discussion, is there any chance we can reuse @joelclimbsthings's PR ( https://github.com/woocommerce/woocommerce/pull/33124/files ) to detect and navigate to links in the client?

We could use his navigateTo function in the inbox-note, but that would still require the above logic. As the current logic in Joel's PR can't differentiate between going to &path=wc-addons or plugins.php.
So it would only work well for going to either WCA pages or non wp admin pages.

The one benefit of using Joel's navigateTo in the inbox-note is that if we were going to a WCA page, it won't do a full page refresh, but makes use of the react router (I think) not a 100% on this.

Ya, that makes sense. This was definitely an oversight on our part allowing partial (WCA only) paths as options.

I may p2 this later to make sure we're all on the same page going forward.

botwoo · 2022-05-27T14:27:08Z

📊 Test reports for this pull request have been published and are accessible through the following links:

REST API Test Report
E2E Test Report (TBC)

Latest commit referenced in the reports: Add changelog 0382853
This comment will automatically be updated with the latest referenced commit when you push new changes to this pull request.

Visit the WooCommerce Test Reports homepage to view all published reports. See the FAQs page if you're having problems accessing them.

joshuatf

Looks good overall; just dropped a question in the code.

Also curious if we might have any use cases for navigating to non-admin URLs in the future.

joshuatf · 2022-05-30T17:06:44Z

plugins/woocommerce/src/Admin/RemoteInboxNotifications/SpecRunner.php

@@ -101,7 +101,10 @@ private static function get_url( $action ) {
 		}

 		if ( isset( $action->url_is_admin_query ) && $action->url_is_admin_query ) {
-			return wc_admin_url( $action->url );
+			if ( strpos( $action->url, '&path' ) === 0 ) {
+				return wc_admin_url( $action->url );


This seems like it will catch most known cases. Just for the sake of discussion, is there any chance we can reuse @joelclimbsthings's PR ( https://github.com/woocommerce/woocommerce/pull/33124/files ) to detect and navigate to links in the client?

As the url_is_admin_query entails, we likely want to phase out that support and always provide a WP Admin link even for WCA pages, but that is just a thought.

Completely agree with this. It probably would have been wise in retrospect to never allow this specific level of partial URLs only for WCA.

octaedro · 2022-05-30T17:34:05Z

...ommerce/tests/legacy/unit-tests/woocommerce-admin/remote-inbox-notifications/spec-runner.php

+/**
+ * class WC_Admin_Tests_RemoteInboxNotifications_SpecRunner
+ */
+class WC_Admin_Tests_RemoteInboxNotifications_SpecRunner extends WC_Unit_Test_Case {


louwie17 · 2022-05-30T18:24:47Z

Also curious if we might have any use cases for navigating to non-admin URLs in the future.

We might, I think one example would be to check out the users homepage for example and link there.
I think in that case we would want to remove url_is_admin_query, and have users put the full path in /wp-admin/plugins.php or / to go to the home page.
Thoughts @joshuatf ?

joshuatf · 2022-05-30T18:35:57Z

and have users put the full path in /wp-admin/plugins.php

The only issue with this is that /wp-admin can easily be changed to something else depending on a user's WP setup. We could put a check in place and make some assumptions around this, but it might feel a bit brittle.

louwie17 · 2022-05-30T19:49:31Z

The only issue with this is that /wp-admin can easily be changed to something else depending on a user's WP setup. We could put a check in place and make some assumptions around this, but it might feel a bit brittle.

Aaah yeah, I forgot about that, thanks for the reminder. In that case if the user has url_is_admin_query set to false and the url starts with / then we will go to a site url. I suppose this might already work like that, given we just return the url in those situations.

joshuatf

Testing well and code looks good! Thanks for the added tests ❤️

@joelclimbsthings Your navigateTo function will be a great add-on to the note actions. There are some good test cases in here worth checking.

joshuatf · 2022-05-31T14:21:08Z

plugins/woocommerce/src/Admin/RemoteInboxNotifications/SpecRunner.php

@@ -101,7 +101,10 @@ private static function get_url( $action ) {
 		}

 		if ( isset( $action->url_is_admin_query ) && $action->url_is_admin_query ) {
-			return wc_admin_url( $action->url );
+			if ( strpos( $action->url, '&path' ) === 0 ) {
+				return wc_admin_url( $action->url );


Ya, that makes sense. This was definitely an oversight on our part allowing partial (WCA only) paths as options.

I may p2 this later to make sure we're all on the same page going forward.

joshuatf

Testing well and code looks good! Thanks for the added tests ❤️

@joelclimbsthings Your navigateTo function will be a great add-on to the note actions. There are some good test cases in here worth checking.

github-actions · 2022-05-31T16:32:29Z

Hi @louwie17, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

Add the release: add testing instructions label

louwie17 added 2 commits May 27, 2022 10:27

Add support for admin url in remote inbox notifications

2efc1de

Add unit tests for the spec runner action url

b9ea2d9

github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels May 27, 2022

Add changelog

0382853

louwie17 marked this pull request as ready for review May 27, 2022 13:56

louwie17 requested a review from a team May 27, 2022 13:56

louwie17 commented May 27, 2022

View reviewed changes

octaedro requested review from octaedro and removed request for a team May 30, 2022 16:53

joshuatf reviewed May 30, 2022

View reviewed changes

octaedro reviewed May 30, 2022

View reviewed changes

joshuatf approved these changes May 31, 2022

View reviewed changes

louwie17 merged commit c0999d4 into trunk May 31, 2022

louwie17 deleted the add/32129_remove_inbox_wp_admin_link_support branch May 31, 2022 16:32

github-actions bot added this to the 6.7.0 milestone May 31, 2022

github-actions bot mentioned this pull request Jul 13, 2022

Test WooCommerce 6.7 UVLabs/location-picker-at-checkout-for-woocommerce#44

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add inbox WP admin link support for remote inbox notifications #33237

Add inbox WP admin link support for remote inbox notifications #33237

louwie17 commented May 27, 2022 •

edited

louwie17 May 27, 2022

joshuatf May 30, 2022

louwie17 May 30, 2022

joshuatf May 31, 2022

botwoo commented May 27, 2022

joshuatf left a comment

joshuatf May 30, 2022

octaedro May 30, 2022

louwie17 commented May 30, 2022

joshuatf commented May 30, 2022

louwie17 commented May 30, 2022

joshuatf left a comment

joshuatf May 31, 2022

joshuatf left a comment

github-actions bot commented May 31, 2022

Add inbox WP admin link support for remote inbox notifications #33237

Add inbox WP admin link support for remote inbox notifications #33237

Conversation

louwie17 commented May 27, 2022 • edited

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

FOR PR REVIEWER ONLY:

louwie17 May 27, 2022

Choose a reason for hiding this comment

joshuatf May 30, 2022

Choose a reason for hiding this comment

louwie17 May 30, 2022

Choose a reason for hiding this comment

joshuatf May 31, 2022

Choose a reason for hiding this comment

botwoo commented May 27, 2022

joshuatf left a comment

Choose a reason for hiding this comment

joshuatf May 30, 2022

Choose a reason for hiding this comment

octaedro May 30, 2022

Choose a reason for hiding this comment

louwie17 commented May 30, 2022

joshuatf commented May 30, 2022

louwie17 commented May 30, 2022

joshuatf left a comment

Choose a reason for hiding this comment

joshuatf May 31, 2022

Choose a reason for hiding this comment

joshuatf left a comment

Choose a reason for hiding this comment

github-actions bot commented May 31, 2022

louwie17 commented May 27, 2022 •

edited