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

Generic Checkout: Invalid URL Param Price Checks #5955

Merged
merged 8 commits into from
Apr 24, 2024

Conversation

paul-daniel-dempsey
Copy link
Contributor

@paul-daniel-dempsey paul-daniel-dempsey commented Apr 23, 2024

What are you doing in this PR?

Missing functionality for the Generic Contributions Checkout

image image image

Trello Card

How to test

https://support.thegulocal.com/uk/contribute#ab-useGenericCheckout=variant

Part of #5722

Copy link
Contributor

github-actions bot commented Apr 23, 2024

Size Change: 0 B

Total Size: 1.79 MB

ℹ️ View Unchanged
Filename Size
./public/compiled-assets/javascripts/ausMomentMap.js 107 kB
./public/compiled-assets/javascripts/contributionsRedirectStyles.js 20 B
./public/compiled-assets/javascripts/digitalSubscriptionLandingPage.js 217 kB
./public/compiled-assets/javascripts/downForMaintenancePage.js 68 kB
./public/compiled-assets/javascripts/error404Page.js 68 kB
./public/compiled-assets/javascripts/error500Page.js 67.9 kB
./public/compiled-assets/javascripts/favicons.js 616 B
./public/compiled-assets/javascripts/paperSubscriptionCheckoutPage.js 168 kB
./public/compiled-assets/javascripts/paperSubscriptionLandingPage.js 84.8 kB
./public/compiled-assets/javascripts/payPalErrorPage.js 66.5 kB
./public/compiled-assets/javascripts/payPalErrorPageStyles.js 20 B
./public/compiled-assets/javascripts/promotionTerms.js 71.2 kB
./public/compiled-assets/javascripts/subscriptionsLandingPage.js 70.3 kB
./public/compiled-assets/javascripts/subscriptionsRedemptionPage.js 124 kB
./public/compiled-assets/javascripts/supporterPlusLandingPage.js 284 kB
./public/compiled-assets/javascripts/unsupportedBrowserStyles.js 20 B
./public/compiled-assets/javascripts/weeklySubscriptionCheckoutPage.js 188 kB
./public/compiled-assets/javascripts/weeklySubscriptionLandingPage.js 85 kB
./public/compiled-assets/webpack/363.js 18.2 kB
./public/compiled-assets/webpack/385.js 81.8 kB
./public/compiled-assets/webpack/735.js 2 kB
./public/compiled-assets/webpack/775.js 18 kB
./public/compiled-assets/webpack/991.js 2.16 kB

compressed-size-action

@paul-daniel-dempsey paul-daniel-dempsey requested a review from a team April 23, 2024 15:59
Comment on lines 230 to 232
if (queryPrice < 1) {
isInvalidAmount = true;
}
Copy link
Contributor

@jamesgorrie jamesgorrie Apr 24, 2024

Choose a reason for hiding this comment

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

Just to double check this - we don't allow e.g. 50p donations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think the above applies for Contributions

Copy link
Contributor

Choose a reason for hiding this comment

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

Happy for this to go in as is - but probably worth double checking as I'm not sure this is validated against anywhere else in the pipeline.

Copy link
Contributor Author

@paul-daniel-dempsey paul-daniel-dempsey Apr 24, 2024

Choose a reason for hiding this comment

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

Pretty sure old S+ checkout guarded against this low entry, not sure how can check the pipeline. Will put in for now and we can revisit if necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually if you change the Contribution amount in the paramUrl for the current checkout to £0.1 it jumps you straight to the S+ product at £10! Think marking it as invalid is fine for moment.

Copy link
Contributor

@jamesgorrie jamesgorrie left a comment

Choose a reason for hiding this comment

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

Just a question on why we have the const queryPrice

@paul-daniel-dempsey paul-daniel-dempsey changed the title Generic Checkout: Invalid URL Param Amount Checks Generic Checkout: Invalid URL Param Price Checks Apr 24, 2024
@paul-daniel-dempsey paul-daniel-dempsey merged commit cb51a83 into main Apr 24, 2024
15 checks passed
@paul-daniel-dempsey paul-daniel-dempsey deleted the generic-checkout-amounts-summary branch April 24, 2024 11:47
@prout-bot
Copy link

Seen on PROD (merged by @paul-daniel-dempsey 10 minutes and 57 seconds ago)

Sentry Release: support-client-side, support

@GHaberis
Copy link
Contributor

GHaberis commented Apr 24, 2024

  • Contributions amounts greater than the SupporterPlus threshold within non-contribution-only countries should be marked as isInvalidAmount

@paul-daniel-dempsey we've recently begun allowing amounts at or above the S+ threshold for users where isContributionsOnlyCountry is true, however we regard them as Recurring Contributions rather than S+. Could we do this on this checkout too? The logic lives here https://github.com/guardian/support-frontend/blob/main/support-frontend/assets/helpers/redux/checkout/product/selectors/isSupporterPlus.ts#L14

@jamesgorrie
Copy link
Contributor

@GHaberis from what I can tell in this PR @paul-daniel-dempsey has allowed exactly this by only checking it against the S+ amount if you are in contribution only countries here.

@paul-daniel-dempsey
Copy link
Contributor Author

paul-daniel-dempsey commented Apr 24, 2024

@GHaberis : we tested with Albania and it allowed a Contribution above the S+ threshold using: https://support.thegulocal.com/eu/checkout?country=AL&product=Contribution&ratePlan=Monthly&price=14
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants