Skip to content

Add/source param to get subscriptions call #35051

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.

Sign up for GitHub

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

Jump to bottom
Merged
jorgeatorres merged 5 commits into from
Nov 14, 2022
Merged

Add/source param to get subscriptions call #35051

jorgeatorres merged 5 commits into from
Nov 14, 2022

Conversation

@KokkieH
Copy link
Contributor

@KokkieH KokkieH commented Oct 12, 2022 •
edited
Loading

All Submissions:

Changes proposed in this Pull Request:

We want to add a source parameter to WooCommerce.com API requests to the subscriptions endpoint

Related issue on WooCommerce.com: 13928-gh-Automattic/woocommerce.com

How to test the changes in this Pull Request:

Test My subscriptions

  1. Open WooCommerce > Extensions
  2. Open the My Subscriptions tab
  3. Connect to WooCommerce.com
  4. Update your list of subscriptions
  5. Confirm that your subscriptions are loaded
  6. Confirm that other Helper functions that use the WC_Helper_API::get() and url() methods still works normally, e.g. visit the Plugins page and ensure you can activate, deactivate, and view the View Details modal for plugins hosted on WooCommerce.com

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 --filter=<project> run changelog add?

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.

andfinally
andfinally reviewed Oct 12, 2022
View reviewed changes
andfinally
andfinally reviewed Oct 12, 2022
View reviewed changes
Copy link
Contributor

@andfinally andfinally left a comment

Choose a reason for hiding this comment

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

Looking good! 👌 Just left some minor thoughts.

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Oct 12, 2022
@KokkieH KokkieH marked this pull request as ready for review October 12, 2022 14:55
@andfinally andfinally requested review from a team and jorgeatorres and removed request for a team October 12, 2022 14:59
jorgeatorres
jorgeatorres previously approved these changes Oct 18, 2022
View reviewed changes
Copy link
Member

@jorgeatorres jorgeatorres left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@andfinally
Copy link
Contributor

andfinally commented Oct 18, 2022 •
edited
Loading

I'm just having a go at testing this, but I'm having trouble checking it out into my local WooCommerce root. I'm getting pathspec 'KokkieH:add/source-param-to-get-subscriptions-call' did not match any file(s) known to git.

@KokkieH
Copy link
Contributor Author

KokkieH commented Oct 19, 2022

@andfinally could it be because I created the PR via a fork of the repo?

@KokkieH
Copy link
Contributor Author

KokkieH commented Oct 19, 2022

@andfinally here's a patch of the changes (minus the changelog update).

@KokkieH
Copy link
Contributor Author

KokkieH commented Oct 19, 2022

@jorgeatorres thanks for the review. Am I right in thinking we can expect this to land in 7.2, given 7.1 is already in code freeze? (If it can still make it into 7.1, then great, but it's not an urgent change.)

andfinally
andfinally previously approved these changes Oct 19, 2022
View reviewed changes
Copy link
Contributor

@andfinally andfinally 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.

Looks good to me, thanks for the help with testing. Confirmed that source=my_subscriptions is added to the URL in the extensions tab request, and that other requests still work as expected.

(The active/inactive switch on the My Subscriptions tab isn't working on my customer test site, but it never has, so I don't think it's related. On wp-env, the switch isn't shown, perhaps because the site URL is http.)

@KokkieH KokkieH dismissed stale reviews from andfinally and jorgeatorres via f31d9d9 October 20, 2022 06:41
@KokkieH KokkieH requested a review from jorgeatorres November 11, 2022 08:54
@KokkieH
Copy link
Contributor Author

KokkieH commented Nov 11, 2022

@jorgeatorres it appears my last changes based on @andfinally 's feedback invalidated you original review. Can you please check and approve this again?

There are also two failing CI checks. I looked through them - I'm not sure what needs to be done for the first, and the second doesn't seem related to any of the changes I made. Can you please take a look as well? Thanks!

@KokkieH @jorgeatorres
Add source param for subscription endpoint calls
ee4d99e 
- Adds an optional $source argument to the WC_Helper_API::url() method
- Get URL of page making WC_Helper::get-subscripitonts() request and add a
  source arg to the WC_Helper_API::get() request made by that method
- Pass that source arg to the WC_Helper_API::url() methd from
  WC_Helper_API::get()
@KokkieH @jorgeatorres
Add changelog file
b7f701a
@KokkieH @jorgeatorres
Use sripos instead of preg_match in WC_Helper::get_subscriptions()
105ea15 
- Update "source" argument name across methods to "query_string" to be
  more generic
- Use separate variables to make it clear the URI of the requesting
  page, and the value passed as the source query string are different
@KokkieH @jorgeatorres
Add @param declaration to function comment
9f4a127
@jorgeatorres jorgeatorres force-pushed the add/source-param-to-get-subscriptions-call branch 2 times, most recently from 2aa2493 to e1d0ca2 Compare November 11, 2022 11:33
@jorgeatorres jorgeatorres force-pushed the add/source-param-to-get-subscriptions-call branch from e1d0ca2 to 96d6407 Compare November 11, 2022 11:39
jorgeatorres
jorgeatorres approved these changes Nov 14, 2022
View reviewed changes
Copy link
Member

@jorgeatorres jorgeatorres left a comment

Choose a reason for hiding this comment

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

LGTM.

@jorgeatorres jorgeatorres merged commit dda1430 into woocommerce:trunk Nov 14, 2022
@github-actions github-actions bot added this to the 7.2.0 milestone Nov 14, 2022
@github-actions github-actions bot mentioned this pull request Dec 15, 2022
Closed
@github-actions github-actions bot mentioned this pull request Jan 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jorgeatorres jorgeatorres jorgeatorres approved these changes

+1 more reviewer

@andfinally andfinally andfinally left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Milestone

7.2.0

Development

Successfully merging this pull request may close these issues.

3 participants

@KokkieH @andfinally @jorgeatorres