Organise review app middleware #3223

colinrotherham · 2023-01-30T20:34:07Z

We started organising our middleware into small exports for unit testing in 807f533

This PR adds a few more whilst I was working on the code

36degrees

The PR description mentions unit testing as a rationale for the change – would it make sense for us to add unit tests for these files as we go?

Generally when code is moved around I find it a lot easier to review if the moves happen in a separate commit with minimal other changes.

36degrees · 2023-02-03T16:41:26Z

app/banner.js

 const BANNER_COOKIE_NAME = 'dismissed-app-banner'

 module.exports = function (app) {
  // Detect if banner should be shown based on cookies set
-  app.use(cookieParser())


Can we split this change out into a separate commit?

36degrees · 2023-02-03T16:41:35Z

app/common/middleware/request.js

+  cookies(),
+
+  // Turn form POSTs into data that can be used for validation
+  express.urlencoded({ extended: true })


Can we split this change out into a separate commit?

It looks like we no longer need body-parser because the functionality is now built into express – is that right? Would be good to capture in the commit message.

✅ Done

It looks like we no longer need body-parser because the functionality is now built into express – is that right?

Yeah, the express.urlencoded() middleware was added to Express.js in v4.16.0
https://expressjs.com/en/api.html#express.urlencoded

36degrees · 2023-02-03T16:42:22Z

app/common/middleware/locals.js

+ * Legacy query handling
+ */
+const locals = (req, res, next) => {
+  res.locals.legacy = ['1', 'true'].includes(req.query.legacy)


I'd also consider splitting this refactor out into a separate commit.

✅ Done

I've configured Express.js with the standard URLSearchParams() too ready for Express.js v5

const { query } = req // Check for query query.has('example') // Get query value query.get(example') // Set query value query.set('example', 'value')

Is that because of the change of the default query parser to simple?

I think the simple query parser would still work for our use case as we're not doing anything fancy with nested params etc. Do you think there's advantages to using URLSearchParams over it?

Ah I remember that, there was a security vulnerability wasn't there

Primarily because we've now got the WHATWG URL and Fetch standards so it's all in Node.js too:

Query to string

// 'example1=value1&example2=value2' req.query.toString()

Query to object

// { example1: 'value1', example2: 'value2' } Object.fromEntries(req.query)

Query via URL object

const url = new URL('http://localhost') url.search = '?example1=value1&example2=value2' url.searchParams.get('example1') // 'value1' url.searchParams.get('example2') // 'value2'

Express.js has said they'll also switch in future
expressjs/express#4990 (comment)

Happy to put it back to objects again

36degrees · 2023-02-03T16:44:34Z

app/common/middleware/request.js

+/**
+ * Request handling
+ */
+const request = [


Not 100% sure about this file as it's just bringing in middleware from dependencies?

Could we just include these in middleware/index.js instead?

Yeah that's an option too

I'll explain my thinking in terms of testing GET or POST requests:

HTTP GET request

Cookies are added to req.cookies

Form submission req.body empty

HTTP POST request

Cookies are added to req.cookies

Form submission req.body populated

We're left with a nice way to test everything "HTTP request related" in one middleware

Hypothetically you'd put things like Content-Encoding gzip and application/json handling here too

import compression from 'compression' export const request = [ compression(), express.json(), express.urlencoded({ extended: true }) ]

I've added tests for this

colinrotherham · 2023-02-03T16:53:50Z

@36degrees Sounds good

One commit per middleware, with a basic test per commit?

Thanks for taking a look

36degrees · 2023-02-08T17:32:45Z

One commit per middleware, with a basic test per commit?

Sounds sensible 👍🏻 Doesn't have to be strict though – the main thing for me is to try and avoid moving and changing code in the same commit as it makes it harder to review.

colinrotherham · 2023-02-10T09:08:51Z

@36degrees Tests added which found a few broken things

Let me know if you want them on a separate PR

Component example back links don't append ?legacy=true
Search example field names/values don't match JavaScript ~~sort~~ order
Search example fields were only partially updated on submit (Organisation checkboxes)

colinrotherham · 2023-02-10T09:13:07Z

app/full-page-examples.test.js

@@ -338,21 +338,21 @@ describe(`http://localhost:${PORT}/full-page-examples/`, () => {
        const body = await response.text()
        const $ = cheerio.load(body)
        // Check the results are correct
-        expect($.html()).toContain('822,411 results')
+        expect($.html()).toContain('121,842 results')


We were building the seed string with:

// 'most-viewedundefinedundefined' const seed = order + brexit + organisation

So these are newly-shuffled random totals using the query string instead:

// 'order=most-viewed' const seed = query.toString()

colinrotherham · 2023-02-10T09:13:49Z

app/views/full-page-examples/search/index.js

+
+        // Make the total more readable
+        total: Number(randomizedTotal)
+          .toLocaleString('en', { useGrouping: true }),


Where useGrouping (in en locale) adds commas to numbers

colinrotherham · 2023-02-10T09:15:50Z

app/views/full-page-examples/search/index.njk

            }
-          ]
+          ],
+          values: values["organisation"]


@36degrees Your nice feature definitely saved a lot of code

Allow selecting options by passing current values #2616

36degrees

Tidy 👍🏻 Great to have those tests!

The `express.urlencoded()` middleware was added to Express.js in v4.16.0 https://expressjs.com/en/api.html#express.urlencoded

From the URL web standard https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams

Consolidates request body and cookie parsing

The query `?legacy=true` is lost when clicking back

They were out of sync with the JavaScript and didn’t update correctly via query string

Include search text input and sort options in form so they’re submitted when the button is pressed

colinrotherham added tech debt Frontend squad labels Jan 30, 2023

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3223 January 30, 2023 20:34 Inactive

kellylee-gds removed the Frontend squad label Feb 2, 2023

36degrees reviewed Feb 3, 2023

View reviewed changes

colinrotherham force-pushed the review-app-offline branch from 08cf40f to 38d8d14 Compare February 6, 2023 19:31

Base automatically changed from review-app-offline to main February 6, 2023 19:37

colinrotherham force-pushed the review-app-middleware branch from 2c94598 to 5f2bf41 Compare February 10, 2023 08:52

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3223 February 10, 2023 08:52 Inactive

colinrotherham commented Feb 10, 2023

View reviewed changes

36degrees approved these changes Feb 10, 2023

View reviewed changes

colinrotherham added 9 commits February 10, 2023 14:43

Replace review app body-parser with built-in middleware

ef040f4

The `express.urlencoded()` middleware was added to Express.js in v4.16.0 https://expressjs.com/en/api.html#express.urlencoded

Move review app middleware into common directory

f8367a4

Parse query with URLSearchParams

7b92f92

From the URL web standard https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams

Set up middleware export for HTTP requests

1888414

Consolidates request body and cookie parsing

Set up middleware export for robots.txt + headers

4b1a369

Set up middleware export for legacy mode

a811631

Fix missing legacy query links

a3b7874

The query `?legacy=true` is lost when clicking back

Fix search example field values

6b7e431

They were out of sync with the JavaScript and didn’t update correctly via query string

Fix search example form submit

e7cf0b2

Include search text input and sort options in form so they’re submitted when the button is pressed

colinrotherham force-pushed the review-app-middleware branch from 5f2bf41 to e7cf0b2 Compare February 10, 2023 14:44

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3223 February 10, 2023 14:44 Inactive

colinrotherham merged commit 22d6ec3 into main Feb 10, 2023

colinrotherham deleted the review-app-middleware branch February 10, 2023 14:48

colinrotherham added assurance consistency dependencies Pull requests that update a dependency file and removed tech debt labels Feb 15, 2023

colinrotherham mentioned this pull request Apr 3, 2023

Split config, lib and tasks into npm packages #3399

Merged

colinrotherham mentioned this pull request Apr 14, 2023

Include "Review app" in npm run build:types check #3501

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Organise review app middleware #3223

Organise review app middleware #3223

colinrotherham commented Jan 30, 2023 •

edited

Loading

36degrees left a comment

36degrees Feb 3, 2023

colinrotherham Feb 10, 2023

36degrees Feb 3, 2023

colinrotherham Feb 10, 2023

36degrees Feb 3, 2023

colinrotherham Feb 10, 2023

36degrees Feb 10, 2023

colinrotherham Feb 10, 2023 •

edited

Loading

36degrees Feb 3, 2023

colinrotherham Feb 8, 2023 •

edited

Loading

colinrotherham Feb 10, 2023

colinrotherham commented Feb 3, 2023

36degrees commented Feb 8, 2023

colinrotherham commented Feb 10, 2023 •

edited

Loading

colinrotherham Feb 10, 2023

colinrotherham Feb 10, 2023

colinrotherham Feb 10, 2023

36degrees left a comment

Organise review app middleware #3223

Organise review app middleware #3223

Conversation

colinrotherham commented Jan 30, 2023 • edited Loading

36degrees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrotherham Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Query to string

Query to object

Query via URL object

Choose a reason for hiding this comment

colinrotherham Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrotherham commented Feb 3, 2023

36degrees commented Feb 8, 2023

colinrotherham commented Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

36degrees left a comment

Choose a reason for hiding this comment

colinrotherham commented Jan 30, 2023 •

edited

Loading

colinrotherham Feb 10, 2023 •

edited

Loading

colinrotherham Feb 8, 2023 •

edited

Loading

colinrotherham commented Feb 10, 2023 •

edited

Loading