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.

Sign up for GitHub

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-11605 Update billing address section #3775

Merged
merged 1 commit into from
Dec 20, 2023
Merged

PP-11605 Update billing address section #3775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions app/assets/javascripts/browsered/form-validation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ var init = function () {
window.Card,
window.Charge
)
var required = chargeValidations.required
var formFields = chargeValidations.required.concat(chargeValidations.optional)

form.addEventListener('submit', function (e) {
checkFormSubmission(e)
Expand Down Expand Up @@ -289,7 +289,7 @@ var init = function () {

var allFields = function () {
var fields = {}
required.forEach(function (requiredField) {
formFields.forEach(function (requiredField) {
var getField = findInputByKey(requiredField)
if (getField) {
fields[requiredField] = getField
Expand All @@ -300,7 +300,7 @@ var init = function () {

var allFieldValues = function () {
var values = {}
required.forEach(function (requiredField) {
formFields.forEach(function (requiredField) {
var getField = findInputByKey(requiredField)
if (getField) {
values[requiredField] = getField.value.trim()
Expand Down
1 change: 0 additions & 1 deletion app/controllers/charge.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ module.exports = {
}
const validator = chargeValidator(i18n.__('fieldErrors'), logger, cardModel, chargeOptions, getLoggingFields(req))

normalise.addressLines(req.body)
normalise.whitespace(req.body)

const { worldpay3dsFlexDdcStatus } = req.body
Expand Down
9 changes: 1 addition & 8 deletions app/services/normalise-charge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,7 @@ module.exports = (function () {
country: body.addressCountry
}
}
// body is passed by reference
const addressLines = function (body) {
if (!body.addressLine1 && body.addressLine2) {
body.addressLine1 = body.addressLine2
delete body.addressLine2
}
}
Comment on lines -72 to -78
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is:

Remove code that would combine address line 1 & 2 together. This code is no longer
required because each line now has its own field label.

However, it looks like that’s not quite what it was doing. What would happen was if there was no text for address line 1 but there was for address line 2, it would shift the address line 2 text up to address line 1. We’re losing that behaviour now. That might be fine but I want to check we understand what we’re doing and why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - we removed this code as I didn't want address line 2 shifting to address line 1 anymore.
As we now have validation for address line 1 and address line 2 individually.


const whitespace = function (body) {
const toIgnore = [
'submitCardDetails',
Expand Down Expand Up @@ -195,7 +189,6 @@ module.exports = (function () {
return {
charge,
addressForApi,
addressLines,
whitespace,
creditCard,
expiryDate,
Expand Down
116 changes: 69 additions & 47 deletions app/views/charge.njk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,20 +86,20 @@
<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">
<div id="error-summary" data-cy="error-summary" class="govuk-error-summary {% if not hasError %}hidden{% endif %}" aria-labelledby="error-summary-title" role="alert" tabindex="-1" data-module="error-summary">
<h2 class="govuk-error-summary__title" id="error-summary-title">
{{ __p("fieldErrors.summary") }}
</h2>
<div class="govuk-error-summary__body">
<ul class="govuk-list govuk-error-summary__list">
{% for currentErrorField in errorFields %}
<li>
<a href="#{{ currentErrorField.cssKey }}-lbl" id="{{ currentErrorField.cssKey }}-error">{{ currentErrorField.value }}</a>
</li>
{% endfor %}
</ul>
<div id="error-summary" data-cy="error-summary" class="govuk-error-summary {% if not hasError %}hidden{% endif %}" aria-labelledby="error-summary-title" role="alert" tabindex="-1" data-module="error-summary">
<h2 class="govuk-error-summary__title" id="error-summary-title">
{{ __p("fieldErrors.summary") }}
</h2>
<div class="govuk-error-summary__body">
<ul class="govuk-list govuk-error-summary__list">
{% for currentErrorField in errorFields %}
<li>
<a href="#{{ currentErrorField.cssKey }}-lbl" id="{{ currentErrorField.cssKey }}-error">{{ currentErrorField.value }}</a>
</li>
{% endfor %}
</ul>
</div>
</div>
</div>
<div id="wallet-options">
{% if (allowApplePay or allowGooglePay) and not savePaymentInstrumentToAgreement %}
<div class="web-payment-button-section govuk-!-margin-bottom-7">
Expand Down Expand Up @@ -381,43 +381,15 @@
<h2 class="govuk-heading-m govuk-!-margin-bottom-1 non-web-payment-button-section">{{ __p("cardDetails.billingAddress") }}</h2>
</legend>

<div id="address-hint" class="govuk-hint govuk-!-margin-bottom-6">{{ __p("cardDetails.billingAddressHint") }}</div>

<div class="govuk-form-group {% if highlightErrorFields.addressCountry %} govuk-form-group--error{% endif %}" data-validation="addressCountry">
<label id="address-country-lbl" for="address-country" class="govuk-label">
<span
class="address-country-label"
data-label-replace="addressCountry"
data-original-label="{{ __p("cardDetails.country") }}">
{{ __p("cardDetails.country") }}
</span>
</label>

{% if highlightErrorFields.addressCountry %}
<p class="govuk-error-message" id="error-address-country">
{{ highlightErrorFields.addressCountry }}
</p>
{% endif %}
<div id="address-hint" class="govuk-body govuk-!-margin-bottom-6">{{ __p("cardDetails.billingAddressHint") }}</div>

<select name="addressCountry" class="govuk-select" id="address-country" autocomplete="billing country-name">
{% if not countryCode | length and service and service.defaultBillingAddressCountry %}
{% set countryCode = service.defaultBillingAddressCountry %}
{% endif %}
{% if not countryCode %}
<option disabled value="" selected>Pick a country or territory</option>
{% endif %}
{% for country in countries %}
<option value="{{ country[1].split(':')[1] }}"{% if country[1].split(':')[1] === countryCode %} selected="selected"{% endif %}>{{ country[0] }}</option>
{% endfor %}
</select>
</div>
<div class="govuk-form-group address{% if highlightErrorFields.addressLine1 %} govuk-form-group--error{% endif %}" data-validation="addressLine1">
<label id="address-line-1-lbl" for="address-line-1" class="govuk-label">
<span
class="address-line-1-label"
data-label-replace="addressLine1"
data-original-label="{{ __p("cardDetails.building") }}">
{{ __p("cardDetails.building") }}
data-original-label="{{ __p("cardDetails.addressLine1") }}">
{{ __p("cardDetails.addressLine1") }}
</span>
</label>

Expand All @@ -434,16 +406,36 @@
class="govuk-input"
value="{{ addressLine1 }}"
autocomplete="billing address-line1"/>
<input id="address-line-2"
</div>

<div class="govuk-form-group address{% if highlightErrorFields.addressLine2 %} govuk-form-group--error{% endif %}" data-validation="addressLine2">
<label id="address-line-2-lbl" for="address-line-2" class="govuk-label">
<span
class="address-line-2-label"
data-label-replace="addressLine2"
data-original-label="{{ __p("cardDetails.addressLine2") }}">
{{ __p("cardDetails.addressLine2") }}
</span>
</label>

{% if highlightErrorFields.addressLine2 %}
<p class="govuk-error-message" id="error-address-line-2">
{{ highlightErrorFields.addressLine2 }}
</p>
{% endif %}

<input id="address-line-2"
type="text"
name="addressLine2"
maxlength="100"
class="govuk-input govuk-!-margin-top-2"
class="govuk-input"
data-last-of-form-group
value="{{ addressLine2 }}"
aria-label="Enter address line 2"
autocomplete="billing address-line2"/>
autocomplete="billing address-line2"
data-cy="address-line-2"/>
</div>

<div class="govuk-form-group{% if highlightErrorFields.addressCity %} govuk-form-group--error{% endif %} govuk-!-width-three-quarters" data-validation="addressCity">
<label id="address-city-lbl" for="address-city" class="govuk-label">
<span
Expand All @@ -468,6 +460,36 @@
value="{{ addressCity }}"
autocomplete="billing address-level2"/>
</div>

<div class="govuk-form-group {% if highlightErrorFields.addressCountry %} govuk-form-group--error{% endif %}" data-validation="addressCountry">
<label id="address-country-lbl" for="address-country" class="govuk-label">
<span
class="address-country-label"
data-label-replace="addressCountry"
data-original-label="{{ __p("cardDetails.country") }}">
{{ __p("cardDetails.country") }}
</span>
</label>

{% if highlightErrorFields.addressCountry %}
<p class="govuk-error-message" id="error-address-country">
{{ highlightErrorFields.addressCountry }}
</p>
{% endif %}

<select name="addressCountry" class="govuk-select" id="address-country" autocomplete="billing country-name">
{% if not countryCode | length and service and service.defaultBillingAddressCountry %}
{% set countryCode = service.defaultBillingAddressCountry %}
{% endif %}
{% if not countryCode %}
<option disabled value="" selected>Pick a country or territory</option>
{% endif %}
{% for country in countries %}
<option value="{{ country[1].split(':')[1] }}"{% if country[1].split(':')[1] === countryCode %} selected="selected"{% endif %}>{{ country[0] }}</option>
{% endfor %}
</select>
</div>

<div class="govuk-form-group{% if highlightErrorFields.addressPostcode %} govuk-form-group--error{% endif %} {% if shouldShowEmail %} govuk-!-margin-bottom-0{% endif %}" data-validation="addressPostcode">
<label id="address-postcode-lbl" for="address-postcode" class="govuk-label">
<span
Expand Down
15 changes: 8 additions & 7 deletions locales/cy.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@
"cardholderName": "Enw ar y cerdyn",
"billingAddress": "Cyfeiriad bilio",
"billingAddressHint": "Hwn yw’r cyfeiriad sy’n gysylltiedig â’r cerdyn",
"building": "Rhif neu enw’r adeilad a heol",
"addressLine1": "Llinell cyfeiriad 1",
"addressLine2": "Llinell cyfeiriad 2 (dewisol)",
"city": "Tref neu ddinas",
"postcode": "Cod post",
"country": "Gwlad neu diriogaeth",
Expand Down Expand Up @@ -143,14 +144,14 @@
"invalidYear": "Nid yw’r flwyddyn dod i ben hwn yn ddilys"
},
"addressLine1": {
"name": "enw/rhif yr adeilad a heol",
"message": "Rhowch gyfeiriad bilio dilys",
"containsTooManyDigits": "Rhowch gyfeiriad bilio dilys"
"name": "llinell cyfeiriad 1",
"message": "Rhowch linell cyfeiriad 1, fel arfer yr adeilad a'r stryd",
"containsTooManyDigits": "Rhowch linell cyfeiriad 1, fel arfer yr adeilad a'r stryd"
},
"addressLine2": {
"name": "llinell cyfeiriad ychwanegol",
"message": "Rhowch wybodaeth cyfeiriad dilys",
"containsTooManyDigits": "Rhowch wybodaeth cyfeiriad dilys"
"name": "llinell cyfeiriad 2",
"message": "Rhowch linell cyfeiriad 2",
"containsTooManyDigits": "Rhowch linell cyfeiriad 2"
},
"addressCity": {
"name": "dref/ddinas",
Expand Down
15 changes: 8 additions & 7 deletions locales/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
"cardholderName": "Name on card",
"billingAddress": "Billing address",
"billingAddressHint": "This is the address associated with the card",
"building": "Building number or name and street",
"addressLine1": "Address line 1",
"addressLine2": "Address line 2 (optional)",
"city": "Town or city",
"postcode": "Postcode",
"country": "Country or territory",
Expand Down Expand Up @@ -140,14 +141,14 @@
"invalidYear": "This expiry year is not a valid"
},
"addressLine1": {
"name": "building name/number and street",
"message": "Enter a valid billing address",
"containsTooManyDigits": "Enter a valid billing address"
"name": "address line 1",
"message": "Enter address line 1, typically the building and street",
"containsTooManyDigits": "Enter address line 1, typically the building and street"
},
"addressLine2": {
"name": "additional address line",
"message": "Enter valid address information",
"containsTooManyDigits": "Enter valid address information"
"name": "address line 2",
"message": "Enter address line 2",
"containsTooManyDigits": "Enter address line 2"
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m a bit concerned that this does not really tell people what the problem is (too many digits). It might trip people up entering foreign addresses. For example, in Germany, the postcode is written immediately before the city so it’s conceivable that someone could put it in the address line 2 input because that’s the one before the city… and German postcodes are all digits (but only five digits, so not enough to trigger our card number check). But since this is an existing issue I’m not going to hold up the review on this.

Copy link
Contributor Author

@iqbalgds iqbalgds Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good point. I didn't realise German postcodes work like that.
We noticed a few minor existing bugs with the validation and raised separate tickets for them.

},
"addressCity": {
"name": "town/city",
Expand Down
6 changes: 3 additions & 3 deletions test/cypress/integration/card/billing-address-collection.test.cy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ describe('Billing address collection', () => {
cy.log('should show the billing address section')

cy.get('h2').contains('Billing address').should('exist')
cy.get('#address-country-select').should('exist')
cy.get('#address-line-1').should('exist')
cy.get('#address-line-2').should('exist')
cy.get('#address-city').should('exist')
cy.get('#address-country-select').should('exist')
cy.get('#address-postcode').should('exist')

cy.log('Should populate default billing address from service')
Expand Down Expand Up @@ -87,10 +87,10 @@ describe('Billing address collection', () => {
cy.log('Should not show the billing address section')

cy.get('h2').contains('Billing address').should('not.exist')
cy.get('#address-country-select').should('not.exist')
cy.get('#address-line-1').should('not.exist')
cy.get('#address-line-2').should('not.exist')
cy.get('#address-city').should('not.exist')
cy.get('#address-country-select').should('not.exist')
cy.get('#address-postcode').should('not.exist')

cy.task('clearStubs')
Expand Down Expand Up @@ -139,10 +139,10 @@ describe('Billing address collection', () => {
cy.log('should not show the billing address section')

cy.get('h2').contains('Billing address').should('not.exist')
cy.get('#address-country-select').should('not.exist')
cy.get('#address-line-1').should('not.exist')
cy.get('#address-line-2').should('not.exist')
cy.get('#address-city').should('not.exist')
cy.get('#address-country-select').should('not.exist')
cy.get('#address-postcode').should('not.exist')

cy.log('should enter card details')
Expand Down
31 changes: 31 additions & 0 deletions test/cypress/integration/card/card-details-page-validation.test.cy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,37 @@ describe('Card details page validation', () => {
cy.get('.address-line-1-label').should('exist')
cy.get('.address-city-label').should('exist')
cy.get('.address-postcode-label').should('exist')

cy.log('No errors should be shown for the optional field address line 2')
cy.get('#address-line-2-error').should('not.exist')
cy.get('.address-line-2-label').should('exist')
})

it('Should perform validation of optional address line 2 correctly', () => {
cy.task('setupStubs', createPaymentChargeStubs)
cy.visit(`/secure/${tokenId}`)

cy.location('pathname').should('eq', `/card_details/${chargeId}`)
cy.window().its('chargeId').should('eq', `${chargeId}`)

cy.get('#card-details').submit()
cy.location('pathname').should('eq', `/card_details/${chargeId}`)

cy.log('No errors should be shown for the optional field address line 2 as the field is not being used')
cy.get('#address-line-2-error').should('not.exist')
cy.get('.address-line-2-label').should('exist')

cy.log('Should error when address line 2 contains an invalid data')
cy.get('[data-cy=address-line-2]').type('1111111111111111')
cy.get('#card-details').submit()

cy.get('#address-line-2-error').should('exist')

cy.log('Should not sure an error when the optional address line 2 is cleared')
cy.get('[data-cy=address-line-2]').clear()
cy.get('#card-details').submit()

cy.get('#address-line-2-error').should('not.exist')
})

it('Should perform inline validation of the expiry date correctly', () => {
Expand Down
4 changes: 2 additions & 2 deletions test/integration/card-details-errors.ft.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,8 @@ describe('chargeTests', function () {
expect($('#error-cardholder-name').text()).to.contains('Enter the name as it appears on the card')
expect($('#cvc-error').text()).to.contains(errorMessages.cvc)
expect($('#error-cvc').text()).to.contains(errorMessages.cvc)
expect($('#address-line-1-error').text()).to.contains('Enter a valid building name/number and street')
expect($('#error-address-line-1').text()).to.contains('Enter a valid billing address')
expect($('#address-line-1-error').text()).to.contains('Enter a valid address line 1')
expect($('#error-address-line-1').text()).to.contains('Enter address line 1, typically the building and street')
expect($('#address-city-error').text()).to.contains(errorMessages.city)
expect($('#error-address-city').text()).to.contains(errorMessages.city)
expect($('#address-postcode-error').text()).to.contains(errorMessages.postcode)
Expand Down
Loading