-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat(plugins): add url redirect for wc subscription renewals #3525
Conversation
1bd87d5
to
3492424
Compare
3492424
to
f70d25b
Compare
src/reader-activation/auth.js
Outdated
@@ -113,7 +113,9 @@ window.newspackRAS.push( function ( readerActivation ) { | |||
/** If there's a pre-auth, signing in redirects to the reader account. */ | |||
if ( reader?.email && ! reader?.authenticated ) { | |||
link.setAttribute( 'data-redirect', link.getAttribute( 'href' ) ); | |||
redirectInput.value = link.getAttribute( 'href' ); | |||
if ( ! redirectInput?.value ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should respect any pre-defined redirect values.
src/reader-activation/auth.js
Outdated
@@ -177,7 +179,7 @@ window.newspackRAS.push( function ( readerActivation ) { | |||
emailInput.value = reader?.email || ''; | |||
} | |||
|
|||
if ( redirectInput && ev?.target?.getAttribute( 'data-redirect' ) ) { | |||
if ( ev?.target?.getAttribute( 'data-redirect' ) && ! redirectInput?.value ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here re: respecting pre-defined redirect values
Got some feedback here so removing the |
f70d25b
to
e29b3a3
Compare
e29b3a3
to
eabb518
Compare
Added some changes to make the redirect happen via a new my-account endpoint Just awaiting feedback to verify the expected behavior before marking this one ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working well! Conditionally approved, with just a couple of questions:
- I had to refresh permalinks before the new endpoint would redirect. Is there anything we need to do to ensure this happens automatically with no downtime for live sites?
- Based on the discussion in feat(subscriptions): add setting to reattempt payment after final retry #3560 (comment) it sounds like we're moving to a
woocommerce-subscriptions
directory with smaller classes inside, so should we refactor to this structure here to avoid a future conflict? This could probably just be moved to thewoocommerce-subscriptions
directory since it's fairly self-contained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per @dkoo review, we need to add a rewrite rule refresh to be execute once after this change is deployed. Also agree with his comment on the file structure.
And just to confirm that naming things is one of the hardest problems in computes science. WDYT of renew-subscription
or subscription-renew
(in the singular form, because that's the main intent of this link, redirect to Subscriptions is a fallback)
dd06a16
to
fa7cdc2
Compare
Thanks for the feedback @leogermani and @dkoo!
Flushed permalinks and reorganized in fa7cdc2
I like |
This looks good and works nicely. One question though. My expectation was - I think it would be awesome - that if I'm not logged in and visit |
We can accomplish this by adding a redirect param with a value of the new endpoint and handling this when rendering the auth form. Added in a01a0b5 Let me know what you think! |
a01a0b5
to
f1f7787
Compare
Hey @chickenn00dle, good job getting this PR merged! 🎉 Now, the Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label. If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label. Thank you! ❤️ |
… renewals (#3525) This PR adds handling for a new `my-account/renew-subscription` endpoint to redirect readers to the first available pending renewal checkout, otherwise the subscriptions my account page.
# [5.10.0-alpha.1](v5.9.1...v5.10.0-alpha.1) (2024-12-12) ### Bug Fixes * duplicate orders save on cron ([#3604](#3604)) ([ec69167](ec69167)) * **ras-acc:** re-add recaptcha to the WooCommerce checkout ([#3605](#3605)) ([f42a75b](f42a75b)) * **ras:** do not require Woo plugins if using NRH ([#3614](#3614)) ([363a834](363a834)) * **wcs:** remove subscriptions expiration feature flag ([#3618](#3618)) ([7c175d9](7c175d9)) * **wcs:** update subscription expiration feature ([#3613](#3613)) ([ebf6e6d](ebf6e6d)) * **wcs:** update subscriptions expiration cli behavior ([#3617](#3617)) ([07e768c](07e768c)) ### Features * **subscriptions:** add cancellation reason metadata ([#3568](#3568)) ([de83e02](de83e02)) * **wc:** duplicate orders admin notice ([#3555](#3555)) ([cb764e3](cb764e3)) * **wcs:** add expired subscription cli tool ([#3593](#3593)) ([5d39398](5d39398)) * **webhooks:** filter request priority ([#3587](#3587)) ([1928a6a](1928a6a)) * **woocommerce-subscriptions:** add url redirect for wc subscription renewals ([#3525](#3525)) ([5b14aeb](5b14aeb)) ### Reverts * Revert "feat: command to initialize cron job to slowly backfill CAP term data (#3425)" (#3620) ([c9a9d45](c9a9d45)), closes [#3425](#3425) [#3620](#3620)
🎉 This PR is included in version 5.10.0-alpha.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [5.10.0](v5.9.2...v5.10.0) (2024-12-16) ### Bug Fixes * dont load textdomain too early ([#3629](#3629)) ([76c1f97](76c1f97)) * duplicate orders save on cron ([#3604](#3604)) ([ec69167](ec69167)) * hide duplicate notices if all was dismissed ([#3630](#3630)) ([cf48188](cf48188)) * **ras-acc:** re-add recaptcha to the WooCommerce checkout ([#3605](#3605)) ([f42a75b](f42a75b)) * **ras:** do not require Woo plugins if using NRH ([#3614](#3614)) ([363a834](363a834)) * **wcs:** remove subscriptions expiration feature flag ([#3618](#3618)) ([7c175d9](7c175d9)) * **wcs:** update subscription expiration feature ([#3613](#3613)) ([ebf6e6d](ebf6e6d)) * **wcs:** update subscriptions expiration cli behavior ([#3617](#3617)) ([07e768c](07e768c)) ### Features * **subscriptions:** add cancellation reason metadata ([#3568](#3568)) ([de83e02](de83e02)) * **wc:** duplicate orders admin notice ([#3555](#3555)) ([cb764e3](cb764e3)) * **wcs:** add expired subscription cli tool ([#3593](#3593)) ([5d39398](5d39398)) * **webhooks:** filter request priority ([#3587](#3587)) ([1928a6a](1928a6a)) * **woocommerce-subscriptions:** add url redirect for wc subscription renewals ([#3525](#3525)) ([5b14aeb](5b14aeb)) ### Reverts * Revert "feat: command to initialize cron job to slowly backfill CAP term data (#3425)" (#3620) ([c9a9d45](c9a9d45)), closes [#3425](#3425) [#3620](#3620)
🎉 This PR is included in version 5.10.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
All Submissions:
Changes proposed in this Pull Request:
Closes https://app.asana.com/0/1206274567818686/1208437232706180/f
This PR adds handling for
aa newnp_renewal
parammy-account/my-renewals
endpoint to redirect readers to the first available pending renewal checkout, otherwise the subscriptions my account page:How to test the changes in this Pull Request:
my-account/my-renewals
endpoint of your test site:site.url/my-account/my-renewals
Other information: