Parse and serve reviewer updates via REST API

Parses updates to a change's reviewers, and serves them with the change
details. Will be picked up by PolyGerrit UI and displayed in change's
messages section in follow-up change.

Feature availability depends on NoteDb migration.

Feature: Issue 4148
Change-Id: I50e75ba9e5a885de4733a3a8d7d156d3729d1d91

Documentation/rest-api-changes.txt

@@ -264,6 +264,12 @@ default. Optional fields are:
   fields when referencing accounts.
 --
 
+[[reviewer-updates]]
+--
+* `REVIEWER_UPDATES`: include updates to reviewers set as
+  link:#review-update-info[ReviewerUpdateInfo] entities.
+--
+
 [[messages]]
 --
 * `MESSAGES`: include messages associated with the change.
@@ -511,8 +517,8 @@ describes the change.
 --
 
 Retrieves a change with link:#labels[labels], link:#detailed-labels[
-detailed labels], link:#detailed-accounts[detailed accounts], and
-link:#messages[messages].
+detailed labels], link:#detailed-accounts[detailed accounts],
+link:#reviewer-updates[reviewer updates], and link:#messages[messages].
 
 Additional fields can be obtained by adding `o` parameters, each
 option requires more database lookups and slows down the query
@@ -656,6 +662,56 @@ REJECTED > APPROVED > DISLIKED > RECOMMENDED.
         }
       ]
     },
+    "reviewer_updates": [
+      {
+        "state": "REVIEWER",
+        "reviewer": {
+          "_account_id": 1000096,
+          "name": "John Doe",
+          "email": "john.doe@example.com",
+          "username": "jdoe"
+        },
+        "updated_by": {
+          "_account_id": 1000096,
+          "name": "John Doe",
+          "email": "john.doe@example.com",
+          "username": "jdoe"
+        },
+        "updated": "2016-07-21 20:12:39.000000000"
+      },
+      {
+        "state": "REMOVED",
+        "reviewer": {
+          "_account_id": 1000096,
+          "name": "John Doe",
+          "email": "john.doe@example.com",
+          "username": "jdoe"
+        },
+        "updated_by": {
+          "_account_id": 1000096,
+          "name": "John Doe",
+          "email": "john.doe@example.com",
+          "username": "jdoe"
+        },
+        "updated": "2016-07-21 20:12:33.000000000"
+      },
+      {
+        "state": "CC",
+        "reviewer": {
+          "_account_id": 1000096,
+          "name": "John Doe",
+          "email": "john.doe@example.com",
+          "username": "jdoe"
+        },
+        "updated_by": {
+          "_account_id": 1000096,
+          "name": "John Doe",
+          "email": "john.doe@example.com",
+          "username": "jdoe"
+        },
+        "updated": "2016-03-23 21:34:02.419000000",
+      },
+    ],
     "messages": [
       {
         "id": "YH-egE",
@@ -4316,6 +4372,11 @@ Possible reviewer states are `REVIEWER`, `CC` and `REMOVED`. +
 `REMOVED`: Users that were previously reviewers on the change, but have
 been removed. +
 Only set if link:#detailed-labels[detailed labels] are requested.
+|`reviewer_updates`|optional|
+Updates to reviewers set for the change as
+link:#review-update-info[ReviewerUpdateInfo] entities.
+Only set if link:#reviewer-updates[reviewer updates] are requested and
+if NoteDb is enabled.
 |`messages`|optional|
 Messages associated with the change as a list of
 link:#change-message-info[ChangeMessageInfo] entities. +
@@ -5019,6 +5080,26 @@ The labels of the review as a map that maps the label names to the
 voting values.
 |===========================
 
+[[review-update-info]]
+=== ReviewerUpdateInfo
+The `ReviewerUpdateInfo` entity contains information about updates to
+change's reviewers set.
+
+[options="header",cols="1,6"]
+|===========================
+|Field Name     |Description
+|`updated`|
+Timestamp of the update.
+|`updated_by`|
+The account which modified state of the reviewer in question as
+link:rest-api-accounts.html#account-info[AccountInfo] entity.
+|`reviewer`|
+The reviewer account added or removed from the change as an
+link:rest-api-accounts.html#account-info[AccountInfo] entity.
+|`state`|
+The reviewer state, one of `REVIEWER`, `CC` or `REMOVED`.
+|===========================
+
 [[review-input]]
 === ReviewInput
 The `ReviewInput` entity contains information for adding a review to a

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

@@ -15,7 +15,9 @@
 package com.google.gerrit.acceptance.rest.change;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.TruthJUnit.assume;
 import static com.google.gerrit.extensions.client.ReviewerState.CC;
+import static com.google.gerrit.extensions.client.ReviewerState.REMOVED;
 import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
 import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
 import static javax.servlet.http.HttpServletResponse.SC_OK;
@@ -31,6 +33,7 @@ import com.google.gerrit.extensions.api.changes.ReviewResult;
 import com.google.gerrit.extensions.client.ReviewerState;
 import com.google.gerrit.extensions.common.AccountInfo;
 import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.ReviewerUpdateInfo;
 import com.google.gerrit.server.change.PostReviewers;
 import com.google.gerrit.server.mail.Address;
 import com.google.gerrit.testutil.FakeEmailSender.Message;
@@ -40,6 +43,7 @@ import org.junit.Test;
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Iterator;
 import java.util.List;
 
 public class ChangeReviewersIT extends AbstractDaemonTest {
@@ -402,6 +406,53 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  public void noteDbAddReviewerToReviewerChangeInfo() throws Exception {
+    assume().that(notesMigration.readChanges()).isTrue();
+
+    PushOneCommit.Result r = createChange();
+    String changeId = r.getChangeId();
+    AddReviewerInput in = new AddReviewerInput();
+    in.reviewer = user.email;
+    in.state = CC;
+    addReviewer(changeId, in);
+
+    in.state = REVIEWER;
+    addReviewer(changeId, in);
+
+    setApiUser(user);
+    // NoteDb adds reviewer to a change on every review.
+    gApi.changes().id(changeId).current().review(ReviewInput.dislike());
+
+    deleteReviewer(changeId, user).assertNoContent();
+
+    ChangeInfo c = gApi.changes().id(changeId).get();
+    assertThat(c.reviewerUpdates).isNotNull();
+    assertThat(c.reviewerUpdates).hasSize(3);
+
+    Iterator<ReviewerUpdateInfo> it = c.reviewerUpdates.iterator();
+    ReviewerUpdateInfo reviewerChange = it.next();
+    assertThat(reviewerChange.state).isEqualTo(CC);
+    assertThat(reviewerChange.reviewer._accountId).isEqualTo(
+        user.getId().get());
+    assertThat(reviewerChange.updatedBy._accountId).isEqualTo(
+        admin.getId().get());
+
+    reviewerChange = it.next();
+    assertThat(reviewerChange.state).isEqualTo(REVIEWER);
+    assertThat(reviewerChange.reviewer._accountId).isEqualTo(
+        user.getId().get());
+    assertThat(reviewerChange.updatedBy._accountId).isEqualTo(
+        admin.getId().get());
+
+    reviewerChange = it.next();
+    assertThat(reviewerChange.state).isEqualTo(REMOVED);
+    assertThat(reviewerChange.reviewer._accountId).isEqualTo(
+        user.getId().get());
+    assertThat(reviewerChange.updatedBy._accountId).isEqualTo(
+        admin.getId().get());
+  }
+
   private AddReviewerResult addReviewer(String changeId, String reviewer)
       throws Exception {
     return addReviewer(changeId, reviewer, SC_OK);
@@ -427,6 +478,12 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
         resp, expectedStatus, AddReviewerResult.class);
   }
 
+  private RestResponse deleteReviewer(String changeId, TestAccount account)
+      throws Exception {
+    return adminRestSession.delete("/changes/" + changeId + "/reviewers/" +
+        account.getId().get());
+  }
+
   private ReviewResult review(
       String changeId, String revisionId, ReviewInput in) throws Exception {
     return review(changeId, revisionId, in, SC_OK);

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ListChangesOption.java

@@ -66,7 +66,10 @@ public enum ListChangesOption {
   COMMIT_FOOTERS(17),
 
   /** Include push certificate information along with any patch sets. */
-  PUSH_CERTIFICATES(18);
+  PUSH_CERTIFICATES(18),
+
+  /** Include change's reviewer updates. */
+  REVIEWER_UPDATES(19);
 
   private final int value;
 

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java

@@ -53,6 +53,7 @@ public class ChangeInfo {
   public Map<String, Collection<String>> permittedLabels;
   public Collection<AccountInfo> removableReviewers;
   public Map<ReviewerState, Collection<AccountInfo>> reviewers;
+  public Collection<ReviewerUpdateInfo> reviewerUpdates;
   public Collection<ChangeMessageInfo> messages;
 
   public String currentRevision;

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ReviewerUpdateInfo.java

@@ -0,0 +1,26 @@
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.extensions.common;
+
+import com.google.gerrit.extensions.client.ReviewerState;
+
+import java.sql.Timestamp;
+
+public class ReviewerUpdateInfo {
+  public Timestamp updated;
+  public AccountInfo updatedBy;
+  public AccountInfo reviewer;
+  public ReviewerState state;
+}

gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java

@@ -140,6 +140,22 @@ public class ApprovalsUtil {
     return notes.load().getReviewers();
   }
 
+  /**
+   * Get updates to reviewer set.
+   * Always returns empty list for ReviewDb.
+   *
+   * @param notes change notes.
+   * @return reviewer updates for the change.
+   * @throws OrmException if reviewer updates for the change could not be read.
+   */
+  public List<ReviewerStatusUpdate> getReviewerUpdates(ChangeNotes notes)
+      throws OrmException {
+    if (!migration.readChanges()) {
+      return ImmutableList.of();
+    }
+    return notes.load().getReviewerUpdates();
+  }
+
   public List<PatchSetApproval> addReviewers(ReviewDb db,
       ChangeUpdate update, LabelTypes labelTypes, Change change, PatchSet ps,
       PatchSetInfo info, Iterable<Account.Id> wantReviewers,

gerrit-server/src/main/java/com/google/gerrit/server/ReviewerStatusUpdate.java

@@ -0,0 +1,36 @@
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server;
+
+import com.google.auto.value.AutoValue;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.notedb.ReviewerStateInternal;
+
+import java.sql.Timestamp;
+
+/** Change to a reviewer's status. */
+@AutoValue
+public abstract class ReviewerStatusUpdate {
+  public static ReviewerStatusUpdate create(
+      Timestamp ts, Account.Id updatedBy, Account.Id reviewer,
+      ReviewerStateInternal state) {
+    return new AutoValue_ReviewerStatusUpdate(ts, updatedBy, reviewer, state);
+  }
+
+  public abstract Timestamp date();
+  public abstract Account.Id updatedBy();
+  public abstract Account.Id reviewer();
+  public abstract ReviewerStateInternal state();
+}

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

@@ -31,6 +31,7 @@ import static com.google.gerrit.extensions.client.ListChangesOption.LABELS;
 import static com.google.gerrit.extensions.client.ListChangesOption.MESSAGES;
 import static com.google.gerrit.extensions.client.ListChangesOption.PUSH_CERTIFICATES;
 import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWED;
+import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWER_UPDATES;
 import static com.google.gerrit.extensions.client.ListChangesOption.WEB_LINKS;
 import static com.google.gerrit.server.CommonConverters.toGitPerson;
 
@@ -70,6 +71,7 @@ import com.google.gerrit.extensions.common.FetchInfo;
 import com.google.gerrit.extensions.common.LabelInfo;
 import com.google.gerrit.extensions.common.ProblemInfo;
 import com.google.gerrit.extensions.common.PushCertificateInfo;
+import com.google.gerrit.extensions.common.ReviewerUpdateInfo;
 import com.google.gerrit.extensions.common.RevisionInfo;
 import com.google.gerrit.extensions.common.WebLinkInfo;
 import com.google.gerrit.extensions.config.DownloadCommand;
@@ -89,6 +91,7 @@ import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.GpgException;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.ReviewerStatusUpdate;
 import com.google.gerrit.server.StarredChangesUtil;
 import com.google.gerrit.server.WebLinks;
 import com.google.gerrit.server.account.AccountLoader;
@@ -473,6 +476,10 @@ public class ChangeJson {
       }
     }
 
+    if (has(REVIEWER_UPDATES)) {
+      out.reviewerUpdates = reviewerUpdates(cd);
+    }
+
     boolean needMessages = has(MESSAGES);
     boolean needRevisions = has(ALL_REVISIONS)
         || has(CURRENT_REVISION)
@@ -507,6 +514,21 @@ public class ChangeJson {
     return out;
   }
 
+  private Collection<ReviewerUpdateInfo> reviewerUpdates(ChangeData cd)
+      throws OrmException {
+    List<ReviewerStatusUpdate> reviewerUpdates = cd.reviewerUpdates();
+    List<ReviewerUpdateInfo> result = new ArrayList<>(reviewerUpdates.size());
+    for (ReviewerStatusUpdate c : reviewerUpdates) {
+      ReviewerUpdateInfo change = new ReviewerUpdateInfo();
+      change.updated = c.date();
+      change.state = c.state().asReviewerState();
+      change.updatedBy = accountLoader.get(c.updatedBy());
+      change.reviewer = accountLoader.get(c.reviewer());
+      result.add(change);
+    }
+    return result;
+  }
+
   private List<SubmitRecord> submitRecords(ChangeData cd) throws OrmException {
     // Maintain our own cache rather than using cd.getSubmitRecords(),
     // since the latter may not have used the same values for

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

@@ -43,6 +43,7 @@ public class GetDetail implements RestReadView<ChangeResource> {
     delegate.addOption(ListChangesOption.DETAILED_LABELS);
     delegate.addOption(ListChangesOption.DETAILED_ACCOUNTS);
     delegate.addOption(ListChangesOption.MESSAGES);
+    delegate.addOption(ListChangesOption.REVIEWER_UPDATES);
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java

@@ -49,6 +49,7 @@ import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.ReviewerSet;
+import com.google.gerrit.server.ReviewerStatusUpdate;
 import com.google.gerrit.server.git.RefCache;
 import com.google.gerrit.server.git.RepoRefCache;
 import com.google.gerrit.server.project.NoSuchChangeException;
@@ -393,6 +394,10 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     return state.reviewers();
   }
 
+  public ImmutableList<ReviewerStatusUpdate> getReviewerUpdates() {
+    return state.reviewerUpdates();
+  }
+
   /**
    *
    * @return a ImmutableSet of all hashtags for this change sorted in alphabetical order.

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java

@@ -60,6 +60,7 @@ import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.ReviewerSet;
+import com.google.gerrit.server.ReviewerStatusUpdate;
 import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
 import com.google.gerrit.server.util.LabelVote;
 
@@ -106,6 +107,7 @@ class ChangeNotesParser {
   // in during the parsing process.
   private final Table<Account.Id, ReviewerStateInternal, Timestamp> reviewers;
   private final List<Account.Id> allPastReviewers;
+  private final List<ReviewerStatusUpdate> reviewerUpdates;
   private final List<SubmitRecord> submitRecords;
   private final Multimap<RevId, PatchLineComment> comments;
   private final TreeMap<PatchSet.Id, PatchSet> patchSets;
@@ -142,6 +144,7 @@ class ChangeNotesParser {
     approvals = new HashMap<>();
     reviewers = HashBasedTable.create();
     allPastReviewers = new ArrayList<>();
+    reviewerUpdates = new ArrayList<>();
     submitRecords = Lists.newArrayListWithExpectedSize(1);
     allChangeMessages = new ArrayList<>();
     changeMessagesByPatchSet = LinkedListMultimap.create();
@@ -198,6 +201,7 @@ class ChangeNotesParser {
         buildApprovals(),
         ReviewerSet.fromTable(Tables.transpose(reviewers)),
         allPastReviewers,
+        buildReviewerUpdates(),
         submitRecords,
         buildAllMessages(),
         buildMessagesByPatchSet(),
@@ -220,6 +224,18 @@ class ChangeNotesParser {
     return result;
   }
 
+  private List<ReviewerStatusUpdate> buildReviewerUpdates() {
+    List<ReviewerStatusUpdate> result = new ArrayList<>();
+    HashMap<Account.Id, ReviewerStateInternal> lastState = new HashMap<>();
+    for (ReviewerStatusUpdate u : Lists.reverse(reviewerUpdates)) {
+      if (lastState.get(u.reviewer()) != u.state()) {
+        result.add(u);
+        lastState.put(u.reviewer(), u.state());
+      }
+    }
+    return result;
+  }
+
   private List<ChangeMessage> buildAllMessages() {
     return Lists.reverse(allChangeMessages);
   }
@@ -755,6 +771,8 @@ class ChangeNotesParser {
       throw invalidFooter(state.getFooterKey(), line);
     }
     Account.Id accountId = noteUtil.parseIdent(ident, id);
+    reviewerUpdates.add(
+        ReviewerStatusUpdate.create(ts, ownerId, accountId, state));
     if (!reviewers.containsRow(accountId)) {
       reviewers.put(accountId, state, ts);
     }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java

@@ -35,6 +35,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.ReviewerSet;
+import com.google.gerrit.server.ReviewerStatusUpdate;
 
 import java.sql.Timestamp;
 import java.util.List;
@@ -63,6 +64,7 @@ public abstract class ChangeNotesState {
         ImmutableListMultimap.<PatchSet.Id, PatchSetApproval>of(),
         ReviewerSet.empty(),
         ImmutableList.<Account.Id>of(),
+        ImmutableList.<ReviewerStatusUpdate>of(),
         ImmutableList.<SubmitRecord>of(),
         ImmutableList.<ChangeMessage>of(),
         ImmutableListMultimap.<PatchSet.Id, ChangeMessage>of(),
@@ -87,6 +89,7 @@ public abstract class ChangeNotesState {
       Multimap<PatchSet.Id, PatchSetApproval> approvals,
       ReviewerSet reviewers,
       List<Account.Id> allPastReviewers,
+      List<ReviewerStatusUpdate> reviewerUpdates,
       List<SubmitRecord> submitRecords,
       List<ChangeMessage> allChangeMessages,
       Multimap<PatchSet.Id, ChangeMessage> changeMessagesByPatchSet,
@@ -113,6 +116,7 @@ public abstract class ChangeNotesState {
         ImmutableListMultimap.copyOf(approvals),
         reviewers,
         ImmutableList.copyOf(allPastReviewers),
+        ImmutableList.copyOf(reviewerUpdates),
         ImmutableList.copyOf(submitRecords),
         ImmutableList.copyOf(allChangeMessages),
         ImmutableListMultimap.copyOf(changeMessagesByPatchSet),
@@ -152,8 +156,11 @@ public abstract class ChangeNotesState {
   abstract ImmutableSet<String> hashtags();
   abstract ImmutableSortedMap<PatchSet.Id, PatchSet> patchSets();
   abstract ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals();
+
   abstract ReviewerSet reviewers();
   abstract ImmutableList<Account.Id> allPastReviewers();
+  abstract ImmutableList<ReviewerStatusUpdate> reviewerUpdates();
+
   abstract ImmutableList<SubmitRecord> submitRecords();
   abstract ImmutableList<ChangeMessage> allChangeMessages();
   abstract ImmutableListMultimap<PatchSet.Id, ChangeMessage>

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java

@@ -50,6 +50,7 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.PatchLineCommentsUtil;
 import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.ReviewerSet;
+import com.google.gerrit.server.ReviewerStatusUpdate;
 import com.google.gerrit.server.StarredChangesUtil;
 import com.google.gerrit.server.change.MergeabilityCache;
 import com.google.gerrit.server.git.GitRepositoryManager;
@@ -349,6 +350,7 @@ public class ChangeData {
   private Set<Account.Id> starredByUser;
   private ImmutableMultimap<Account.Id, String> stars;
   private ReviewerSet reviewers;
+  private List<ReviewerStatusUpdate> reviewerUpdates;
   private PersonIdent author;
   private PersonIdent committer;
 
@@ -940,6 +942,21 @@ public void setPatchSets(Collection<PatchSet> patchSets) {
     return reviewers;
   }
 
+  public List<ReviewerStatusUpdate> reviewerUpdates() throws OrmException {
+    if (reviewerUpdates == null) {
+      reviewerUpdates = approvalsUtil.getReviewerUpdates(notes());
+    }
+    return reviewerUpdates;
+  }
+
+  public void setReviewerUpdates(List<ReviewerStatusUpdate> reviewerUpdates) {
+    this.reviewerUpdates = reviewerUpdates;
+  }
+
+  public List<ReviewerStatusUpdate> getReviewerUpdates() {
+    return reviewerUpdates;
+  }
+
   public Collection<PatchLineComment> publishedComments()
       throws OrmException {
     if (publishedComments == null) {