Allow symptoms to be highlighted to image readers#1386
Allow symptoms to be highlighted to image readers#1386
Conversation
swebberuk
force-pushed
the
DTOSS-12818-highlight-symptom
branch
2 times, most recently
from
12413ac to
a3b19ea
Compare
| migrations.AddField( | ||
| model_name='symptom', | ||
| name='highlight_to_readers', | ||
| field=models.BooleanField(default=True), |
There was a problem hiding this comment.
Defaulting to true. Means any existing symptoms will be marked as "highlight to image readers".
|
|
||
| def _get_heading_description(self): | ||
| suffix = " a recognised symptom of breast cancer. Information recorded here will be highlighted during image reading." | ||
| if self.symptom_type_name == "lump": |
There was a problem hiding this comment.
Feels awkward that BaseSymptomFormView has logic about lumps.
Could override get_context_data for lumps, but then would have to duplicate in AddSymptomLumpView and UpdateSymptomLumpView.
There was a problem hiding this comment.
I'd prefer a bit of duplication to having symptom-specific logic in the base class. Could you override get_context_data for each symptom that needs it and just hardcode the actual string to be used?
That also solves the problem mentioned above of setting heading_description values even when it's not appropriate for the symptom.
There was a problem hiding this comment.
Now override get_context_data for each view that requires heading_description.
| "caption": participant.full_name, | ||
| "heading": f"Details of the {self.symptom_type_name.lower()}", | ||
| "page_title": f"Details of the {self.symptom_type_name.lower()}", | ||
| "heading_description": self._get_heading_description(), |
There was a problem hiding this comment.
BaseSymptomFormView is setting heading_description for all symptom types - even breast pain and 'other' that shouldn't have the "Information recorded here will be highlighted during image reading" message displayed.
This works as breast pain and 'other' have their individual .jinja templates that do not use heading_description, but isn't obvious that's how it works.
| recently_resolved = models.BooleanField(null=False, default=False) | ||
| when_resolved = models.CharField(blank=True, null=False) | ||
|
|
||
| highlight_to_readers = models.BooleanField(null=False, default=True) |
There was a problem hiding this comment.
Defaults to True, so any symptom types that do not ask "Highlight this symptom to readers?" are highlighted to image readers by default.
Add option for breast pain and 'other' symptoms to be highlighted to readers Display info text for symptoms that are always highlighted to readers Add highlight_to_readers column to participants_symptom Use 'Highlight to image readers' tag on medical info page Add heading_description to skin and nipple change templates
swebberuk
force-pushed
the
DTOSS-12818-highlight-symptom
branch
from
a3b19ea to
194faa5
Compare
|
|
||
| if self._symptom.highlight_to_readers: | ||
| key = { | ||
| "html": format_html( |
There was a problem hiding this comment.
I don't think we should be using django's html utils here...
- Strings marked safe by django are not considered safe by jinja as they use different mechanisms. So I think even though this looks correct locally, I wouldn't trust that it will be handled correctly by the templates.
- I think we should avoid wrangling html in python if we can, or at least restrict it to a few common helpers. The html aware code looks very similar to unsafe string interpolation so there's possibilities for xss issues to creep in if we're not careful. Doing it in Jinja should generally be safer as there is autoescaping by default.
There was a problem hiding this comment.
These values are being used by
{{ summaryList({
"rows": symptom_rows
}) }}
Would you prefer that the presenter returns something like:
key = {
"text": self._symptom.symptom_type.name,
"highlight": self._symptom.highlight_to_readers,
}
and a new symptomsList jinja macro adds the "Highlight to image readers" tag to all symptoms that require it, before it calls summaryList?
There was a problem hiding this comment.
Added symptoms_summary macro.
|
|
||
| def _get_heading_description(self): | ||
| suffix = " a recognised symptom of breast cancer. Information recorded here will be highlighted during image reading." | ||
| if self.symptom_type_name == "lump": |
There was a problem hiding this comment.
I'd prefer a bit of duplication to having symptom-specific logic in the base class. Could you override get_context_data for each symptom that needs it and just hardcode the actual string to be used?
That also solves the problem mentioned above of setting heading_description values even when it's not appropriate for the symptom.
Adds 'Highlight to readers' tag if required
|
| }) }} | ||
| {% endmacro %} | ||
|
|
||
| {% macro symptoms_summary(symptom_rows, classes='') %} |
There was a problem hiding this comment.
symptoms_summary adds "Highlight to image readers" tag if required, then calls summaryList.
Alternative to formatting HTML in SymptomPresenter.
2 participants
Description
Add option for breast pain and 'other' symptoms to be highlighted to image readers.
Breast pain and 'other' are the only two symptoms where the user has to choose to highlight them to image readers.
Lump, 'swelling or shape change', skin change and nipple change are all always highlighted to image readers. For those symptoms, display info text stating "Information recorded here will be highlighted during image reading".
Add
highlight_to_readerscolumn to symptons tableDisplay 'Highlight to image readers' tag on medical info page.
Jira link
https://nhsd-jira.digital.nhs.uk/browse/DTOSS-12818