Skip to content
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): add post checkout ras settings #3533

Merged
merged 8 commits into from
Nov 8, 2024

Conversation

chickenn00dle
Copy link
Contributor

@chickenn00dle chickenn00dle commented Nov 7, 2024

All Submissions:

Changes proposed in this Pull Request:

This PR adds post checkout success message settings to the Checkout Configuration section of the engagement wizard

Forced checkout registration enabled:
Screenshot 2024-11-08 at 12 12 52

And disabled:
Screenshot 2024-11-08 at 12 12 57

How to test the changes in this Pull Request:

  1. As admin, go to Newspack > Engagement > Show Advanced Settings
  2. Scroll to the Checkout Configuration section
  3. Verify the new fields are present (pictured above)
  4. Edit and save the two fields
  5. Verify your change persist through a refresh

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 ran tests with your changes locally?

@chickenn00dle
Copy link
Contributor Author

@thomasguillot assigning you to review the post-checkout registration success text

@leogermani
Copy link
Contributor

@thomasguillot do you think we need some further explanation in this section? The option to not require registration before checkout gives the impression that if you don't enable it the user won't register.

Maybe we can start by saying that a user account will always be created for readers who make purchases. And then there's the option to require the registration upfront - otherwise the registration will happen behind the scenes... in which case that post-checkout message will be displayed...

Maybe things can be a little better organized there?

@chickenn00dle chickenn00dle added the [Status] Needs Review The issue or pull request needs to be reviewed label Nov 8, 2024
@leogermani
Copy link
Contributor

I'm confused with what to expect there in the text areas.

I thought that the "Post-checkout registration success message" was needed for when we register the user in the background, so we need to inform them that an account was created, and the plain "Post-checkout success message" for when they have explicitly created an account before the checkout.

I tested both cases and I got the same message in the post-checkout (the "Post-checkout success message"). The other message was never displayed anywhere...

I wonder if we ever need to display both options. Isn't it one post-checkout message for each case, so we should see one if the toggle is ON and the other if it's OFF?

Also, what do you think of adding an additional explanation to the toggle description:

Prompt users who are not logged in to sign in or register a new account before proceeding to checkout.
(If turned off, a user account will be created with the email used in the checkout)

@chickenn00dle
Copy link
Contributor Author

Thanks for the review @leogermani and sorry for the delay with getting this updated. Rendering the checkout registration message when forced registration was off require some small changes with how we set and clear the registration session flag.

I thought that the "Post-checkout registration success message" was needed for when we register the user in the background, so we need to inform them that an account was created, and the plain "Post-checkout success message" for when they have explicitly created an account before the checkout.

You are right! This was my mixup 🙇 This has been fixed.

I tested both cases and I got the same message in the post-checkout (the "Post-checkout success message"). The other message was never displayed anywhere...

To be sure, this PR only adds the new settings to the wizard. You will also need to checkout Automattic/newspack-blocks#1941 to see the changes applied to modal checkout

I wonder if we ever need to display both options. Isn't it one post-checkout message for each case, so we should see one if the toggle is ON and the other if it's OFF?

When forced registration is on, we only need to display one option since we will never need to show the checkout registration message. But when forced registration is off, we want to be able to show the non-registration message for already logged in readers, or logged out readers that us an email address already associated with an account.

Also, what do you think of adding an additional explanation to the toggle description

Good idea! I've updated the help text in c3eada0

Let me know if this works.

Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

Working as expected.

I have one minor suggestion on the copy:

Where we read "The success message to display to readers that have created a new account during checkout."
I think we should change to something that represents that this was a passive action. The readers didn't create an account, we created an account for them... so something like:
"The success message to display to readers that had an account created during checkout."
or something in the lines.. maybe even more explicit "an account created under the hood during checkout and need to be informed about it..."


and then I saw a few things that I don't think are related to this PR

The "Save payment information" checkbox is glued together without any spacing to the cover fees checkbox:

image

When making a purchase, without requiring registration upfront, but using an existing email, there is some noise in the modal along with the post-checkout message: (the "Thank you" and the "please login" messages)

image

@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Nov 8, 2024
@chickenn00dle
Copy link
Contributor Author

Good call @leogermani!

I updated this to "The success message to display to new readers that have an account automatically created after completing checkout"

I'll merge this one as is since it was approved and if you have further feedback on the text I'll get another PR to update.

@chickenn00dle chickenn00dle merged commit d5d05cb into epic/ras-acc Nov 8, 2024
7 checks passed
@leogermani leogermani deleted the feat/add-post-checkout-ras-setting branch November 8, 2024 22:20
@chickenn00dle
Copy link
Contributor Author

Created asana tasks for the unrelated bugs:

  • 1207817176293825-as-1208730419008814
  • 1207817176293825-as-1208730419008818
  • 1207817176293825-as-1208730419008823

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ras-acc testing [Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants