-
Notifications
You must be signed in to change notification settings - Fork 4
Add additional image details #938
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
base: main
Are you sure you want to change the base?
Conversation
df7cb95 to
60085ba
Compare
| {% endfor %} | ||
| {% for error_list in form.non_field_errors() %} | ||
| {% set ns.errors = ns.errors + [{"text": ",".join(error_list)}] %} | ||
| {% for error in form.non_field_errors() %} |
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.
| {% block bodyEnd %} | ||
| {{ super() }} | ||
|
|
||
| <script> |
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.
MatMoore
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.
The backend looks good, but it looks like there's some missing CSS for progressive enhancement.
I'm not sure if need any more validation on the javascript side for invalid inputs. I just noticed that we are coercing bad values to 0 which seemed a bit suspicious.
| additional_details=self.cleaned_data.get("additional_details", ""), | ||
| ) | ||
|
|
||
| self._add_series_if_provided(study, "MLO", "R", "rmlo_count") |
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.
[Suggestion; not blocking] We could potentially optimise this by using bulk_create instead of create https://docs.djangoproject.com/en/6.0/ref/models/querysets/#bulk-create
manage_breast_screening/mammograms/jinja2/mammograms/add_additional_image_details.jinja
Outdated
Show resolved
Hide resolved
manage_breast_screening/mammograms/jinja2/mammograms/add_additional_image_details.jinja
Outdated
Show resolved
Hide resolved
| // Function to get all currently visible count inputs | ||
| const getCountInputs = () => { | ||
| return [ | ||
| document.getElementById('id_rmlo_count'), |
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.
I'm wondering if it would be simpler to use a data attribute rather than looking these up individually by ID. Something like document.querySelectorAll('[data-count-input]')?
@colinrotherham are there any conventions we should follow for this kind of thing?
manage_breast_screening/mammograms/jinja2/mammograms/add_additional_image_details.jinja
Outdated
Show resolved
Hide resolved
manage_breast_screening/mammograms/jinja2/mammograms/add_additional_image_details.jinja
Outdated
Show resolved
Hide resolved
manage_breast_screening/mammograms/jinja2/mammograms/add_additional_image_details.jinja
Outdated
Show resolved
Hide resolved
| view_position=view_position, | ||
| laterality=laterality, | ||
| count=count, | ||
| ) |
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.
We'll need to decide how we handle a scenario where the user presses the back button and re-submits the form (with or without changes to the field values). Currently this flow creates an additional set of Series records with each re-submission.
Potential options:
- Delete all Series under the given Study and re-create
- Detect existing Series records in the view and redirect somewhere (Check information?) with an info message
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.
I've gone with a recreate in #943 (for the case where the default number of images were taken)
You might be able to reuse the StudyService from there.
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.
Altered to use StudyService.
| "<uuid:pk>/add-additional-image-details/", | ||
| add_additional_image_details_view.AddAdditionalImageDetailsView.as_view(), | ||
| name="add_additional_image_details", | ||
| ), |
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.
Thinking about naming here, and opting for nouns to describe resources where possible — would /image-details suffice here?
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.
I suspect we've been a bit consistent with URLs, and could adopt a common convention.
e.g. we could do something like this, throughout the service:
| Method | URL | Description |
|---|---|---|
| GET | /NOUN |
display a collection |
| GET | /NOUN/<id> |
display a specific item |
| GET/POST | /NOUN/new |
form for adding an item |
| GET/POST | /NOUN/<id>/edit |
form for editing an item |
| GET/DELETE | /NOUN/<id>/delete |
delete an item |
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.
💯
41b49e2 to
5d51dd6
Compare
a9c2eb4 to
8dfc5a9
Compare
If any counts > 1 then show message about repeat images
Added app-js-only to hide elements when javascript not enabled
Renamed add_additional_image_details to add_image_details Replaced /add-additional-image-details/ with /image-details/new
0cea346 to
a537064
Compare
a537064 to
6f3eee7
Compare
|




Add "Add additional image details" page. This is the page that is shown when a user selects "No, add additional image details..." from the "Record images taken" page.
Also included changes to have an incrementing total count of images, and to show "Repeat or extra image details captured on the next screen" message if any count > 1.
Note: This PR does not include the stepper input component. In this PR the form uses
IntegerFieldfor the image count fields. Will add stepper input component in a later PR.Page is not linked to from any other pages yet. Can be accessed directly at http://127.0.0.1:8000/mammograms/1cbea52c-e47d-45e6-a12d-360b37056131/add-additional-image-details/
Description
Jira link
https://nhsd-jira.digital.nhs.uk/browse/DTOSS-11826
Review notes
Review checklist