Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PYIC-7797: route failed + reverification journeys to use process-candidate-identity-lambda #2867

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

thebauSoftwire
Copy link
Contributor

@thebauSoftwire thebauSoftwire commented Jan 13, 2025

Proposed changes

What changed

Added routing to failed and reverification journeys to use new process-candidate-identity lambda

Why did it change

We made a new lambda to consolidate a bunch of other lambdas, this PR uses that lambda on journeys that don't yet use it yet.

Issue tracking

@thebauSoftwire
Copy link
Contributor Author

We also have operational profile migration/reuse journeys which call only the call-ticf-cri lambda. The only identityType which suits this is the INCOMPLETE one which doesn't it because it's not an incomplete identity. We could instead add a OPERATIONAL identityType?

@@ -389,7 +389,7 @@ core:
sqsAsync: true
kidJarHeaderEnabled: true
drivingLicenceAuthCheck: false
processCandidateIdentity: false
processCandidateIdentity: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

still having difficulty running the api tests consistently locally, fetch is now failing at initialise ipv session :( so testing here and leaving this comment as a note to self to remove this when ready to be merged

@@ -33,6 +39,20 @@ states:
- IPV_USER_DETAILS_UPDATE_END
auditContext:
successful: false
checkFeatureFlag:
processCandidateIdentity:
targetState: PROCESS_INCOMPLETE_IDENTITY_COULD_NOT_CONFIRM_WITH_DELETION_INVALID_ID
Copy link
Contributor

@Joe-Edwards-GDS Joe-Edwards-GDS Jan 14, 2025

Choose a reason for hiding this comment

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

Do we need the 'with deletion' any more? - It's fine, but these super long state names feel a bit awkward!

Sadly, I don't think we can update the existing states without a two-step deploy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed the new ones but haven't updated the others. For my own learning, what would a two step deployment look like for updating the existing states and why do we need it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's say you had:

FIRST:
  events:
    next:
      targetState: SECOND

SECOND:
  events:
    next:
      targetState: THIRD

THIRD:
   ...

and you want to rename SECOND...

You'd need to do it in two steps:

FIRST:
  events:
    next:
      targetState: SECOND_NEW

SECOND_NEW:
  events:
    next:
      targetState: THIRD

SECOND:
  events:
    next:
      targetState: THIRD

THIRD:
   ...

and then

FIRST:
  events:
    next:
      targetState: SECOND_NEW

SECOND_NEW:
  events:
    next:
      targetState: THIRD

THIRD:
   ...

The reason is that users might be in SECOND when you deploy the first change, so the journey map still needs to know about it (otherwise they'll get 'unexpected state' when trying to continue).

If it's a very quick state (e.g. a process state) then the window for this is only a few seconds, and we might be able to get away without the two steps, but if it's a CRI state, then it could cause quite a few errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for explaining :) I've made the changes now to do the two-step deployment

alternate-doc-invalid-dl:
targetState: COULD_NOT_CONFIRM_DETAILS_PAGE_WITH_DELETION_VALID_ID
alternate-doc-invalid-passport:
targetState: COULD_NOT_CONFIRM_DETAILS_PAGE_WITH_DELETION_VALID_ID
Copy link
Contributor

Choose a reason for hiding this comment

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

(Happy to leave since it's unchanged but this looks a bit odd - if you've got an outstanding CI here, surely the ID is not valid 🤔)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah didn't notice this that's a good point 👀

alternate-doc-invalid-dl:
targetState: COULD_NOT_UPDATE_DETAILS_PAGE
alternate-doc-invalid-passport:
targetState: COULD_NOT_UPDATE_DETAILS_PAGE
Copy link
Contributor

Choose a reason for hiding this comment

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

(ditto)

@thebauSoftwire
Copy link
Contributor Author

thebauSoftwire commented Jan 15, 2025

All existing api tests passed 🟢 I've only turned on the processCandidateIdentity feature set for the audit events as those changed with the routing changes when introducing the new lambda. Only have one audit event test to fix related to the process-async-credential lambda

@thebauSoftwire thebauSoftwire force-pushed the pyic-7797-route-failed-journeys branch from c27b345 to 870abd6 Compare January 15, 2025 09:35
Copy link
Contributor

@Joe-Edwards-GDS Joe-Edwards-GDS left a comment

Choose a reason for hiding this comment

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

Looks good - a couple of things flagged up by the audit event tests :)

}
},
{
"event_name": "IPV_GPG45_PROFILE_MATCHED",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this one actually makes me a little nervous - it means we'll record this event twice (you can see it a couple of events up as well)...

I wonder if we should remove the profile matched event from check-existing-identity, and rely on this to do it, when the feature is enabled? - Will that drop it from any other scenarios?

@@ -222,7 +252,7 @@
"event_name": "IPV_IDENTITY_STORED",
"component_id": "https://identity.local.account.gov.uk",
"extensions": {
"identity_type": "new",
"identity_type": "NEW",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting - is this a deliberate difference?

Looks like IdentityType has a JsonProperty annotation to make it lowercase, but CandidateIdentityType doesn't?

Perhaps we need to add similar annotations, or map one onto the other here...

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly the sort of issue we would have missed without these tests!

api-tests/features/audit-events.feature Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking forward to the cleanup here!

@thebauSoftwire thebauSoftwire marked this pull request as ready for review January 16, 2025 13:09
@thebauSoftwire thebauSoftwire requested review from a team as code owners January 16, 2025 13:09
Copy link
Contributor

@Joe-Edwards-GDS Joe-Edwards-GDS left a comment

Choose a reason for hiding this comment

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

I think this is good, just a couple of typos :)

"I activate the {string} feature set(s)",
function (this: World, featureSet: string) {
this.featureSet = featureSet;
/I activate the '([\w,]+)' feature set(?:s)?( along with the existing feature set(?:s)?)?/,
Copy link
Contributor

Choose a reason for hiding this comment

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

(I sort of wonder if this should be the default, but neat either way!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea 👍

@@ -142,7 +142,7 @@
}
],
"successful": true,
"age": "type[number]"
"age": 59
Copy link
Contributor

Choose a reason for hiding this comment

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

(accidental reversion here)

@@ -76,7 +79,7 @@ Feature: Audit Events
| evidence_requested | {"scoringPolicy":"gpg45","strengthScore":3} |
Then I get a 'page-face-to-face-handoff' page response

Given I activate the 'pendingF2FResetEnabled' feature set
Given I activate the 'pendingF2FResetEnabled' feature set along with the e
Copy link
Contributor

Choose a reason for hiding this comment

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

eeee

@thebauSoftwire thebauSoftwire force-pushed the pyic-7797-route-failed-journeys branch from 5806401 to eb04486 Compare January 17, 2025 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants