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

Allow adding, viewing information about and deleting soft-blocks in the admin #22765

Merged
merged 11 commits into from
Oct 29, 2024

Conversation

diox
Copy link
Member

@diox diox commented Oct 14, 2024

Fixes mozilla/addons#15013

Description

Like other blocklist operations, everything goes through a BlocklistSubmission, which gains a new block_type field to determine what kind of block to apply for this particular submission. Users are not allowed to modify Blocks directly.

Note: If a version is already soft-blocked or hard-blocked, that version won't be selectable when creating a new BlocklistSubmission, even of another type. However, the code underneath should handle overriding the block type for a given blocked version in a submission, and future actions will be added to soften or harden blocks.

Testing

Play around with the admin and add/delete BlocklistSubmission, and then inspect the blocks until you're satisfied. Few things you can try to mix together:

  • Make soft or hard blocks
  • Make soft and hard blocks on add-ons with multiple versions (requires separate submissions)
  • BlocklistSubmission for add-ons with average_daily_users above settings.DUAL_SIGNOFF_AVERAGE_DAILY_USERS_THRESHOLD are subject to approval as before. You can check that it still behaves as expected. You shouldn't be able to modify a block type for a submission you're approving (arbitrary decision on my part, we could change this later)
  • Make soft and/or hard blocks for multiple guids at the same time, for add-ons containing multiple versions
  • Etc

…he admin

Like other blocklist operations, it goes through a BlocklistSubmission, which
gains a new "block_type" field to determine what kind of block to apply for
this particular submission.

Note: If a version is already soft-blocked or hard-blocked, that version won't
be selectable when creating a new BlocklistSubmission, even of another type.
Future actions will be added to soften or harden blocks.
@@ -407,7 +409,6 @@ def add_view(self, request, **kwargs):
'errors': admin.helpers.AdminErrorList(form, []),
'preserved_filters': self.get_preserved_filters(request),
# extra context we use in our custom template
'is_delete': is_delete,
Copy link
Member Author

Choose a reason for hiding this comment

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

Was not actually used (blocks.html widget has its own separate context)

@diox diox marked this pull request as ready for review October 15, 2024 12:17
@diox diox requested review from a team and KevinMind and removed request for a team October 15, 2024 12:24
@KevinMind
Copy link
Contributor

Note: If a version is already soft-blocked or hard-blocked, that version won't be selectable when creating a new BlocklistSubmission, even of another type. However, the code underneath should handle overriding the block type for a given blocked version in a submission, and future actions will be added to soften or harden blocks.

What is the rationale for this? If the logic to determine the underlying block type is internalized, why does blocksubmission need a block type at all?

@diox diox requested a review from eviljeff October 24, 2024 10:20
Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

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

If a version is already soft-blocked or hard-blocked, that version won't be selectable when creating a new BlocklistSubmission, even of another type. However, the code underneath should handle overriding the block type for a given blocked version in a submission, and future actions will be added to soften or harden blocks.

Do you know how this would work? Isn't there a concern that to implement that feature will require changes to what you're landing here?

src/olympia/blocklist/utils.py Outdated Show resolved Hide resolved
src/olympia/blocklist/utils.py Show resolved Hide resolved
src/olympia/blocklist/models.py Show resolved Hide resolved
src/olympia/blocklist/models.py Show resolved Hide resolved
@diox
Copy link
Member Author

diox commented Oct 24, 2024

If a version is already soft-blocked or hard-blocked, that version won't be selectable when creating a new BlocklistSubmission, even of another type. However, the code underneath should handle overriding the block type for a given blocked version in a submission, and future actions will be added to soften or harden blocks.

Do you know how this would work? Isn't there a concern that to implement that feature will require changes to what you're landing here?

I'm not entirely sure how hardening/softening actions will work 100%, but the rough plan I've played with is separate explicit actions that allow you to select only relevant blocked versions, similar to how the add/delete actions work. Some tweaks will be required but allowing overrides will help (since ultimately soften/harden are overrides on top of an existing blockversion), even if it's not currently exposed in the admin in this PR.

@eviljeff
Copy link
Member

If a version is already soft-blocked or hard-blocked, that version won't be selectable when creating a new BlocklistSubmission, even of another type. However, the code underneath should handle overriding the block type for a given blocked version in a submission, and future actions will be added to soften or harden blocks.

Do you know how this would work? Isn't there a concern that to implement that feature will require changes to what you're landing here?

I'm not entirely sure how hardening/softening actions will work 100%, but the rough plan I've played with is separate explicit actions that allow you to select only relevant blocked versions, similar to how the add/delete actions work. Some tweaks will be required but allowing overrides will help (since ultimately soften/harden are overrides on top of an existing blockversion), even if it's not currently exposed in the admin in this PR.

The code appears to support Operations making changes to an existing version's hard/soft status, while blocking new versions (so a mix of add and change), but I can't see how the UI in this patch could allow that. If you've got a plan to get there, then that's fine.

@diox
Copy link
Member Author

diox commented Oct 24, 2024

Yeah, no UI supporting softening/hardening is in this PR. It's only the small backend bit I've changed that supports the future softening/hardening actions.

@wagnerand
Copy link
Member

wagnerand commented Oct 28, 2024

I've manually increased the height of the list in devtools to display everything

@diox would you mind adding that to the PR? The box is too small for many cases.

@diox
Copy link
Member Author

diox commented Oct 28, 2024

I've manually increased the height of the list in devtools to display everything

@diox would you mind adding that to the PR? The box is too small for many cases.

Done in 8cc5c34

- BlockVersion says Hard/Soft Blocked
- BlocklistSubmission says Hard/Soft Block (no "ed")
@diox diox requested a review from eviljeff October 29, 2024 17:07
@diox diox merged commit 97fe21c into mozilla:master Oct 29, 2024
31 checks passed
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.

[Task]: Make it possible to add, view or delete soft-blocks in the admin
4 participants