-
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(ras-acc): add standalone newsletter signup form #3120
feat(ras-acc): add standalone newsletter signup form #3120
Conversation
19005db
to
8dd3017
Compare
29ce0f2
to
1b5a171
Compare
Looks good! The only issue I'm seeing with this modal step specifically is that there should be 4px padding between the "Get [publication name] directly…" and "Sending to …" lines. |
Thanks @kevinzweerink! I'll get a PR up for adjusting the spacing between these lines tomorrow. Since this is the only design feedback, I'll go ahead and mark this for code review. |
540bfda
to
42f366e
Compare
Noting I've updated the spacing between the two lines of copy mentioned in the comment above in 42f366e |
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.
Thanks for tackling this one, @chickenn00dle! 🙌
The newsletter step looks good when I can trigger it, but it doesn't seem to work as described when I run a donation:
If I use an incognito window and run a donation using the donate block, I get the screens to create an account, add billing details, add payment details, and then success. Rerunning it again from the same spot (without refreshing) also doesn't show the newsletter step.
However, if I refresh the page after account creation -- either after running through a whole donation, or just refreshing the browser after completing the 'Create Account' step -- I do get the prompt.
The sign ups are successfully being synced to MailChimp for me.
When I re-run the checkout as a user that's already signed up for a newsletter, that one will appear pre-selected at the checkout step, which was a little unexpected (I thought it would go away; and unselecting it and submitting the form doesn't unsubscribe you). Once I've subscribed to all, though, the form stops showing!
Just let me know if you have questions about any of this; I have a test site that can be used!
33fb5f1
to
fc8ccc6
Compare
Thanks @laurelfulford!
I think I've found the issue. Looks like the newsletters modal markup required a valid email address in order to fetch and filter email lists in the form. But when the user is not logged in, there is no valid email address yet, so the newsletters modal markup would not render until the page has been reloaded AFTER the user went through the sign in process. I've made some changes in fc8ccc6 to allow the modal to render when there is no email address with all available lists |
This shouldn't be happening I've reworked the logic for filtering current lists for a given reader in 76e8a34 That said, there are two caveats:
|
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.
Thanks @chickenn00dle! These updates seem to have fixed the initial sign up behaviour, but are causing the subscriptions not to "take" for me:
I've made some changes in fc8ccc6 to allow the modal to render when there is no email address with all available lists
This fixed when the newsletter form shows for me! It displays during the initial sign up.
However, when I subscript to a newsletter, it doesn't actually subscribe it -- it doesn't show up as subscribed as that user under My Account > Newsletters, and it doesn't get sent to MailChimp (unless I have the test site set to send it otherwise, like with any reader revenue transactions -- and then, it's the reader info, but not with any newsletter lists).
I don't think this has to do with double-opt-in, because I think I've set this up the same way as the last time I tested it, but let me know if it sounds like I'm screwing something up! I have this happening on a public-facing test site I can send you creds for.
Edited to add: When I do subscribe to lists using the My Account > Newsletters screen, the modal correctly only shows the list I haven't subscribed to there yet, so that bit is also fixed!
I think it will be pretty uncommon for a reader to make back to back purchases without navigating around the site, so this IMO, this is probably okay.
I agree -- it's a very QAing-a-site path to take, not an actual reader-making-a-donation path at all 🙂
76e8a34
to
2aa80b2
Compare
I found out why this is happening. It's related to how WordPress uses user ID and session data to generate and verify nonces. For cases when a user attempts checkout while not logged in, because the form is rendered (hidden) before the reader actually signs up, but the newsletter signup form is submitted after the reader signs up, the created nonce will fail verification. I'm not sure what the best approach to handling this mismatched login state when handling the nonce, but for now I've gone with a hidden input similar to how we handle the auth form submission in 2aa80b2 Let me know if you know of a better approach for this! |
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.
This is working really well, @chickenn00dle! I didn't run into any issues aside from the watch
error we discussed in #3152. You don't necessarily need to solve the issue that way, but I'd like to resolve it somehow before merging this. Other than that, this looks good to go.
@@ -1310,6 +1363,148 @@ public static function render_auth_modal() { | |||
<?php | |||
} | |||
|
|||
|
|||
/** |
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.
Non-blocking nit: I'd almost consider splitting these new newsletter-related methods into their own class inside the reader-activation
folder, since there may be more to add to this in the future and the class-reader-activation.php file is getting quite large.
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.
This is a good idea, although my preference would be to wait and see if we really do end up adding more newsletters related methods before doing this.
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.
Thanks @chickenn00dle! 🙌
In re-testing this, things are getting close! Sign ups are correctly syncing to MailChimp for me, and newsletters correctly become unavailable in the checkout flow as the reader signs up for them.
The only other issue I'm seeing is in the final bit of the testing steps: when I set the checkout button block to redirect to a different page on success, it doesn't redirect, the modal just closes. This happens when the newsletter coda is enabled and all the newsletters have already been signed up for, and also when it's disabled.
I don't think this is the intended behaviour right now, but I wonder if it should also redirect when the newsletters sign up step is shown, since it's something we're letting publishers control? (That might be getting into the weeds a bit though!!)
Just let me know if you have any questions about this, or can't recreate!
2aa80b2
to
bb7bcf6
Compare
Thanks for the review @dkoo and @laurelfulford!
I found the root cause of the
I did find an issue when the checkout button behavior was set to go back to the previous page after success and got a fix up for that in Automattic/newspack-blocks@e19da43 @laurelfulford. However, on my end the modal checkout flow was respecting the Custom URL and Close modal settings when the newsletters signup was disabled as well as when the reader was already signed up to all non-premium lists. Here are screen recordings after applying the previous page fix. Signups disabled: Screen.Recording.2024-05-31.at.13.05.34.movSignups enabled, but user has signed up for all newsletters listed: Screen.Recording.2024-05-31.at.13.07.19.mov |
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.
Thanks for double-checking that, @chickenn00dle!
I retested, and I think I've narrowed it down to a really specific case:
When I test with a regular product, the redirect to the previous page and a specific page both worked great!
It's only when I test with a product with variations that I see the issue: none of the customizations (button text, or redirect behaviour) get applied, so I think it's just something missing from that specific checkout.
Please let me know if you still can't recreate it with a variable product -- if not and since it also looks good to Derrick, this could just be something weird about how I'm testing this!
Great info, @laurelfulford—I missed this in my review. Thanks for bumping it! |
Good catch @laurelfulford! Fixed in Automattic/newspack-blocks@3d3f825 |
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.
Awesome, thanks @chickenn00dle!
I re-ran a regular product and a variable product, and confirmed that the custom text works, redirects work, and newsletters work (display the ones you haven't subscribed to and when you sign up for one during checkout, they get successfully sent to Mailchimp). 🙌
🎉 This PR is included in version 4.1.0-epic-ras-acc.5 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 4.2.0-epic-ras-acc.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 4.4.0-epic-ras-acc.1 🎉 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/1206943664367857/1205668937700578/f
This PR is paired with the following blocks PR: Automattic/newspack-blocks#1737
This PR separates the post-checkout newsletters signup form from the checkout thank you modal.
This PR also moves logic for rendering and handling the newsletters signup form from the blocks plugin over to the main Newspack plugin since the form only renders whenever RAS is active. (Let me know if there is a preference to move this back to blocks)
How to test the changes in this Pull Request:
Before testing, be sure to also checkout the changes in Automattic/newspack-blocks#1737
Other information: