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.

Sign up for GitHub

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

Jump to bottom

[CAPT-2051] Better OL name #3454

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[CAPT-2051] Better OL name #3454

wants to merge 4 commits into from

Conversation

asmega
Copy link
Contributor

@asmega asmega commented Dec 5, 2024 •
edited
Loading

Context

Changes

  • Persist all the names that OL returns
  • When checking against OL name we match entered name matches the start and end of returned OL name
  • onelogin_idv_full_name is no longer a virtual attribute and returns the full name OL returns
  • We backfill onelogin_idv_full_name with data we have which is concatenation of first and last name

Notes

  • Once this is complete we can work to remove onelogin_idv_first_name and onelogin_idv_last_name as these will become redundant

@asmega asmega force-pushed the better-ol-name branch 2 times, most recently from 45e400f to 429c5b3 Compare December 5, 2024 15:24
@asmega asmega added the deploy Deploy a review app for this PR label Dec 5, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 5, 2024

Deployments

Journey URL
Additional Payments https://claim-additional-payments-for-teaching-review-3454-web.test.teacherservices.cloud/additional-payments/claim
Student Loans https://claim-additional-payments-for-teaching-review-3454-web.test.teacherservices.cloud/student-loans/claim
Further Education https://claim-additional-payments-for-teaching-review-3454-web.test.teacherservices.cloud/further-education-payments/landing-page
Early Years Payment https://claim-additional-payments-for-teaching-review-3454-web.test.teacherservices.cloud/early-years-payment/landing-page
Relocation Payments https://claim-additional-payments-for-teaching-review-3454-web.test.teacherservices.cloud/get-a-teacher-relocation-payment/landing-page
Admin https://claim-additional-payments-for-teaching-review-3454-web.test.teacherservices.cloud/admin

slorek
slorek reviewed Dec 20, 2024
View reviewed changes
Copy link
Contributor

@slorek slorek left a comment

Choose a reason for hiding this comment

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

Alternatively we could consider keeping the separate first name and last name fields, but concatenating the name parts for GivenName and FamilyName types, rather than only selecting the first of each

app/models/one_login/core_identity_validator.rb Outdated
@@ -21,6 +21,10 @@ def date_of_birth
Date.parse(decoded_jwt[0]["vc"]["credentialSubject"]["birthDate"][0]["value"])
end

def full_name
name_parts.map { |hash| hash["value"] }.join(" ")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we ensure that the FamilyName part(s) are always last? I suspect there's no guarantee it will be ordered this way already.

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 assume its in the correct order as theres no other data to signify the position of each part. regardless I've tweaked the implementation so FamilyName parts are always at the end

@asmega
Copy link
Contributor Author

asmega commented Jan 2, 2025

Alternatively we could consider keeping the separate first name and last name fields, but concatenating the name parts for GivenName and FamilyName types, rather than only selecting the first of each

Not sure I understand. Are you saying we should use all GivenName parts for the first name? If so we also collect middle_name in the app which would probably cause issues if all given names become the first name

@asmega asmega marked this pull request as ready for review January 2, 2025 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slorek slorek slorek left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
deploy Deploy a review app for this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@asmega @slorek