They belong there right next to the JGit PushResultSubject, they were
just put in the wrong place initially.
Change-Id: Iaef009136888c813f1bd20b7988e4f59259e860e
DynamicMap#plugins() creates a new TreeSet on every invocation. We were
just doing this to presize an ArrayList, which makes it seem like we
care about reducing allocations, but was probably actually increasing
allocations. Fortunately, we can just presize using the multimap we
already have. This also allows us to remove a redundant instance field.
Change-Id: Ie5998add61d4848e184a37f4f237da132802a00f
This emphasizes to the reader that this map is not supposed to be
mutated after construction time.
It is tempting to store Extensions instead of constructing our own map.
However, repeatedly calling Extensions#get() could cause performance
problems. Leave a comment about why we aren't doing this.
Change-Id: I466b2c17ea1e1b8a9304c97a579cdd3184e033fc
This method was definitely not safe to call multiple times, since it
would add duplicate entries to the mutable map stored in the instance
field. The map should really only be initialized once, during
construction.
Change-Id: I6cbb792adb6ae9dbec70df3f45586dc518faf913
Debugging production problems is much more difficult when exceptions are
completely swallowed. At the same time, if a plugin fails on all inputs,
logging one entry for every change in a large query result set would be
terrible as well. Split the difference and log once a minute; Flogger
makes this very easy. We can always decide to come back later and
complexify it by logging e.g. once per plugin or per factory, but this
is dead simple and pretty quiet.
Change-Id: If512583aef10e48c745660c42f021837caeb2994
A plugin is really just a name plus one or more module classes. We
already have the TestServerPlugin magic which is used by
LightweightPluginDaemonTest, so reuse that. The difference is that the
use case for this new method is for testing the plugin plumbing itself,
not testing a specific real-world plugin.
Change-Id: I3ca4eda806519ad9bdd653e0b5f897e69ed119af
Subclasses that override resetProjects may want to add to the default
config, rather than recreating it all from scratch. Previously, the
default config was never exposed to subclasses. Now, they can get it via
super.resetProjects() (or not, as current implementors all prefer).
Change-Id: If116e5fce9e0fe0f826b66c1f84774660f20dec7
Allowing plugins to specify custom action types may be feasible, but at
a minimum requires changing the type of the metrics.
Change-Id: Ieb659c8e86f59085e57dbcb134e3582039025223
Found with:
for f in $(find [a-z]* -name \*.java); do echo "$f $(head -n1 $f)"; done | grep -v '// Copyright'
The remaining classes not touched in this file all have nonstandard
license headers, because they were copied from other projects. Or
they're plugins, which should be fixed separately.
Copyright dates are taken from original creation of the file.
Change-Id: I3be07ac46fd999f07c230eacbf4508f39fda1b77
* stable-2.16:
Fix missing `</section>` in gr-settings-view
Suggest --no-edit when Change-Id is missing
PG: Assume weblinks have correct direct or relative urls
PG: Make commitlink selection configurable and consistent
Add config gerrit.primaryWeblinkName
gerrit.sh: Improve error message when JRE cannot be found
PrologEnvironment#setPredicate: Reduce log level to DEBUG
ElasticContainer: Bump V7_0 test server to 7.0.0-beta1
Change-Id: I8bef4ae02e935809de6d7abf59e1a8de6800432c
* stable-2.15:
gerrit.sh: Improve error message when JRE cannot be found
PrologEnvironment#setPredicate: Reduce log level to DEBUG
Change-Id: I26703bd42f91a96dc985a2de8e90f93be78bdd63
Add gerrit.primaryWeblinkName to ServerInfo.
Lets the UI know which is the preferred weblink in instances were
only one weblink can be used, for example in inline links.
Change-Id: I01deaca3418d5c0486e3f5ebabf10827d47c3631
We want to improve the UX of private changes by limiting the number of
merged private changes. As a first step, we stop allowing users to mark
merged (or abandoned) changes private.
A subsequent commit will auto-publish private changes on submission and
include a UI dialog that tells the user about this process.
Change-Id: Ia4039099b5b23b9db6c88183be51687168e59580
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
