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

HOFF-160: Create postcode lookup component #466

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

module.exports = {
addressLookup: require('./address-lookup'),
postcodeLookup: require('./postcode-lookup'),
clearSession: require('./clear-session'),
combineAndLoopFields: require('./combine-and-loop-fields'),
date: require('./date'),
Expand Down
31 changes: 31 additions & 0 deletions components/postcode-lookup/defaults.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/* eslint max-len: 0, no-process-env: 0 */

Copy link
Contributor

Choose a reason for hiding this comment

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

either the naming of the file or the exported object structure can be improved. It looks like the file is just default error messages, so you can either rename the file to something like defaultErrorMessages.js or change the structure of the object returned to something like:

`module.exports = {
errorMessages: {
CANT_FIND: '...',
...;
}

}`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've now moved the variables linked to an issue with the postcode lookup to within the POSTCODE_ERROR structure within this file. The rest of the properties are not issues with the lookup, so have kept them as independent variables.

'use strict';

module.exports = {
CANT_FIND: 'I cannot find the correct address',
CHANGE: 'Change postcode',
SEARCH_BY_POSTCODE: 'Search address by postcode',
POSTCODE_HEADING: 'What is your UK postcode?',
POSTCODE_LABEL: 'Postcode',
POSTCODE_HINT: 'Enter a full UK postcode, for example AA3 1AB',
ADDRESS_LOOKUP_HEADING: 'Select the main applicant’s address',
MANUAL_ADDRESS_HEADING: 'Enter the main applicant’s address',
MANUAL_ADDRESS_PARAGRAPH: 'This must match the applicant’s address in Home Office records.',
SEARCH_ERROR_HEADING: 'Sorry, there is a problem with the postcode search',
SELECT_LABEL: 'Select the address',
ADDRESS_LINE_1_LABEL: 'Address line 1',
ADDRESS_LINE_2_LABEL: 'Address line 2 (optional)',
TOWN_OR_CITY_LABEL: 'Town or city',
POSTCODE_MANUAL_LABEL: 'Postcode',
POSTCODE_ERROR: {
'not-found': 'Sorry – we couldn’t find any addresses for that postcode, enter your address manually',
'cant-connect': 'Sorry – we couldn’t connect to the postcode lookup service at this time, enter your address manually'
},
NO_ADDRESS_HEADING: 'No address found',
ENTER_MANUALLY: 'Enter address manually',
ADDRESS_LOOKUP_SEARCH_TITLE: 'Postcode lookup search - GOV.UK',
ADDRESS_LOOKUP_TITLE: 'Address lookup - GOV.UK',
ADDRESS_DETAILS_TITLE: 'Address details - GOV.UK',
ADDRESS_LOOKUP_PROBLEM_TITLE: 'Address lookup problem - GOV.UK'
};
339 changes: 339 additions & 0 deletions components/postcode-lookup/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,339 @@
/* eslint-disable func-names */
'use strict';

const axios = require('axios');
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't see axios included in package.json? How is this working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it back into package.json with the same version in master

const path = require('path');
const querystring = require('querystring');
const _ = require('lodash');

const defaults = require('./defaults');

const conditionalTranslate = (key, t) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use more meaningful variable name, think about the next dev looking at this code, what is t? what is the key for? Why does the logic need to compare the value in t indexed by key with the key itself?

Think about how you name your functions and variables, most of the time, if you named them well, no need for documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the variable name to something more suitable.

let result = t(key);
if (result === key) {
result = null;
}
return result;
};

const getFields = (key, t, requiredValidate, req) => ({
[`${key}-postcode`]: {
label: conditionalTranslate(`fields.${key}-postcode.label`, t) || defaults.POSTCODE_LABEL,
labelClassName: 'visuallyhidden',
hint: conditionalTranslate(`fields.${key}-postcode.hint`, t) || defaults.POSTCODE_HINT,
mixin: 'input-text',
validate: requiredValidate ? ['required', 'postcodeValidation'] : 'postcodeValidation',
formatter: 'uppercase',
className: ['govuk-input', 'govuk-input--width-10']
},
[`${key}-select`]: {
label: conditionalTranslate(`fields.${key}-select.label`, t) || defaults.SELECT_LABEL,
mixin: 'radio-group',
validate: 'required',
options: req.sessionModel.get('addressesOptions')
},
[key]: {
label: conditionalTranslate(`fields.${key}.label`, t) || defaults.ADDRESS_LINE_1_LABEL,
mixin: 'input-text',
validate: 'required',
labelClassName: 'bold'
},
[`${key}-address-line-2`]: {
label: conditionalTranslate(`fields.${key}-address-line-2.label`, t) || defaults.ADDRESS_LINE_2_LABEL,
mixin: 'input-text',
'ignore-defaults': true,
labelClassName: 'bold'
},
[`${key}-town-or-city`]: {
label: conditionalTranslate(`fields${key}-town-or-city.label`, t) || defaults.TOWN_OR_CITY_LABEL,
mixin: 'input-text',
validate: 'required',
'ignore-defaults': true,
labelClassName: 'bold'
},
[`${key}-postcode-manual`]: {
label: conditionalTranslate(`fields.${key}-postcode-manual.label`, t) || defaults.POSTCODE_MANUAL_LABEL,
mixin: 'input-text',
validate: requiredValidate ? ['required', 'postcodeValidation'] : 'postcodeValidation',
formatter: 'uppercase',
className: ['govuk-input', 'govuk-input--width-10'],
labelClassName: 'bold'
}
});

const getConfig = key => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not really a key, it is the value of the field name prefix, I would name this variable differently to avoid confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed variable name.

postcode: {
fields: [`${key}-postcode`],
template: 'postcode'
},
lookup: {
fields: [`${key}-select`],
template: 'address-lookup'
},
manual: {
fields: [`${key}`, `${key}-address-line-2`, `${key}-town-or-city`, `${key}-postcode-manual`],
template: 'address'
},
postcodeSearchIssue: {
fields: [],
template: 'postcode-search-problem'
}
});

module.exports = config => {
let postcode = '';
const apiKey = config.apiKey;
const apiURL = config.apiURL;
const required = config.required;
const addressKey = config.addressKey;
Copy link
Contributor

Choose a reason for hiding this comment

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

not really addressKey, it is the address field names prefix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed name of constant.


return superclass => class extends superclass {
configure(req, res, callback) {
req.query.step = req.query.step || 'postcode';
const subSteps = getConfig(addressKey);
const step = subSteps[req.query.step];
_.merge(req.form.options, {
subSteps,
addressKey,
fields: _.pick(getFields(addressKey, req.translate, required, req), step.fields),
apiError: null
});
if (step.template) {
req.form.options.template = path.resolve(__dirname, `./templates/${step.template}.html`);
}
super.configure(req, res, callback);
}

getNextStep(req, res, callback) {
const step = super.getNextStep(req, res, callback);

if (req.query.step === 'postcode' && req.form.values[`${addressKey}-postcode`]) {
let nextSubStep = '';
// Go to manual entry page with pre-populated fields
nextSubStep = req.sessionModel.get(`${addressKey}-addresses`) ? 'lookup' : 'address';
const qs = querystring.stringify(_.merge({}, req.query, {
step: nextSubStep
}));
return `?${qs}`;
} else if (req.query.step === 'lookup' && req.sessionModel.get(`${addressKey}-select`) !== '') {
req.form.values[`${addressKey}-select`] = req.form.options.fields[`${addressKey}-select`].apiAddress;
let nextSubStep = '';
nextSubStep = 'manual';
req.query.step = 'manual';
req.form.values.address = '';
const qs = querystring.stringify(_.merge({}, req.query, {
step: nextSubStep
}));
return `?${qs}`;
}
return step;
}

getValues(req, res, callback) {
if (req.query.step === 'manual') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a better code structure, instead of keep adding if else if, use an array of all the steps that needs to be checked as the key, and the function that needs to be called as the value, this array is like a mapping definition, then just loops through the array and check each key against the step, if it matches, execute the value as a function. Pseudo code:

const stepValueProcessingMap = { 'manual': function() {}, 'lookup': function(){}, }
for (let prop in stepValueProcessingMap) { stepValueProcessingMap.prop(); }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, no longer needed since there is only one if statement in this block after removing the other one that wasn't needed.

req.sessionModel.unset([
`${addressKey}-postcodeApiMeta`
]);
} else if (req.query.step === 'lookup') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe refactoring this part with sub functions, essentially all this function is doing:

  • process and build the address options
  • create the select control that uses the options created above

Would be easier to ready to have each of the bullet point as a function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored this logic into two separate functions.

const addresses = req.sessionModel.get(`${addressKey}-addresses`);
req.sessionModel.set('addressesOptions', addresses.map(obj => {
const addressValue = obj.ADDRESS;

// Different addresses have different properties returned by the API
let addressLine1 = '';
let addressLine2 = '';
if (obj.ORGANISATION_NAME !== undefined && obj.BUILDING_NAME !== undefined &&
Copy link
Contributor

Choose a reason for hiding this comment

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

refactoring this into its own method called buildingAddressLineValue would help people reading to understand what you are trying to do here, also maybe worth discussing how to simplify this logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored this logic into a separate function.

obj.BUILDING_NUMBER !== undefined) {
addressLine1 = obj.ORGANISATION_NAME + ', ' + obj.BUILDING_NAME;
addressLine2 = obj.BUILDING_NUMBER + ' ' + obj.THOROUGHFARE_NAME;
} else if (obj.ORGANISATION_NAME !== undefined && obj.BUILDING_NAME !== undefined) {
addressLine1 = obj.ORGANISATION_NAME + ', ' + obj.BUILDING_NAME;
addressLine2 = obj.THOROUGHFARE_NAME;
} else if (obj.ORGANISATION_NAME !== undefined && obj.BUILDING_NUMBER !== undefined) {
addressLine1 = obj.ORGANISATION_NAME;
addressLine2 = obj.BUILDING_NUMBER + ' ' + obj.THOROUGHFARE_NAME;
} else if (obj.BUILDING_NAME !== undefined && obj.THOROUGHFARE_NAME !== undefined) {
addressLine1 = obj.BUILDING_NAME + ' ' + obj.THOROUGHFARE_NAME;
} else {
addressLine1 = obj.BUILDING_NUMBER + ' ' + obj.THOROUGHFARE_NAME;
}

const townOrCity = obj.POST_TOWN;
// Get element ID to be used in determining if radio button is checked in the saveValues section
const radioButtonId = `${addressKey}-select-${obj.ADDRESS}`;
return {
elementId: radioButtonId,
label: addressValue,
value: addressValue,
addressLine1: addressLine1,
addressLine2: addressLine2,
townOrCity: townOrCity
};
}));

req.form.options.fields[`${addressKey}-select`] = {
mixin: 'radio-group',
legend: {
className: 'visuallyhidden'
},
className: 'govuk-radios--inline',
validate: ['required'],
options: req.sessionModel.get('addressesOptions')
};
req.sessionModel.set('addressCount', addresses.length);
}
super.getValues(req, res, callback);
}

locals(req, res, callback) {
const isManual = req.query.step === 'manual';
const locals = super.locals(req, res, callback);
const sessionPostcode = req.sessionModel.get(`${addressKey}-postcode`);
const section = this.options.route.replace(/^\//, '');
const editLink = conditionalTranslate('pages.address-lookup.edit', req.translate) || defaults.CHANGE;
const searchByPostcodeLink = conditionalTranslate('pages.address.searchByPostcode', req.translate) ||
defaults.SEARCH_BY_POSTCODE;
const cantFind = conditionalTranslate('pages.address-lookup.cantfind', req.translate) || defaults.CANT_FIND;

let postcodeApiMessageKey;
let postcodeError;

if (!isManual) {
postcodeApiMessageKey = (req.sessionModel.get(`${addressKey}-postcodeApiMeta`) || {}).messageKey;
Copy link
Contributor

Choose a reason for hiding this comment

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

just want to make sure I understand this correctly, if the session api meta is not there, this variable is set to the messageKey property of an empty object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the postcodeApiMeta value altogether now since we are not displaying an error on the screen, but instead redirecting to an error screen using the response codes returned by the API

} else {
req.form.values['postcode-manual'] = sessionPostcode;
}

if (postcodeApiMessageKey) {
const key = `pages.address-lookup.postcode-api.${postcodeApiMessageKey}`;
postcodeError = conditionalTranslate(key, req.translate) ||
defaults.POSTCODE_ERROR[postcodeApiMessageKey];
}

let addressCount = '';
if (req.sessionModel.get('addressCount') === 1) {
addressCount = '1 address for ';
} else {
addressCount = req.sessionModel.get('addressCount') + ' addresses for ';
}

return _.merge({}, locals, {
enterManually: conditionalTranslate('pages.address.lookup.enter-manually', req.translate) ||
defaults.ENTER_MANUALLY,
postcodeHeading: conditionalTranslate('pages.postcode.postcode-heading', req.translate) ||
defaults.POSTCODE_HEADING,
addressLookupHeading: conditionalTranslate('pages.address-lookup.address-lookup-heading', req.translate) ||
defaults.ADDRESS_LOOKUP_HEADING,
manualAddressHeading: conditionalTranslate('pages.address-lookup.manual-address-heading', req.translate) ||
defaults.MANUAL_ADDRESS_HEADING,
manualAddressParagraph: conditionalTranslate('pages.address-lookup.manual-address-paragraph', req.translate) ||
defaults.MANUAL_ADDRESS_PARAGRAPH,
searchErrorHeading: conditionalTranslate('pages.postcode-search-problem.searchErrorHeading', req.translate) ||
defaults.SEARCH_ERROR_HEADING,
postcodeEntered: sessionPostcode,
addressesExist: (req.sessionModel.get('addresses') !== undefined &&
req.sessionModel.get('addresses').length > 0) ? true : false,
noAddressHeading: conditionalTranslate('pages.postcode-search-problem.noAddressHeading', req.translate) ||
defaults.NO_ADDRESS_HEADING,
addressCount: addressCount,
postcodeSearchTitle: conditionalTranslate('pages.postcode-search.title', req.translate) ||
defaults.ADDRESS_LOOKUP_SEARCH_TITLE,
addressLookupTitle: conditionalTranslate('pages.address-lookup.title', req.translate) ||
defaults.ADDRESS_LOOKUP_TITLE,
addressDetailsTitle: conditionalTranslate('pages.address-details.title', req.translate) ||
defaults.ADDRESS_DETAILS_TITLE,
lookupProblemTitle: conditionalTranslate('pages.address-lookup-problem.title', req.translate) ||
defaults.ADDRESS_LOOKUP_PROBLEM_TITLE,
route: this.options.route,
editLink,
cantFind,
searchByPostcodeLink,
postcodeError,
section
});
}

postcode(req, res, callback) {
// Clear the value stored in the addresses radio button group
req.sessionModel.set(`${addressKey}-select`, '');
// Call OS Places API to return list of addresses by postcode
const enteredPostcode = req.form.values[`${addressKey}-postcode`];

axios.get(apiURL + '?postcode=' + enteredPostcode + '&key=' + apiKey)
.then(function (response) {
const addressList = [];
_.forOwn(response.data.results, function (value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do some checks on the response, make sure it is of the expected structure & type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check for the 'data' property in the response, but didn't check for any sub-properties of that because the component needs to still go to the address lookup page when there are no results returned for a given postcode.

addressList.push(value.DPA);
});

req.sessionModel.set(`${addressKey}-addresses`, addressList);
return callback();
}).catch(error => {
if(error.response.status === '404' || error.response.status === '429' ||
error.response.status === '503' || error.response.status === '500') {
req.query.step === 'postcodeSearchIssue';
Copy link
Contributor

Choose a reason for hiding this comment

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

too generic, are there detailed info on the API documentation on what the errors means?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added logger within the block of code where the function is called and got rid of the 'if' block from the code above to remove duplication. Also, modified the logic so that instead of checking for these specific error codes, it will now log the error regardless of the error code.

}
return callback(error);
});
}

setupRadioButton(req, addresses) {
req.form.options.fields[`${addressKey}-addresses`] = {
mixin: 'radio-group',
isPageHeading: true,
validate: ['required'],
options: addresses.map(obj => {
const addressValue = obj.ADDRESS;
return {
label: addressValue,
value: addressValue
};
})
};
}

saveValues(req, res, callback) {
const step = req.query.step;
if (step === 'postcode') {
postcode = req.form.values[`${addressKey}-postcode`];
req.form.values[`${addressKey}`] = '';
req.form.values[`${addressKey}-address-line-2`] = '';
req.form.values[`${addressKey}-town-or-city`] = '';
req.form.values[`${addressKey}-postcode-manual`] = '';
return this.postcode(req, res, err => {
if (err) {
req.sessionModel.set('addresses', []);
return res.redirect(req.baseUrl + '/postcode?step=postcodeSearchIssue');
}
return super.saveValues(req, res, callback);
});
} else if (step === 'lookup') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to suggestion above, see if you can think of a way to avoid using if else if else if structure and a switch statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, moved the logic in the if statements to separate functions for clarity.

const addressResult = req.form.options.fields[`${addressKey}-select`].options.filter(obj => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should do some sanitisation of the input values gathered, strip out dodgy characters like < , {, ! and ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added logic to remove any instances of those characters

return obj.value === req.form.values[`${addressKey}-select`];
});
const addressLine1 = addressResult[0].addressLine1;
const addressLine2 = addressResult[0].addressLine2;
const townOrCity = addressResult[0].townOrCity;
req.form.values[`${addressKey}`] = addressLine1;
req.form.values[`${addressKey}-address-line-2`] = addressLine2;
req.form.values[`${addressKey}-town-or-city`] = townOrCity;
req.form.values[`${addressKey}-postcode-manual`] = postcode;
req.sessionModel.set('lookup-postcode', postcode);
req.sessionModel.set('lookup-address-line-1', addressLine1);
req.sessionModel.set('lookup-address-line-2', addressLine2);
req.sessionModel.set('lookup-town-or-city', townOrCity);

req.form.values[`${addressKey}-select`] = req.form.options.fields[`${addressKey}-select`].apiAddress;
}
return super.saveValues(req, res, callback);
}

getBackLink(req, res) {
let backLink = super.getBackLink(req, res);
if (_.includes(['manual', 'address', 'lookup'], req.query.step)) {
backLink = req.baseUrl + req.path;
}
return backLink;
}
};
};
Loading
Loading