Launchpad itself

Can request a review from someone who can't see the merge proposal

Bug #330290 reported by Martin Albisetti
58
This bug affects 7 people
Affects Status Importance Assigned to Milestone
Launchpad itself
Fix Released
Low
Ian Booth

Bug Description

It turns out that you can request a review from a person who doesn't have permissions to view the (private) branch, so we end up in a situation where I need to review something I can't see :)
In my case, see: https://code.edge.launchpad.net/~barry/launchpad/affiliations-db/+merge/3526

Related branches

lp:~wallyworld/launchpad/confirm-reviewer-subscription-330290
j.c.sackett (community): Approve
Richard Harding (community): Approve (code*)
Diff: 670 lines (+470/-65)
10 files modified
lib/lp/app/javascript/picker/picker_patcher.js (+69/-1)
lib/lp/app/javascript/picker/tests/test_picker_patcher.js (+1/-1)
lib/lp/app/widgets/templates/form-picker-macros.pt (+2/-28)
lib/lp/app/widgets/tests/test_popup.py (+1/-1)
lib/lp/bugs/javascript/bugtask_index.js (+5/-6)
lib/lp/bugs/javascript/tests/test_bugtask_delete.js (+11/-5)
lib/lp/code/javascript/branchmergeproposal.nominate.js (+133/-0)
lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.html (+51/-0)
lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js (+191/-0)
lib/lp/code/templates/branch-register-merge.pt (+6/-23)
Revision history for this message
Jonathan Lange (jml) wrote :

Related to bug 319405. We might want to do it in conjunction with other privacy work for 2.2.3.

Changed in launchpad-bazaar:
importance: Undecided → Low
status: New → Triaged
Revision history for this message
Barry Warsaw (barry) wrote :

I understand that it's possible to make a mistake when selecting a reviewer, and thus accidentally giving someone permission to see a private branch. Perhaps we can do things to help avoid that, such as looking to see if the requested reviewer is in the default review team. Alternatively, let's provide a u/i after the reviewer has been requested to 1) indicate in big red blinking letters if the reviewer does not have permission to see the branch; 2) let me click to give them permission.

Tim Penhey (thumper)
summary: - Can request a review from someone without permission
+ Can request a review from someone who can't see the merge proposal
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

I think what barry suggests makes a great deal of sense. Dunno how hard it is to implement though -- how is the wizard support in lazr-js coming along?

Curtis Hovey (sinzui)
tags: added: disclosure
Revision history for this message
Curtis Hovey (sinzui) wrote :

We have added rules to the person picker that ask you to confirm the action because there is a concern. The call back to the confirmation should subscribe the user to the private branch. This issue might need nothing more than a message and a js call back that subscribes the user/team.

Revision history for this message
Robert Collins (lifeless) wrote : Re: [Bug 330290] Re: Can request a review from someone who can't see the merge proposal

So, the necessary rules are:
If and only if the user cannot see the branch under review then
subscribe to it, to its stacked-on branch, and its stacked on branch
recursively, with 'no mail' and 'no code review' as the subscription
settings.
Ditto for the branch landing target.

I think this is complex enough to be best done as a new API - probably
on the MP itself. Or perhaps part of the api for adding a reviewer.

-Rob

Ian Booth (wallyworld)
Changed in launchpad:
status: Triaged → In Progress
assignee: nobody → Ian Booth (wallyworld)
Revision history for this message
Launchpad QA Bot (lpqabot) wrote :

Fixed in stable r14744 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/stable/revision/14744>.

tags: added: qa-needstesting
Changed in launchpad:
status: In Progress → Fix Committed
Ian Booth (wallyworld)
tags: added: qa-ok
removed: qa-needstesting
William Grant (wgrant)
Changed in launchpad:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.