Fix non-unique MRNs are permitted when "Require MRN" setting is enabled #86
Conversation
| if not patient_api.is_patient_required(): | ||
| # MRN is not a required field | ||
| return | ||
| empty = not data.mrn |
There was a problem hiding this comment.
This could be a better test here: len(data.mrn) == 0
But the above seems to be also ok, even if "0" as MRN comes in.
| if not data.mrn: | ||
| raise Invalid(_("Patient Medical Record is missing or empty")) | ||
| if empty and required: | ||
| # empty and required. It cannot be |
There was a problem hiding this comment.
To be or not to be – it cannot be ...
| patient = patient_api.get_patient_by_mrn( | ||
| data.mrn, full_object=False, include_inactive=True) | ||
| elif empty: | ||
| # empty and non required |
There was a problem hiding this comment.
small typo in non, but ignorable
| if not patient_api.is_mrn_unique(data.mrn): | ||
| raise Invalid( | ||
| _("invalid_patient_mrn_non_unique", | ||
| default="Patient Medical Record # must be unique")) |
There was a problem hiding this comment.
I would rather write "Patient Medical Record Number must be unique" and above "Patient Medical Record Number is required"
| @@ -430,8 +436,7 @@ def setMRN(self, value): | |||
| return | |||
|
|
|||
| # Check if a patient with this same MRN already exists | |||
| if patient_api.get_patient_by_mrn(value, full_object=False, | |||
| include_inactive=True): | |||
| if not patient_api.is_mrn_unique(value): | |||
| raise ValueError("Value is not unique: {}".format(value)) | |||
There was a problem hiding this comment.
Maybe use the same message pattern here: "Patient Medical Record Number in not unique"
| for="senaite.patient.content.patient.IPatient | ||
| Products.DCWorkflow.interfaces.IBeforeTransitionEvent" | ||
| handler=".patient.on_before_transition" | ||
| /> |
There was a problem hiding this comment.
At the time of writing, I thought this subscriber was kind of smart. But I agree to remove it to avoid unneccessary brain twists and that it is probably just confusing instead.
There was a problem hiding this comment.
Since mrn must be unique regardless of patient status, this subscriber is no longer needed
Description of the issue/feature this PR addresses
This Pull Request purges duplicates and ensures uniqueness of MRNs, except when empty
Current behavior before PR
When the setting "Require MRN" is enabled, user can set non-unique MRNs on patient creation
Desired behavior after PR is merged
User can never set non-unique MRNs on patient creation
--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.