Fix: Also copy parentUuid in copy constructor of Comment

The copy constructor is currently only used by the code which updates
a draft comment. Interestingly, nobody seemed to have noticed that this
REST API actually had a bug and the parentUuid was lost when the draft
comment was updated. Users of Gerrit's API didn't run into this bug
as the frontend always sends all comment parameters as input to the
draft update along.

By the way, this issue was noticed as a side-effect of testing the
implementation of another new feature (ported comments, see I4e96af5ac).
Writing even seemingly trivial tests (e.g. that the parentUuid of a
ported comment is kept as in the original) seemed to have paid off in
this case.

Change-Id: I75a65bd57fb71cb8efe9d2460bc210b557546165
This commit is contained in:
Alice Kober-Sotzek
2020-08-13 20:47:58 +02:00
parent efa8507bac
commit 322bf38e5c
2 changed files with 39 additions and 0 deletions

View File

@@ -244,6 +244,7 @@ public abstract class Comment {
c.unresolved); c.unresolved);
this.lineNbr = c.lineNbr; this.lineNbr = c.lineNbr;
this.realAuthor = c.realAuthor; this.realAuthor = c.realAuthor;
this.parentUuid = c.parentUuid;
this.range = c.range != null ? new Range(c.range) : null; this.range = c.range != null ? new Range(c.range) : null;
this.tag = c.tag; this.tag = c.tag;
this.revId = c.revId; this.revId = c.revId;

View File

@@ -31,11 +31,16 @@ import com.google.common.collect.Lists;
import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
import com.google.gerrit.acceptance.testsuite.change.TestHumanComment;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.HumanComment; import com.google.gerrit.entities.HumanComment;
import com.google.gerrit.entities.Patch; import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.RefNames; import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.DeleteCommentInput; import com.google.gerrit.extensions.api.changes.DeleteCommentInput;
import com.google.gerrit.extensions.api.changes.DraftInput; import com.google.gerrit.extensions.api.changes.DraftInput;
@@ -90,6 +95,8 @@ public class CommentsIT extends AbstractDaemonTest {
@Inject private ProjectOperations projectOperations; @Inject private ProjectOperations projectOperations;
@Inject private Provider<ChangesCollection> changes; @Inject private Provider<ChangesCollection> changes;
@Inject private Provider<PostReview> postReview; @Inject private Provider<PostReview> postReview;
@Inject private ChangeOperations changeOperations;
@Inject private AccountOperations accountOperations;
@Inject private RequestScopeOperations requestScopeOperations; @Inject private RequestScopeOperations requestScopeOperations;
@Inject private CommentsUtil commentsUtil; @Inject private CommentsUtil commentsUtil;
@Inject private TestCommentHelper testCommentHelper; @Inject private TestCommentHelper testCommentHelper;
@@ -726,6 +733,37 @@ public class CommentsIT extends AbstractDaemonTest {
assertThat(comment.tag).isEqualTo(tag); assertThat(comment.tag).isEqualTo(tag);
} }
@Test
public void updatedDraftStillPointsToParentComment() throws Exception {
Account.Id accountId = accountOperations.newAccount().create();
Change.Id changeId = changeOperations.newChange().create();
PatchSet.Id patchsetId = changeOperations.change(changeId).currentPatchset().get().patchsetId();
String parentCommentUuid =
changeOperations.change(changeId).patchset(patchsetId).newComment().create();
String draftCommentUuid =
changeOperations
.change(changeId)
.patchset(patchsetId)
.newDraftComment()
.parentUuid(parentCommentUuid)
.author(accountId)
.create();
// Each user can only see their own drafts.
requestScopeOperations.setApiUser(accountId);
DraftInput draftInput = newDraft(FILE_NAME, Side.REVISION, 0, "bar");
draftInput.message = "Another comment text.";
gApi.changes()
.id(changeId.get())
.revision(patchsetId.get())
.draft(draftCommentUuid)
.update(draftInput);
TestHumanComment comment =
changeOperations.change(changeId).draftComment(draftCommentUuid).get();
assertThat(comment.parentUuid()).hasValue(parentCommentUuid);
}
@Test @Test
public void listDrafts() throws Exception { public void listDrafts() throws Exception {
String file = "file"; String file = "file";