Conversation
MatMoore
changed the title
MatMoore
changed the title
Base automatically changed from
DTOSS-12517-prevent-other-users-accessing-in-progress-appointments
to
main
March 31, 2026 14:58
MatMoore
changed the title
MatMoore
force-pushed
the
DTOSS-12517-fix-confirm-identity-completed-state
branch
2 times, most recently
from
2e33c74 to
5436ce6
Compare
MatMoore
commented
Apr 1, 2026
| for step in AppointmentWorkflowStepCompletion.StepNames: | ||
| is_completed = step.name in self.completed_steps | ||
| is_current = step.name == active_workflow_step | ||
| is_disabled = not all_previous_steps_completed and not is_current |
Contributor
Author
There was a problem hiding this comment.
Changed this line so that in the case where the current step is complete it can still be disabled.
In the case of taking over an appointment, completed_steps will include later steps completed by the original user, but not CONFIRM_IDENTITY.
MatMoore
commented
Apr 1, 2026
| self.appointment.completed_workflow_steps.values_list( | ||
| "step_name", flat=True | ||
| ).distinct() | ||
| self.appointment.completed_workflow_steps.filter( |
Contributor
Author
There was a problem hiding this comment.
This now filters out CONFIRM_IDENTITY steps completed by someone other than the current user.
swebberuk
approved these changes
Apr 14, 2026
Contributor
swebberuk
left a comment
swebberuk
left a comment
There was a problem hiding this comment.
Looks good to me. Will need to resolve conflicts before merging.
swebberuk
force-pushed
the
DTOSS-12517-fix-confirm-identity-completed-state
branch
from
f9c18c7 to
5436ce6
Compare
Previously this test was mixed in with the test of medical history, but I'd like to cover everything to do with workflow state in test_mammogram_workflow.py. The previous test actually tested a nonsensical situation where pausing and resuming kicks you back to confirm identity. This is because the medical history test is jumping into the middle of an appointment, and AppointmentWorkflowStepCompletion hasn't been populated properly. This is the case for several other appointment tests as well. It is likely not causing problems because of a lax enforcement of workflow state that we should address. The new version of the test goes through the workflow from the beginning so the AppointmentWorkflowCompletion data is in the correct state.
When computing the completed workflow steps, don't count Confirm Identity as completed if it was completed by another user.
- Delegate to AppointmentWorkflowService for this - Extract out into a different presenter, as AppointmentPresenter has too many responsibilities
In the case where a user takes over an appointment from another user, we redirect them to confirm identity regardless of whether the other user completed it. However, if the first user completed later steps, the workflow sidebar was showing these steps as clickable links, providing a route to bypass the identity check. This commit ensures that those later steps will be considered disabled and the link not rendered. In this case the steps are both disabled and complete, so we should show a checked icon, but in grey (to match disabled steps with numbers).
MatMoore
force-pushed
the
DTOSS-12517-fix-confirm-identity-completed-state
branch
from
5436ce6 to
20110fa
Compare
|
|
The review app at this URL has been deleted: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
We have implemented a rule,
is_identity_confirmed_by_user, meaning that an identity check completed by one user doesn't count if a second user takes the appointment over.This logic is being used to redirect the user to the appropriate step when resuming an appointment, but it was not being applied when rendering the workflow sidebar, creating two bugs:
I've changed this so that the identity check step is correctly marked as not completed, and the following steps are greyed out even if complete.
Taking over other people's appointments has not been fully considered in the prototype yet, but we have a design ticket to think about this further: https://nhsd-jira.digital.nhs.uk/browse/DTOSS-11468
In the meantime, this change should make the sidebar and redirect behaviour consistent with each other.
It is still possible to bypass workflow steps by hacking the URL, but I've raised DTOSS-12594 to deal with that separately.
Jira link
https://nhsd-jira.digital.nhs.uk/browse/DTOSS-12593
Review notes
Please let me know if there's any improvements that could be made to the tests here. I'd like to ensure we have good coverage, and there might be states I haven't thought of.
Review checklist
/api/v1/), confirm whether it is a breaking change — if so, a new major version (/api/v2/) is required (see ADR-006)