Skip to content

WooCommerce Services: Fix the redirect URL for Woo Services JITM CTAs#13752

Merged
jeherve merged 4 commits intomasterfrom
fix/woo_services_redirect_url
Oct 17, 2019
Merged

WooCommerce Services: Fix the redirect URL for Woo Services JITM CTAs#13752
jeherve merged 4 commits intomasterfrom
fix/woo_services_redirect_url

Conversation

@kbrown9
Copy link
Copy Markdown
Member

@kbrown9 kbrown9 commented Oct 14, 2019 •
edited
Loading

The redirect after clicking a Woo Services JITM CTA leads to a 404.

Changes proposed in this Pull Request:

  • Use home_url() instead of admin_url() to create the redirect URL. $SERVER['REQUEST_URI'] is used to create the redirect URL, and it includes wp-admin/. So, the redirect URL created using admin_url() will include an extra wp-admin/. Fix this by using home_url(), which does not include wp-admin/.
  • Some browsers strip the 'referer' header (see PR WooCommerce Services JITM: use redirect parameter. #9239). So let's just use admin_url() as the fallback redirect URL if $_GET['redirect'] is empty.
  • Fix most of the PHPCS errors and warnings in 3rd-party/woocommerce-services.php

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • This fixes a bug in an existing feature.

Testing instructions:

NOTE: The changes in PR #13720 are required to test this branch. Without those changes, the redirect fixed in this PR doesn't occur.

  1. Install and activate Jetpack and WooCommerce. Do not install WooCommerce Services.
  2. Connect Jetpack.
  3. Navigate to the WooCommerce -> Orders page.
  4. Click the Add New button to create a new order.
  5. Click the Create button for the new order.
  6. Navigate to wp-admin -> WooCommerce -> Settings.
  7. Set the Country / State to United States (any state).
  8. Refresh the WooCommerce -> Settings page.
  9. Dismiss the JITMs (which are located above the tabs) until you see one with the button text "Install WooCommerce Services". Do not dismiss this JITM.
  10. Click the “Install WooCommerce Services” button. Notice that you’re redirected to 404 with a URL containing:
    wp-admin/wp-admin/admin.php?page=wc-settings
  11. Navigate to wp-admin -> Plugins.
  12. Uninstall the WooCommerce Services plugin.
  13. Apply this branch.
  14. Navigate to wp-admin -> WooCommerce ->Settings. The "Install WooCommerce Services" JITM should still be displayed at the top of the page.
  15. Click the “Install WooCommerce Services” button. Notice that you’re redirected back to the WC Settings page as expected.
  16. Navigate to Plugins and verify that the WooCommerce Services plugin is installed and activated.

Proposed changelog entry for your changes:

  • TBD

@kbrown9 kbrown9 added Bug When a feature is broken and / or not performing as intended [Status] Needs Review This PR is ready for review. [Feature] JITM Just In Time Messages - pop-up tips and suggestions that appear on the dashboard and sidebar. labels Oct 14, 2019
@kbrown9 kbrown9 requested a review from a team October 14, 2019 19:16
@kbrown9 kbrown9 self-assigned this Oct 14, 2019
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Oct 14, 2019 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: November 5, 2019.
Scheduled code freeze: October 29, 2019

Generated by 🚫 dangerJS against 73c5479

jeherve
jeherve previously approved these changes Oct 15, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This works well for me, it should be good to merge.

Fix most of the PHPCS errors and warnings in 3rd-party/woocommerce-services.php

Thank you for that!

I hope you don't mind, I figured I'd take care of that last warning and add the file to the pre-commit hook whitelist: e687f337ff207b56e3f4afee20b9fdd426a05c7c

@jeherve jeherve added this to the 7.9 milestone Oct 15, 2019
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Oct 15, 2019
kbrown9 and others added 4 commits October 16, 2019 15:48
@kbrown9 @kraftbj
WooCommerce Services: Use home_url to create redirect url
33bc420 
The redirect URL is created using the value of $_SERVER['REQUEST_URI'], which includes
'wp-admin/'. When admin_url() is used to create the redirect URL, the resulting URL is
incorrect. Use home_url() instead. Also properly unslash and sanitize $_GET['redirect'].
@kbrown9 @kraftbj
WooCommerce Services: Use admin_url as the fallback URL
188b49f 
In PR #9239, wp_get_referer() was described as unrealiable because some browsers strip
the 'referer' header. So let's just use admin_url() as the fallback redirect URL if
$_GET['redirect'] is empty.
@kbrown9 @kraftbj
WooCommerce Services: Fix PHPCS errors and warnings.
c7a2a4d
@jeherve @kraftbj
Fix last PHPCS warning and add file to whitelist.
73c5479
@kraftbj kraftbj force-pushed the fix/woo_services_redirect_url branch from e687f33 to 73c5479 Compare October 16, 2019 20:48
@kraftbj
Copy link
Copy Markdown
Contributor

kraftbj commented Oct 16, 2019

Rebased against master to make it easier to test from JN. Reviewing now.

@kraftbj
Copy link
Copy Markdown
Contributor

kraftbj commented Oct 16, 2019

I may be doing something wrong, the JITM isn't loading. @jeherve Can you give it a go or I'll try again tomorrow.

@kbrown9
Copy link
Copy Markdown
Member Author

kbrown9 commented Oct 17, 2019

I may be doing something wrong, the JITM isn't loading. @jeherve Can you give it a go or I'll try again tomorrow.

I think you're not seeing it because my testing instructions missed a step. Sorry about that 😦

You also need to add at least one order:

  • Navigate to WooCommerce -> Orders and click Add New.
  • Click create.

jeherve
jeherve approved these changes Oct 17, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

LGTM! Merging.

@jeherve jeherve merged commit 93a5f3e into master Oct 17, 2019
@jeherve jeherve deleted the fix/woo_services_redirect_url branch October 17, 2019 10:10
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 17, 2019
jeherve added a commit that referenced this pull request Oct 23, 2019
@jeherve
Changelog: add #13752
ca17a6d
jeherve added a commit that referenced this pull request Oct 29, 2019
@jeherve
7.9: Changelog (#13608)
6cd1621 
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

Assignees

@kbrown9 kbrown9

Labels

Bug When a feature is broken and / or not performing as intended [Feature] JITM Just In Time Messages - pop-up tips and suggestions that appear on the dashboard and sidebar.

Projects

None yet

Milestone

7.9

Development

Successfully merging this pull request may close these issues.

5 participants

@kbrown9 @jetpackbot @kraftbj @jeherve @matticbot