-
Notifications
You must be signed in to change notification settings - Fork 12
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
PP-13377 show test payment notification on card payment screens #3945
Conversation
- update charge retrieval middleware to make `isTestPayment` available to all views - show phase banner with test status when charge is for a test account - show notification banner on card input and confirmation screens when charge is for a test account
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.
looks good, some very minor comments
@@ -85,7 +85,8 @@ | |||
<div class="govuk-width-container govuk-main-wrapper"> | |||
<div class="govuk-grid-row govuk-!-margin-bottom-9"> | |||
<main class="charge-new govuk-grid-column-two-thirds" id="main-content" role="main"> | |||
<div class="charge-new__content"> | |||
{% include "includes/test-payment-notification-banner.njk" %} | |||
<div class="charge-new__content"> |
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.
you've added a div here but you haven't added a closing tag - is that deliberate?
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 div is just moved, not removed?
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.
ha, yes clearly it is.
cy.location('pathname').should('eq', `/card_details/${chargeId}`) | ||
shouldCheckPhaseBanner() | ||
shouldCheckNotificationBanner() | ||
cy.task('clearStubs') |
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.
I think it would be easier to follow if there was a line break each time the stubs are cleared, but that may be a personal thing so I wouldn't hold it back for that reason
WHAT
isTestPayment
available to all viewsHOW
test-payment-notification
test to see the difference between live and testSCREENS
LIVE (no change)
TEST (new banners)