Add bulk banning functionality to the admin#24939
Conversation
Also make banning through the changelist action use the same async task underneath to scale to a larger number of users.
eviljeff
left a comment
eviljeff
left a comment
There was a problem hiding this comment.
I expected there to be ADMIN_USER_BANNED entries in activitylog but there wasn't - potentially because it's being run in the task?
Also some comments about cleaning/validation.
|
|
||
| def clean_user_ids(self): | ||
| data = self.cleaned_data.get('user_ids', '') | ||
| user_ids = {id_ for id_ in data.splitlines() if id_.isdigit()} |
There was a problem hiding this comment.
We probably want to trim here too - a trailing space makes the number no longer isdigit() is True.
| data = self.cleaned_data.get('user_ids', '') | ||
| user_ids = {id_ for id_ in data.splitlines() if id_.isdigit()} | ||
| if not user_ids: | ||
| raise forms.ValidationError('This field must contain a least one user id') |
There was a problem hiding this comment.
I do wonder if the form shouldn't raise on bad input rather than just silently ignore. I don't see a use case of Operations pasting in content with a mix of valid user ids and other random text they want to be ignored - accidentaly adding an extra character on the same line as a user id seems a more common case, that they would want to know about and correct.
There was a problem hiding this comment.
My reasoning was, it could be coming from a csv with headers and we should ignore those, but for the rest, the confirmation that shows how many users we found should be enough (Operations folks can ensure what they paste has unique ids to compare the number shown in the confirmation page with the number of lines from their input)
This comment was marked as resolved.
This comment was marked as resolved.
2 participants
Also make banning through the changelist action use the same async task underneath to scale to a larger number of users.
Fixes mozilla/addons#16224
Fixes mozilla/addons#15580