From 8c850ece6096e2298a9b6ecca9207c0a993bd665 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Thu, 22 Nov 2012 22:31:55 -0800 Subject: [PATCH] Update web UI to use /review Instead of using the older style JSON-RPC interface, publish comments with the new /review REST API. Inline comment drafts are still stored in the database and published by setting drafts to 'PUBLISH' rather than the default 'DELETE'. The SSH review command is also updated to use the same backend. Change-Id: I690c04ba20c4f3371e352be3f30ef53f1eaa74e9 --- .../common/data/PatchDetailService.java | 11 +- .../gerrit/client/changes/ChangeApi.java | 2 +- .../client/changes/PublishCommentScreen.java | 51 ++- .../rpc/patch/PatchDetailServiceImpl.java | 14 +- .../gerrit/server/change/PostReview.java | 14 +- .../server/config/GerritRequestModule.java | 2 - .../gerrit/server/patch/PublishComments.java | 380 ------------------ .../gerrit/sshd/commands/ApproveOption.java | 5 +- .../gerrit/sshd/commands/ReviewCommand.java | 28 +- 9 files changed, 71 insertions(+), 436 deletions(-) delete mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchDetailService.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchDetailService.java index 91ecb92d34..3c0c584985 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchDetailService.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchDetailService.java @@ -18,17 +18,16 @@ import com.google.gerrit.common.audit.Audit; import com.google.gerrit.common.auth.SignInRequired; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountDiffPreference; -import com.google.gerrit.reviewdb.client.ApprovalCategoryValue; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Patch; +import com.google.gerrit.reviewdb.client.Patch.Key; import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.PatchSet; -import com.google.gerrit.reviewdb.client.Patch.Key; import com.google.gwtjsonrpc.common.AsyncCallback; import com.google.gwtjsonrpc.common.RemoteJsonService; import com.google.gwtjsonrpc.common.RpcImpl; -import com.google.gwtjsonrpc.common.VoidResult; import com.google.gwtjsonrpc.common.RpcImpl.Version; +import com.google.gwtjsonrpc.common.VoidResult; import java.util.List; import java.util.Set; @@ -65,12 +64,6 @@ public interface PatchDetailService extends RemoteJsonService { @SignInRequired void deleteDraftPatchSet(PatchSet.Id psid, AsyncCallback callback); - @Audit - @SignInRequired - void publishComments(PatchSet.Id psid, String message, - Set approvals, - AsyncCallback callback); - @Audit @SignInRequired void addReviewers(Change.Id id, List reviewers, boolean confirmed, diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java index 7a02d92ffd..329e653099 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java @@ -70,7 +70,7 @@ public class ChangeApi { return new RestApi("/changes/" + id + "/" + action); } - private static String emptyToNull(String str) { + public static String emptyToNull(String str) { return str == null || str.isEmpty() ? null : str; } } 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 9c45486c98..8806f5b030 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 @@ -17,8 +17,8 @@ package com.google.gerrit.client.changes; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.patches.CommentEditorContainer; import com.google.gerrit.client.patches.CommentEditorPanel; -import com.google.gerrit.client.patches.PatchUtil; import com.google.gerrit.client.rpc.GerritCallback; +import com.google.gerrit.client.rpc.RestApi; import com.google.gerrit.client.rpc.ScreenLoadCallback; import com.google.gerrit.client.ui.AccountScreen; import com.google.gerrit.client.ui.PatchLink; @@ -36,6 +36,7 @@ import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; +import com.google.gwt.core.client.JavaScriptObject; import com.google.gwt.event.dom.client.ClickEvent; import com.google.gwt.event.dom.client.ClickHandler; import com.google.gwt.user.client.ui.Button; @@ -53,7 +54,6 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; @@ -62,6 +62,7 @@ public class PublishCommentScreen extends AccountScreen implements private static SavedState lastState; private final PatchSet.Id patchSetId; + private String revision; private Collection approvalButtons; private ChangeDescriptionBlock descBlock; private ApprovalTable approvals; @@ -252,8 +253,7 @@ public class PublishCommentScreen extends AccountScreen implements continue; } - final ValueRadioButton b = - new ValueRadioButton(buttonValue, ct.getCategory().getName()); + ValueRadioButton b = new ValueRadioButton(ct.getCategory(), buttonValue); b.setText(buttonValue.format()); if (lastState != null && patchSetId.equals(lastState.patchSetId) @@ -292,6 +292,7 @@ public class PublishCommentScreen extends AccountScreen implements draftsPanel.clear(); commentEditors = new ArrayList(); + revision = r.getPatchSetInfo().getRevId(); if (!r.getDrafts().isEmpty()) { draftsPanel.add(new SmallHeading(Util.C.headingPatchComments())); @@ -348,20 +349,23 @@ public class PublishCommentScreen extends AccountScreen implements } private void onSend2(final boolean submit) { - final Map values = - new HashMap(); + ReviewInput data = ReviewInput.create(); + data.message(ChangeApi.emptyToNull(message.getText().trim())); + data.init(); for (final ValueRadioButton b : approvalButtons) { if (b.getValue()) { - values.put(b.value.getCategoryId(), b.value.getId()); + data.label(b.category.getLabelName(), b.value.getValue()); } } enableForm(false); - PatchUtil.DETAIL_SVC.publishComments(patchSetId, message.getText().trim(), - new HashSet(values.values()), - new GerritCallback() { - public void onSuccess(final VoidResult result) { - if(submit) { + new RestApi("/changes/" + patchSetId.getParentKey().get() + + "/revisions/" + revision + "/review") + .data(data) + .post(new GerritCallback() { + @Override + public void onSuccess(ReviewInput result) { + if (submit) { submit(); } else { saveStateOnUnload = false; @@ -377,6 +381,23 @@ public class PublishCommentScreen extends AccountScreen implements }); } + private static class ReviewInput extends JavaScriptObject { + static ReviewInput create() { + return (ReviewInput) createObject(); + } + + final native void message(String m) /*-{ if(m)this.message=m; }-*/; + final native void label(String n, short v) /*-{ this.labels[n]=v; }-*/; + final native void init() /*-{ + this.labels = {}; + this.strict_labels = true; + this.drafts = 'PUBLISH'; + }-*/; + + protected ReviewInput() { + } + } + private void submit() { Util.MANAGE_SVC.submit(patchSetId, new GerritCallback() { @@ -399,10 +420,12 @@ public class PublishCommentScreen extends AccountScreen implements } private static class ValueRadioButton extends RadioButton { + final ApprovalCategory category; final ApprovalCategoryValue value; - ValueRadioButton(final ApprovalCategoryValue v, final String label) { - super(label); + ValueRadioButton(ApprovalCategory c, ApprovalCategoryValue v) { + super(c.getLabelName()); + category = c; value = v; } } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java index 40c9b843ab..232a79c758 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java @@ -24,25 +24,22 @@ import com.google.gerrit.common.data.ReviewResult; import com.google.gerrit.common.data.ReviewerResult; import com.google.gerrit.common.errors.NoSuchEntityException; import com.google.gerrit.httpd.rpc.BaseServiceImplementation; -import com.google.gerrit.httpd.rpc.Handler; import com.google.gerrit.httpd.rpc.changedetail.ChangeDetailFactory; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountDiffPreference; import com.google.gerrit.reviewdb.client.AccountPatchReview; import com.google.gerrit.reviewdb.client.ApprovalCategory; -import com.google.gerrit.reviewdb.client.ApprovalCategoryValue; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Patch; +import com.google.gerrit.reviewdb.client.Patch.Key; import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; -import com.google.gerrit.reviewdb.client.Patch.Key; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.account.AccountInfoCacheFactory; import com.google.gerrit.server.changedetail.DeleteDraftPatchSet; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; -import com.google.gerrit.server.patch.PublishComments; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.workflow.FunctionState; @@ -71,7 +68,6 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements private final DeleteDraftPatchSet.Factory deleteDraftPatchSetFactory; private final RemoveReviewerHandler.Factory removeReviewerHandlerFactory; private final FunctionState.Factory functionStateFactory; - private final PublishComments.Factory publishCommentsFactory; private final PatchScriptFactory.Factory patchScriptFactoryFactory; private final SaveDraft.Factory saveDraftFactory; private final ChangeDetailFactory.Factory changeDetailFactory; @@ -87,7 +83,6 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements final DeleteDraftPatchSet.Factory deleteDraftPatchSetFactory, final FunctionState.Factory functionStateFactory, final PatchScriptFactory.Factory patchScriptFactoryFactory, - final PublishComments.Factory publishCommentsFactory, final SaveDraft.Factory saveDraftFactory, final ChangeDetailFactory.Factory changeDetailFactory) { super(schema, currentUser); @@ -100,7 +95,6 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements this.deleteDraftPatchSetFactory = deleteDraftPatchSetFactory; this.functionStateFactory = functionStateFactory; this.patchScriptFactoryFactory = patchScriptFactoryFactory; - this.publishCommentsFactory = publishCommentsFactory; this.saveDraftFactory = saveDraftFactory; this.changeDetailFactory = changeDetailFactory; } @@ -178,12 +172,6 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements }); } - public void publishComments(final PatchSet.Id psid, final String msg, - final Set tags, - final AsyncCallback cb) { - Handler.wrap(publishCommentsFactory.create(psid, msg, tags, false)).to(cb); - } - /** * Update the reviewed status for the file by user @code{account} */ 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 5011f31bf3..cc188439ba 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 @@ -51,14 +51,14 @@ import java.util.Iterator; import java.util.List; import java.util.Map; -class PostReview implements RestModifyView { +public class PostReview implements RestModifyView { private static final Logger log = LoggerFactory.getLogger(PostReview.class); - static class Input { + public static class Input { @DefaultInput - String message; + public String message; - Map labels; + public Map labels; Map> comments; /** @@ -68,16 +68,16 @@ class PostReview implements RestModifyView { * execute anyway, but the proposed labels given by the user will be * modified to be the "best" value allowed by the access controls. */ - boolean strictLabels = true; + public boolean strictLabels = true; /** * How to process draft comments already in the database that were not also * described in this input request. */ - DraftHandling drafts = DraftHandling.DELETE; + public DraftHandling drafts = DraftHandling.DELETE; } - static enum DraftHandling { + public static enum DraftHandling { DELETE, PUBLISH, KEEP; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java index a8f3ae2bdd..604f934174 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java @@ -44,7 +44,6 @@ import com.google.gerrit.server.mail.RebasedPatchSetSender; import com.google.gerrit.server.mail.ReplacePatchSetSender; import com.google.gerrit.server.mail.RestoredSender; import com.google.gerrit.server.patch.AddReviewer; -import com.google.gerrit.server.patch.PublishComments; import com.google.gerrit.server.patch.RemoveReviewer; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.CreateProject; @@ -81,7 +80,6 @@ public class GerritRequestModule extends FactoryModule { factory(AddReviewerSender.Factory.class); factory(CreateChangeSender.Factory.class); factory(DeleteDraftPatchSet.Factory.class); - factory(PublishComments.Factory.class); factory(PublishDraft.Factory.class); factory(RebaseChange.Factory.class); factory(ReplacePatchSetSender.Factory.class); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java deleted file mode 100644 index fdabcaaedb..0000000000 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java +++ /dev/null @@ -1,380 +0,0 @@ -// Copyright (C) 2009 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.patch; - -import com.google.gerrit.common.ChangeHooks; -import com.google.gerrit.common.data.ApprovalType; -import com.google.gerrit.common.data.ApprovalTypes; -import com.google.gerrit.reviewdb.client.ApprovalCategory; -import com.google.gerrit.reviewdb.client.ApprovalCategoryValue; -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.PatchSet; -import com.google.gerrit.reviewdb.client.PatchSetApproval; -import com.google.gerrit.reviewdb.client.PatchSetInfo; -import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.server.ChangeUtil; -import com.google.gerrit.server.IdentifiedUser; -import com.google.gerrit.server.git.WorkQueue; -import com.google.gerrit.server.mail.CommentSender; -import com.google.gerrit.server.project.ChangeControl; -import com.google.gerrit.server.project.InvalidChangeOperationException; -import com.google.gerrit.server.project.NoSuchChangeException; -import com.google.gerrit.server.util.RequestScopePropagator; -import com.google.gerrit.server.workflow.FunctionState; -import com.google.gwtjsonrpc.common.VoidResult; -import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.SchemaFactory; -import com.google.inject.Inject; -import com.google.inject.assistedinject.Assisted; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.concurrent.Callable; - -public class PublishComments implements Callable { - private static final Logger log = - LoggerFactory.getLogger(PublishComments.class); - - public interface Factory { - PublishComments create(PatchSet.Id patchSetId, String messageText, - Set approvals, boolean forceMessage); - } - - private final SchemaFactory schemaFactory; - private final ReviewDb db; - private final IdentifiedUser user; - private final ApprovalTypes types; - private final CommentSender.Factory commentSenderFactory; - private final PatchSetInfoFactory patchSetInfoFactory; - private final ChangeControl.Factory changeControlFactory; - private final FunctionState.Factory functionStateFactory; - private final ChangeHooks hooks; - private final WorkQueue workQueue; - private final RequestScopePropagator requestScopePropagator; - - private final PatchSet.Id patchSetId; - private final String messageText; - private final Set approvals; - private final boolean forceMessage; - - private Change change; - private PatchSet patchSet; - private ChangeMessage message; - private List drafts; - - @Inject - PublishComments(final SchemaFactory sf, final ReviewDb db, - final IdentifiedUser user, - final ApprovalTypes approvalTypes, - final CommentSender.Factory commentSenderFactory, - final PatchSetInfoFactory patchSetInfoFactory, - final ChangeControl.Factory changeControlFactory, - final FunctionState.Factory functionStateFactory, - final ChangeHooks hooks, - final WorkQueue workQueue, - final RequestScopePropagator requestScopePropagator, - - @Assisted final PatchSet.Id patchSetId, - @Assisted final String messageText, - @Assisted final Set approvals, - @Assisted final boolean forceMessage) { - this.schemaFactory = sf; - this.db = db; - this.user = user; - this.types = approvalTypes; - this.patchSetInfoFactory = patchSetInfoFactory; - this.commentSenderFactory = commentSenderFactory; - this.changeControlFactory = changeControlFactory; - this.functionStateFactory = functionStateFactory; - this.hooks = hooks; - this.workQueue = workQueue; - this.requestScopePropagator = requestScopePropagator; - - this.patchSetId = patchSetId; - this.messageText = messageText; - this.approvals = approvals; - this.forceMessage = forceMessage; - } - - @Override - public VoidResult call() throws NoSuchChangeException, - InvalidChangeOperationException, OrmException { - final Change.Id changeId = patchSetId.getParentKey(); - final ChangeControl ctl = changeControlFactory.validateFor(changeId); - change = ctl.getChange(); - patchSet = db.patchSets().get(patchSetId); - if (patchSet == null) { - throw new NoSuchChangeException(changeId); - } - drafts = drafts(); - - db.changes().beginTransaction(changeId); - try { - publishDrafts(); - - final boolean isCurrent = patchSetId.equals(change.currentPatchSetId()); - if (isCurrent && change.getStatus().isOpen()) { - publishApprovals(ctl); - } else if (approvals.isEmpty() || forceMessage) { - publishMessageOnly(); - } else { - throw new InvalidChangeOperationException("Change is closed"); - } - - touchChange(); - db.commit(); - } finally { - db.rollback(); - } - - email(); - fireHook(); - return VoidResult.INSTANCE; - } - - private void publishDrafts() throws OrmException { - for (final PatchLineComment c : drafts) { - c.setStatus(PatchLineComment.Status.PUBLISHED); - c.updated(); - } - db.patchComments().update(drafts); - } - - private void publishApprovals(ChangeControl ctl) - throws InvalidChangeOperationException, OrmException { - ChangeUtil.updated(change); - - final Set dirty = new HashSet(); - final List ins = new ArrayList(); - final List upd = new ArrayList(); - final Collection all = - db.patchSetApprovals().byPatchSet(patchSetId).toList(); - final Map mine = mine(all); - - // Ensure any new approvals are stored properly. - // - for (final ApprovalCategoryValue.Id want : approvals) { - PatchSetApproval a = mine.get(want.getParentKey()); - if (a == null) { - a = new PatchSetApproval(new PatchSetApproval.Key(// - patchSetId, user.getAccountId(), want.getParentKey()), want.get()); - a.cache(change); - ins.add(a); - all.add(a); - mine.put(a.getCategoryId(), a); - dirty.add(a.getCategoryId()); - } - } - - // Normalize all of the items the user is changing. - // - final FunctionState functionState = - functionStateFactory.create(ctl, patchSetId, all); - for (final ApprovalCategoryValue.Id want : approvals) { - final PatchSetApproval a = mine.get(want.getParentKey()); - final short o = a.getValue(); - a.setValue(want.get()); - a.cache(change); - if (!ApprovalCategory.SUBMIT.equals(a.getCategoryId())) { - functionState.normalize(types.byId(a.getCategoryId()), a); - } - if (want.get() != a.getValue()) { - throw new InvalidChangeOperationException( - types.byId(a.getCategoryId()).getCategory().getLabelName() - + "=" + want.get() + " not permitted"); - } - if (o != a.getValue()) { - // Value changed, ensure we update the database. - // - a.setGranted(); - dirty.add(a.getCategoryId()); - } - if (!ins.contains(a)) { - upd.add(a); - } - } - - // Format a message explaining the actions taken. - // - final StringBuilder msgbuf = new StringBuilder(); - for (final ApprovalType at : types.getApprovalTypes()) { - if (dirty.contains(at.getCategory().getId())) { - final PatchSetApproval a = mine.get(at.getCategory().getId()); - if (a.getValue() == 0 && ins.contains(a)) { - // Don't say "no score" for an initial entry. - continue; - } - - final ApprovalCategoryValue val = at.getValue(a); - if (msgbuf.length() > 0) { - msgbuf.append("; "); - } - if (val != null && val.getName() != null && !val.getName().isEmpty()) { - msgbuf.append(val.getName()); - } else { - msgbuf.append(at.getCategory().getName()); - msgbuf.append(" "); - if (a.getValue() > 0) msgbuf.append('+'); - msgbuf.append(a.getValue()); - } - } - } - - // Update dashboards for everyone else. - // - for (PatchSetApproval a : all) { - if (!user.getAccountId().equals(a.getAccountId())) { - a.cache(change); - upd.add(a); - } - } - - db.patchSetApprovals().update(upd); - db.patchSetApprovals().insert(ins); - - summarizeInlineComments(msgbuf); - message(msgbuf.toString()); - } - - private void publishMessageOnly() throws OrmException { - StringBuilder msgbuf = new StringBuilder(); - summarizeInlineComments(msgbuf); - message(msgbuf.toString()); - } - - private void message(String actions) throws OrmException { - if ((actions == null || actions.isEmpty()) - && (messageText == null || messageText.isEmpty())) { - // They had nothing to say? - // - return; - } - - final StringBuilder msgbuf = new StringBuilder(); - msgbuf.append("Patch Set " + patchSetId.get() + ":"); - if (actions != null && !actions.isEmpty()) { - msgbuf.append(" "); - msgbuf.append(actions); - } - msgbuf.append("\n\n"); - msgbuf.append(messageText != null ? messageText : ""); - - message = new ChangeMessage(new ChangeMessage.Key(change.getId(),// - ChangeUtil.messageUUID(db)), user.getAccountId(), patchSetId); - message.setMessage(msgbuf.toString()); - db.changeMessages().insert(Collections.singleton(message)); - } - - private Map mine( - Collection all) { - Map r = - new HashMap(); - for (PatchSetApproval a : all) { - if (user.getAccountId().equals(a.getAccountId())) { - r.put(a.getCategoryId(), a); - } - } - return r; - } - - private void touchChange() { - try { - ChangeUtil.touch(change, db); - } catch (OrmException e) { - } - } - - private List drafts() throws OrmException { - return db.patchComments().draftByPatchSetAuthor(patchSetId, user.getAccountId()).toList(); - } - - private void email() { - if (message == null) { - return; - } - - workQueue.getDefaultQueue() - .submit(requestScopePropagator.wrap(new Runnable() { - @Override - public void run() { - PatchSetInfo patchSetInfo; - try { - ReviewDb reviewDb = schemaFactory.open(); - try { - patchSetInfo = patchSetInfoFactory.get(reviewDb, patchSetId); - } finally { - reviewDb.close(); - } - } catch (PatchSetInfoNotAvailableException e) { - log.error("Cannot read PatchSetInfo of " + patchSetId, e); - return; - } catch (Exception e) { - log.error("Cannot email comments for " + patchSetId, e); - return; - } - - try { - final CommentSender cm = commentSenderFactory.create(change); - cm.setFrom(user.getAccountId()); - cm.setPatchSet(patchSet, patchSetInfo); - cm.setChangeMessage(message); - cm.setPatchLineComments(drafts); - cm.send(); - } catch (Exception e) { - log.error("Cannot email comments for " + patchSetId, e); - } - } - - @Override - public String toString() { - return "send-email comments"; - } - })); - } - - private void fireHook() throws OrmException { - final Map changed = - new HashMap(); - for (ApprovalCategoryValue.Id v : approvals) { - changed.put(v.getParentKey(), v); - } - - hooks.doCommentAddedHook(change, user.getAccount(), patchSet, messageText, changed, db); - } - - private void summarizeInlineComments(StringBuilder in) { - if (!drafts.isEmpty()) { - if (in.length() != 0) { - in.append("\n\n"); - } - if (drafts.size() == 1) { - in.append("(1 inline comment)"); - } else { - in.append("(" + drafts.size() + " inline comments)"); - } - } - } -} diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ApproveOption.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ApproveOption.java index a1b8988406..b631553a89 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ApproveOption.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ApproveOption.java @@ -15,7 +15,6 @@ package com.google.gerrit.sshd.commands; import com.google.gerrit.common.data.ApprovalType; -import com.google.gerrit.reviewdb.client.ApprovalCategory; import com.google.gerrit.reviewdb.client.ApprovalCategoryValue; import org.kohsuke.args4j.CmdLineException; @@ -100,8 +99,8 @@ final class ApproveOption implements Option, Setter { return false; } - ApprovalCategory.Id getCategoryId() { - return type.getCategory().getId(); + String getLabelName() { + return type.getCategory().getLabelName(); } public static class Handler extends OneArgumentOptionHandler { diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java index 2bfa14aac9..6e4f03cbbe 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java @@ -14,11 +14,14 @@ package com.google.gerrit.sshd.commands; +import com.google.common.base.Strings; +import com.google.common.collect.Maps; import com.google.gerrit.common.data.ApprovalType; import com.google.gerrit.common.data.ApprovalTypes; import com.google.gerrit.common.data.ReviewResult; import com.google.gerrit.common.data.ReviewResult.Error.Type; import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.reviewdb.client.ApprovalCategory; import com.google.gerrit.reviewdb.client.ApprovalCategoryValue; @@ -28,11 +31,12 @@ import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.change.Abandon; import com.google.gerrit.server.change.ChangeResource; +import com.google.gerrit.server.change.PostReview; +import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.changedetail.DeleteDraftPatchSet; import com.google.gerrit.server.changedetail.PublishDraft; import com.google.gerrit.server.changedetail.RestoreChange; import com.google.gerrit.server.changedetail.Submit; -import com.google.gerrit.server.patch.PublishComments; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; @@ -124,7 +128,7 @@ public class ReviewCommand extends SshCommand { private Provider abandonProvider; @Inject - private PublishComments.Factory publishCommentsFactory; + private Provider reviewProvider; @Inject private PublishDraft.Factory publishDraftFactory; @@ -195,23 +199,29 @@ public class ReviewCommand extends SshCommand { changeComment = ""; } - Set aps = new HashSet(); + PostReview.Input review = new PostReview.Input(); + review.message = Strings.emptyToNull(changeComment); + review.labels = Maps.newTreeMap(); + review.drafts = PostReview.DraftHandling.PUBLISH; + review.strictLabels = false; for (ApproveOption ao : optionList) { Short v = ao.value(); if (v != null) { - aps.add(new ApprovalCategoryValue.Id(ao.getCategoryId(), v)); + review.labels.put(ao.getLabelName(), v); } } try { - publishCommentsFactory.create(patchSetId, changeComment, aps, forceMessage).call(); + ChangeControl ctl = + changeControlFactory.controlFor(patchSetId.getParentKey()); + reviewProvider.get().apply(new RevisionResource( + new ChangeResource(ctl), + db.patchSets().get(patchSetId)), review); if (abandonChange) { final Abandon abandon = abandonProvider.get(); final Abandon.Input input = new Abandon.Input(); input.message = changeComment; - ChangeControl ctl = - changeControlFactory.controlFor(patchSetId.getParentKey()); try { abandon.apply(new ChangeResource(ctl), input); } catch(AuthException e) { @@ -234,6 +244,10 @@ public class ReviewCommand extends SshCommand { throw error(e.getMessage()); } catch (IllegalStateException e) { throw error(e.getMessage()); + } catch (AuthException e) { + throw error(e.getMessage()); + } catch (BadRequestException e) { + throw error(e.getMessage()); } if (publishPatchSet) {