Skip to content
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

Reduce reliance on get_absolute_url #244

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

meshy
Copy link
Contributor

@meshy meshy commented Jun 13, 2024

TODO: Split this PR into consumable chunks.

meshy added 13 commits June 13, 2024 20:50
The block above this line fetches the latest matching version, so it's
pointless to then use that to get the latest version's URL.

Instead, we get the URL of the object we already have.
This will allow us to add custom queryset methods in an upcoming commit.
Django models shouldn't know anything about URLs. This is a step in the
direction of removing `get_absolute_url` from the models.
Before this change, we used a model helper method to fetch the URL of
the latest version of a matching class.

Now we fetch the latest version, and then get its URL.
Part of the process of removing the models is ensuring that we don't
rely on any methods that live on them.

This change removes most references to `get_absolute_url` on `Klass`,
leaving behind those in the templates.
We're in the process of moving logic off the models.
This model method was only used by one view.

This moves the logic from the model to the view.
We were previously manually doing what defaultdict does for us.
Before this change, we didn't have a test which covered rendering
overridden attributes.

This change adds UpdateView to the snapshot tests, to catch regressions.
We previously tested against a couple of simple artificial examples,
but I didn't trust it to cover all the possible cases.

This change ensures that we're testing against a complicated real-world
example, UpdateView, which I believe to represent sufficient complexity.
@meshy meshy force-pushed the remove-url-knowledge-from-models branch from 5e1dc15 to e9f1b79 Compare June 16, 2024 18:43
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.

1 participant