Hoff 737: session timeout continue form #464

Rhodine-orleans-lindsay · 2024-08-02T12:00:05Z

What?

Create session timeout warning for forms. This ticket is for forms that do not use the save and return feature, so the user will either stay on the page or exit the form but if they exit the form, their information will not be saved. - HOFF-737
This is modelled of off the NHS IHS form (which leverages hof) (see code repo here) and the govuk design system backlog issue for timeout warnings (see code repo here)

Why?

To comply with accessibility requirements

How?

configure session timeout warning functionality in frontend/themes/gov-uk/client-js/session-timeout-dialog.js
add frontend/template-partials/partials/session-timeout-warning.html
amend frontend/template-partials/partials/page.html to include session timeout warning in pages with forms.
add styling for session timeout warning
add exit form button to session timeout warning dialog which resets session and goes to the exit page.
create session timeout warning component to make exit page and session timeout warning dialog content customisable
add session timeout warning component readme
amend confirmation page so it doesn't show popup
sandbox changes

Testing?

added tests for session timeout warning component and sessionDialog functions
tests passing locally
beta version tested in services locally and on branch

Anything Else? (optional)

amended pr template name so it will be picked up
PR for documentation can be found in hof-guide - https://github.com/UKHomeOfficeForms/hof-guide/pull/22/files
Demo can be tried on the HOF Demo app

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

Screenshots (optional)

Default Session timeout warning

Default exit page after user clicks 'Exit this form'

default exit page after clicking exit this form

sulthan-ahmed · 2024-08-13T13:57:59Z

Nice work I haven't yet reviewed the code. I noticed you linked to a 2018 govuk issues link and with some discussion. It would be interesting to get a view point across government about the design with input from UCD after you've approved and merged your work.

sulthan-ahmed · 2024-08-13T13:58:22Z

CHANGELOG.md

+ - allows for customisation of session timeout warning dialog content and exit page content
+* Fixes accessibility issues
+* Sandbox area for testing hof changes
+* Updates patch and minor dependency versions


components/session-timeout-warning/Readme.md

components/session-timeout-warning/index.js

sulthan-ahmed · 2024-08-13T14:37:18Z

components/session-timeout-warning/index.js

+  configure(req, res, next) {
+    // reset the session if user chooses to exit on session timeout warning
+    if (req.form.options.route === '/exit') {
+      req.sessionModel.reset();
+    }
+    return super.configure(req, res, next);
+  }


you could add a try and catch without async as there could be issues with the session when resetting

Suggested change

configure(req, res, next) {

// reset the session if user chooses to exit on session timeout warning

if (req.form.options.route === '/exit') {

req.sessionModel.reset();

}

return super.configure(req, res, next);

}

try {

// Reset the session if the user chooses to exit on session timeout warning

if (req.form.options.route === '/exit') {

req.sessionModel.reset();

}

return super.configure(req, res, next);

} catch (error) {

console.error('Error during session reset:', error);

next(error); // Pass the error to the next middleware for centralised handling

}

}

I would use the hof-logger I can't remember how that works

I have added in the try/catch block with the hof logger now

sulthan-ahmed · 2024-08-13T14:40:12Z

package.json

@@ -1,7 +1,7 @@
 {
  "name": "hof",
  "description": "A bootstrap for HOF projects",
-  "version": "21.0.1",
+  "version": "22.0.0-timeout-warning-beta.13",


Did you want to change this?

👍🏾 I have amended this now

sulthan-ahmed · 2024-08-13T14:40:36Z

package.json

@@ -21,13 +21,13 @@
    "url": "https://github.com/UKHomeOfficeForms/hof/issues"
  },
  "scripts": {
-    "test": "yarn run unit && yarn run test:cookie-banner && yarn run test:functional && yarn run test:client && yarn run test:lint",
+    "test": "yarn run unit && yarn run test:jest && yarn run test:functional && yarn run test:client && yarn run test:lint",


we are using jest now?

I quite like how it works when mocking. The existing cookie-banner funtionality tests are already using jest and it seems there is also popularity among the team to move to using jest more

yes I prefer jest. mocha requires too much setup and is difficult with stubbing

sulthan-ahmed · 2024-08-13T15:12:11Z

test/components/session-timeout-warning.spec.js

+      instance.locals(req, res).should.not.have.property('dialogTitle');
+      instance.locals(req, res).should.not.have.property('dialogText');
+      instance.locals(req, res).should.not.have.property('timeoutContinueButton');
+      instance.locals(req, res).should.not.have.property('dialogExitLink');
+    });


instance.locals(req, res) is used a lot could you create a function for it

Suggested change

instance.locals(req, res).should.not.have.property('dialogTitle');

instance.locals(req, res).should.not.have.property('dialogText');

instance.locals(req, res).should.not.have.property('timeoutContinueButton');

instance.locals(req, res).should.not.have.property('dialogExitLink');

});

const checkDialogProperties = (expected) => {

instance.locals(req, res).should.have.property('dialogTitle').and.deep.equal(expected);

instance.locals(req, res).should.have.property('dialogText').and.deep.equal(expected);

instance.locals(req, res).should.have.property('timeoutContinueButton').and.deep.equal(expected);

instance.locals(req, res).should.have.property('dialogExitLink').and.deep.equal(expected);

};

you could also do this,

const locals = instance.locals(req, res); locals.should.have.property('dialogTitle').and.deep.equal(true); locals.should.have.property('dialogText').and.deep.equal(true);

👍🏾 I have amended this now

sulthan-ahmed · 2024-08-13T15:17:47Z

test/components/session-timeout-warning.spec.js

@@ -0,0 +1,121 @@
+'use strict';
+
+const Component = require('../../components').sessionTimeoutWarning;


Suggested change

const Component = require('../../components').sessionTimeoutWarning;

const { sessionTimeoutWarning: Component } = require('../../components');

I have amended this now

test/components/session-timeout-warning.spec.js

sulthan-ahmed · 2024-08-13T17:07:03Z

test/components/session-timeout-warning.spec.js

+    });
+
+    afterEach(() => {
+      Base.prototype.locals.restore();


Your thing works but I read about this being better and simple

Suggested change

Base.prototype.locals.restore();

sinon.restore(); // Ensures all stubs/mocks are reset

👍🏾 I have amended this now

sulthan-ahmed · 2024-08-13T17:09:07Z

test/frontend/jest/sessionDialog.test.js

+const sessionTimeoutWarningHtml = fs.readFileSync(path.resolve(__dirname, '../../../frontend/template-partials/views/partials/session-timeout-warning.html'), 'utf8');
+
+jest
+  .dontMock('fs');


I think jest is much better than mocha so this is good tbh

sulthan-ahmed · 2024-08-13T17:10:24Z

test/frontend/jest/sessionDialog.test.js

+jest
+  .dontMock('fs');


Suggested change

jest

.dontMock('fs');

jest.dontMock('fs');

I have amended this now

sulthan-ahmed · 2024-08-13T17:19:45Z

test/frontend/jest/sessionDialog.test.js

+  let options;
+  let originalHTMLDialogElement;
+
+  beforeAll(() => {


beforeAll, can be dangerous because it only does a setup once and then it runs through the tests so it's not fresh but beforeEach takes up more resource. I prefer beforeEach just to be on the safe side but I'm sure it's fine in your scenario

test/frontend/jest/sessionDialog.test.js

sulthan-ahmed · 2024-08-19T10:52:03Z

README.md

@@ -945,7 +945,7 @@ Using the translation key `fields.field-name.label` will return different values

 # HOF Components

-## Date Component
+# Date Component


shouldn't this be a sub heading rather than a main heading?

👍🏾 I have amended this now, All the other components were also using the main heading levels so I have made them subheadings and also adjusted the levels of their corresponding subheadings

sulthan-ahmed

sublime work, big props to you for getting this done. It's been a common request and makes hof more modern 👍

- configure session timeout warning in frontend/themes/gov-uk/client-js/session-timeout-dialog.js - add frontend/template-partials/partials/session-timeout-warning.html - amend frontend/template-partials/partials/page.html to include session timeout warning in pages with forms. - add styling for session timeout warning - add leave form button in dialog box which resets session and goes to exit page informing user info has not been saved. This is for non save and return forms. - create session timeout component exit page customisable and session timeout warning dialog content customisable - add JSDoc and try/catch block to session timeout warning component - add hof logger to session timeout warning component - add session timeout warning component documentation to readme - amend sub heading levels for components in readme - amend confirmation page so it doesn't show popup - amend pr template name so it is picked up - add tests - update change log - update yarn.lock

Rhodine-orleans-lindsay force-pushed the HOFF-737-session-timeout-continue-form branch 2 times, most recently from 17b17b4 to bcf9075 Compare August 6, 2024 01:05

Rhodine-orleans-lindsay marked this pull request as ready for review August 6, 2024 16:49

Rhodine-orleans-lindsay requested review from sulthan-ahmed and shamiluwais August 6, 2024 16:59