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

Add BlockType enum to BlockVersion model #22706

Closed
wants to merge 3 commits into from

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented Sep 26, 2024

Relates to: mozilla/addons#15014

Description

TBD

Context

Testing

Checklist

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.
  • Add before and after screenshots (Only for changes that impact the UI).
  • Add or update relevant docs reflecting the changes made.

@KevinMind KevinMind force-pushed the soft-block-bloomfilter branch 4 times, most recently from e94f814 to ba87c89 Compare September 26, 2024 18:26
Comment on lines 170 to 176
_queryset_class = BlockVersionQuerySet

def soft_blocked(self):
return self.get_queryset().soft_blocked()

def hard_blocked(self):
return self.get_queryset().hard_blocked()
Copy link
Member

@diox diox Sep 26, 2024

Choose a reason for hiding this comment

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

You might be able to replace that with just... nothing, and defining objects below with as_manager(), like this: objects = BlockVersionQuerySet.as_manager().

For more complex cases where you do want the functionalty ManagerBase brings we can also define an empty manager inheriting from ManagerBase and use from_queryset() like this: BlockVersionManager.from_queryset(BlockVersionQuerySet)()

If we have custom methods in the manager that somehow don't belong in the queryset but do call queryset stuff, then it's no longer appropriate, but most of the time that should be enough. We have a bunch of old code using the method you used, but for new cases without anything fancy we should try the modern Django way and avoid repetition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This belongs in the docs.

@KevinMind KevinMind force-pushed the soft-block-bloomfilter branch 2 times, most recently from 9056990 to 07e91ff Compare September 27, 2024 15:07
@KevinMind KevinMind changed the title MLBF Add BlockType enum to BlockVersion model Sep 27, 2024
@KevinMind KevinMind requested a review from diox September 27, 2024 15:15
@KevinMind KevinMind marked this pull request as ready for review September 27, 2024 15:15
@KevinMind KevinMind marked this pull request as draft September 30, 2024 09:23
@KevinMind KevinMind closed this Oct 11, 2024
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