931237 - [commbadge] don't allow reviewers to hide review comments from senior reviewers (remove checkboxes)

Status: RESOLVED FIXED

(Marketplace Graveyard :: Reviewer Tools, defect, P2)

Description

[Kevin Ngo [:ngoke]]

On an app's review page, when making a comment, checkboxes are displayed under "Make this action visible to". The checkboxes are "Developer", "Reviewer", "Senior Reviewer", "Staff", and "Mozilla Contact".

Reviewers have said they don't want these checkboxes besides developer one. The use case they didn't want to allow was reviewers hiding actions from senior reviewers.

I need clarification whether they want the other checkboxes besides "Senior Reviewer" to stick around. Are there other use cases such as Senior Reviewers hiding comments from Reviewers, or Reviewers wanting to hide comments from just plain Mozilla staff. Or should stick to have default permission levels on who see what depending on what the action is?

Let me know if you have thoughts or questions.

Comment 1

[Andrew Williamson [:eviljeff]]

I can think of scenarios where we might want to overshare information (i.e. let the developer know why we escalated an app - but that doesn't seem to be possible anyway).  But in general I don't think we need to be able to alter the defaults and it will get confusing later unless every single review entry is clearly marked with its visibility (more UX!)

If the back-end code is going to stay then it shouldn't be possible to limit any message to more than your level.  So Developer < Moz-Contact < Reviewer < Snr-Reviewer < Staff - i.e. a Reviewer shouldn't be able to remove visibility from Snr-Reviewer or Staff.  Tbh it might be easier just to remove the ability to change visibility all together.

Comment 2

[Kevin Ngo [:ngoke]]

I agree, I will stick the visibilities to the current defaults.

I will need to check the code, but I believe they are:

escalation: visible to reviewers
approve/reject/comment/more-info: visible to developers + reviewers

Comment 3

[Kevin Ngo [:ngoke]]

edit:

escalation/comment: visible to reviewers
approve/reject/more-info: visible to developers + reviewers

Comment 4

[Kevin Ngo [:ngoke]]

https://github.com/mozilla/zamboni/pull/1525

Comment 5

[Victor Carciu]

Kevin , can you please add some specific STRs for this case or mark the bug as [qa-]?

Comment 6

[Kevin Ngo [:ngoke]]

STR:

1. Go to review page for an app (/reviewers/app/XXX/review).
2. Click on each of the reviewer action buttons at the bottom.

Expected:

No checkboxes related to visibility, all review actions work.

Actual: 

Checkboxes related to visibility.

Comment 7

[Victor Carciu]

Thanks Kevin!
No checkboxes related to visibility were displayed, all actions worked but I am not sure about a scenario.
I performed all review actions and at the end I tried to push the app to public (The app was rejected and disabled by me before) but I received the "Oops..." message : http://screencast.com/t/5ZKwZJcvY3q

I think that I should be able to push app to public even if I rejected or disabled it in the past.
Please let me know what do you think.

Please note that I was able to push an app to public if no other actions were performed on this app previously.

Comment 8

[Andrew Williamson [:eviljeff]]

(In reply to Victor Carciu from comment #7)
> Thanks Kevin!
> No checkboxes related to visibility were displayed, all actions worked but I
> am not sure about a scenario.
> I performed all review actions and at the end I tried to push the app to
> public (The app was rejected and disabled by me before) but I received the
> "Oops..." message : http://screencast.com/t/5ZKwZJcvY3q
> 
> I think that I should be able to push app to public even if I rejected or
> disabled it in the past.
> Please let me know what do you think.
> 
> Please note that I was able to push an app to public if no other actions
> were performed on this app previously.

I've seen that issue before, but I don't know if its been filed (I just end up fixing the status first).  So I don't think its related to this commit.

Comment 9

[Kevin Ngo [:ngoke]]

I filed the issue Victor found @ https://bugzilla.mozilla.org/show_bug.cgi?id=957432

This can be verified if those checkboxes are gone and basic review actions work.