Skip to content

Look up NHS numbers for pending changes #3591

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
Closed
wants to merge 3 commits into from
Closed

Look up NHS numbers for pending changes #3591

wants to merge 3 commits into from

Conversation

thomasleese
Copy link
Contributor

This ensures that when uploading twins, the second patient will have an NHS number in the pending changes and the users will see clearly that it is two different parents and both need to be kept.

Jira Ticket - MAV-273

@thomasleese thomasleese added this to the v2.3.0 milestone May 23, 2025
@thomasleese thomasleese added the bug Something isn't working label May 23, 2025
@tvararu tvararu temporarily deployed to mavis-pr-3591 May 23, 2025 16:17 Inactive
Base automatically changed from dev to main May 23, 2025 16:25
@tvararu tvararu temporarily deployed to mavis-pr-3591 May 23, 2025 16:29 Inactive
@thomasleese thomasleese requested a review from a team as a code owner June 3, 2025 08:42
jhenderson
jhenderson reviewed Jun 3, 2025
View reviewed changes
app/lib/update_patients_from_pds.rb Outdated
priority:,
queue:,
wait: index * wait_between_jobs
wait: jobs_queued * wait_between_jobs
Copy link
Contributor

Choose a reason for hiding this comment

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

How large do you think patients could be? If the collection was large, later jobs could have very significant delays.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree we need to think about this, but maybe we just need to deal with this until we can get Sidekiq in place, which I'd like to start on really soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I agree we have this problem, I don't think this change makes it significantly worse. Most parents don't have pending changes, and therefore there won't be that many additional jobs queued.

@thomasleese thomasleese force-pushed the nhs-lookup-staged branch 2 times, most recently from 5d5926c to 6d86338 Compare June 7, 2025 06:27
misaka
misaka reviewed Jun 9, 2025
View reviewed changes
app/jobs/patient_nhs_number_lookup_with_pending_changes_job.rb Outdated
Comment on lines 10 to 18
patient_with_pending_changes.nhs_number =
nil unless patient.nhs_number_changed?

return unless patient_with_pending_changes.changed?

if patient_with_pending_changes.nhs_number.present? &&
!patient_with_pending_changes.invalidated?
return
end
Copy link
Contributor

Choose a reason for hiding this comment

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

The goal of this bit is really unclear to me. Is it just me? Would it be worth adding a comment just to make it a bit clearer what it is doing? Or maybe put it behind a named method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah agreed. It's basically doing this logic but for the patient with pending changes, but I will make it clearer.

@thomasleese thomasleese removed this from the v2.3.0 milestone Jun 10, 2025
@thomasleese thomasleese requested a review from a team as a code owner June 10, 2025 18:27
@thomasleese thomasleese changed the base branch from main to next June 12, 2025 16:08
@thomasleese thomasleese force-pushed the nhs-lookup-staged branch 2 times, most recently from 5cb7b60 to 059779f Compare June 18, 2025 18:00
@thomasleese thomasleese force-pushed the next branch 2 times, most recently from d0a8129 to 56df2b6 Compare June 19, 2025 17:15
@thomasleese thomasleese force-pushed the nhs-lookup-staged branch 7 times, most recently from 1cf6b30 to 4e1811d Compare July 15, 2025 13:48
@thomasleese thomasleese force-pushed the nhs-lookup-staged branch 11 times, most recently from dc444db to e60f12e Compare August 12, 2025 10:23
@thomasleese thomasleese force-pushed the nhs-lookup-staged branch 5 times, most recently from 65b8905 to e5a081b Compare August 16, 2025 12:26
@benilovj
Copy link
Contributor

@thomasleese @misaka @murugapl how does the current PDS work affect this change? Is it worth getting this one over the line e.g. in v3.2.0?

@thomasleese
Copy link
Contributor Author

@thomasleese @misaka @murugapl how does the current PDS work affect this change? Is it worth getting this one over the line e.g. in v3.2.0?

I've been wondering about this, I actually think the PDS work makes this particular PR redundant, as we'll be looking up NHS numbers prior to importing them, so this problem goes away (whereas now the matching is happening before the PDS lookup).

@thomasleese
Add PatientNHSNumberLookupWithPendingChanges
e5dc0ab 
This adds a job which handles updating the NHS number for a patient, if
it has pending changes, and those changes are different enough to cause
the NHS number to be different. This fixes a scenario where twins are
uploaded and get flagged as duplicates, but the NHS number shows as the
same meaning it's not obvious from the duplicate resolution UI that the
patient needs to be created new.
@thomasleese
Enqueue PatientNHSNumberLookupWithPendingChangesJob
2787b2f 
At the same time as looking up the NHS number we should look up the NHS
number on any pending changes to highlight this if the NHS number should
be different.
@thomasleese
Add a test for uploading twins
20c8015 
This adds a feature test to cover the scenario where twins are uploaded
with the same last name, address and date of birth one after the other,
ensuring that the separate NHS numbers are shown in the duplicate
resolution screen.
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@benilovj
Copy link
Contributor

@thomasleese @misaka @murugapl how does the current PDS work affect this change? Is it worth getting this one over the line e.g. in v3.2.0?

I've been wondering about this, I actually think the PDS work makes this particular PR redundant, as we'll be looking up NHS numbers prior to importing them, so this problem goes away (whereas now the matching is happening before the PDS lookup).

Ok cool. Is there any test coverage that is worth bringing in to cover the bug, even if the implementation is ok?

@thomasleese
Copy link
Contributor Author

Ok cool. Is there any test coverage that is worth bringing in to cover the bug, even if the implementation is ok?

Yes, the twins feature test we could definitely keep I think.

@thomasleese thomasleese mentioned this pull request Aug 18, 2025
Merged
@thomasleese
Copy link
Contributor Author

I've added the test in #4315.

@thomasleese thomasleese deleted the nhs-lookup-staged branch September 5, 2025 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@misaka misaka misaka left review comments

@jhenderson jhenderson jhenderson approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thomasleese @benilovj @misaka @jhenderson @tvararu