From ec65f84869860f3a26ffff49b61123d78f8da82e Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Wed, 21 Jan 2015 15:09:46 -0800 Subject: [PATCH] Change label and tooltip of submit button if submitting atomic topics This is just changing the ui. There is no change in functionality yet. Traditionally if you could not submit the change, the submit button was just not there (visible=false). This behavior is still there if atomic topics are enabled and the submission is prevented by the current change. That doesn't break with the history. If however another change in the same topic blocks submission the submit button is shown, but disabled. It would be a nice follow up change to give the reason to the user what exactly blocks submission, but that blocker may not be accessible for the current user. Designing a precise and helpful error message, which doesn't leak any secrets is done in a follow up commit. Change-Id: Ib32a4511a0308da32a8c5d1c7b1a6f124a03576e --- .../google/gerrit/server/change/Submit.java | 96 ++++++++++++++++--- .../query/change/InternalChangeQuery.java | 9 ++ 2 files changed, 91 insertions(+), 14 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index be7d05c398..0bb29ef708 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java @@ -61,8 +61,10 @@ import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.SubmitRuleEvaluator; import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gwtorm.server.AtomicUpdate; import com.google.gwtorm.server.OrmException; +import com.google.gwtorm.server.OrmRuntimeException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -72,6 +74,8 @@ import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.io.IOException; import java.sql.Timestamp; @@ -82,8 +86,12 @@ import java.util.Map; @Singleton public class Submit implements RestModifyView, UiAction { + private static final Logger log = LoggerFactory.getLogger(Submit.class); + private static final String DEFAULT_TOOLTIP = "Submit patch set ${patchSet} into ${branch}"; + private static final String DEFAULT_TOPIC_TOOLTIP = + "Submit all ${topicSize} changes of the same topic"; public enum Status { SUBMITTED, MERGED @@ -114,6 +122,10 @@ public class Submit implements RestModifyView, private final ChangesCollection changes; private final String label; private final ParameterizedString titlePattern; + private final String submitTopicLabel; + private final ParameterizedString submitTopicTooltip; + private final boolean submitWholeTopic; + private final Provider queryProvider; @Inject Submit(@GerritPersonIdent PersonIdent serverIdent, @@ -129,7 +141,8 @@ public class Submit implements RestModifyView, ChangesCollection changes, ChangeIndexer indexer, LabelNormalizer labelNormalizer, - @GerritServerConfig Config cfg) { + @GerritServerConfig Config cfg, + Provider queryProvider) { this.serverIdent = serverIdent; this.dbProvider = dbProvider; this.repoManager = repoManager; @@ -149,6 +162,14 @@ public class Submit implements RestModifyView, this.titlePattern = new ParameterizedString(MoreObjects.firstNonNull( cfg.getString("change", null, "submitTooltip"), DEFAULT_TOOLTIP)); + submitWholeTopic = cfg.getBoolean("change", null, "submitWholeTopic" , false); + this.submitTopicLabel = MoreObjects.firstNonNull( + Strings.emptyToNull(cfg.getString("change", null, "submitTopicLabel")), + "Submit whole topic"); + this.submitTopicTooltip = new ParameterizedString(MoreObjects.firstNonNull( + cfg.getString("change", null, "submitTopicTooltip"), + DEFAULT_TOPIC_TOOLTIP)); + this.queryProvider = queryProvider; } @Override @@ -209,22 +230,69 @@ public class Submit implements RestModifyView, } } + private boolean areChangesSubmittable(List changes, + IdentifiedUser identifiedUser) { + for (ChangeData c : changes) { + try { + ChangeControl changeControl = c.changeControl().forUser( + identifiedUser); + if (!changeControl.canSubmit()) { + return false; + } + } catch (OrmException e) { + log.error("Failed to get a ChangeControl for Change.Id " + + String.valueOf(c.getId()), e); + return false; + } + } + return true; + } + @Override public UiAction.Description getDescription(RevisionResource resource) { PatchSet.Id current = resource.getChange().currentPatchSetId(); - RevId revId = resource.getPatchSet().getRevision(); - Map params = ImmutableMap.of( - "patchSet", String.valueOf(resource.getPatchSet().getPatchSetId()), - "branch", resource.getChange().getDest().getShortName(), - "commit", ObjectId.fromString(revId.get()).abbreviate(7).name()); - - return new UiAction.Description() - .setLabel(label) - .setTitle(Strings.emptyToNull(titlePattern.replace(params))) - .setVisible(!resource.getPatchSet().isDraft() - && resource.getChange().getStatus().isOpen() - && resource.getPatchSet().getId().equals(current) - && resource.getControl().canSubmit()); + String topic = resource.getChange().getTopic(); + boolean visible = !resource.getPatchSet().isDraft() + && resource.getChange().getStatus().isOpen() + && resource.getPatchSet().getId().equals(current) + && resource.getControl().canSubmit(); + if (!visible) { + return new UiAction.Description() + .setLabel("") + .setTitle("") + .setVisible(false); + } + if (submitWholeTopic && !topic.isEmpty()) { + List changesByTopic = null; + try { + changesByTopic = queryProvider.get().byTopic(topic); + } catch (OrmException e) { + throw new OrmRuntimeException(e); + } + Map params = ImmutableMap.of( + "topicSize", String.valueOf(changesByTopic.size())); + // TODO(sbeller): + // If the button is visible but disabled the problem of submitting + // is at another change in the same topic. Tell the user via + // tooltip. Caution: Check access control for those changes. + return new UiAction.Description() + .setLabel(submitTopicLabel) + .setTitle(Strings.emptyToNull( + submitTopicTooltip.replace(params))) + .setVisible(true) + .setEnabled(areChangesSubmittable( + changesByTopic, resource.getUser())); + } else { + RevId revId = resource.getPatchSet().getRevision(); + Map params = ImmutableMap.of( + "patchSet", String.valueOf(resource.getPatchSet().getPatchSetId()), + "branch", resource.getChange().getDest().getShortName(), + "commit", ObjectId.fromString(revId.get()).abbreviate(7).name()); + return new UiAction.Description() + .setLabel(label) + .setTitle(Strings.emptyToNull(titlePattern.replace(params))) + .setVisible(true); + } } /** diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java index 97fcffbbdb..c50c3a2474 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java @@ -52,6 +52,10 @@ public class InternalChangeQuery { return new ChangeStatusPredicate(status); } + private static Predicate topic(String topic) { + return new TopicPredicate(topic); + } + private final QueryProcessor qp; @Inject @@ -114,6 +118,11 @@ public class InternalChangeQuery { return query(and(project(project), open())); } + public List byTopic(String topic) + throws OrmException { + return query(topic(topic)); + } + private List query(Predicate p) throws OrmException { try { return qp.queryChanges(p).changes();