With this new "sendEmail" bit, we will have more control
for whether emails are going to be sent out after a new
patch set is added. This is useful for avoiding duplicate
emails, e.g merge (already have an email to notify a change
is merged, no need to have another email to say a new patch
set is uploaded).
Change-Id: I43c5d7c6560316d5a6034dc8e711f7e87b7af932
To check a change's state the caller currently has to call getState()
and then do a comparison on the enum value.
To make this easier, introduce new methods on Change:
- isNew
- isMerged
- isAbandoned
- isClosed
Note that we don't add "isOpen" since there are no longer multiple
states that could be considered "open". Previously there were "new"
and "draft", but the draft workflow was removed.
Update callers to use the new methods.
Change-Id: Ic047f74b114e2277e66c878dace73ea8e0babab6
* changes:
NotifyResolver: Use SetMultimap for accounts to notify
Pass notification settings through BatchUpdate's Context
Consistently pass NotifyResolver.Result instead of separate args
Refactor NotifyUtil analogously to AccountResolver
PostReview: Fix default notification settings, kind of
Rename FakeEmailSenderSubject#notSent to didNotSend
ChangeNotificationsIT: Consistently check sender queue is empty
ChangeNotificationsIT: Don't trigger review after setting ready
With Flogger we must use '%s' as placeholder for parameters.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I834c84516b86b7bdcf800ab06748da30babd9a4f
Different systems may implement reactivation of accounts in different
ways. On googlesource.com, for example, accounts are automatically
reactivated on login through the web UI, but there are other codepaths
where reactivation may not happen automatically. Filtering out inactive
users when resolving "self" is therefore a regression: it would return
no results where prior to the AccountResolver rewrite it would have
returned results.
Fix the regression by resolving "self" even when inactive. I'm not
arguing that not auto-reactivating is the best behavior for
googlesource.com, just switching back to the old behavior of
AccountResolver.
The regressed behavior was especially confusing to debug because it
resulted in an exception message saying "Resolving account 'self'
requires login", even though the user was technically logged in.
Change-Id: I5aaf20eeb94ea6de268a96230d14ac85c4eb1863
This special case was erroneously removed in I7113aa7a. Add a regression
test that would have caught it.
I had thought it was safe to remove because AccountResolver now supports
"self. However, I didn't realize that the isSelf case worked for
anonymous users, while AccountResolver intentionally does not.
Change-Id: I393f74b37215b9a6296eedf1da6a935f1c9518a9
An account ID should only appear once per RecipientType; allowing
duplicates of the same account ID with the same RecipientType doesn't
make sense. This shouldn't have any effect in practice, since these
accounts are converted to Sets within OutgoingEmail, but the code is
cleaner this way.
Change-Id: I677def1384151bfe52fa20e291864b260a9df3fb
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
NotifyUtil#resolveAccounts was basically a wrapper around
AccountResolver. Give the class a new name, NotifyResolver, and have it
resolve inputs to a Result type. This Result type encapsulates both the
NotifyHandling enum and the ListMultimap that got passed around
everywhere. This has several significant advantages.
First, this pair of objects is used in many places, so cutting down two
arguments (including the unwieldy ListMultimap type) to one is a win on
its own. This change fixes some such methods, and the rest will be fixed
in future cleanups.
Second and more subtly, this change results in more explicit and less
error-prone behavior when a null notify field is passed in a REST API
object. Callers have to explicitly specify a non-null NotifyHandling
input to NotifyResolver#resolve, which typically means they have to do a
firstNonNull(input.notify, <some default>). It turns out that in the
REST API docs, there are several different defaults for the different
*Input types, including some that have different behavior for WIP/ready
changes. The documentation is structured per Input type, which means
it's always clear from the documentation what the default should be if
you have in front of you a FooInput. In other words, this pattern makes
it easier to inspect the code for correctness.
Change-Id: I609674d22b6a16b8fac32aeea1a57d293ee601d5
The code was confused between the input.notify field and the local
reviewerNotify variable; this was clearly a bug. Unfortunately,
attempting to fix it breaks a surprising number of things.
Opt to keep the assignment to input.notify, since later accesses to this
field depend on that side effect.
This bug was causing no emails to be sent in the case where review has
started. The intent of I93395e36 was clearly to send emails to everyone
on a WIP change that has previously started review.
The other set of tests that would fail after properly overwriting the
input.notify field is checking that emails are sent to the owner when
the change is WIP and review hasn't started. In this case, it wasn't
clear to me what the actual intent was. Err on the conservative side,
and change the logic to explicitly email nobody in this case instead of
emailing the owner, under the assumption that users aren't currently
upset by the lack of emails.
Change-Id: Ie0fb1237d5d1aac6e7fda5e4a6ccdc6b5688a89a
"Assert that sender did not send" is slightly more grammatical than "not
sent", and is barely longer.
Change-Id: I16f5b4decee0357e9862b6bedc7b04a9589ab17e
In addition to testing the contents of the next email in the queue,
tests should also be asserting that there are no bogus emails being
sent. This can prevent bugs: for example, in PostReview, there is code
to suppress emails from the AddReviewersOp so that a single email can be
sent at the end. Now the tests will actually catch it if that
suppression breaks.
Change all the stage* methods to clear the sender before returning, so
that test methods are just inspecting the messages they intend to send.
Tests for emails generated by API calls that happen to be in the
implementation of stage* methods should be tested directly in dedicated
methods instead.
While we're in there, expand the failure messages to make such failures
easier to debug.
Change-Id: I49329a1ca82b4a060541dd3ebf32a0bb5214c994
* stable-2.16:
Support for setting max/min value for "Query Limit" and "Batch Changes"
Mark DeletePrivate input as @Nullable
ListProjects: print projects list using secondary index
ListProjects: re-implement using secondary index
Fix replacing ${project-base-name} in gr-repo
Change-Id: Id9123dcfbd123a529612fccaecd64ee0c196b216
The documentation says that an input is not required. The code checks
for null values correctly, but the injection methods lack @Nullable
which makes Guice fail to create the object when null is passed to
the factory method.
Annotate the input object as @Nullable to fix this.
Change-Id: I0d424e45d6039fd4ad8386b6811b9134cd600dfd
Render the list of projects to OutputStream leveraging the
projects secondary index instead of relying on the in-memory cache.
Change-Id: I864e1c3cd63b206c0f9dc76bbaa6de99ddfdc0d4
The documentation says that an input is not required. The code checks
for null values correctly, but the injection methods lack @Nullable
which makes Guice fail to create the object when null is passed to
the factory method.
Annotate the input object as @Nullable to fix this.
Change-Id: I0d424e45d6039fd4ad8386b6811b9134cd600dfd
The GWT UI and other parts of Gerrit still rely on the in-memory
cache for rendering the project list.
This is the first step that moves some use-cases to the QueryProjects
engine: full list without filters and showing only the active and readonly
projects.
All other existing use-cases are still based on the in-memory
cache and are going to be addressed in the follow-up of this change.
With regards to filtering by project name substring, it is not
implemented on top of the secondary index because of Issue 10446.
Bug: Issue 10380
Change-Id: I8effed5f75bdf353d9b23a3d349009e5f0535186
The apply(TopLevelResource) method doesn't actually use the given
resource parameter, and only exists to satisfy the interface of its
parent RestReadView.
A variant of the method without the TopLevelResource parameter was
added in change Ia8ded07a5. Modify callers to use that.
Change-Id: I15af0d1cb35750c8aec4b2f42339901c48332889
* stable-2.16:
queryLimit: specify that limit applies only to PolyGerrit UI
Disable "prefer-promise-reject-errors" in eslint
Set version to 2.15.10
Daemon: Show status of slave and headless mode in 'ready' log
QueryProjects: introduce apply() without parameters
Daemon: Don't install online reindexer in slave mode
ListProjectsIT: test for display to output stream
RevisionApi: Add method to list votes per revision
ChangeJson#getApprovalInfo: Add @Nullable annotations
cmd-review: Add another example to clarify review using change number and PS
Update .mailmap
Update cmd-review documentation
Daemon: use regular binding for auditModule
CommitValidators: trim "ERROR" shouting from "forge committer" check
ReceiveCommits: uniformize commit validation error messages.
Inline "Change-Id" string into error messages
Always end the "Change-Id missing" error message with \n.
Print only one hint about Change-Ids at a time
Clean up Change-Id hint text
CommitValidators: Replace indexOf calls with String#contains
CommitValidators: Prefer using Splitter to String.split
Change-Id: I9fad1352d3332f5caff8e317eeba7b3bc0760008
* stable-2.15:
Disable "prefer-promise-reject-errors" in eslint
Set version to 2.15.10
Daemon: Show status of slave and headless mode in 'ready' log
Daemon: Don't install online reindexer in slave mode
RevisionApi: Add method to list votes per revision
ChangeJson#getApprovalInfo: Add @Nullable annotations
cmd-review: Add another example to clarify review using change number and PS
Update .mailmap
Update cmd-review documentation
Daemon: use regular binding for auditModule
CommitValidators: trim "ERROR" shouting from "forge committer" check
ReceiveCommits: uniformize commit validation error messages.
Inline "Change-Id" string into error messages
Always end the "Change-Id missing" error message with \n.
Print only one hint about Change-Ids at a time
Clean up Change-Id hint text
CommitValidators: Replace indexOf calls with String#contains
CommitValidators: Prefer using Splitter to String.split
Changes I0bfe06bd and I58f8a0e5 are intentionally omitted because
they do not apply to the structure of the stable-2.16 branch.
Change-Id: I711837f062aeefeafa3631601b0150448e879575
QueryProjects REST API does not depend on the resource passed
as input but purely on its query parameters.
Introduce an apply() method without parameters so that the
absence of dependency with the resource becomes more explicit.
Change-Id: Ia8ded07a56a00af15f1fa44679886f24d8eabdd9
Files that have no extension can now be matched with the 'ext' search
operator by an empty string: ext:""
This makes the handling of files with no extension consistent between
the 'ext' and 'onlyexts' search operators.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I42774ce8641c429e6fc7aa55948769cc0a1fa48e
skipMetadata indicates that all meta refs should be omitted. This is
more of a performance option than a security option, but for the sake of
completeness, this commit also omits single change ref evaluation in
case the option is provided.
Change-Id: I8bfb80fc727b8abf928d1c6ac1fd715149bfbe3a
* changes:
Rename AccountResolver#byId to accountCache
Update {account-id} documentation
Rename AccountResolver2 to AccountResolver
Remove now-unused old AccountResolver implementation
Remove unnecessary explicit binding of AccountResolver
Use AccountResolver2 to resolve strings to IdentifiedUsers
AccountResolver2: Allow resolving "self" and "me"
AccountResolver2: Expose more useful exception information
AccountResolver: Mark more methods as private
Implement new AccountResolver2#resolveByNameOrEmail
Rewrite AccountResolver with consistent interface and semantics
* stable-2.16:
Update codemirror-editor submodule
LuceneProjectIndex: fix NPE when project disappeared on disk
Change-Id: I0048f6245f81c58dbe8bd2ed43ffa48e6846d23c
Fix a NPE in the decoding of ProjectState to ProjectData when a project
is removed from disk while Gerrit is still running.
Change-Id: I9bbf4e88a3b41d677e17121d10f38d8f901b2e99
GroupAuditService is the interface to the underlying audit
system, while AuditService is the actual implementation.
Instead of directly injecting the implementation class, rely on
the interface and leave the association to AuditService to the
Guice binding.
This is useful because tests may want to replace, at times, the
actual implementation of the audit-trail with a fake in-memory
store.
Change-Id: I31bbb522fb974c2504635ccf1251287d4890af89
