HOFF-160: Create postcode lookup component by mislam987 · Pull Request #466 · UKHomeOfficeForms/hof

mislam987 · 2024-08-12T22:26:29Z

What?

Create the postcode lookup component as a reusable solution, using the OS Places API - (HOFF-754, HOFF-768, HOFF-770, & HOFF-771)

Also includes a bug fix to allow punctuations in postcodes - HOFF-986

Why?

To replace the existing postcode lookup functionality across HOF forms
To have a configurable solution to enable customisations for each form
To enable users to enter a postcode and see a list of addresses to select from

How?

created new component folder /components/postcode-lookup including:
- /templates folder with the postcode lookup HTML views
- defaults.js file to include the default values used in the component
- index.js file to handle the logic behind the postcode lookup (configuring fields, routing, OS Places API call, etc.). The values within this are set up dynamically to allow any form to customise the wording as required
added component reference to /components/index.js
added validations for fields in /controller/validation/validators.js
added functional test scenarios to /test/functional-tests/index.js
added mock steps configuration in /test/functional-tests/apps/postcode-lookup-default.js
added mock JSON data set to be used as a substitute for the actual API call in the functional tests to /test/data/postcode-lookup-data.json

Testing?

Tested the component on a service

Anything Else? (optional)

Check list

I have reviewed my own pull request for linting issues (e.g. adding new lines)
I have written tests (if relevant)
I have created a JIRA number for my branch
I have created a JIRA number for my commit
I have followed the chris beams method for my commit https://cbea.ms/git-commit/
here is an example commit
Ensure drone builds are green especially tests
I will squash the commits before merging

* create postcode lookup component to replace existing lookups used in HOF forms: - use OS Places API to retrieve address data - make the component reusable and configurable by forms

william-gu-hof

Great work, functionality is good, just need some more consideration of code structure, readability, and some data structure checks / sanitisation

william-gu-hof · 2024-08-13T09:14:45Z

components/postcode-lookup/index.js

+
+const defaults = require('./defaults');
+
+const conditionalTranslate = (key, t) => {


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

Changed the variable name to something more suitable.

william-gu-hof · 2024-08-13T09:26:05Z

components/postcode-lookup/index.js

+/* eslint-disable func-names */
+'use strict';
+
+const axios = require('axios');


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

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

william-gu-hof · 2024-08-13T09:37:40Z

controller/validation/validators.js

+
+  postcodeValidation(value) {
+    // eslint-disable-next-line max-len
+    return Validators.regex(value, /^(([GIRgir] ?[ ]*0[ ]*[Aa]{2}[ ]*)|((([A-Za-z][ ]*[0-9]{1,2}[ ]*)|(([A-Za-z][ ]*[A-Ha-hJ-Yj-y][ ]*[0-9][ ]*[0-9]?)[ ]*|(([A-Za-z][ ]*[0-9][ ]*[A-Za-z][ ]*)|([A-Za-z][ ]*[A-Ha-hJ-Yj-y][ ]*[0-9]?[ ]*[A-Za-z][ ]*))))[-,. ]?[ ]*[0-9][ ]*[A-Za-z][ ]*[A-Za-z][ ]*))$/);


apart from the fact that the existing postcode validation rule allows for empty string, what are the other differences between the new one and the existing one? (looks like your regex is checking a lot more)

Just thinking about maintainability here of having 2 separate post code validation rules, not sure that is practical, depending on what the new regex is doing, maybe we can just overwrite the existing rule, and in your component, use the required and postcode validation together?

The new validation also allows punctuation within the postcode, which I'm not sure if that is accepted in all postcode fields outside of this lookup. Would need reviewing to determine if this new postcode validation would suffice for all postcode inputs across our services.

william-gu-hof · 2024-08-13T09:42:20Z

package.json

  "name": "hof",
  "description": "A bootstrap for HOF projects",
-  "version": "20.5.4",
+  "version": "21.1.0",


Do we have a defined list of what each of the 3 part of the version number means? Is this a breaking change? Why adding a new component would be a breaking change?

Changed this to be 21.2.0 since it's not a breaking change.

william-gu-hof · 2024-08-13T11:06:55Z

components/postcode-lookup/index.js

+  }
+});
+
+const getConfig = key => ({


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

Changed variable name.

william-gu-hof · 2024-08-14T09:39:34Z

test/functional-tests/index.js

+          assert.ok(url.includes('step=manual'));
+        }));
+
+      it('redirects to the lookup step on a successful postcode lookup', () => browser.url('/postcode-default-one')


Once you managed to mock the request, we need to test on success lookup, the correct results are displayed

Added a unit test to cover this.

william-gu-hof · 2024-08-14T09:40:22Z

test/functional-tests/index.js

 const App = require('./lib/app');
 const assert = require('assert');

+// TO BE REVISITED FOR CREATING MOCK API CALL


Would also be good to write some unit tests after you refactor the index.js of the component with more functions, test them individually

Added some unit tests for some of the functions in the component.

william-gu-hof · 2024-08-14T09:44:18Z

components/postcode-lookup/index.js

+          return super.saveValues(req, res, callback);
+        });
+      } else if (step === 'lookup') {
+        const addressResult = req.form.options.fields[`${addressKey}-select`].options.filter(obj => {


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

Added logic to remove any instances of those characters

william-gu-hof · 2024-08-14T09:45:18Z

components/postcode-lookup/index.js

+        }).catch(error => {
+          if(error.response.status === '404' || error.response.status === '429' ||
+            error.response.status === '503' || error.response.status === '500') {
+            req.query.step === 'postcodeSearchIssue';


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

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.

william-gu-hof · 2024-08-14T09:46:29Z

components/postcode-lookup/index.js

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


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

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.

- add feature to enable the toggle of de-indexing so that it can be set per service

william-gu-hof

Code is organised much better.

Just some small questions/suggestions that may help with code readability

william-gu-hof · 2024-09-17T10:26:03Z

.vscode/settings.json

@@ -0,0 +1,2 @@
+{


maybe add this folder to the .gitignore file

Added the folder to the .gitignore file

william-gu-hof · 2024-09-17T10:29:44Z

components/postcode-lookup/defaults.js

@@ -0,0 +1,31 @@
+/* eslint max-len: 0, no-process-env: 0 */
+


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: '...',
...;
}

}`

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.

william-gu-hof · 2024-09-17T10:32:42Z

config/hof-defaults.js

      return convertPage(page);
    }
  },
+  deIndexForm: process.env.DEINDEX_FORM || 'false',


is this needed for the work on postcode lookup?

Accidentally left in from the de-indexing changes. Removed this now.

william-gu-hof · 2024-09-17T10:33:23Z

index.js

 const health = require('./lib/health');
 const serveStatic = require('./lib/serve-static');
 const gaTagSetup = require('./lib/ga-tag');
+const deIndexer = require('./lib/deindex');


similar to above, is this needed for the postcode lookup work?

Removed this now.

- amend postcode-lookup component to remove punctuation from postcode during api call to fix 400 error - amend postcode-lookup component to remove punctuation from postcode in locals - amend postcode-lookup component to remove punctuation from postcode in prepopulated manual fields - amend postcode validation to allow all punctuation - update yarn.lock - remove unused vscode folder

sulthan-ahmed

I've just reviewed @Rhodine-orleans-lindsay change and that makes sense

HOFF-160: Create postcode lookup component

f998c18

* create postcode lookup component to replace existing lookups used in HOF forms: - use OS Places API to retrieve address data - make the component reusable and configurable by forms

mislam987 force-pushed the HOFF-768 branch from 928dbaa to f998c18 Compare August 12, 2024 22:52

mislam987 requested a review from william-gu-hof August 13, 2024 08:54

william-gu-hof requested changes Aug 14, 2024

View reviewed changes

mislam987 added 5 commits September 4, 2024 18:49

Added postcode-lookup.js and functional tests

6adf929

HOFF-672: Add de-indexing toggle functionality

3d4bc13

- add feature to enable the toggle of de-indexing so that it can be set per service

Code refactoring

f1208fd

Further code changes

6f200d0

Added test cases and refactored code

db0e251

mislam987 requested a review from william-gu-hof September 16, 2024 20:50

william-gu-hof reviewed Sep 17, 2024

View reviewed changes

Further code refactoring

7967a12

william-gu-hof approved these changes Sep 17, 2024

View reviewed changes

Merge branch 'master' into HOFF-768

b7a67fb

Rhodine-orleans-lindsay force-pushed the HOFF-768 branch 2 times, most recently from ab3906b to 34f4f76 Compare October 21, 2024 08:11

Rhodine-orleans-lindsay force-pushed the HOFF-768 branch from 34f4f76 to 2440040 Compare October 21, 2024 13:35

sulthan-ahmed reviewed Oct 21, 2024

View reviewed changes


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

		const conditionalTranslate = (key, t) => {

Conversation

mislam987 commented Aug 12, 2024 • edited by Rhodine-orleans-lindsay Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What?

Why?

How?

Testing?

Anything Else? (optional)

Check list

Uh oh!

william-gu-hof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

william-gu-hof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sulthan-ahmed left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

mislam987 commented Aug 12, 2024 •

edited by Rhodine-orleans-lindsay

Loading