opendev/gerrit
Code Issues Proposed changes

ChangeUpdate reformats for AttentionSet

Browse Source 
Currently, we call the method that updates the attention set
multiple times in ChangeUpdate. This is a problem since we
don't check that we don't update the same account more than
once in the same ChangeUpdate.

This can't really happen that we will update the same account
twice, but there was no guarantee. Also, in a follow-up change
we will make adjusments to "PostReview" (review) endpoint, and
those adjusments will make it possible to specify more than one
update for the same account. In such case, the first update of
that account will occur, ignoring all else.
E.g, if a reviewer replies then they should be removed from the
attention set, but if they reply on their own comment maybe they
should also be added to the attention set? Technically yes, and
in this case the second update will be ignored.

Also, the AttentionSetOps always set the change message. This
change adds the option to not set the change message in this case.
Added tests that ensure ChangeMessages are added when added or
removed explicitly (e.g AddToAttentionSet/RemoveFromAttentionSet)
but not added otherwise.

Currently, the AttentionSetOps set the AttentionSetUpdates field
in ChangeUpdate. Now, instead of setting this field, we will add
more AttentionSetUpdates rather than overriding it. This way, we will
be able to call multiple Ops that add AttentionSetUpdates.

Also fixed the test ChangeNotes#addAttentionStatus_rejectIfSameUserTwice
since now we reject such request instead of ignoring the second user.
It is not valid to specify the same user twice in the same *set* of
updates when calling ChangeUpdate#addToPlannedAttentionSetUpdates, since
there is no way to define which update should be the one to apply (since
we only apply the first one).

Change-Id: I33caf0574b55d759b678b98f204a7b066de64bb3
This commit is contained in:
Gal Paikin
2020-06-05 14:06:48 +03:00
parent 90add1fe5d
commit fb6642911c
8 changed files with 129 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
java/com/google/gerrit/server/change/AddToAttentionSetOp.java
View File

@@ -36,24 +36,36 @@ import java.util.function.Function;
public class AddToAttentionSetOp implements BatchUpdateOp {
public interface Factory {
AddToAttentionSetOp create(Account.Id attentionUserId, String reason);
AddToAttentionSetOp create(
Account.Id attentionUserId, String reason, boolean withChangeMessage);
}
private final ChangeData.Factory changeDataFactory;
private final ChangeMessagesUtil cmUtil;
private final Account.Id attentionUserId;
private final String reason;
private final boolean withChangeMessage;
/**
* Add a specified user to the attention set.
*
* @param attentionUserId the id of the user we want to add to the attention set.
* @param reason The reason for adding that user.
* @param withChangeMessage Whether or not we wish to add a change message detailing about adding
* that user to the attention set.
*/
@Inject
AddToAttentionSetOp(
ChangeData.Factory changeDataFactory,
ChangeMessagesUtil cmUtil,
@Assisted Account.Id attentionUserId,
@Assisted String reason) {
@Assisted String reason,
@Assisted boolean withChangeMessage) {
this.changeDataFactory = changeDataFactory;
this.cmUtil = cmUtil;
this.attentionUserId = requireNonNull(attentionUserId, "user");
this.reason = requireNonNull(reason, "reason");
this.withChangeMessage = withChangeMessage;
}
@Override
@@ -68,15 +80,17 @@ public class AddToAttentionSetOp implements BatchUpdateOp {
}
ChangeUpdate update = ctx.getUpdate(ctx.getChange().currentPatchSetId());
update.setAttentionSetUpdates(
update.addToPlannedAttentionSetUpdates(
ImmutableSet.of(
AttentionSetUpdate.createForWrite(
attentionUserId, AttentionSetUpdate.Operation.ADD, reason)));
addMessage(ctx, update);
if (withChangeMessage) {
addChangeMessage(ctx, update);
}
return true;
}
private void addMessage(ChangeContext ctx, ChangeUpdate update) {
private void addChangeMessage(ChangeContext ctx, ChangeUpdate update) {
String message = "Added to attention set: " + attentionUserId;
cmUtil.addChangeMessage(
update,

24
java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java
View File

@@ -36,24 +36,36 @@ import java.util.function.Function;
public class RemoveFromAttentionSetOp implements BatchUpdateOp {
public interface Factory {
RemoveFromAttentionSetOp create(Account.Id attentionUserId, String reason);
RemoveFromAttentionSetOp create(
Account.Id attentionUserId, String reason, boolean withChangeMessage);
}
private final ChangeData.Factory changeDataFactory;
private final ChangeMessagesUtil cmUtil;
private final Account.Id attentionUserId;
private final String reason;
private final boolean withChangeMessage;
/**
* Remove a specified user from the attention set.
*
* @param attentionUserId the id of the user we want to add to the attention set.
* @param reason The reason for adding that user.
* @param withChangeMessage Whether or not we wish to add a change message detailing about adding
* that user to the attention set.
*/
@Inject
RemoveFromAttentionSetOp(
ChangeData.Factory changeDataFactory,
ChangeMessagesUtil cmUtil,
@Assisted Account.Id attentionUserId,
@Assisted String reason) {
@Assisted String reason,
@Assisted boolean withChangeMessage) {
this.changeDataFactory = changeDataFactory;
this.cmUtil = cmUtil;
this.attentionUserId = requireNonNull(attentionUserId, "user");
this.reason = requireNonNull(reason, "reason");
this.withChangeMessage = withChangeMessage;
}
@Override
@@ -68,14 +80,16 @@ public class RemoveFromAttentionSetOp implements BatchUpdateOp {
}
ChangeUpdate update = ctx.getUpdate(ctx.getChange().currentPatchSetId());
update.setAttentionSetUpdates(
update.addToPlannedAttentionSetUpdates(
ImmutableSet.of(
AttentionSetUpdate.createForWrite(attentionUserId, Operation.REMOVE, reason)));
addMessage(ctx, update);
if (withChangeMessage) {
addChangeMessage(ctx, update);
}
return true;
}
private void addMessage(ChangeContext ctx, ChangeUpdate update) {
private void addChangeMessage(ChangeContext ctx, ChangeUpdate update) {
String message = "Removed from attention set: " + attentionUserId;
cmUtil.addChangeMessage(
update,

72
java/com/google/gerrit/server/notedb/ChangeUpdate.java
View File

@@ -74,6 +74,7 @@ import java.io.IOException;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.Date;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
@@ -131,7 +132,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
private String submissionId;
private String topic;
private String commit;
private Set<AttentionSetUpdate> attentionSetUpdates;
private Map<Account.Id, AttentionSetUpdate> plannedAttentionSetUpdates;
private Optional<Account.Id> assignee;
private Set<String> hashtags;
private String changeMessage;
@@ -374,17 +375,35 @@ public class ChangeUpdate extends AbstractChangeUpdate {
/**
* All updates must have a timestamp of null since we use the commit's timestamp. There also must
* not be multiple updates for a single user.
* not be multiple updates for a single user. Only the first update takes place because of the
* different priorities: e.g, if we want to add someone to the attention set but also want to
* remove someone from the attention set, we should ensure to add/remove that user based on the
* priority of the addition and removal. If most importantly we want to remove the user, then we
* must first create the removal, and the addition will not take effect.
*/
public void setAttentionSetUpdates(Set<AttentionSetUpdate> attentionSetUpdates) {
public void addToPlannedAttentionSetUpdates(Set<AttentionSetUpdate> updates) {
if (updates == null || updates.isEmpty()) {
return;
}
checkArgument(
attentionSetUpdates.stream().noneMatch(a -> a.timestamp() != null),
updates.stream().noneMatch(a -> a.timestamp() != null),
"must not specify timestamp for write");
checkArgument(
attentionSetUpdates.stream().map(AttentionSetUpdate::account).distinct().count()
== attentionSetUpdates.size(),
updates.stream().map(AttentionSetUpdate::account).distinct().count() == updates.size(),
"must not specify multiple updates for single user");
this.attentionSetUpdates = attentionSetUpdates;
if (plannedAttentionSetUpdates == null) {
plannedAttentionSetUpdates = new HashMap();
}
Set<Account.Id> currentAccountUpdates =
plannedAttentionSetUpdates.values().stream()
.map(u -> u.account())
.collect(Collectors.toSet());
updates.stream()
.filter(u -> !currentAccountUpdates.contains(u.account()))
.forEach(u -> plannedAttentionSetUpdates.putIfAbsent(u.account(), u));
}
public void setAssignee(Account.Id assignee) {
@@ -586,10 +605,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
if (status != null) {
addFooter(msg, FOOTER_STATUS, status.name().toLowerCase());
if (status.equals(Change.Status.ABANDONED)) {
clearAttentionSet(msg, "Change was abandoned");
clearAttentionSet("Change was abandoned");
}
if (status.equals(Change.Status.MERGED)) {
clearAttentionSet(msg, "Change was submitted");
clearAttentionSet("Change was submitted");
}
}
@@ -601,10 +620,6 @@ public class ChangeUpdate extends AbstractChangeUpdate {
addFooter(msg, FOOTER_COMMIT, commit);
}
if (attentionSetUpdates != null) {
updateAttentionSet(msg, attentionSetUpdates);
}
if (assignee != null) {
if (assignee.isPresent()) {
addFooter(msg, FOOTER_ASSIGNEE);
@@ -631,7 +646,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
addFooter(msg, e.getValue().getFooterKey());
addIdent(msg, e.getKey()).append('\n');
}
applyReviewerUpdatesToAttentionSet(msg);
applyReviewerUpdatesToAttentionSet();
for (Map.Entry<Address, ReviewerStateInternal> e : reviewersByEmail.entrySet()) {
addFooter(msg, e.getValue().getByEmailFooterKey(), e.getKey().toString());
@@ -694,7 +709,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
if (workInProgress != null) {
addFooter(msg, FOOTER_WORK_IN_PROGRESS, workInProgress);
if (workInProgress) {
clearAttentionSet(msg, "Change was marked work in progress");
clearAttentionSet("Change was marked work in progress");
}
}
@@ -706,6 +721,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
addFooter(msg, FOOTER_CHERRY_PICK_OF, cherryPickOf);
}
if (plannedAttentionSetUpdates != null) {
updateAttentionSet(msg);
}
CommitBuilder cb = new CommitBuilder();
cb.setMessage(msg.toString());
try {
@@ -719,11 +738,11 @@ public class ChangeUpdate extends AbstractChangeUpdate {
return cb;
}
private void clearAttentionSet(StringBuilder msg, String reason) {
private void clearAttentionSet(String reason) {
if (getNotes().getAttentionSet() == null) {
return;
}
Set<AttentionSetUpdate> toClear =
Set<AttentionSetUpdate> removeAll =
AttentionSetUtil.additionsOnly(getNotes().getAttentionSet()).stream()
.map(
a ->
@@ -731,10 +750,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
a.account(), AttentionSetUpdate.Operation.REMOVE, reason))
.collect(Collectors.toSet());
updateAttentionSet(msg, toClear);
addToPlannedAttentionSetUpdates(removeAll);
}
private void applyReviewerUpdatesToAttentionSet(StringBuilder msg) {
private void applyReviewerUpdatesToAttentionSet() {
if (workInProgress != null || getNotes().getChange().isWorkInProgress()) {
// Users shouldn't be added to the attention set if the change is work in progress.
return;
@@ -759,11 +778,18 @@ public class ChangeUpdate extends AbstractChangeUpdate {
reviewer.getKey(), AttentionSetUpdate.Operation.REMOVE, "Reviewer was removed"));
}
}
updateAttentionSet(msg, updates);
addToPlannedAttentionSetUpdates(updates);
}
private void updateAttentionSet(StringBuilder msg, Set<AttentionSetUpdate> updates) {
for (AttentionSetUpdate attentionSetUpdate : updates) {
/**
* Any updates to the attention set must be done in {@link #addToPlannedAttentionSetUpdates}. This
* method is called after all the updates are finished to do the updates once and for real.
*/
private void updateAttentionSet(StringBuilder msg) {
if (plannedAttentionSetUpdates == null) {
return;
}
for (AttentionSetUpdate attentionSetUpdate : plannedAttentionSetUpdates.values()) {
addFooter(msg, FOOTER_ATTENTION, noteUtil.attentionSetUpdateToJson(attentionSetUpdate));
}
}
@@ -794,7 +820,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
&& status == null
&& submissionId == null
&& submitRecords == null
&& attentionSetUpdates == null
&& plannedAttentionSetUpdates == null
&& assignee == null
&& hashtags == null
&& topic == null

2
java/com/google/gerrit/server/restapi/change/AddToAttentionSet.java
View File

@@ -84,7 +84,7 @@ public class AddToAttentionSet
try (BatchUpdate bu =
updateFactory.create(
changeResource.getChange().getProject(), changeResource.getUser(), TimeUtil.nowTs())) {
AddToAttentionSetOp op = opFactory.create(attentionUserId, input.reason);
AddToAttentionSetOp op = opFactory.create(attentionUserId, input.reason, true);
bu.addOp(changeResource.getId(), op);
bu.execute();
return Response.ok(accountLoaderFactory.create(true).fillOne(attentionUserId));

2
java/com/google/gerrit/server/restapi/change/RemoveFromAttentionSet.java
View File

@@ -61,7 +61,7 @@ public class RemoveFromAttentionSet
updateFactory.create(
changeResource.getProject(), changeResource.getUser(), TimeUtil.nowTs())) {
RemoveFromAttentionSetOp op =
opFactory.create(attentionResource.getAccountId(), input.reason);
opFactory.create(attentionResource.getAccountId(), input.reason, true);
bu.addOp(changeResource.getId(), op);
bu.execute();
}

29
javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
View File

@@ -133,6 +133,18 @@ public class AttentionSetIT extends AbstractDaemonTest {
assertThat(r.getChange().attentionSet()).containsExactly(expectedAttentionSetUpdate);
}
@Test
public void changeMessageWhenAddedAndRemovedExplicitly() throws Exception {
PushOneCommit.Result r = createChange();
change(r).addToAttentionSet(new AddToAttentionSetInput(user.email(), "user"));
assertThat(Iterables.getLast(r.getChange().messages()).getMessage())
.contains("Added to attention set");
change(r).attention(user.id().toString()).remove(new RemoveFromAttentionSetInput("foo"));
assertThat(Iterables.getLast(r.getChange().messages()).getMessage())
.contains("Removed from attention set");
}
@Test
public void removeUnrelatedUser() throws Exception {
PushOneCommit.Result r = createChange();
@@ -219,6 +231,23 @@ public class AttentionSetIT extends AbstractDaemonTest {
assertThat(attentionSet.reason()).isEqualTo("Reviewer was removed");
}
@Test
public void noChangeMessagesWhenAddedOrRemovedImplictly() throws Exception {
PushOneCommit.Result r = createChange();
change(r).addReviewer(user.id().toString());
change(r).reviewer(user.email()).remove();
assertThat(
r.getChange().messages().stream()
.noneMatch(u -> u.getMessage().contains("Added to attention set")))
.isTrue();
assertThat(
r.getChange().messages().stream()
.noneMatch(u -> u.getMessage().contains("Removed from attention set")))
.isTrue();
}
@Test
public void reviewersAddedAndRemovedByEmailFromAttentionSet() throws Exception {
PushOneCommit.Result r = createChange();

16
javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
View File

@@ -704,7 +704,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
ChangeUpdate update = newUpdate(c, changeOwner);
AttentionSetUpdate attentionSetUpdate =
AttentionSetUpdate.createForWrite(changeOwner.getAccountId(), Operation.ADD, "test");
update.setAttentionSetUpdates(ImmutableSet.of(attentionSetUpdate));
update.addToPlannedAttentionSetUpdates(ImmutableSet.of(attentionSetUpdate));
update.commit();
ChangeNotes notes = newNotes(c);
@@ -717,12 +717,12 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
ChangeUpdate update = newUpdate(c, changeOwner);
AttentionSetUpdate attentionSetUpdate =
AttentionSetUpdate.createForWrite(changeOwner.getAccountId(), Operation.ADD, "test");
update.setAttentionSetUpdates(ImmutableSet.of(attentionSetUpdate));
update.addToPlannedAttentionSetUpdates(ImmutableSet.of(attentionSetUpdate));
update.commit();
update = newUpdate(c, changeOwner);
attentionSetUpdate =
AttentionSetUpdate.createForWrite(changeOwner.getAccountId(), Operation.REMOVE, "test");
update.setAttentionSetUpdates(ImmutableSet.of(attentionSetUpdate));
update.addToPlannedAttentionSetUpdates(ImmutableSet.of(attentionSetUpdate));
update.commit();
ChangeNotes notes = newNotes(c);
@@ -739,23 +739,24 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() -> update.setAttentionSetUpdates(ImmutableSet.of(attentionSetUpdate)));
() -> update.addToPlannedAttentionSetUpdates(ImmutableSet.of(attentionSetUpdate)));
assertThat(thrown).hasMessageThat().contains("must not specify timestamp for write");
}
@Test
public void addAttentionStatus_rejectMultiplePerUser() throws Exception {
public void addAttentionStatus_rejectIfSameUserTwice() throws Exception {
Change c = newChange();
ChangeUpdate update = newUpdate(c, changeOwner);
AttentionSetUpdate attentionSetUpdate0 =
AttentionSetUpdate.createForWrite(changeOwner.getAccountId(), Operation.ADD, "test 0");
AttentionSetUpdate attentionSetUpdate1 =
AttentionSetUpdate.createForWrite(changeOwner.getAccountId(), Operation.ADD, "test 1");
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() ->
update.setAttentionSetUpdates(
update.addToPlannedAttentionSetUpdates(
ImmutableSet.of(attentionSetUpdate0, attentionSetUpdate1)));
assertThat(thrown)
.hasMessageThat()
@@ -771,7 +772,8 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
AttentionSetUpdate attentionSetUpdate1 =
AttentionSetUpdate.createForWrite(otherUser.getAccountId(), Operation.ADD, "test");
update.setAttentionSetUpdates(ImmutableSet.of(attentionSetUpdate0, attentionSetUpdate1));
update.addToPlannedAttentionSetUpdates(
ImmutableSet.of(attentionSetUpdate0, attentionSetUpdate1));
update.commit();
ChangeNotes notes = newNotes(c);

4
javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
View File

@@ -60,9 +60,9 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
+ "\n"
+ "Reviewer: Gerrit User 1 <1@gerrit>\n"
+ "CC: Gerrit User 2 <2@gerrit>\n"
+ "Attention: {\"person_ident\":\"Gerrit User 1 \\u003c1@gerrit\\u003e\",\"operation\":\"ADD\",\"reason\":\"Reviewer was added\"}\n"
+ "Label: Code-Review=-1\n"
+ "Label: Verified=+1\n",
+ "Label: Verified=+1\n"
+ "Attention: {\"person_ident\":\"Gerrit User 1 \\u003c1@gerrit\\u003e\",\"operation\":\"ADD\",\"reason\":\"Reviewer was added\"}\n",
commit);
PersonIdent author = commit.getAuthorIdent();