PP-11681 Replace 'base-client' with 'axios-base-client' in 'adminusers.client' #4194

JFSGDS · 2024-03-13T16:56:13Z

https://payments-platform.atlassian.net/browse/PP-11681

WHAT

Change ‘adminusers.client’ methods to use ‘axios-base-client’.
Refactor call parameters initialisation.
Use ‘async/await’ syntax.
Extend ‘instanceof RESTClientError’ test in registration controller to include a test for constructor name. This is in order to identify an error thrown by the new 'axios-base-client'. Possibly needs refactoring.

HOW

All automated tests should pass without the need to be amended.
Existing functionality should remain unaffected.

iqbalgds · 2024-03-14T09:22:45Z

app/controllers/registration/registration.controller.js

@@ -53,7 +53,7 @@ async function submitEmailPage (req, res, next) {

    res.redirect(paths.register.checkEmail)
  } catch (err) {
-    if (err instanceof RESTClientError) {
+    if (err instanceof RESTClientError | err.constructor.name === 'RESTClientError') {


Interesting - I am wondering why this is needed?
We can chat about this.

This has been updated - resolving.

iqbalgds

Looks great - just the test that needs fixing.

nataliecarey · 2024-03-15T10:59:51Z

app/services/clients/adminusers.client.js

-      }
-    )
+  async function getUserByExternalId (externalId) {
+    const url = `${baseUrl}${userResource}/${externalId}`


Ideally this should use encodeURIComponent or a URL builder to avoid injection vulnerabilities or encoding issues if special chars are included.

Good point. Presumably this should be done for for all constructs of url with query string parameters (or at least those though to be provided by user input)?
@iqbalgds and I thought this may be better done as a separate PR. Would you agree?

So I tend to do it with everything that's not hard-coded but it's particularly important for user input. In some projects I end up creating a URL model that would be something like:

const url = buildUrl({ base: baseUrl, uriParts: [userResource, externalId], query: { theKey: 'theValue' } })

For this ticket I think it's reasonable to just use encodeURIComponent on each piece of user input in URLs, but personally if it were me doing it I'd rather use it for all URL parts that aren't hard-coded as I'd find that to be less effort than figuring out which ones are or might be from user input.

Thanks Natalie. We'll apply this as a separate PR.

nataliecarey · 2024-03-15T11:09:49Z

app/services/clients/adminusers.client.js

-    )
+  async function getUsersByExternalIds (externalIds = []) {
+    const url = `${baseUrl}${userResource}?ids=${externalIds.join()}`
+    this.client = new Client(SERVICE_NAME)


Using this in this context seems to be unhelpful. I'd suggest using const client = instead.

Having got further through the code I'm wondering whether you'd prefer to use a shared client. I'm not sure from just reading the code whether that's a suitable solution or not but it seems like it might be a good match as the client seems to need configuring before use and the configuration seems to be generic.

Agreed, this approach seems better. Code changed as suggested.

nataliecarey · 2024-03-15T11:24:49Z

app/services/clients/adminusers.client.js

-      }
-    )
+  async function getValidatedInvite (inviteCode) {
+    const url = `${baseUrl}/v1/api/invites/${inviteCode}`


I'm assuming that inviteCode is from user input (or something the user can manipulate) so it makes my previous comment about encoding more important in this instance.

Good point. Please see earlier comment.

nataliecarey · 2024-03-15T11:31:45Z

app/services/clients/adminusers.client.js

@@ -2,7 +2,8 @@

 const lodash = require('lodash')

-const baseClient = require('./base-client/base.client')
+const { Client } = require('@govuk-pay/pay-js-commons/lib/utils/axios-base-client/axios-base-client')


I know it's not related to this PR but I'd suggest avoiding being so specific about the path inside the other project. I'd personally go with:

const { AxiosClient } = require('@govuk-pay/pay-js-commons')

Or if you want it kept separate there are ways to have a shorter path e.g.

const { Client } = require('@govuk-pay/pay-js-commons/axios')

To be really clear: This would need to be changed in pay-js-commons and it's not really part of this PR but it's related to this wider piece of work.

Good suggestion. I agree it would be much neater.
I also agree that it's not really part of this PR so I propose creating a new ticket to cover this. It should be applied to other repositories already amended for the new version of pay-js-commons containing the new axios-base-client namely pay-products-ui and pay-frontend.

I have created a new ticket for this:
https://payments-platform.atlassian.net/browse/PP-12468

iqbalgds · 2024-03-22T10:43:42Z

app/controllers/registration/registration.controller.js

@@ -53,7 +53,7 @@ async function submitEmailPage (req, res, next) {

    res.redirect(paths.register.checkEmail)
  } catch (err) {
-    if (err instanceof RESTClientError) {
+    if (err instanceof RESTClientError | err.constructor.name === 'RESTClientError') {


This has been updated - resolving.

…rs.client' - Change ‘adminusers.client’ methods to use ‘axios-base-client’. - Refactor call parameters initialisation. - Use ‘async/await’ syntax.

iqbalgds reviewed Mar 14, 2024

View reviewed changes

iqbalgds requested changes Mar 14, 2024

View reviewed changes

nataliecarey reviewed Mar 15, 2024

View reviewed changes

JFSGDS requested a review from nataliecarey March 18, 2024 12:56

nataliecarey approved these changes Mar 22, 2024

View reviewed changes

JFSGDS marked this pull request as ready for review March 22, 2024 10:42

iqbalgds approved these changes Mar 22, 2024

View reviewed changes

PP-11681 Replace ‘base-client’ with ‘axios-base-client’ for 'adminuse…

bdf0a32

…rs.client' - Change ‘adminusers.client’ methods to use ‘axios-base-client’. - Refactor call parameters initialisation. - Use ‘async/await’ syntax.

JFSGDS force-pushed the PP-11681_replace_request_with_axios_adminusers_client branch from 09e5f65 to bdf0a32 Compare March 22, 2024 10:51

JFSGDS merged commit c2c208f into master Mar 22, 2024
12 checks passed

JFSGDS deleted the PP-11681_replace_request_with_axios_adminusers_client branch March 22, 2024 11:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PP-11681 Replace 'base-client' with 'axios-base-client' in 'adminusers.client' #4194

PP-11681 Replace 'base-client' with 'axios-base-client' in 'adminusers.client' #4194

JFSGDS commented Mar 13, 2024

iqbalgds Mar 14, 2024

iqbalgds Mar 22, 2024

iqbalgds left a comment

nataliecarey Mar 15, 2024

JFSGDS Mar 18, 2024 •

edited

Loading

nataliecarey Mar 19, 2024

JFSGDS Mar 21, 2024

nataliecarey Mar 15, 2024

nataliecarey Mar 15, 2024

JFSGDS Mar 15, 2024

nataliecarey Mar 15, 2024

JFSGDS Mar 18, 2024

nataliecarey Mar 15, 2024

JFSGDS Mar 15, 2024

iqbalgds Mar 21, 2024

iqbalgds Mar 22, 2024

PP-11681 Replace 'base-client' with 'axios-base-client' in 'adminusers.client' #4194

PP-11681 Replace 'base-client' with 'axios-base-client' in 'adminusers.client' #4194

Conversation

JFSGDS commented Mar 13, 2024

WHAT

HOW

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iqbalgds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JFSGDS Mar 18, 2024 • edited Loading

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

JFSGDS Mar 18, 2024 •

edited

Loading