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

[LUPEYALPHA-1172] EY admin matching details #3348

Merged
merged 4 commits into from
Nov 7, 2024
Merged

[LUPEYALPHA-1172] EY admin matching details #3348

merged 4 commits into from
Nov 7, 2024

Conversation

alkesh
Copy link
Contributor

@alkesh alkesh commented Oct 28, 2024 •
edited
Loading

@alkesh alkesh force-pushed the LUPEYALPHA-1172-ey-admin-matching-details branch 2 times, most recently from b6d4cb6 to e9dc9d5 Compare October 28, 2024 14:19
@alkesh alkesh added the deploy Deploy a review app for this PR label Oct 28, 2024
@github-actions GitHub Actions
Copy link

Deployments

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

@alkesh alkesh force-pushed the LUPEYALPHA-1172-ey-admin-matching-details branch from e9dc9d5 to 075f628 Compare October 28, 2024 14:32
Base automatically changed from LUPEYALPHA-1170-EY-full-claim-view to master October 28, 2024 15:59
@alkesh alkesh force-pushed the LUPEYALPHA-1172-ey-admin-matching-details branch from 1946719 to 075f628 Compare October 28, 2024 16:18
@alkesh alkesh force-pushed the LUPEYALPHA-1172-ey-admin-matching-details branch from 075f628 to 79bb707 Compare October 30, 2024 09:54
slorek
slorek reviewed Oct 30, 2024
View reviewed changes
app/mailers/claim_mailer.rb Show resolved Hide resolved
app/models/claim/matching_attribute_finder.rb Outdated
@@ -43,9 +43,9 @@ def matching_claims

concatenated_columns = "CONCAT(#{attributes.join(",")})"
policies_to_find_matches.map { |policy|
policy::Eligibility.where("LOWER(#{concatenated_columns}) = LOWER(?)", vals.join)
policy::Eligibility.where("LOWER(#{concatenated_columns}) = LOWER(?)", vals.join) if (attributes - policy::Eligibility.column_names).empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 could this result in a missed match? Perhaps it's better to strip out any columns which don't exist but still match those which do, rather than skipping entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So right now, there is never more than one column, so we probably don't need to concatenate here.
I can refactor this code to remove the concat - not sure why it's here.
The column is based on the ELIGIBILITY_MATCHING_ATTRIBUTES for the policy,
which is teacher_reference_number for TSLR, LUPP, FE and ECP.
and passport_number for IRP.
I guess in theory, you could specify multiple columns per policy, but we haven't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To answer your question - I don't think this will result in a missed match.
That where clause is to check for eligibilities that match all of the columns in concatenated_columns.
If one of those columns does not exist on an eligibility, then there will never be a match with that eligibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this commit should let us still do the comparison if the eligibilities have some different attributes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah - that is better 👍🏽

@alkesh alkesh force-pushed the LUPEYALPHA-1172-ey-admin-matching-details branch from 79bb707 to cc7e686 Compare October 30, 2024 14:34
@alkesh alkesh force-pushed the LUPEYALPHA-1172-ey-admin-matching-details branch from cc7e686 to ed5dc73 Compare October 30, 2024 14:41
@alkesh alkesh requested a review from slorek October 31, 2024 09:21
@alkesh alkesh force-pushed the LUPEYALPHA-1172-ey-admin-matching-details branch from ed5dc73 to 908089c Compare October 31, 2024 16:09
@alkesh alkesh force-pushed the LUPEYALPHA-1172-ey-admin-matching-details branch from 908089c to 166ec2a Compare October 31, 2024 17:24
@rjlynch rjlynch force-pushed the LUPEYALPHA-1172-ey-admin-matching-details branch from 13e17f0 to af037ed Compare November 4, 2024 15:14
alkesh and others added 2 commits November 7, 2024 09:53
@alkesh @rjlynch
improve MatchingAttributeFinder and spec
3e5d8b7
@rjlynch
Don't skip check if eligibility is missing attrs
806c202 
If the two eligibilities don't have all the same attributes, rather than
skipping the comparison only compare the attributes they have in common.
Also uses select rather than pluck so we don't need to run a query for
each iteration of the loop
@rjlynch rjlynch force-pushed the LUPEYALPHA-1172-ey-admin-matching-details branch from af037ed to 806c202 Compare November 7, 2024 09:53
@rjlynch rjlynch merged commit 635bd4e into master Nov 7, 2024
14 checks passed
@rjlynch rjlynch deleted the LUPEYALPHA-1172-ey-admin-matching-details branch November 7, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjlynch rjlynch rjlynch left review comments

@slorek slorek slorek approved these changes

@asmega asmega asmega approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@alkesh @asmega @slorek @rjlynch