Feature/as 1157 spocs and inspectors #70

lvnhmd · 2019-04-03T19:19:53Z

Please do not merge this, I am getting Error: There is an existing open task for this record on delete ( yes, this comes from the resolver) and I need to reuse this functionality for PoCs

lennym · 2019-04-04T13:14:00Z

pages/common/helpers/index.js

@@ -0,0 +1,10 @@
+const getInspectors = req =>
+  req.api(`/search/profiles`)


This should have a specific endpoint created to get the list of ASRU users. Using search will either only return a subset of the data (because paginated) or return all the data, which is worse (because 27,000 people in prod)

lennym · 2019-04-04T13:17:38Z

pages/inspectors/index.js

+        return res.redirect(req.buildRoute('establishment.dashboard', { establishmentId: req.establishment.id }));
+        // return res.redirect(req.buildRoute('dashboard'));
+      })
+      .catch(next);


These two endpoints (post('/') and post('/delete')) are basically identical except the API method. I'd extract them into a single parameterised function.

We have an extra parameter we get from the request, which makes this extraction very difficult.

joefitter · 2019-04-08T09:24:45Z

pages/asru/index.js

+    ...settings
+  });
+
+  app.use(bodyParser.urlencoded({ extended: true }));


the form router attached the bodyParser middleware on POST - do we need it here too? Also the form uses extended: false to allow string/array values - is there a reason it is true here?

removed it.

actually, it does not seem to be case ... I mean , I can see the attachment in https://github.com/UKHomeOffice/asl-pages/blob/e676092c4a6f01dc48ad63e534ecd06e5f9c2f6d/pages/common/routers/form.js#L172 , but if I remove app.use(bodyParser.urlencoded({ extended: false })); parameters are not found in req.body ...

aah - that's because we are parsing the body on POST / but not POST /delete. In that case I would add the middleware in the post handler that needs it -

app.post('/delete', bodyParser.urlencoded({ extended: false }), (req, res, next) => { // your code })

TBH, I'd probably move body-parsing to a global level. There's basically never going to be a context in which we're doing a POST and don't want to parse the body.

joefitter · 2019-04-08T09:29:25Z

pages/asru/views/index.jsx

+    <Fragment>
+      {
+        asru.map(profile => (
+          <p key={`${profile.id}`}>{profile.firstName} {profile.lastName}


key={profile.id}

joefitter · 2019-04-08T09:29:50Z

pages/asru/views/index.jsx

+      {
+        asru.map(profile => (
+          <p key={`${profile.id}`}>{profile.firstName} {profile.lastName}
+            <form method='POST' action={`${baseUrl}/delete`}>


pretty sure you can't have a form as a child of a p

in Fragment ?

You don't need to wrap it in anything if its single element - just return the <form>. You might need to add display: block

The <p> is still necessary for structural reasons.

You need to reorder the tags so that the <form> wraps the <p>. Otherwise React will throw an error when run in production mode.

joefitter · 2019-04-08T09:31:47Z

pages/search/views/index.jsx

@@ -25,7 +25,7 @@ const formatters = {
        return establishments.map((establishment, i) => [
          // separate links with line breaks #hack


I know this isn't part of this PR but.... 🤢

lennym · 2019-04-08T11:44:38Z

Can you rename the page to something more specific than asru please? It's really quite ambiguous.

asru-assignment perhaps?

lennym · 2019-04-08T13:08:53Z

pages/asru-assignment/index.js

+        p => p.asruInspector
+      ) : res.locals.static.asru = req.establishment.asru.filter(
+        p => p.asruLicensing
+      );


Can you use an if statement here please? Ternaries for "not assignment" break my brain.

lennym · 2019-04-08T13:10:39Z

lib/urls.js

+    read: '/establishments/:establishmentId/details',
+    asru: '/establishments/:establishmentId/asru/:asruUser',
+    inspectors: '/establishments/:establishmentId/asru/inspectors',
+    spocs: '/establishments/:establishmentId/asru/spocs'


We seem to have the same URL defined 3 times here. If we're mounting onto the parameterised URL then any link builders should use the parameterised version, with the parameter defined.

lennym · 2019-04-08T13:11:30Z

pages/asru-assignment/index.js

+          })
+          .then(() => next())
+          .catch(next);
+    }


As above. Use an if statement unless doing assignment please.

lennym · 2019-04-09T08:15:09Z

This is failing for reasons that seem to be outside of the scope of this PR, so I am going to merge and then attempt to fix the memory usage issues in Drone.

lennym reviewed Apr 4, 2019

View reviewed changes

lvnhmd force-pushed the feature/AS-1157-SPoC-and-Inspectors branch from 05026db to 226e846 Compare April 5, 2019 11:34

joefitter reviewed Apr 8, 2019

View reviewed changes

lvnhmd added 5 commits April 8, 2019 11:00

Add 'add inspectors' pages

fb4cd70

Initial commit

187dd1c

Call specific endpoint to get asru users, tidy up

934375a

Reuse pages for spocs and inspectors

5d56eae

Add extra path to asru related uls

b4e0453

lvnhmd force-pushed the feature/AS-1157-SPoC-and-Inspectors branch from 7f9b06d to b4e0453 Compare April 8, 2019 10:01

move body-parsing to a global level, rename asru, tidy-up

94f08b8

lvnhmd changed the title *WIP* Feature/as 1157 s po c and inspectors Feature/as 1157 s po c and inspectors Apr 8, 2019

lvnhmd changed the title ~~Feature/as 1157 s po c and inspectors~~ Feature/as 1157 spocs and inspectors Apr 8, 2019

lennym reviewed Apr 8, 2019

View reviewed changes

lvnhmd added 2 commits April 8, 2019 14:25

Remove redundant urls, use if instead of ternary

bfad2de

Update asl-pages

8d62aa5

lennym merged commit ee9cd9d into master Apr 9, 2019

wheelsandcogs deleted the feature/AS-1157-SPoC-and-Inspectors branch August 8, 2022 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/as 1157 spocs and inspectors #70

Feature/as 1157 spocs and inspectors #70

lvnhmd commented Apr 3, 2019

lennym Apr 4, 2019

lvnhmd Apr 5, 2019

lennym Apr 4, 2019

lvnhmd Apr 4, 2019

joefitter Apr 8, 2019

lvnhmd Apr 8, 2019

lvnhmd Apr 8, 2019

joefitter Apr 8, 2019

lennym Apr 8, 2019

lvnhmd Apr 8, 2019

joefitter Apr 8, 2019

lvnhmd Apr 8, 2019

joefitter Apr 8, 2019

lvnhmd Apr 8, 2019

joefitter Apr 8, 2019

lennym Apr 8, 2019

lvnhmd Apr 8, 2019

joefitter Apr 8, 2019

lvnhmd Apr 8, 2019

lennym commented Apr 8, 2019

lennym Apr 8, 2019

lvnhmd Apr 8, 2019

lennym Apr 8, 2019

lvnhmd Apr 8, 2019

lennym Apr 8, 2019

lvnhmd Apr 8, 2019

lennym commented Apr 9, 2019

		@@ -0,0 +1,10 @@
		const getInspectors = req =>
		req.api(`/search/profiles`)

		@@ -25,7 +25,7 @@ const formatters = {
		return establishments.map((establishment, i) => [
		// separate links with line breaks #hack

Feature/as 1157 spocs and inspectors #70

Feature/as 1157 spocs and inspectors #70

Conversation

lvnhmd commented Apr 3, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lennym commented Apr 8, 2019

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

lennym commented Apr 9, 2019