LIME-1273 - Adding new call to personInfo endpoint #306

ElliotMurphyGDS · 2024-10-29T09:07:20Z

What changed

Updated Root.js Controller to call new personInfo Endpoint and map returned values into sessionModel
made mapping of values to sessionModel conditional for compatibility with existing web route
Updated Config to include new personInfo endpoint path
Added tests for root controller

gds-jbentham · 2024-10-29T16:11:57Z

src/app/drivingLicence/controllers/root.js

+    };
+
+    //axios.post to new personInfo endpoint, put licence details into req.session.shared_claims shared claims
+    const personInfoApiResponse = await req.axios.post(


For security can we wrap this in an if statement with an env var so we can merge without triggering new code yet

Yes this actually means I can remove feature toggle from corresponding API PR

gds-jbentham · 2024-10-29T16:13:47Z

src/app/drivingLicence/controllers/root.js

    }
-    super.saveValues(req, res, next);


Is this not still needed?

gds-jbentham · 2024-10-29T16:15:26Z

src/lib/config.js

@@ -6,7 +6,8 @@ module.exports = {
    PATHS: {
      SESSION: "session",
      CHECK: "check-driving-licence",
-      AUTHORIZATION: "authorization"
+      AUTHORIZATION: "authorization",
+      PERSON_INFO: "person-info"


Is this correct its different to whats on the ticket

Endpoint on ticket was taken from a similar piece of work kiwi did so an internal decision not linked to any requirement, I opted to add the hyphen to keep inline with existing convention

Can we update the ticket

gds-jbentham · 2024-10-29T16:16:13Z

src/app/drivingLicence/controllers/root.test.js

  it("should be an instance of BaseController", () => {
    expect(root).to.be.an.instanceof(BaseController);
  });
-
-  it("should retrieve shared_claims from session and store it in the journeyModel", async () => {
+  it("should retrieve shared_claims from session and store it in the sessionModel", async () => {


Do we have a negative check for if any of these values are absent

smsgds · 2024-10-30T10:27:34Z

src/app/drivingLicence/controllers/root.js

-        req.journeyModel.set("surname", sharedClaims.names[0]?.familyName);
-        req.journeyModel.set("givenNames", sharedClaims.names[0]?.givenNames);
-      }
+    let personInfoApiResponse = {};


The should be within the if for the feature flag.

smsgds · 2024-10-30T10:29:40Z

src/app/drivingLicence/controllers/root.js

+        { headers: headers }
+      );
+    }
+    if (personInfoApiResponse.data.drivingPermit) {


This should be in the above feature flag if.

It also needs to handle a failure of personInfoApiResponse not being returned and .data.drivingPermit not being present.

gds-jbentham · 2024-10-30T12:34:42Z

src/app/drivingLicence/controllers/root.js

 class RootController extends BaseController {
  async saveValues(req, res, next) {
-    const sharedClaims = req.session?.shared_claims;
+    req.sessionModel.reset();
+    req.sessionModel.set("isAuthSourceRoute", false);


Do we want to be resetting the session model we run the risk here of something failing and putting the user down a different route to the first one they were on if they navigate back

gds-jbentham · 2024-10-30T12:36:29Z

src/app/drivingLicence/controllers/root.js

+      session_id: req.session?.tokenId
+    };
+
+    if (process.env.AUTH_SOURCE_ENABLED === "true") {


Should this be isAuthSourceRoute
if (process.env.AUTH_SOURCE_ENABLED && !isAuthSourceRoute)

That way we only run it once per journey

gds-jbentham · 2024-10-31T10:53:09Z

src/app/drivingLicence/controllers/root.js

+      personInfoApiResponse.data?.drivingPermit[0]?.expiryDate &&
+      personInfoApiResponse.data?.drivingPermit[0]?.issueDate &&
+      personInfoApiResponse.data?.drivingPermit[0]?.issuedBy &&
+      personInfoApiResponse.data?.drivingPermit[0]?.issueNumber &&


I think this needs to be more complicated. For instance we only expect an issue number if its DVLA

gds-jbentham · 2024-10-31T10:54:23Z

src/app/drivingLicence/controllers/root.js

      }
    }
    super.saveValues(req, res, next);
  }
+
+  async checkForValidSharedClaimsData(req, personInfoApiResponse) {


On naming can we make this return true or false and update the state in the calling method

Signed-off-by: ElliotMurphyGDS <[email protected]>

sonarcloud · 2024-10-31T19:16:34Z

Quality Gate failed

Failed conditions
76.6% Coverage on New Code (required ≥ 80%)
2 New Code Smells (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

ElliotMurphyGDS requested a review from a team as a code owner October 29, 2024 09:07

ElliotMurphyGDS force-pushed the LIME-1273 branch 7 times, most recently from d9b5034 to 7ecc8c1 Compare October 29, 2024 13:39

gds-jbentham reviewed Oct 29, 2024

View reviewed changes

src/app/drivingLicence/controllers/root.js

}

super.saveValues(req, res, next);

Copy link

Contributor

gds-jbentham Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not still needed?

gds-jbentham reviewed Oct 29, 2024

View reviewed changes

ElliotMurphyGDS force-pushed the LIME-1273 branch from 7ecc8c1 to 8d5ee51 Compare October 30, 2024 09:02

ElliotMurphyGDS mentioned this pull request Oct 30, 2024

Lime 1311 #305

Merged

smsgds requested changes Oct 30, 2024

View reviewed changes

gds-jbentham reviewed Oct 30, 2024

View reviewed changes

ElliotMurphyGDS force-pushed the LIME-1273 branch 11 times, most recently from 6a91151 to 186733e Compare October 31, 2024 10:41

gds-jbentham reviewed Oct 31, 2024

View reviewed changes

ElliotMurphyGDS force-pushed the LIME-1273 branch 4 times, most recently from 7ef5276 to 734d630 Compare October 31, 2024 18:17

LIME-1273 - Adding new call to personInfo endpoint

8646f35

Signed-off-by: ElliotMurphyGDS <[email protected]>

ElliotMurphyGDS force-pushed the LIME-1273 branch from 734d630 to 8646f35 Compare October 31, 2024 19:10

smsgds approved these changes Oct 31, 2024

View reviewed changes

ElliotMurphyGDS merged commit d294bf0 into main Oct 31, 2024
5 of 6 checks passed

ElliotMurphyGDS deleted the LIME-1273 branch October 31, 2024 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LIME-1273 - Adding new call to personInfo endpoint #306

LIME-1273 - Adding new call to personInfo endpoint #306

ElliotMurphyGDS commented Oct 29, 2024

gds-jbentham Oct 29, 2024

ElliotMurphyGDS Oct 30, 2024

gds-jbentham Oct 29, 2024

gds-jbentham Oct 29, 2024

ElliotMurphyGDS Oct 30, 2024

gds-jbentham Oct 30, 2024

gds-jbentham Oct 29, 2024

smsgds Oct 30, 2024

smsgds Oct 30, 2024

gds-jbentham Oct 30, 2024

gds-jbentham Oct 30, 2024

gds-jbentham Oct 31, 2024

gds-jbentham Oct 31, 2024

sonarcloud bot commented Oct 31, 2024

LIME-1273 - Adding new call to personInfo endpoint #306

LIME-1273 - Adding new call to personInfo endpoint #306

Conversation

ElliotMurphyGDS commented Oct 29, 2024

What changed

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

sonarcloud bot commented Oct 31, 2024

Quality Gate failed