Page MenuHomePhabricator

Code-Review and Verified votes can be removed from changes after they are merged
Open, LowPublic

Description

Code-Review and Verified votes can be removed from changes after they are merged. Today I accidentally removed my CR+2 vote from https://gerrit.wikimedia.org/r/c/VisualEditor/VisualEditor/+/517459 (I meant to remove a CR+2 vote from another change that failed to merge, but I clicked the button in the wrong browser tab). I am pretty sure this used to not be possible.

Event Timeline

This seems like a bug, since you shouldn’t be able to remove a +2 after a change is merged, but it’s expected that you can remove votes after a change is merged.

I’ve spoken with upstream who said that this is intentional due to the remove reviews right.

This seems like a bug, since you shouldn’t be able to remove a +2 after a change is merged, but it’s expected that you can remove votes after a change is merged.

This doesn't make much sense, and your message contradicts what you are saying...

I just removed C+2 and V+2 from https://gerrit.wikimedia.org/r/#/c/VisualEditor/VisualEditor/+/517459/ which definitely seems like a bug to me... Has this always been the case? Or is it a recent change? The little x next to them don't seem to be new, but I can't say I paid much attention to them on merged commits.

Screenshot 2019-06-18 at 13.06.43.png (752×848 px, 96 KB)

Screenshot 2019-06-18 at 13.07.59.png (404×2 px, 194 KB)

@Reedy Yes, I thought it was a bug before I spoke with upstream.

This is expected due to the “remove reviewer” right.

Which I presume you and matmarex were granted.

Though I did ask about changing this behaviour to prevent removing a voting vote (eg +2).

(Upstream did say it might be worth me bringing this up on the mailing list (https://groups.google.com/forum/m/#!forum/repo-discuss))

(We’ve granted remove reviewers rights to project owners according you All-Projects)

@Reedy Removing reviewers and reviews definitely has been possible for a while, but I am pretty sure it used to only be available on open changes.

hashar subscribed.

At least the reviews notes are still correct:

$ fetch origin refs/notes/review:refs/notes/review
$ git log -n1 --format=%N --notes=review a13c86033723dddbec6f615d7a137dab28133591Code-Review+2: Bartosz Dziewoński <matma.rex@gmail.com>
Verified+2: jenkins-bot
Submitted-by: jenkins-bot
Submitted-at: Tue, 18 Jun 2019 00:03:39 +0000
Reviewed-on: https://gerrit.wikimedia.org/r/517459
Project: VisualEditor/VisualEditor
Branch: refs/heads/master

The notes are not altered once a change has been closed. But indeed ones can delete votes:

$ git fetch origin refs/changes/59/517459/meta
$ git log -n2 FETCH_HEAD
commit 983bd02ed6ea6655b64c23561e2b035822d04496
Author: Reedy <xxx>
Date:   Tue Jun 18 12:05:41 2019 +0000

    Update patch set 1
    
    Removed Verified+2 by jenkins-bot (75)
    
    
    Patch-set: 1
    Tag: autogenerated:gerrit:deleteVote
    Label: -Verified jenkins-bot <xxx>

commit 987a3111c0d03d20f854c4d6fe3cbaa0091f2a4e
Author: Reedy <xxx>
Date:   Tue Jun 18 12:05:39 2019 +0000

    Update patch set 1
    
    Removed Code-Review+2 by Bartosz Dziewoński <matma.rex@gmail.com>
    
    
    Patch-set: 1
    Tag: autogenerated:gerrit:deleteVote
    Label: -Code-Review Bartosz Dziewoński <xxx>

That is from the Remove Reviewer right which is always granted to project owners.

I remember that at some point Gerrit behavior changed regarding actions that could be done on a closed change. But I don't think it prevented removing votes/reviewers.

Iwould guess we can fill it to Upstream and check whether the default should be to prevent removing reviewers once a change is merged. I am setting this low priority meanwhile, the audit trail still shows up in the web interface and is available in the review notes (refs/notes/review).