From 2345703a50aa978966a45afb20bdcae757d63fb4 Mon Sep 17 00:00:00 2001 From: Gal Paikin Date: Wed, 16 Oct 2019 15:09:56 +0200 Subject: [PATCH] Add AssigneeStatusUpdate Keep details for assignee changes, so that it would be possible to query for changes in the assignee field. Assignee and pastAssignees are removed from ChangeNotesParser as they are not needed. Assignee and pastAssignee are also removed from cache as they can be inferred. Change-Id: I57cd4a5bd5f5886daee26f42a308f0dad1bd8b8b --- .../gerrit/server/AssigneeStatusUpdate.java | 21 ++++++ .../gerrit/server/notedb/ChangeNotes.java | 22 +++++- .../server/notedb/ChangeNotesCache.java | 7 +- .../server/notedb/ChangeNotesParser.java | 23 ++---- .../server/notedb/ChangeNotesState.java | 66 ++++++++++------- .../server/notedb/ChangeNotesParserTest.java | 20 +++++ .../server/notedb/ChangeNotesStateTest.java | 73 ++++++++++++------- .../gerrit/server/notedb/ChangeNotesTest.java | 30 ++++++++ proto/cache.proto | 16 +++- 9 files changed, 202 insertions(+), 76 deletions(-) create mode 100644 java/com/google/gerrit/server/AssigneeStatusUpdate.java diff --git a/java/com/google/gerrit/server/AssigneeStatusUpdate.java b/java/com/google/gerrit/server/AssigneeStatusUpdate.java new file mode 100644 index 0000000000..0da5edf981 --- /dev/null +++ b/java/com/google/gerrit/server/AssigneeStatusUpdate.java @@ -0,0 +1,21 @@ +package com.google.gerrit.server; + +import com.google.auto.value.AutoValue; +import com.google.gerrit.entities.Account; +import java.sql.Timestamp; +import java.util.Optional; + +/** Change to an assignee's status. */ +@AutoValue +public abstract class AssigneeStatusUpdate { + public static AssigneeStatusUpdate create( + Timestamp ts, Account.Id updatedBy, Optional currentAssignee) { + return new AutoValue_AssigneeStatusUpdate(ts, updatedBy, currentAssignee); + } + + public abstract Timestamp date(); + + public abstract Account.Id updatedBy(); + + public abstract Optional currentAssignee(); +} diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java index 384eebcb4e..8cf00461dc 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.notedb; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.gerrit.entities.RefNames.changeMetaRef; import static java.util.Comparator.comparing; import static java.util.Objects.requireNonNull; @@ -29,6 +30,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.ListMultimap; +import com.google.common.collect.Lists; import com.google.common.collect.MultimapBuilder; import com.google.common.collect.Multimaps; import com.google.common.collect.Ordering; @@ -48,6 +50,7 @@ import com.google.gerrit.entities.Project; import com.google.gerrit.entities.RefNames; import com.google.gerrit.entities.RobotComment; import com.google.gerrit.exceptions.StorageException; +import com.google.gerrit.server.AssigneeStatusUpdate; import com.google.gerrit.server.ReviewerByEmailSet; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.ReviewerStatusUpdate; @@ -366,9 +369,24 @@ public class ChangeNotes extends AbstractChangeNotes { return state.reviewerUpdates(); } - /** @return an ImmutableSet of Account.Ids of all users that have been assigned to this change. */ + /** + * @return an ImmutableSet of Account.Ids of all users that have been assigned to this change. The + * order of the set is the order in which they were assigned. + */ public ImmutableSet getPastAssignees() { - return state.pastAssignees(); + return Lists.reverse(state.assigneeUpdates()).stream() + .map(AssigneeStatusUpdate::currentAssignee) + .filter(Optional::isPresent) + .map(Optional::get) + .collect(toImmutableSet()); + } + + /** + * @return an ImmutableList of AssigneeStatusUpdate of all the updates to the assignee field to + * this change. The order of the list is from most recent updates to least recent. + */ + public ImmutableList getAssigneeUpdates() { + return state.assigneeUpdates(); } /** @return a ImmutableSet of all hashtags for this change sorted in alphabetical order. */ diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java index 03073d2fe2..8f5fefec59 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java @@ -61,7 +61,7 @@ public class ChangeNotesCache { .weigher(Weigher.class) .maximumWeight(10 << 20) .diskLimit(-1) - .version(1) + .version(2) .keySerializer(Key.Serializer.INSTANCE) .valueSerializer(ChangeNotesState.Serializer.INSTANCE); } @@ -148,11 +148,8 @@ public class ChangeNotesCache { + str(state.columns().originalSubject()) + P + str(state.columns().submissionId()) - + ptr(state.columns().assignee(), K) // assignee + P // status + P - + set(state.pastAssignees(), K) - + P + set(state.hashtags(), str(10)) + P + list(state.patchSets(), patchSet()) @@ -169,6 +166,8 @@ public class ChangeNotesCache { + P + list(state.reviewerUpdates(), 4 * O + K + K + P) + P + + list(state.assigneeUpdates(), 4 * O + K + K) + + P + list(state.submitRecords(), P + list(2, str(4) + P + K) + P) + P + list(state.changeMessages(), changeMessage()) diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java index c4c227ee88..a3c47470a5 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -67,6 +67,7 @@ import com.google.gerrit.entities.PatchSetApproval; import com.google.gerrit.entities.RefNames; import com.google.gerrit.mail.Address; import com.google.gerrit.metrics.Timer1; +import com.google.gerrit.server.AssigneeStatusUpdate; import com.google.gerrit.server.ReviewerByEmailSet; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.ReviewerStatusUpdate; @@ -115,6 +116,7 @@ class ChangeNotesParser { private final Table reviewersByEmail; private final List allPastReviewers; private final List reviewerUpdates; + private final List assigneeUpdates; private final List submitRecords; private final ListMultimap comments; private final Map patchSets; @@ -129,8 +131,6 @@ class ChangeNotesParser { private String branch; private Change.Status status; private String topic; - private Optional assignee; - private List pastAssignees; private Set hashtags; private Timestamp createdOn; private Timestamp lastUpdatedOn; @@ -172,6 +172,7 @@ class ChangeNotesParser { pendingReviewersByEmail = ReviewerByEmailSet.empty(); allPastReviewers = new ArrayList<>(); reviewerUpdates = new ArrayList<>(); + assigneeUpdates = new ArrayList<>(); submitRecords = Lists.newArrayListWithExpectedSize(1); allChangeMessages = new ArrayList<>(); comments = MultimapBuilder.hashKeys().arrayListValues().build(); @@ -231,9 +232,7 @@ class ChangeNotesParser { topic, originalSubject, submissionId, - assignee != null ? assignee.orElse(null) : null, status, - Sets.newLinkedHashSet(Lists.reverse(pastAssignees)), firstNonNull(hashtags, ImmutableSet.of()), buildPatchSets(), buildApprovals(), @@ -243,6 +242,7 @@ class ChangeNotesParser { pendingReviewersByEmail, allPastReviewers, buildReviewerUpdates(), + assigneeUpdates, submitRecords, buildAllMessages(), comments, @@ -361,7 +361,7 @@ class ChangeNotesParser { } parseHashtags(commit); - parseAssignee(commit); + parseAssigneeUpdates(ts, commit); if (submissionId == null) { submissionId = parseSubmissionId(commit); @@ -566,10 +566,8 @@ class ChangeNotesParser { } } - private void parseAssignee(ChangeNotesCommit commit) throws ConfigInvalidException { - if (pastAssignees == null) { - pastAssignees = Lists.newArrayList(); - } + private void parseAssigneeUpdates(Timestamp ts, ChangeNotesCommit commit) + throws ConfigInvalidException { String assigneeValue = parseOneFooter(commit, FOOTER_ASSIGNEE); if (assigneeValue != null) { Optional parsedAssignee; @@ -580,12 +578,7 @@ class ChangeNotesParser { PersonIdent ident = RawParseUtils.parsePersonIdent(assigneeValue); parsedAssignee = Optional.ofNullable(legacyChangeNoteRead.parseIdent(ident, id)); } - if (assignee == null) { - assignee = parsedAssignee; - } - if (parsedAssignee.isPresent()) { - pastAssignees.add(parsedAssignee.get()); - } + assigneeUpdates.add(AssigneeStatusUpdate.create(ts, ownerId, parsedAssignee)); } } diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesState.java b/java/com/google/gerrit/server/notedb/ChangeNotesState.java index 53965086c0..896cca3ba4 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesState.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesState.java @@ -18,7 +18,6 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableListMultimap.toImmutableListMultimap; -import static com.google.common.collect.ImmutableSet.toImmutableSet; import static java.util.Objects.requireNonNull; import com.google.auto.value.AutoValue; @@ -50,10 +49,12 @@ import com.google.gerrit.entities.converter.ProtoConverter; import com.google.gerrit.json.OutputFormat; import com.google.gerrit.mail.Address; import com.google.gerrit.proto.Protos; +import com.google.gerrit.server.AssigneeStatusUpdate; import com.google.gerrit.server.ReviewerByEmailSet; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.ReviewerStatusUpdate; import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto; +import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.AssigneeStatusUpdateProto; import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ChangeColumnsProto; import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ReviewerByEmailSetEntryProto; import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ReviewerSetEntryProto; @@ -67,6 +68,7 @@ import com.google.protobuf.MessageLite; import java.sql.Timestamp; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import org.eclipse.jgit.lib.ObjectId; @@ -103,9 +105,7 @@ public abstract class ChangeNotesState { @Nullable String topic, @Nullable String originalSubject, @Nullable String submissionId, - @Nullable Account.Id assignee, @Nullable Change.Status status, - Set pastAssignees, Set hashtags, Map patchSets, ListMultimap approvals, @@ -115,6 +115,7 @@ public abstract class ChangeNotesState { ReviewerByEmailSet pendingReviewersByEmail, List allPastReviewers, List reviewerUpdates, + List assigneeUpdates, List submitRecords, List changeMessages, ListMultimap publishedComments, @@ -147,13 +148,11 @@ public abstract class ChangeNotesState { .topic(topic) .originalSubject(originalSubject) .submissionId(submissionId) - .assignee(assignee) .isPrivate(isPrivate) .workInProgress(workInProgress) .reviewStarted(reviewStarted) .revertOf(revertOf) .build()) - .pastAssignees(pastAssignees) .hashtags(hashtags) .serverId(serverId) .patchSets(patchSets.entrySet()) @@ -164,6 +163,7 @@ public abstract class ChangeNotesState { .pendingReviewersByEmail(pendingReviewersByEmail) .allPastReviewers(allPastReviewers) .reviewerUpdates(reviewerUpdates) + .assigneeUpdates(assigneeUpdates) .submitRecords(submitRecords) .changeMessages(changeMessages) .publishedComments(publishedComments) @@ -211,9 +211,6 @@ public abstract class ChangeNotesState { @Nullable abstract String submissionId(); - @Nullable - abstract Account.Id assignee(); - abstract boolean isPrivate(); abstract boolean workInProgress(); @@ -247,8 +244,6 @@ public abstract class ChangeNotesState { abstract Builder submissionId(@Nullable String submissionId); - abstract Builder assignee(@Nullable Account.Id assignee); - abstract Builder status(@Nullable Change.Status status); abstract Builder isPrivate(boolean isPrivate); @@ -274,8 +269,6 @@ public abstract class ChangeNotesState { abstract ChangeColumns columns(); // Other related to this Change. - abstract ImmutableSet pastAssignees(); - abstract ImmutableSet hashtags(); @Nullable @@ -297,6 +290,8 @@ public abstract class ChangeNotesState { abstract ImmutableList reviewerUpdates(); + abstract ImmutableList assigneeUpdates(); + abstract ImmutableList submitRecords(); abstract ImmutableList changeMessages(); @@ -339,7 +334,9 @@ public abstract class ChangeNotesState { change.setTopic(Strings.emptyToNull(c.topic())); change.setLastUpdatedOn(c.lastUpdatedOn()); change.setSubmissionId(c.submissionId()); - change.setAssignee(c.assignee()); + if (!assigneeUpdates().isEmpty()) { + change.setAssignee(assigneeUpdates().get(0).currentAssignee().orElse(null)); + } change.setPrivate(c.isPrivate()); change.setWorkInProgress(c.workInProgress()); change.setReviewStarted(c.reviewStarted()); @@ -359,7 +356,6 @@ public abstract class ChangeNotesState { static Builder empty(Change.Id changeId) { return new AutoValue_ChangeNotesState.Builder() .changeId(changeId) - .pastAssignees(ImmutableSet.of()) .hashtags(ImmutableSet.of()) .patchSets(ImmutableList.of()) .approvals(ImmutableList.of()) @@ -369,6 +365,7 @@ public abstract class ChangeNotesState { .pendingReviewersByEmail(ReviewerByEmailSet.empty()) .allPastReviewers(ImmutableList.of()) .reviewerUpdates(ImmutableList.of()) + .assigneeUpdates(ImmutableList.of()) .submitRecords(ImmutableList.of()) .changeMessages(ImmutableList.of()) .publishedComments(ImmutableListMultimap.of()) @@ -383,8 +380,6 @@ public abstract class ChangeNotesState { abstract Builder serverId(String serverId); - abstract Builder pastAssignees(Set pastAssignees); - abstract Builder hashtags(Iterable hashtags); abstract Builder patchSets(Iterable> patchSets); @@ -403,6 +398,8 @@ public abstract class ChangeNotesState { abstract Builder reviewerUpdates(List reviewerUpdates); + abstract Builder assigneeUpdates(List assigneeUpdates); + abstract Builder submitRecords(List submitRecords); abstract Builder changeMessages(List changeMessages); @@ -438,7 +435,6 @@ public abstract class ChangeNotesState { b.setServerId(object.serverId()); b.setHasServerId(true); } - object.pastAssignees().forEach(a -> b.addPastAssignee(a.get())); object.hashtags().forEach(b::addHashtag); object .patchSets() @@ -469,6 +465,7 @@ public abstract class ChangeNotesState { object.allPastReviewers().forEach(a -> b.addPastReviewer(a.get())); object.reviewerUpdates().forEach(u -> b.addReviewerUpdate(toReviewerStatusUpdateProto(u))); + object.assigneeUpdates().forEach(u -> b.addAssigneeUpdate(toAssigneeStatusUpdateProto(u))); object .submitRecords() .forEach(r -> b.addSubmitRecord(GSON.toJson(new StoredSubmitRecord(r)))); @@ -508,9 +505,6 @@ public abstract class ChangeNotesState { if (cols.submissionId() != null) { b.setSubmissionId(cols.submissionId()).setHasSubmissionId(true); } - if (cols.assignee() != null) { - b.setAssignee(cols.assignee().get()).setHasAssignee(true); - } if (cols.status() != null) { b.setStatus(STATUS_CONVERTER.reverse().convert(cols.status())).setHasStatus(true); } @@ -550,6 +544,17 @@ public abstract class ChangeNotesState { .build(); } + private static AssigneeStatusUpdateProto toAssigneeStatusUpdateProto(AssigneeStatusUpdate u) { + AssigneeStatusUpdateProto.Builder builder = + AssigneeStatusUpdateProto.newBuilder() + .setDate(u.date().getTime()) + .setUpdatedBy(u.updatedBy().get()) + .setHasCurrentAssignee(u.currentAssignee().isPresent()); + + u.currentAssignee().ifPresent(assignee -> builder.setCurrentAssignee(assignee.get())); + return builder.build(); + } + @Override public ChangeNotesState deserialize(byte[] in) { ChangeNotesStateProto proto = Protos.parseUnchecked(ChangeNotesStateProto.parser(), in); @@ -561,8 +566,6 @@ public abstract class ChangeNotesState { .changeId(changeId) .columns(toChangeColumns(changeId, proto.getColumns())) .serverId(proto.getHasServerId() ? proto.getServerId() : null) - .pastAssignees( - proto.getPastAssigneeList().stream().map(Account::id).collect(toImmutableSet())) .hashtags(proto.getHashtagList()) .patchSets( proto.getPatchSetList().stream() @@ -581,6 +584,7 @@ public abstract class ChangeNotesState { .allPastReviewers( proto.getPastReviewerList().stream().map(Account::id).collect(toImmutableList())) .reviewerUpdates(toReviewerStatusUpdateList(proto.getReviewerUpdateList())) + .assigneeUpdates(toAssigneeStatusUpdateList(proto.getAssigneeUpdateList())) .submitRecords( proto.getSubmitRecordList().stream() .map(r -> GSON.fromJson(r, StoredSubmitRecord.class).toSubmitRecord()) @@ -624,9 +628,6 @@ public abstract class ChangeNotesState { if (proto.getHasSubmissionId()) { b.submissionId(proto.getSubmissionId()); } - if (proto.getHasAssignee()) { - b.assignee(Account.id(proto.getAssignee())); - } if (proto.getHasStatus()) { b.status(STATUS_CONVERTER.convert(proto.getStatus())); } @@ -677,5 +678,20 @@ public abstract class ChangeNotesState { } return b.build(); } + + private static ImmutableList toAssigneeStatusUpdateList( + List protos) { + ImmutableList.Builder b = ImmutableList.builder(); + for (AssigneeStatusUpdateProto proto : protos) { + b.add( + AssigneeStatusUpdate.create( + new Timestamp(proto.getDate()), + Account.id(proto.getUpdatedBy()), + proto.getHasCurrentAssignee() + ? Optional.of(Account.id(proto.getCurrentAssignee())) + : Optional.empty())); + } + return b.build(); + } } } diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java index ed79be62fa..7882518f9a 100644 --- a/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java @@ -213,6 +213,26 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { assertParseFails("Update change\n\nPatch-set: 1\nReviewer: 1@gerrit\n"); } + @Test + public void parseAssignee() throws Exception { + assertParseSucceeds( + "Update change\n" + + "\n" + + "Branch: refs/heads/master\n" + + "Change-id: I577fb248e474018276351785930358ec0450e9f7\n" + + "Patch-set: 1\n" + + "Assignee: Change Owner <1@gerrit>\n" + + "Subject: This is a test change\n"); + assertParseSucceeds( + "Update change\n" + + "\n" + + "Branch: refs/heads/master\n" + + "Change-id: I577fb248e474018276351785930358ec0450e9f7\n" + + "Patch-set: 2\n" + + "Assignee:\n" + + "Subject: This is a test change\n"); + } + @Test public void parseTopic() throws Exception { assertParseSucceeds( diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java index d381e99e96..0d7f2bd171 100644 --- a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java @@ -38,10 +38,12 @@ import com.google.gerrit.entities.converter.ChangeMessageProtoConverter; import com.google.gerrit.entities.converter.PatchSetApprovalProtoConverter; import com.google.gerrit.entities.converter.PatchSetProtoConverter; import com.google.gerrit.mail.Address; +import com.google.gerrit.server.AssigneeStatusUpdate; import com.google.gerrit.server.ReviewerByEmailSet; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.ReviewerStatusUpdate; import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto; +import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.AssigneeStatusUpdateProto; import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ChangeColumnsProto; import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ReviewerByEmailSetEntryProto; import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ReviewerSetEntryProto; @@ -241,17 +243,6 @@ public class ChangeNotesStateTest { .build()); } - @Test - public void serializeAssignee() throws Exception { - assertRoundTrip( - newBuilder().columns(cols.toBuilder().assignee(Account.id(2000)).build()).build(), - ChangeNotesStateProto.newBuilder() - .setMetaId(SHA_BYTES) - .setChangeId(ID.get()) - .setColumns(colsProto.toBuilder().setAssignee(2000).setHasAssignee(true)) - .build()); - } - @Test public void serializeStatus() throws Exception { assertRoundTrip( @@ -307,19 +298,6 @@ public class ChangeNotesStateTest { .build()); } - @Test - public void serializePastAssignees() throws Exception { - assertRoundTrip( - newBuilder().pastAssignees(ImmutableSet.of(Account.id(2002), Account.id(2001))).build(), - ChangeNotesStateProto.newBuilder() - .setMetaId(SHA_BYTES) - .setChangeId(ID.get()) - .setColumns(colsProto) - .addPastAssignee(2002) - .addPastAssignee(2001) - .build()); - } - @Test public void serializeHashtags() throws Exception { assertRoundTrip( @@ -610,6 +588,35 @@ public class ChangeNotesStateTest { .build()); } + @Test + public void serializeAssigneeUpdates() throws Exception { + assertRoundTrip( + newBuilder() + .assigneeUpdates( + ImmutableList.of( + AssigneeStatusUpdate.create( + new Timestamp(1212L), Account.id(1000), Optional.of(Account.id(2001))), + AssigneeStatusUpdate.create( + new Timestamp(3434L), Account.id(1000), Optional.empty()))) + .build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .addAssigneeUpdate( + AssigneeStatusUpdateProto.newBuilder() + .setDate(1212L) + .setUpdatedBy(1000) + .setCurrentAssignee(2001) + .setHasCurrentAssignee(true)) + .addAssigneeUpdate( + AssigneeStatusUpdateProto.newBuilder() + .setDate(3434L) + .setUpdatedBy(1000) + .setHasCurrentAssignee(false)) + .build()); + } + @Test public void serializeSubmitRecords() throws Exception { SubmitRecord sr1 = new SubmitRecord(); @@ -721,7 +728,6 @@ public class ChangeNotesStateTest { .put("changeId", Change.Id.class) .put("serverId", String.class) .put("columns", ChangeColumns.class) - .put("pastAssignees", new TypeLiteral>() {}.getType()) .put("hashtags", new TypeLiteral>() {}.getType()) .put( "patchSets", @@ -738,6 +744,9 @@ public class ChangeNotesStateTest { .put( "reviewerUpdates", new TypeLiteral>() {}.getType()) + .put( + "assigneeUpdates", + new TypeLiteral>() {}.getType()) .put("submitRecords", new TypeLiteral>() {}.getType()) .put("changeMessages", new TypeLiteral>() {}.getType()) .put( @@ -762,7 +771,6 @@ public class ChangeNotesStateTest { .put("topic", String.class) .put("originalSubject", String.class) .put("submissionId", String.class) - .put("assignee", Account.Id.class) .put("status", Change.Status.class) .put("isPrivate", boolean.class) .put("workInProgress", boolean.class) @@ -843,6 +851,19 @@ public class ChangeNotesStateTest { "state", ReviewerStateInternal.class)); } + @Test + public void assigneeStatusUpdateMethods() throws Exception { + assertThatSerializedClass(AssigneeStatusUpdate.class) + .hasAutoValueMethods( + ImmutableMap.of( + "date", + Timestamp.class, + "updatedBy", + Account.Id.class, + "currentAssignee", + new TypeLiteral>() {}.getType())); + } + @Test public void submitRecordFields() throws Exception { assertThatSerializedClass(SubmitRecord.class) diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java index 2ed145ee0d..a4c395c11b 100644 --- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -48,6 +48,7 @@ import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.PatchSetApproval; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.mail.Address; +import com.google.gerrit.server.AssigneeStatusUpdate; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.ReviewerSet; @@ -744,6 +745,35 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { assertThat(notes.getPastAssignees()).hasSize(2); } + @Test + public void assigneeStatusUpdateChangeNotes() throws Exception { + Change c = newChange(); + ChangeUpdate update = newUpdate(c, otherUser); + update.setAssignee(otherUserId); + update.commit(); + + update = newUpdate(c, changeOwner); + update.removeAssignee(); + update.commit(); + + update = newUpdate(c, changeOwner); + update.setAssignee(changeOwner.getAccountId()); + update.commit(); + + update = newUpdate(c, changeOwner); + update.setAssignee(otherUserId); + update.commit(); + + ChangeNotes notes = newNotes(c); + ImmutableList statusUpdates = notes.getAssigneeUpdates(); + assertThat(statusUpdates).hasSize(4); + assertThat(statusUpdates.get(3).updatedBy()).isEqualTo(otherUserId); + assertThat(statusUpdates.get(3).currentAssignee()).hasValue(otherUserId); + assertThat(statusUpdates.get(2).currentAssignee()).isEmpty(); + assertThat(statusUpdates.get(1).currentAssignee()).hasValue(changeOwner.getAccountId()); + assertThat(statusUpdates.get(0).currentAssignee()).hasValue(otherUserId); + } + @Test public void hashtagCommit() throws Exception { Change c = newChange(); diff --git a/proto/cache.proto b/proto/cache.proto index 2a0764098c..10e0216cf1 100644 --- a/proto/cache.proto +++ b/proto/cache.proto @@ -75,7 +75,7 @@ message ChangeNotesKeyProto { // Instead, we just take the tedious yet simple approach of having a "has_foo" // field for each nullable field "foo", indicating whether or not foo is null. // -// Next ID: 20 +// Next ID: 23 message ChangeNotesStateProto { // Effectively required, even though the corresponding ChangeNotesState field // is optional, since the field is only absent when NoteDb is disabled, in @@ -110,8 +110,8 @@ message ChangeNotesStateProto { string submission_id = 13; bool has_submission_id = 14; - int32 assignee = 15; - bool has_assignee = 16; + reserved 15; // assignee + reserved 16; // has_assignee string status = 17; bool has_status = 18; @@ -130,7 +130,7 @@ message ChangeNotesStateProto { // which case attempting to use the ChangeNotesCache is programmer error. ChangeColumnsProto columns = 3; - repeated int32 past_assignee = 4; + reserved 4; // past_assignee repeated string hashtag = 5; @@ -189,6 +189,14 @@ message ChangeNotesStateProto { string server_id = 20; bool has_server_id = 21; + + message AssigneeStatusUpdateProto { + int64 date = 1; + int32 updated_by = 2; + int32 current_assignee = 3; + bool has_current_assignee = 4; + } + repeated AssigneeStatusUpdateProto assignee_update = 22; }