PostReview and AddReviewer APIs have slightly different semantics when
re-adding existing reviewer to the change. In PostReview endpoint
addReviewer operations gets called with suppressed notification feature.
At the end of processing batch mail notification processing is invoked.
The reason for this optimization is to avoid, that for each new reviewer
all existing reviewers are notified multiple times.
For existing reviewer user1, another two reviewers are added: user2 and
user3. If notifications would have been preserved in addReviewer
operations, there would be two "Reviewers Added" notifications:
* 1. Recipients: user1, user2
* 2. Recipients: user1, user2, user3
Note, that user1 and user2 would get two notifications. With batch
notification step in PostReview endpoint, only one single message is
sent:
* 1. Recipients: user1, user2, user3
However, the batch notification implementation in PostReview endpoint
missed the fact, that addReviewer API compares the new and existing
reviewers and removes all reviewers that were already added as reviewers
to the change. As the consequence, if exactly the same reviewers are
added multiple times using PostReview endpoint then the notification would
be sent multiple times.
That comparison step was missed in batch notification implementation in
PostReview endpoint. This was not noticed for a long time, because the
problem is not exposed on the UI: it filters out the existing reviewers.
So that it is not possible to add the same reviewer multiple times and
thus trigger multiple notification problem.
The problem arises when plugins are using the PostReview API to add
reviewers programmatically, most notably reviewers plugin. The plugin
implements RevisionCreated listener to react on new patch set event and
adds reviewers according to the project configuration. Even if there
were no new reviewers added to the change, the notifications are still
sent.
To rectify, evaluate result of AddReviewer operation and only add
notifications in PostReview endpoint, if reviewers were actually added
in AddReviewer operation, in suppressed notification mode.
Bug: Issue 11625
Change-Id: I9b30af95ca64cef35a2f170553e0680e831c7623
201b086716