addons
addons copied to clipboard
[Bug]: ReviewerActionReason has invalid validation rules
What happened?
We have loose restrictions on the ReviewActionReason model
But strict restrictions on the admin form. This leads to being able to create reasons via django that are not visible in reviewer tools and or wouldn't pass admin validation.
What did you expect to happen?
We should have consistent validation for the model regardless of where it is created from unless there are special reasons to differ in admin for example. I don't think there are in this case.
~Additionally, there are no default reasons included in our make initialize command meaning any new environment cannot reject versions when a reason is required.~ https://github.com/mozilla/addons/issues/14930
Is there an existing issue for this?
- [X] I have searched the existing issues
┆Issue is synchronized with this Jira Task
You can create an active ReviewerActionReason with no canned response ; it wouldn't show up in reviewer tools.
@eviljeff thinks it should be fixed at the model level by making either blocked or canned response mandatory. It should then (hopefully!) apply to the admin automatically.
As mentioned in the description, we should also create default ones at initialize time.
Additionally, there are no default reasons included in our make initialize command meaning any new environment cannot reject versions when a reason is required.
We could also split this off into a separate ticket if that introduces notable additional work. On top of creating ReviewActionReasons, we also need to create CinderPolicies and link the two together.
I tried to add new review action reasons in reviewactionreason and I've noticed that:
-
a cinder policy is mandatory (for -stage too if I checked). Is that expected?
-
canned response or canned block response is now required
-
when adding canned response the new reason is present in rev tools pages
-
when adding canned block reason this will show up in a blocksubmission
-
or it can show up in rev tools and blocksubmissions with canned response and canned block response. Example
- a cinder policy is mandatory (for -stage too if I checked). Is that expected?
I didn't change it, but it appears to have been the case since https://github.com/mozilla/addons/issues/9474
I can save the review action reason without adding a Cinder policy when I uncheck "Is active" but then I would not have this reason in rev tools or blocklisting. Hmmm , not making sense yet, even if I'm reading https://github.com/mozilla/addons/issues/9474
I can save the review action reason without adding a Cinder policy when I uncheck "Is active" but then I would not have this reason in rev tools or blocklisting. Hmmm , not making sense yet, even if I'm reading #9474
From my reading of #9474, that seems to be the intended behaviour, but I didn't work on that issue.
Ah ok, I was thinking what if there would be reasons to be added and not be necessarily Cinder related. Nevermind! Thanks!