commit f1598c54311460db7632d06cc855b8a18bbcf494
author Edwin Kempin <ekempin@google.com> Mon Apr 15 10:12:56 2024 +0000
committer Edwin Kempin <ekempin@google.com> Tue Apr 16 09:06:31 2024 +0000
tree ebf9fbbd7256a0e329f39bd867d090a0ce69a227
parent a356b05807119733e392e94b29fe1304d15895e9

Disallow removing votes from merged changes by removing reviewers

Once a change has been merged it should be no longer possible to
remove/reduce approvals of the change as this could remove approvals
that were necessary for the submission and it's confusing to see a
merged change which doesn't have the necessary approvals to fulfill the
submit requirements. E.g. this is why PostReview doesn't allow to
remove/reduce votes (see PostReviewOp#validatePostSubmitLabels).

Hence it's a bug that having the 'Remove Reviewer' permission allows
users to remove approvals from merged changes.

If approvals are removed from a change after it has been merged we can
have the following inconsistencies:
* the change is shown as merged with fulfilled submit requirements but
  without any votes which is confusing to users, note that the submit
  requirements are still shown as fulfilled because the submit
  requirement results are persisted in NoteDb when the change is
  submitted
* tools that analyse merged changes may get confused if there are merged
  changes without approvals
* the submitter is recorded by an approval on the magic SUBM label which
  can be lost when the submitter is removed from the reviewers of the
  change

RemoveReviewerControl intended to prevent removing reviewers that have
non-zero votes on merged changes (see check in
canRemoveReviewerWithoutPermissionCheck) but failed to do so because
having the "Remove Reviewer" permission bypassed this in
testRemoveReview/checkRemoveReviewer).

In addition ChangeJson wrongly assumed that having the "Remove Reviewer"
permission is sufficient to remove any vote, even on merged changes and
it also doesn't take the magic SUBM approval into account
(removableReviewers only iterated over the labels that are returned to
the caller where the SUBM approval had been filtered out, although the
presence of the SUBM approval still makes the submitter a reviewer).

Not needing to check whether approvals with non-zero votes can be
removed for merged changes can also improve the latency for change
queries that return merged changes, since the "Get removable reviewers"
is not as a reason for change quries to be slow.

Release-Notes: Disallowed removing votes from merged changes by removing reviewers
Bug: Google b/334796368
Bug: Google b/335095952
Change-Id: Ia8edde3652c93251902cdafa8aba3f57b8e0915b
Signed-off-by: Edwin Kempin <ekempin@google.com>