More visibility/flexibility of the number of results returned from search #203
Conversation
|
🚀 deployment of nhsuk/gp-finder succeeded (http://gp-finder-pr-203.dev.beta.nhschoices.net) |
beseven
force-pushed
the
feature/add-result-numbers-to-metadata
branch
from
bd91de0 to
4f3d409
Compare
|
🚀 deployment of nhsuk/gp-finder succeeded (http://gp-finder-pr-203.dev.beta.nhschoices.net) |
beseven
force-pushed
the
feature/add-result-numbers-to-metadata
branch
from
4f3d409 to
d99ead7
Compare
|
🚀 deployment of nhsuk/gp-finder succeeded (http://gp-finder-pr-203.dev.beta.nhschoices.net) |
app/middleware/setLocals.js
Outdated
| @@ -3,6 +3,8 @@ const backLinkUtils = require('../lib/utils/backLink'); | |||
| function fromRequest(req, res, next) { | |||
| res.locals.search = req.query.search; | |||
| res.locals.postcodeSearch = req.query.postcode; | |||
| res.locals.resultsReturnedCount = 30; | |||
There was a problem hiding this comment.
? rename to resultsLimit.
Also, given that this isn't fromRequest is would be better to have it in config/config.js and app/middleware/locals.js as for the analytics Id's?
If pulled from an env var it will need a default (e.g. resultsLimit: process.env.RESULTS_LIMIT || 30).
Same probably applies to the other hard coded values but they can have an issue raised.
package.json
Outdated
| @@ -50,6 +50,7 @@ | |||
| "nunjucks": "^3.0.1", | |||
| "postcodesio-client": "billinghamj/postcodesio-client-node", | |||
| "require-environment-variables": "^1.1.2", | |||
| "tunnel-agent": "^0.6.0", | |||
There was a problem hiding this comment.
Is this package used?
There was a problem hiding this comment.
It's a dependency of node-sass that snyk was complaining about. I guess it should be fixed another way. Will take a look.
There was a problem hiding this comment.
If node-sass needs it then OK. Just couldn't find a require for it.
There was a problem hiding this comment.
Upgrading sub-dependencies is not a thing in yarn yet (yarnpkg/yarn#2394) so this is a valid workaround of manually upgrading the sub-dependency of node-sass to be able to pass snyk.
There was a problem hiding this comment.
Hm, decided to update the policy for the moment. Snyk uses npm update instead, so the manual yarn upgrade doesn't quite do the trick.
| @@ -25,7 +25,7 @@ function assertSearchResponse(search, postcode, done, assertions) { | |||
| describe('Results page', () => { | |||
| const noOnlineBookingLinkMessage = 'This surgery doesn't have an online booking system.'; | |||
|
|
|||
| describe('page layout', () => { | |||
| describe('layout', () => { | |||
There was a problem hiding this comment.
Maybe an integration test for the presence of a greater than 0 resultLimit and resultsCount.
beseven
force-pushed
the
feature/add-result-numbers-to-metadata
branch
from
d99ead7 to
4aa8bf4
Compare
|
🚀 deployment of nhsuk/gp-finder succeeded (http://gp-finder-pr-203.dev.beta.nhschoices.net) |
|
This is stalled pending outcome of #208 |
|
Now #208 is resolved then the snyk/yarn.lock changes should no longer be required. |
beseven
force-pushed
the
feature/add-result-numbers-to-metadata
branch
from
4aa8bf4 to
a646b66
Compare
|
🚀 deployment of nhsuk/gp-finder succeeded (http://gp-finder-pr-203.dev.beta.nhschoices.net) |
beseven
force-pushed
the
feature/add-result-numbers-to-metadata
branch
from
a646b66 to
fb6a182
Compare
|
🚀 deployment of nhsuk/gp-finder succeeded (http://gp-finder-pr-203.dev.beta.nhschoices.net) |
Description
Return a set number of results, pass on display results and total results as meta tags
Enable the number of results to be set as an ENV var
Related Issue
https://trello.com/c/VHkRE78n/452-send-flexible-no-of-results-and-shown-results-as-metadata-wt
Motivation and Context
More flexibility in how many results we are displaying, returning the number of total results for reporting purposes
Checklist