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

fix: tiles object manager #13630

Merged
merged 10 commits into from Jan 12, 2023
Merged

fix: tiles object manager #13630

merged 10 commits into from Jan 12, 2023

Conversation

pauldambra
Copy link
Member

@pauldambra pauldambra commented Jan 10, 2023

Problem

Since extending soft delete to dashboard tiles we've had to remember to exclude deleted tiles (which is easy to forget e.g. #13629)

It's also easy to cause N+1 by excluding or not when the ORM isn't expecting it

was stacked on top of #13629

Changes

  • overrides the ModelManager on Insight, Dashboard, and DashboardTile to always exclude soft-deleted models
  • adds a model manager to each to allow access to a queryset including soft-deleted items
  • removes an N+1 we had to add in fix: deleted tile aware insights #13599

How did you test this code?

running the tests, checking locally

Copy link
Collaborator

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

This is way more obvious, with the small exception of including_soft_deleted 😅

class DashboardTile(models.Model):
objects = DashboardTileManager()
including_soft_deleted = models.Manager()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, definitely not typical to have two managers on a model. Lots of redundancy. Can we maybe instead add a method like all_even_soft_deleted() to DashboardTileManager()? That'd be plain super().get_queryset()

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm 🤔 I pretty much lifted this straight out of the docs https://docs.djangoproject.com/en/4.1/topics/db/managers/#modifying-a-manager-s-initial-queryset

How about rename to objects_including_soft_deleted? That way as someone starts to type objects... intellisense will show them there are two options?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh fair enough, looks like I skipped out on that part of the docs :D

Copy link
Member Author

Choose a reason for hiding this comment

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

You've not memorised the entire Django docs 🤯 🤣

Copy link
Collaborator

Choose a reason for hiding this comment

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

You bet I've used summaries for the boring required reading books in school

@pauldambra pauldambra merged commit 3af39de into master Jan 12, 2023
@pauldambra pauldambra deleted the fix/tiles-object-manager branch January 12, 2023 11:51
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.

None yet

2 participants