opendev/gerrit
Code Issues Proposed changes

Merge changes from topic 'change-messages'

Browse Source 
* changes:
  Store change messages in notedb when notedb is rebuilt
  Add change message when hashtags are added/removed
This commit is contained in:
David Pursehouse
2016-01-27 01:05:23 +00:00
committed by Gerrit Code Review
parent c3f258ef58 0c0a203fcb
commit 41217fe7b0
5 changed files with 126 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

53
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/HashtagsIT.java
View File

@@ -17,16 +17,22 @@ package com.google.gerrit.acceptance.rest.change;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
import com.google.common.collect.Iterables;
import com.google.common.collect.Sets;
import com.google.common.truth.IterableSubject;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.extensions.api.changes.HashtagsInput;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gwtorm.server.OrmException;
import org.junit.Before;
import org.junit.Test;
import java.sql.Timestamp;
@NoHttpd
public class HashtagsIT extends AbstractDaemonTest {
@Before
@@ -48,11 +54,13 @@ public class HashtagsIT extends AbstractDaemonTest {
// Adding a single hashtag returns a single hashtag.
addHashtags(r, "tag2");
assertThatGet(r).containsExactly("tag2");
assertMessage(r, "Hashtag added: tag2");
// Adding another single hashtag to change that already has one hashtag
// returns a sorted list of hashtags with existing and new.
addHashtags(r, "tag1");
assertThatGet(r).containsExactly("tag1", "tag2").inOrder();
assertMessage(r, "Hashtag added: tag1");
}
@Test
@@ -62,11 +70,13 @@ public class HashtagsIT extends AbstractDaemonTest {
// Adding multiple hashtags returns a sorted list of hashtags.
addHashtags(r, "tag3", "tag1");
assertThatGet(r).containsExactly("tag1", "tag3").inOrder();
assertMessage(r, "Hashtags added: tag1, tag3");
// Adding multiple hashtags to change that already has hashtags returns a
// sorted list of hashtags with existing and new.
addHashtags(r, "tag2", "tag4");
assertThatGet(r).containsExactly("tag1", "tag2", "tag3", "tag4").inOrder();
assertMessage(r, "Hashtags added: tag2, tag4");
}
@Test
@@ -76,10 +86,16 @@ public class HashtagsIT extends AbstractDaemonTest {
PushOneCommit.Result r = createChange();
addHashtags(r, "tag2");
assertThatGet(r).containsExactly("tag2");
assertMessage(r, "Hashtag added: tag2");
Timestamp lastMsgDate = getLastMessage(r).date;
addHashtags(r, "tag2");
assertThatGet(r).containsExactly("tag2");
assertNoNewMessageSince(r, lastMsgDate);
addHashtags(r, "tag1", "tag2");
assertThatGet(r).containsExactly("tag1", "tag2").inOrder();
assertMessage(r, "Hashtag added: tag1");
}
@Test
@@ -89,30 +105,37 @@ public class HashtagsIT extends AbstractDaemonTest {
// Leading # is stripped from added tag.
addHashtags(r, "#tag1");
assertThatGet(r).containsExactly("tag1");
assertMessage(r, "Hashtag added: tag1");
// Leading # is stripped from multiple added tags.
addHashtags(r, "#tag2", "#tag3");
assertThatGet(r).containsExactly("tag1", "tag2", "tag3").inOrder();
assertMessage(r, "Hashtags added: tag2, tag3");
// Leading # is stripped from removed tag.
removeHashtags(r, "#tag2");
assertThatGet(r).containsExactly("tag1", "tag3").inOrder();
assertMessage(r, "Hashtag removed: tag2");
// Leading # is stripped from multiple removed tags.
removeHashtags(r, "#tag1", "#tag3");
assertThatGet(r).isEmpty();
assertMessage(r, "Hashtags removed: tag1, tag3");
// Leading # and space are stripped from added tag.
addHashtags(r, "# tag1");
assertThatGet(r).containsExactly("tag1");
assertMessage(r, "Hashtag added: tag1");
// Multiple leading # are stripped from added tag.
addHashtags(r, "##tag2");
assertThatGet(r).containsExactly("tag1", "tag2").inOrder();
assertMessage(r, "Hashtag added: tag2");
// Multiple leading spaces and # are stripped from added tag.
addHashtags(r, "# # tag3");
assertThatGet(r).containsExactly("tag1", "tag2", "tag3").inOrder();
assertMessage(r, "Hashtag added: tag3");
}
@Test
@@ -124,12 +147,14 @@ public class HashtagsIT extends AbstractDaemonTest {
assertThatGet(r).containsExactly("tag1");
removeHashtags(r, "tag1");
assertThatGet(r).isEmpty();
assertMessage(r, "Hashtag removed: tag1");
// Removing a single tag from a change that has multiple tags returns a
// sorted list of remaining tags.
addHashtags(r, "tag1", "tag2", "tag3");
removeHashtags(r, "tag2");
assertThatGet(r).containsExactly("tag1", "tag3").inOrder();
assertMessage(r, "Hashtag removed: tag2");
}
@Test
@@ -141,6 +166,7 @@ public class HashtagsIT extends AbstractDaemonTest {
assertThatGet(r).containsExactly("tag1", "tag2").inOrder();
removeHashtags(r, "tag1", "tag2");
assertThatGet(r).isEmpty();
assertMessage(r, "Hashtags removed: tag1, tag2");
// Removing multiple tags from a change that has multiple tags returns a
// sorted list of remaining tags.
@@ -148,6 +174,7 @@ public class HashtagsIT extends AbstractDaemonTest {
assertThatGet(r).containsExactly("tag1", "tag2", "tag3", "tag4").inOrder();
removeHashtags(r, "tag2", "tag4");
assertThatGet(r).containsExactly("tag1", "tag3").inOrder();
assertMessage(r, "Hashtags removed: tag2, tag4");
}
@Test
@@ -155,20 +182,26 @@ public class HashtagsIT extends AbstractDaemonTest {
// Removing a single hashtag from change that has no hashtags returns an
// empty list.
PushOneCommit.Result r = createChange();
Timestamp lastMsgDate = getLastMessage(r).date;
removeHashtags(r, "tag1");
assertThatGet(r).isEmpty();
assertNoNewMessageSince(r, lastMsgDate);
// Removing a single non-existing tag from a change that only has one other
// tag returns a list of only one tag.
addHashtags(r, "tag1");
lastMsgDate = getLastMessage(r).date;
removeHashtags(r, "tag4");
assertThatGet(r).containsExactly("tag1");
assertNoNewMessageSince(r, lastMsgDate);
// Removing a single non-existing tag from a change that has multiple tags
// returns a sorted list of tags without any deleted.
addHashtags(r, "tag1", "tag2", "tag3");
lastMsgDate = getLastMessage(r).date;
removeHashtags(r, "tag4");
assertThatGet(r).containsExactly("tag1", "tag2", "tag3").inOrder();
assertNoNewMessageSince(r, lastMsgDate);
}
@Test
@@ -181,6 +214,7 @@ public class HashtagsIT extends AbstractDaemonTest {
input.remove = Sets.newHashSet("tag1");
gApi.changes().id(r.getChange().getId().get()).setHashtags(input);
assertThatGet(r).containsExactly("tag2", "tag3", "tag4");
assertMessage(r, "Hashtags added: tag3, tag4\nHashtag removed: tag1");
// Adding and removing the same hashtag actually removes it.
addHashtags(r, "tag1", "tag2");
@@ -189,6 +223,7 @@ public class HashtagsIT extends AbstractDaemonTest {
input.remove = Sets.newHashSet("tag3");
gApi.changes().id(r.getChange().getId().get()).setHashtags(input);
assertThatGet(r).containsExactly("tag1", "tag2", "tag4");
assertMessage(r, "Hashtag removed: tag3");
}
private IterableSubject<
@@ -217,4 +252,22 @@ public class HashtagsIT extends AbstractDaemonTest {
.id(r.getChange().getId().get())
.setHashtags(input);
}
private void assertMessage(PushOneCommit.Result r, String expectedMessage)
throws RestApiException, OrmException {
assertThat(getLastMessage(r).message).isEqualTo(expectedMessage);
}
private void assertNoNewMessageSince(PushOneCommit.Result r, Timestamp date)
throws RestApiException, OrmException {
assertThat(getLastMessage(r).date).isEqualTo(date);
}
private ChangeMessageInfo getLastMessage(PushOneCommit.Result r)
throws RestApiException, OrmException {
ChangeMessageInfo lastMessage = Iterables.getLast(
gApi.changes().id(r.getChange().getId().get()).get().messages, null);
assertThat(lastMessage).isNotNull();
return lastMessage;
}
}

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java
View File

@@ -1181,7 +1181,7 @@ public class ChangeScreen extends Screen {
related.set(info, revision);
reviewers.set(info);
if (Gerrit.isNoteDbEnabled()) {
hashtags.set(info);
hashtags.set(info, revision);
} else {
setVisible(hashtagTableRow, false);
}

32
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Hashtags.java
View File

@@ -54,6 +54,7 @@ public class Hashtags extends Composite {
private static final String REMOVE;
private static final String DATA_ID = "data-id";
private PatchSet.Id psId;
private boolean canEdit;
static {
@@ -135,7 +136,11 @@ public class Hashtags extends Composite {
this.style = style;
}
void set(ChangeInfo info) {
void set(ChangeInfo info, String revision) {
psId = new PatchSet.Id(
info.legacyId(),
info.revisions().get(revision)._number());
canEdit = info.hasActions() && info.actions().containsKey("hashtags");
this.changeId = info.legacyId();
display(info);
@@ -219,14 +224,9 @@ public class Hashtags extends Composite {
new GerritCallback<JsArrayString>() {
@Override
public void onSuccess(JsArrayString result) {
hashtagTextBox.setEnabled(true);
UIObject.setVisible(error, false);
error.setInnerText("");
hashtagTextBox.setText("");
if (result != null && result.length() > 0) {
updateHashtagList(result);
}
Gerrit.display(PageLinks.toChange(
psId.getParentKey(),
String.valueOf(psId.get())));
}
@Override
@@ -240,20 +240,6 @@ public class Hashtags extends Composite {
});
}
protected void updateHashtagList() {
ChangeApi.detail(changeId.get(),
new GerritCallback<ChangeInfo>() {
@Override
public void onSuccess(ChangeInfo result) {
display(result);
}
});
}
protected void updateHashtagList(JsArrayString hashtags){
display(hashtags);
}
public static class PostInput extends JavaScriptObject {
public static PostInput create(String add, String remove) {
PostInput input = createObject().cast();

43
gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java
View File

@@ -17,7 +17,9 @@ package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.change.HashtagsUtil.extractTags;
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Ordering;
import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.api.changes.HashtagsInput;
@@ -25,6 +27,9 @@ import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.git.BatchUpdate;
import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
import com.google.gerrit.server.git.BatchUpdate.Context;
@@ -46,6 +51,7 @@ public class SetHashtagsOp extends BatchUpdate.Op {
SetHashtagsOp create(HashtagsInput input);
}
private final ChangeMessagesUtil cmUtil;
private final ChangeHooks hooks;
private final DynamicSet<HashtagValidationListener> validationListeners;
private final HashtagsInput input;
@@ -59,9 +65,11 @@ public class SetHashtagsOp extends BatchUpdate.Op {
@AssistedInject
SetHashtagsOp(
ChangeMessagesUtil cmUtil,
ChangeHooks hooks,
DynamicSet<HashtagValidationListener> validationListeners,
@Assisted @Nullable HashtagsInput input) {
this.cmUtil = cmUtil;
this.hooks = hooks;
this.validationListeners = validationListeners;
this.input = input;
@@ -109,12 +117,47 @@ public class SetHashtagsOp extends BatchUpdate.Op {
updated.addAll(toAdd);
updated.removeAll(toRemove);
update.setHashtags(updated);
addMessage(ctx, update);
}
updatedHashtags = ImmutableSortedSet.copyOf(updated);
return true;
}
private void addMessage(Context ctx, ChangeUpdate update)
throws OrmException {
StringBuilder msg = new StringBuilder();
appendHashtagMessage(msg, "added", toAdd);
appendHashtagMessage(msg, "removed", toRemove);
ChangeMessage cmsg = new ChangeMessage(
new ChangeMessage.Key(
change.getId(),
ChangeUtil.messageUUID(ctx.getDb())),
ctx.getUser().getAccountId(), ctx.getWhen(),
change.currentPatchSetId());
cmsg.setMessage(msg.toString());
cmUtil.addChangeMessage(ctx.getDb(), update, cmsg);
}
private void appendHashtagMessage(StringBuilder b, String action,
Set<String> hashtags) {
if (isNullOrEmpty(hashtags)) {
return;
}
if (b.length() > 0) {
b.append("\n");
}
b.append("Hashtag");
if (hashtags.size() > 1) {
b.append("s");
}
b.append(" ");
b.append(action);
b.append(": ");
b.append(Joiner.on(", ").join(Ordering.natural().sortedCopy(hashtags)));
}
@Override
public void postUpdate(Context ctx) throws OrmException {
if (updated() && runHooks) {

20
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java
View File

@@ -26,6 +26,7 @@ import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.PatchLineComment.Status;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -131,6 +132,9 @@ public class ChangeRebuilder {
events.add(new ApprovalEvent(psa));
}
for (ChangeMessage msg : db.changeMessages().byChange(changeId)) {
events.add(new ChangeMessageEvent(msg));
}
Collections.sort(events);
BatchMetaDataUpdate batch = null;
@@ -366,4 +370,20 @@ public class ChangeRebuilder {
draftUpdate.insertComment(c);
}
}
private static class ChangeMessageEvent extends Event {
private final ChangeMessage message;
ChangeMessageEvent(ChangeMessage message) {
super(message.getPatchSetId(), message.getAuthor(),
message.getWrittenOn());
this.message = message;
}
@Override
void apply(ChangeUpdate update) throws OrmException {
checkUpdate(update);
update.setChangeMessage(message.getMessage());
}
}
}