addons icon indicating copy to clipboard operation
addons copied to clipboard

[Bug]: ReviewerActionReason has invalid validation rules

Open KevinMind opened this issue 1 year ago • 2 comments

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

KevinMind avatar Jul 18 '24 15:07 KevinMind

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.

diox avatar Jul 23 '24 14:07 diox

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.

wagnerand avatar Jul 23 '24 14:07 wagnerand

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 a reason

  • 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

ioanarusiczki avatar Aug 06 '24 06:08 ioanarusiczki

  • 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

eviljeff avatar Aug 06 '24 09:08 eviljeff

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

ioanarusiczki avatar Aug 06 '24 10:08 ioanarusiczki

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.

eviljeff avatar Aug 06 '24 10:08 eviljeff

Ah ok, I was thinking what if there would be reasons to be added and not be necessarily Cinder related. Nevermind! Thanks!

ioanarusiczki avatar Aug 06 '24 10:08 ioanarusiczki