Skip to content

Conversation

@james-allan
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

This PR adds error handling to the WooCommerce > Subscriptions (experiment) page if the "Get Started" buttons fails to install WooCommerce Payments.

part of #32694

How to test the changes in this Pull Request:

  1. Checkout this branch and run pnpm nx build-watch woocommerce-admin to build assets
  2. Add the 'DISALLOW_FILE_MODS' const to your site (see example below).
  3. With WooCommerce Payments and WooCommerce Subscriptions inactive, navigate to WooCommerce > Subscriptions
  4. Click the "Get started" button.
  5. Once the plugin activation is attempted, you should see the following error message.
define( 'DISALLOW_FILE_MODS', true );

Screen Shot 2022-05-03 at 2 16 22 pm

Note: without the DISALLOW_FILE_MODS set to true, the "Get started" button will install (if necessary) and activate WooCommerce Payments but the page will appear to hang and the button will be stuck on processing. This is intended for now and will be handled by the rest of #32694 once the experiment allocation is settled.

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 by running pnpm nx affected --target=changelog?

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.

* Navigate to either newSubscriptionProductUrl or onboardingUrl
* depending on the which treatment the user is assigned to.
*/
// eslint-disable-next-line no-console
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding this was necessary to get over WC's linting on commit. It can be removed once we remove the following console log.

@james-allan
Copy link
Contributor Author

Just noting that I changed (b15cfd3) the error message to follow a better sentence structure IMO. We'll want to get broader feedback on this messaging anyway but just wanted to highlight it.

Screen Shot 2022-05-03 at 4 54 15 pm

Copy link
Contributor

@aprea aprea left a comment

Choose a reason for hiding this comment

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

This LGTM. I tested both the happy and sad paths, both are working as expected.

@james-allan james-allan merged commit 9b7b574 into add/experiment-subscriptions-admin-menu-1 May 4, 2022
@james-allan james-allan deleted the add/experiment-subscriptions-error-notice branch May 4, 2022 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants