Pass notification settings through BatchUpdate's Context

Previously, any notification settings that a BatchUpdateOp wanted to
respect in its postUpdate method needed to be resolved from the
corresponding REST API input and plumbed through the Op constructor so
they could be looked up as needed. Explicit can be good, but this verged
into the territory of _too_ explicit.

Instead, treat the notification settings as a property of the
BatchUpdate. This is somewhat magical, but in this case the magic is
justified. Conceptually, associating notification settings with the
BatchUpdate makes sense: there is a single set of notification settings
that is parsed at the top level from the input, and those settings
should apply to *all* emails sent as a result of that BatchUpdate,
regardless of the particulars of where they're sent from. Putting them
in the Context allows us to treat them as a per-BatchUpdate singleton.

We retain the ability to override the NotifyHandling enum on a
per-*change* basis, rather than a per-*op* basis, to account for the
reduced notifications from WIP changes.

Without this change, there's always the possibility of having multiple
Ops in the same BatchUpdate that send different emails with different
notification settings. For example, we had special code in PostReview
to ignore the notification settings embedded in the constituent
AddReviewerInputs of a ReviewInput. After this change, we still ignore
those settings (as the REST API docs specify), but we don't have to do
anything special to do so: we just set the NotifyResolver.Result on the
BatchUpdate to the one from the ReviewInput, completely ignoring the
AddReviewerInputs.

This change also has the effect of removing any logic around
notification settings from ReviewerAdder; all the notification logic is
in the caller. We still retain a separate mechanism for suppressing all
emails from ReviewerAdder, so that PostReview can send only a single
email. However, this is stored as a simple boolean, rather than passing
around a whole NotifyResolver.Result. It's still not completely ideal,
but considering everything else ReviewerAdder has to deal with, it's a
small win. Plus, simplifying ReviewerAdder will pave the way for
rewriting it in the near future.

One downside here is that any check to set NotifyHandling based on
properties of the change (e.g. the WIP bit) is racy, since the change
was read outside of the BatchUpdate. The risk and consequences of such a
race are low enough that the benefits described above still outweigh it.
(And it's not like it's the only such race, even though we do try to
keep them to a minimum.)

This change should have minimal behavior changes. One exception is that
moving around calls to NotifyResolver#resolve from the body of a try
block to the top level of a REST API handler might result in a 400/422
for invalid input in notify_details rather than logging and silently not
sending an email. Some endpoints already had this behavior, so making
the failure explicit and consistent is considered a feature.

Change-Id: If6f8a44f382f57b9cb10490f74c2b144e904ece8
This commit is contained in:
Dave Borowitz
2019-01-30 14:12:34 -08:00
parent 8e24b76649
commit 0312eb2315
39 changed files with 249 additions and 216 deletions

View File

@@ -182,7 +182,6 @@ public class RebaseChangeOp implements BatchUpdateOp {
patchSetInserterFactory
.create(notes, rebasedPatchSetId, rebasedCommit)
.setDescription("Rebase")
.setNotify(NotifyResolver.Result.none())
.setFireRevisionCreated(fireRevisionCreated)
.setCheckAddPatchSetPermission(checkAddPatchSetPermission)
.setValidate(validate);