Add change message when hashtags are added/removed

At the moment one can see only the current hashtags, but when we have
change messages when the hashtags change we can see from the messages
how the hashtags changed over time. This is similar to how topic
changes can be seen from the change messages.

Having change messages whenever hashtags are changed will allow us to
rebuild the hashtag history when migrating to notedb.

On adding a hashtag we must now reload the change screen so that the
new message becomes visible.

Change-Id: Ic3f1554c02252a695edc698aa85b06677c06e81d
Signed-off-by: Edwin Kempin <ekempin@google.com>

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

@@ -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;
+  }
 }

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

@@ -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);
     }

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

@@ -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();

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

@@ -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) {