refactor medical history views #824
Conversation
This allows us to handle things like auditing in a single place, so we don't have to verify that every form handles it correctly.
This defines context that is common to all of them, and back/forward navigation that goes back to the record medical information page.
Use "Added/Updated/Deleted <THING>" format for all medical history forms.
| If needed, individual messages can be overriden in subclasses. | ||
| """ | ||
|
|
||
| thing_name = None |
There was a problem hiding this comment.
The idea here is that each concrete view defines this variable and we will generate default messages/titles according to the pattern.
Alternatively we could use model instead of thing_name and use the verbose_name of the model, like some of the built in generic views do, but that would be a bit less flexible.
|
|
||
| class BaseMastectomyOrLumpectomyHistoryView(InProgressAppointmentMixin, FormView): | ||
| class AddMastectomyOrLumpectomyHistoryView(MedicalInformationMixin, AddWithAuditView): | ||
| form_class = MastectomyOrLumpectomyHistoryItemForm |
There was a problem hiding this comment.
There is duplication between the Add and Update versions of the history views. Need to duplicate form_class, template_name, thing_name and get_form_kwargs. I think its okay, but could consider how to share.
malcolmbaig
left a comment
malcolmbaig
left a comment
There was a problem hiding this comment.
This looks good to me 👍🏼 . I think the things covered by the mixins is easily understood, and if we wanted to add shared functionality to our class based views, it's pretty clear what should go in the generic views and what should go in a mixin.
swebberuk
left a comment
swebberuk
left a comment
There was a problem hiding this comment.
Looks good to me. 👍
Noticed there is some duplication between the Add and Update versions of the history views - but we could review again later if we find it becomes too repetitive.
Yeah, I wasn't sure how far to take it so I stopped here, but we can definitely do another refactor later. I feel like this multiple inheritance style can itself be a bit hard to maintain though so I wanted to avoid adding too many classes to the inheritance hierarchy. |
3 participants
Description
This PR takes the responsibility for auditing out of the forms, and moves it to generic create/update views. By doing this we can be confident it's handled for all the medical history forms without individually testing each one.
I've also tried to align the flash messages as they were inconsistent with each other. Generally we've gone with "Created/updated/deleted XYZ"
Jira link
https://nhsd-jira.digital.nhs.uk/browse/DTOSS-11781
Review notes
Is there too much magic going on with the mixins? If we prefer the views to be more explicit I can move context setting / messaging back into the concrete views. I thought it might be useful to do this in a generic way, but I'm not sure about it.
Review checklist