Normalize approvals when submitting instead of when merging
If we normalize approvals at merge time, there is a potentially long
race period between the submit rule evaluator being run (only once, at
submit time) and the change being merged. During this period,
permissions can change in such a way that the change would no longer
be submittable, but the submit rule evaluator is not checked again at
merge time.
Instead, normalize approvals at submit time. It is still possible that
the normalized values for a submitted/merged change would not make
sense given a later state of the label configuration, e.g. if a label
range is increased. But at least we are now consistent (up to a much
shorter race window during submit, at least) with the state when the
submit rule evaluator was run.
To simplify the code, this change also removes the behavior whereby
approvals that were normalized to 0 are deleted entirely, leaving only
the ChangeMessage to indicate that a user was ever a reviewer. This
changes results in some corner cases in ways that should mostly be
acceptable. For example, if a user voted on a change but then lost
permissions before submit time, ChangeJson will now be able to report
them as a reviewer (who didn't vote), which it would not have done if
all PatchSetApprovals for that user were deleted. That said, this is
just to justify that this behavior is not a serious regression; in the
medium term we will be removing the PatchSetApprovals table in favor
of the much saner notedb implementation.
Change-Id: I143a5a890e59461c9f9e348c10d1d16996d95c1b
3 files changed
