-
Notifications
You must be signed in to change notification settings - Fork 545
Allow policies to be selected directly for reviewer tools actions #23437
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 policies to be selected directly for reviewer tools actions #23437
Conversation
8fb3367
to
c7fd3b5
Compare
c7fd3b5
to
3761403
Compare
3761403
to
e0befef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments on the code. I will try to verify tomorrow.
def create_option( | ||
self, name, value, label, selected, index, subindex=None, attrs=None | ||
): | ||
obj = label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is breaking by brain... label is an argument, that is set to obj and then the stringified version of that is set back to label?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hah, yeah - we use the same trick for a number of fields/widgets for reviewer forms - the field knows about the instances, but by the time it's passed to create_option
in the widget the instance is unavailable, so we can't render html based on it. The trick is in WidgetRenderedModelMultipleChoiceField
where rather than returning a string for the label that represents the instance, we return the instance object as the label; so we can then use it in create_option
.
Merge conflicts? |
undoubtedly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚢
mmm, I didn't notice that, but I can see why that'd happen (they're not django form fields) I'll file a follow-up. --edit mozilla/addons#15612 |
Fixes: mozilla/addons#15513
Description
Allow CinderPolicy to be selected directly for reviewer tools actions, rather then ReviewActionReasons (which were associated with a policy). placeholder values for policies can be filled in by the reviewer. With the waffle switch enabled, policies are available everywhere ReviewActionReasons were - except reviewer Reply action which has it's own email template and code path (it will be addressed in follow-ups)
Context
The (in theory) last piece of the work to replace ReviewActionReason - this took more work than expected as I didn't really consider the extra changes to show some policies for some actions and not others (e.g. we don't want the Approve policy shown for the Reject action).
Testing
First check that with the waffle switch off everything is as before:
The enable the waffle switch
expose in reviewer tools
for some policies{PLACEHOLDER}
values in them{XXX}
should have an input field to fill in a value forXXX
Checklist
#ISSUENUM
at the top of your PR to an existing open issue in the mozilla/addons repository.