Skip to content

Conversation

@malcolmbaig
Copy link
Contributor

@malcolmbaig malcolmbaig commented Oct 17, 2025

Description

With the goal of ensuring we have a set of conventions for scoping certain queries by provider, this is an alternative to the approach taken in #558 .

Here we:

  • Add appointments, clinics and participants methods on Provider, each defining how to traverse the relationship.
  • Use request.current_provider.appointments (for example) in views instead of direct access to the Appointment model.

There are also these related changes:

  • Illustrate how we'd remove get_object_or_404 usage (which makes linting easier)
  • Illustrate expanding the approach above by adding appointments methods to Clinic and Participants models as well.
  • It looks like we should be using string references to models when defining foreign keys, to avoid circular import issues. This seems like a useful thing to do regardless of the other changes in this PR.

Jira link

https://nhsd-jira.digital.nhs.uk/browse/DTOSS-11229

Review notes

  • The test fixes in the first two commits don't need review — they're the same as the changes in [11229] Add repository objects for Appointments, Participants and Clinics #558 .
  • More methods can be moved to the various QuerySet classes to tidy up our views (this seems to be fairly common practice in Django, as it is in Rails).
  • Wiring up the linter to CI and an ADR will follow
  • This approach is similar to how you'd access related records in Rails (although there the methods are defined for you as part of the process of declaring relationships between models via has_many, has_many: :through, etc). Does it make sense to do something like this in Django?

@malcolmbaig malcolmbaig requested a review from a team October 17, 2025 14:51
@malcolmbaig malcolmbaig force-pushed the 11229-provider-scoped-queries--v1--methods-on-provider branch from 3119145 to bb18d9e Compare October 17, 2025 14:59
def __str__(self):
return self.name

@property
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go with this approach we might also want to consider customising related_name for some of our foreign keys, so that the naming is more consistent when accessing related objects https://docs.djangoproject.com/en/5.2/ref/models/fields/#django.db.models.ForeignKey.related_name

Otherwise we will have a mix of the pluralised form and "foo_set"

This methods define the relationship traversals for a provider's
appointments and clinics. They allow view code to retrieve these records
correctly scoped to provider.
We can use strings instead of actual models when defining our foreign
keys to make sure we don't trigger circular dependency issues on model
import.
We want to write a linter to discourage the use of direct record
retrieval from Appointment, Clinic, and Participants models. The linter
will look for 'ModelName.objects'. With this in mind, it would be useful
to discourage use of get_object_or_404, which simply accepts the bare
ModelName as an argument.

We'll also likely replace get_object_or_404 at some point with some
custom behaviour.

Replace use of this method with a slightly more verbose alternative.
Allows simpler querying of associated appointments in the participant
show view.
Allows for:

- Retrieval of appointments scoped to a clinic record
- Refactoring of `Appointment.objects.for_clinic_and_filter` to
  `for_filter`.
This linter checks for two things:

- If Model.objects is being used in views, where Model is one of
  Appointment, Clinic or Participant.
- Use of get_object_or_404

We check for the latter because:

- We're going to replace it eventually anyway, probably with some kind
  of flash message usage
- It accepts a model as an arg, with no call to objects. Not using it
  makes it easier to lint for unscoped queries.
@malcolmbaig malcolmbaig force-pushed the 11229-provider-scoped-queries--v1--methods-on-provider branch from 3c171ff to cee05b0 Compare October 22, 2025 13:07
@malcolmbaig malcolmbaig merged commit bb278d8 into main Oct 22, 2025
12 checks passed
@malcolmbaig malcolmbaig deleted the 11229-provider-scoped-queries--v1--methods-on-provider branch October 22, 2025 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants