From 0fd0dbe04ae8f7513d94a6ce83f5a8ea8e1e62d0 Mon Sep 17 00:00:00 2001 From: Ismo Haataja Date: Mon, 16 Jun 2014 14:36:21 +0300 Subject: Always allow commenting a patch set Prevent giving review scores but allow commenting for a change in CI states (STAGING, STAGED or INTEGRATING). No need to block it with error message like before. Also if the state has changed to one of CI states while reviewing, review scores are dropped, just comments are saved. Task-number: QTQAINFRA-858 Change-Id: I684f004a1d680b2db184fedb50d936337f603721 Reviewed-by: Oswald Buddenhagen Reviewed-by: Ismo Haataja --- .../client/changes/PatchSetComplexDisclosurePanel.java | 10 +--------- .../gerrit/client/changes/PublishCommentScreen.java | 8 +++++++- .../java/com/google/gerrit/reviewdb/client/Change.java | 5 +++++ .../java/com/google/gerrit/server/change/PostReview.java | 16 ++++++++++++++++ 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java index ed519e9ead..ec737ea901 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java @@ -634,15 +634,7 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel b.addClickHandler(new ClickHandler() { @Override public void onClick(final ClickEvent event) { - if (changeDetail.getChange().getStatus() == Change.Status.INTEGRATING - || changeDetail.getChange().getStatus() == Change.Status.STAGED - || changeDetail.getChange().getStatus() == Change.Status.STAGING) { - alertMessageBox(Util.C.headingReviewDisabled(), - Util.C.messageReviewDisabled()) - .center(); - } else { - Gerrit.display(Dispatcher.toPublish(patchSet.getId())); - } + Gerrit.display(Dispatcher.toPublish(patchSet.getId())); } }); actionsPanel.add(b); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java index 46607c4749..d1aee8c4a2 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java @@ -15,6 +15,7 @@ package com.google.gerrit.client.changes; +import com.google.gerrit.client.ErrorDialog; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.changes.ChangeInfo.ApprovalInfo; import com.google.gerrit.client.changes.ChangeInfo.LabelInfo; @@ -340,7 +341,7 @@ public class PublishCommentScreen extends AccountScreen implements descBlock.display(changeDetail, null, false, r.getPatchSetInfo(), r.getAccounts(), r.getSubmitTypeRecord(), commentLinkProcessor); - if (r.getChange().getStatus().isOpen()) { + if (r.getChange().getStatus().isOpen() && !r.getChange().getStatus().isCI()) { initApprovals(approvalPanel); approvals.display(change); } else { @@ -442,6 +443,9 @@ public class PublishCommentScreen extends AccountScreen implements staging(); } else { saveStateOnUnload = false; + if (!result.getMessage().isEmpty()) { + new ErrorDialog(result.getMessage()).center(); + } goChange(); } } @@ -467,6 +471,8 @@ public class PublishCommentScreen extends AccountScreen implements this.drafts = 'PUBLISH'; }-*/; + public final native String getMessage() /*-{ return this.message; }-*/; + protected ReviewInput() { } } diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java index 14a32b94a8..98fbf37cfe 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java @@ -342,6 +342,11 @@ public final class Change { return closed; } + public boolean isCI() { + return (code == STATUS_INTEGRATING + || code == STATUS_STAGED || code == STATUS_STAGING); + } + public static Status forCode(final char c) { for (final Status s : Status.values()) { if (s.code == c) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index 51720f2104..026a269e22 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java @@ -106,6 +106,7 @@ public class PostReview implements RestModifyView { static class Output { Map labels; + String message; } private final ReviewDb db; @@ -178,6 +179,14 @@ public class PostReview implements RestModifyView { Output output = new Output(); output.labels = input.labels; + if (input.labels != null && change.getStatus().isCI()) { + output.message = + "The change was staged while you were reviewing it. " + + "Due to this, only your comments were published, while your review scores were dropped."; + } + else { + output.message = ""; + } return output; } @@ -359,6 +368,13 @@ public class PostReview implements RestModifyView { // TODO Allow updating some labels even when closed. continue; } + if (change.getStatus().isCI()) { + // If the state of a change is INTEGRATING, STAGED or STAGING review + // scores are not allowed. This check is necessary here because some + // other user may have changed the state of a change while another + // user is reviewing it. Scores are dropped but comments are kept. + continue; + } PatchSetApproval c = current.remove(name); if (ent.getValue() == null || ent.getValue() == 0) { -- cgit v1.2.3