-
Notifications
You must be signed in to change notification settings - Fork 4
Medical history - update history of cysts #760
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.
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
Conversation
eb20c10 to
7cb5280
Compare
7cb5280 to
223e68e
Compare
| ) | ||
| except CystHistoryItem.DoesNotExist: | ||
| logger.exception("History item does not exist for kwargs=%s", self.kwargs) | ||
| return redirect(self.get_success_url()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay to return a HttpResponseRedirect here?
When I alter the URL to contain a UUID that is not in the database then I see the "History item does not exist for" log message but then get a django error page for a TypeError and message "manage_breast_screening.mammograms.forms.cyst_history_form.CystHistoryUpdateForm() argument after ** must be a mapping, not HttpResponseRedirect"
Does get_form_kwargs() need to always return a dict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, this was a bug 🤦🏻 I've just pushed a fix for this.
|
|
||
| class ChangeCystHistoryView(BaseCystHistoryView): | ||
| form_class = CystHistoryUpdateForm | ||
| template_name = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to include template_name here? Can we use the one from BaseCystHistoryView instead?
this HTML is temporary until we implement the proper design for the record medical information page.
This is visually hidden, and ensures that we don't have multiple links close together that are hard to distinguish by users of assistive tech
This is now moved to the AppointmentMixin
a108e58 to
c960cf7
Compare
|
@swebberuk I've just addressed the above comments and rebased the branch - could you give it another look? |
swebberuk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. 👍
Description
This extends the cyst history item form to allow for editing.
The display of these items is just a placeholder at the moment - we will have another ticket to sort out this page. For now I've just added the change links above each summary list.

Jira link
https://nhsd-jira.digital.nhs.uk/browse/DTOSS-11525
Review notes
Review checklist