diff --git a/Documentation/dev-stars.txt b/Documentation/dev-stars.txt index 1fb871a9c9..d0bf72e67d 100644 --- a/Documentation/dev-stars.txt +++ b/Documentation/dev-stars.txt @@ -74,6 +74,25 @@ patchset has been uploaded. The ChangeInfo muted-field will show if the change is currently in a mute state. +[[reviewed-star]] +== Reviewed Star + +If the "reviewed/"-star is set by a user, and +matches the current patch set, the change is always reported as "reviewed" +in the ChangeInfo. + +This allows users to "de-highlight" changes in a dashboard until a new +patchset has been uploaded. + +[[unreviewed-star]] +== Unreviewed Star + +If the "unreviewed/"-star is set by a user, and +matches the current patch set, the change is always reported as "unreviewed" +in the ChangeInfo. + +This allows users to "highlight" changes in a dashboard. + [[query-stars]] == Query Stars diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index fa91c2d5bb..d44bd791be 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -2363,6 +2363,42 @@ Unmutes a change. PUT /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/unmute HTTP/1.0 ---- +[[mark-as-reviewed]] +=== Mark as Reviewed +-- +'PUT /changes/link:#change-id[\{change-id\}]/reviewed' +-- + +Marks a change as reviewed. + +This allows users to "de-highlight" changes in their dashboard until a new +patch set is uploaded. + +This differs from the link:#ignore[ignore] endpoint, which will mute +emails and hide the change from dashboard completely until it is +link:#unignore[unignored] again. + + +.Request +---- + PUT /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/reviewed HTTP/1.0 +---- + +[[mark-as-unreviewed]] +=== Mark as Unreviewed +-- +'PUT /changes/link:#change-id[\{change-id\}]/unreviewed' +-- + +Marks a change as unreviewed. + +This allows users to "highlight" changes in their dashboard + +.Request +---- + PUT /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/unreviewed HTTP/1.0 +---- + [[get-hashtags]] === Get Hashtags -- diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index 65e5909ce5..b3d6935d49 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -3409,6 +3409,103 @@ public class ChangeIT extends AbstractDaemonTest { gApi.changes().id(changeId).mute(true); } + @Test + public void markAsReviewed() throws Exception { + TestAccount user2 = accountCreator.user2(); + + PushOneCommit.Result r = createChange(); + + AddReviewerInput in = new AddReviewerInput(); + in.reviewer = user.email; + gApi.changes().id(r.getChangeId()).addReviewer(in); + + setApiUser(user); + assertThat(gApi.changes().id(r.getChangeId()).get().reviewed).isNull(); + gApi.changes().id(r.getChangeId()).markAsReviewed(true); + assertThat(gApi.changes().id(r.getChangeId()).get().reviewed).isTrue(); + + setApiUser(user2); + sender.clear(); + amendChange(r.getChangeId()); + + setApiUser(user); + assertThat(gApi.changes().id(r.getChangeId()).get().reviewed).isNull(); + + List messages = sender.getMessages(); + assertThat(messages).hasSize(1); + assertThat(messages.get(0).rcpt()).containsExactly(user.emailAddress); + } + + @Test + public void cannotSetUnreviewedLabelForPatchSetThatAlreadyHasReviewedLabel() throws Exception { + String changeId = createChange().getChangeId(); + + setApiUser(user); + gApi.changes().id(changeId).markAsReviewed(true); + assertThat(gApi.changes().id(changeId).get().reviewed).isTrue(); + + exception.expect(BadRequestException.class); + exception.expectMessage( + "The labels " + + StarredChangesUtil.REVIEWED_LABEL + + "/" + + 1 + + " and " + + StarredChangesUtil.UNREVIEWED_LABEL + + "/" + + 1 + + " are mutually exclusive. Only one of them can be set."); + gApi.accounts() + .self() + .setStars( + changeId, new StarsInput(ImmutableSet.of(StarredChangesUtil.UNREVIEWED_LABEL + "/1"))); + } + + @Test + public void cannotSetReviewedLabelForPatchSetThatAlreadyHasUnreviewedLabel() throws Exception { + String changeId = createChange().getChangeId(); + + setApiUser(user); + gApi.changes().id(changeId).markAsReviewed(false); + assertThat(gApi.changes().id(changeId).get().reviewed).isNull(); + + exception.expect(BadRequestException.class); + exception.expectMessage( + "The labels " + + StarredChangesUtil.REVIEWED_LABEL + + "/" + + 1 + + " and " + + StarredChangesUtil.UNREVIEWED_LABEL + + "/" + + 1 + + " are mutually exclusive. Only one of them can be set."); + gApi.accounts() + .self() + .setStars( + changeId, new StarsInput(ImmutableSet.of(StarredChangesUtil.REVIEWED_LABEL + "/1"))); + } + + @Test + public void setReviewedAndUnreviewedLabelsForDifferentPatchSets() throws Exception { + String changeId = createChange().getChangeId(); + + setApiUser(user); + gApi.changes().id(changeId).markAsReviewed(true); + assertThat(gApi.changes().id(changeId).get().reviewed).isTrue(); + + amendChange(changeId); + assertThat(gApi.changes().id(changeId).get().reviewed).isNull(); + + gApi.changes().id(changeId).markAsReviewed(false); + assertThat(gApi.changes().id(changeId).get().reviewed).isNull(); + + assertThat(gApi.accounts().self().getStars(changeId)) + .containsExactly( + StarredChangesUtil.REVIEWED_LABEL + "/" + 1, + StarredChangesUtil.UNREVIEWED_LABEL + "/" + 2); + } + @Test public void cannotSetInvalidLabel() throws Exception { String changeId = createChange().getChangeId(); diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java index 1fc04b4576..3aaa297a42 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java @@ -132,6 +132,14 @@ public interface ChangeApi { */ boolean muted() throws RestApiException; + /** + * Mark this change as reviewed/unreviewed. + * + * @param reviewed flag to decide if this change should be marked as reviewed ({@code true}) or + * unreviewed ({@code false}) + */ + void markAsReviewed(boolean reviewed) throws RestApiException; + /** * Create a new change that reverts this change. * @@ -592,6 +600,11 @@ public interface ChangeApi { throw new NotImplementedException(); } + @Override + public void markAsReviewed(boolean reviewed) throws RestApiException { + throw new NotImplementedException(); + } + @Override public PureRevertInfo pureRevert() throws RestApiException { throw new NotImplementedException(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java index 13c24e0b8c..e300a807fd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java @@ -17,6 +17,7 @@ package com.google.gerrit.server; import static com.google.common.base.Preconditions.checkNotNull; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.stream.Collectors.joining; +import static java.util.stream.Collectors.toSet; import com.google.auto.value.AutoValue; import com.google.common.base.CharMatcher; @@ -26,6 +27,7 @@ import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; +import com.google.common.collect.Sets; import com.google.common.primitives.Ints; import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.Account; @@ -49,6 +51,7 @@ import java.io.IOException; import java.util.Collection; import java.util.HashSet; import java.util.List; +import java.util.Optional; import java.util.Set; import java.util.SortedSet; import java.util.TreeSet; @@ -155,6 +158,8 @@ public class StarredChangesUtil { public static final String DEFAULT_LABEL = "star"; public static final String IGNORE_LABEL = "ignore"; public static final String MUTE_LABEL = "mute"; + public static final String REVIEWED_LABEL = "reviewed"; + public static final String UNREVIEWED_LABEL = "unreviewed"; public static final ImmutableSortedSet DEFAULT_LABELS = ImmutableSortedSet.of(DEFAULT_LABEL); @@ -360,6 +365,40 @@ public class StarredChangesUtil { return isMutedBy(rsrc.getChange(), rsrc.getUser().asIdentifiedUser().getAccountId()); } + private static String getReviewedLabel(Change change) { + return getReviewedLabel(change.currentPatchSetId().get()); + } + + private static String getReviewedLabel(int ps) { + return REVIEWED_LABEL + "/" + ps; + } + + private static String getUnreviewedLabel(Change change) { + return getUnreviewedLabel(change.currentPatchSetId().get()); + } + + private static String getUnreviewedLabel(int ps) { + return UNREVIEWED_LABEL + "/" + ps; + } + + public void markAsReviewed(ChangeResource rsrc) throws OrmException, IllegalLabelException { + star( + rsrc.getUser().asIdentifiedUser().getAccountId(), + rsrc.getProject(), + rsrc.getChange().getId(), + ImmutableSet.of(getReviewedLabel(rsrc.getChange())), + ImmutableSet.of(getUnreviewedLabel(rsrc.getChange()))); + } + + public void markAsUnreviewed(ChangeResource rsrc) throws OrmException, IllegalLabelException { + star( + rsrc.getUser().asIdentifiedUser().getAccountId(), + rsrc.getProject(), + rsrc.getChange().getId(), + ImmutableSet.of(getUnreviewedLabel(rsrc.getChange())), + ImmutableSet.of(getReviewedLabel(rsrc.getChange()))); + } + private static StarRef readLabels(Repository repo, String refName) throws IOException { Ref ref = repo.exactRef(refName); if (ref == null) { @@ -394,6 +433,25 @@ public class StarredChangesUtil { if (labels.containsAll(ImmutableSet.of(DEFAULT_LABEL, IGNORE_LABEL))) { throw new MutuallyExclusiveLabelsException(DEFAULT_LABEL, IGNORE_LABEL); } + + Set reviewedPatchSets = + labels + .stream() + .filter(l -> l.startsWith(REVIEWED_LABEL)) + .map(l -> Integer.valueOf(l.substring(REVIEWED_LABEL.length() + 1))) + .collect(toSet()); + Set unreviewedPatchSets = + labels + .stream() + .filter(l -> l.startsWith(UNREVIEWED_LABEL)) + .map(l -> Integer.valueOf(l.substring(UNREVIEWED_LABEL.length() + 1))) + .collect(toSet()); + Optional ps = + Sets.intersection(reviewedPatchSets, unreviewedPatchSets).stream().findFirst(); + if (ps.isPresent()) { + throw new MutuallyExclusiveLabelsException( + getReviewedLabel(ps.get()), getUnreviewedLabel(ps.get())); + } } private static void validateLabels(Collection labels) throws InvalidLabelsException { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java index d43327f538..79e312989f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java @@ -69,6 +69,8 @@ import com.google.gerrit.server.change.Index; import com.google.gerrit.server.change.ListChangeComments; import com.google.gerrit.server.change.ListChangeDrafts; import com.google.gerrit.server.change.ListChangeRobotComments; +import com.google.gerrit.server.change.MarkAsReviewed; +import com.google.gerrit.server.change.MarkAsUnreviewed; import com.google.gerrit.server.change.Move; import com.google.gerrit.server.change.Mute; import com.google.gerrit.server.change.PostHashtags; @@ -142,6 +144,8 @@ class ChangeApiImpl implements ChangeApi { private final Unignore unignore; private final Mute mute; private final Unmute unmute; + private final MarkAsReviewed markAsReviewed; + private final MarkAsUnreviewed markAsUnreviewed; private final SetWorkInProgress setWip; private final SetReadyForReview setReady; private final PutMessage putMessage; @@ -187,6 +191,8 @@ class ChangeApiImpl implements ChangeApi { Unignore unignore, Mute mute, Unmute unmute, + MarkAsReviewed markAsReviewed, + MarkAsUnreviewed markAsUnreviewed, SetWorkInProgress setWip, SetReadyForReview setReady, PutMessage putMessage, @@ -230,6 +236,8 @@ class ChangeApiImpl implements ChangeApi { this.unignore = unignore; this.mute = mute; this.unmute = unmute; + this.markAsReviewed = markAsReviewed; + this.markAsUnreviewed = markAsUnreviewed; this.setWip = setWip; this.setReady = setReady; this.putMessage = putMessage; @@ -700,6 +708,22 @@ class ChangeApiImpl implements ChangeApi { } } + @Override + public void markAsReviewed(boolean reviewed) throws RestApiException { + // TODO(dborowitz): Convert to RetryingRestModifyView. Needs to plumb BatchUpdate.Factory into + // StarredChangesUtil. + try { + if (reviewed) { + markAsReviewed.apply(change, new MarkAsReviewed.Input()); + } else { + markAsUnreviewed.apply(change, new MarkAsUnreviewed.Input()); + } + } catch (OrmException | IllegalLabelException e) { + throw asRestApiException( + "Cannot mark change as " + (reviewed ? "reviewed" : "unreviewed"), e); + } + } + @Override public PureRevertInfo pureRevert() throws RestApiException { return pureRevert(null); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index e2237f3676..3c6aa7e40a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -561,11 +561,10 @@ public class ChangeJson { } if (in.getStatus().isOpen() && has(REVIEWED) && user.isIdentifiedUser()) { - Account.Id accountId = user.getAccountId(); if (out.muted != null) { out.reviewed = true; } else { - out.reviewed = cd.reviewedBy().contains(accountId) ? true : null; + out.reviewed = cd.isReviewedBy(user.getAccountId()) ? true : null; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/MarkAsReviewed.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/MarkAsReviewed.java new file mode 100644 index 0000000000..265b2b0200 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/MarkAsReviewed.java @@ -0,0 +1,78 @@ +// Copyright (C) 2017 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.change; + +import com.google.gerrit.extensions.restapi.Response; +import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.extensions.restapi.RestModifyView; +import com.google.gerrit.extensions.webui.UiAction; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.StarredChangesUtil; +import com.google.gerrit.server.StarredChangesUtil.IllegalLabelException; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Singleton +public class MarkAsReviewed + implements RestModifyView, UiAction { + private static final Logger log = LoggerFactory.getLogger(MarkAsReviewed.class); + + public static class Input {} + + private final Provider dbProvider; + private final ChangeData.Factory changeDataFactory; + private final StarredChangesUtil stars; + + @Inject + MarkAsReviewed( + Provider dbProvider, + ChangeData.Factory changeDataFactory, + StarredChangesUtil stars) { + this.dbProvider = dbProvider; + this.changeDataFactory = changeDataFactory; + this.stars = stars; + } + + @Override + public Description getDescription(ChangeResource rsrc) { + return new UiAction.Description() + .setLabel("Mark Reviewed") + .setTitle("Mark the change as reviewed to unhighlight it in the dashboard") + .setVisible(!isReviewed(rsrc)); + } + + @Override + public Response apply(ChangeResource rsrc, Input input) + throws RestApiException, OrmException, IllegalLabelException { + stars.markAsReviewed(rsrc); + return Response.ok(""); + } + + private boolean isReviewed(ChangeResource rsrc) { + try { + return changeDataFactory + .create(dbProvider.get(), rsrc.getNotes()) + .isReviewedBy(rsrc.getUser().asIdentifiedUser().getAccountId()); + } catch (OrmException e) { + log.error("failed to check if change is reviewed", e); + } + return false; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/MarkAsUnreviewed.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/MarkAsUnreviewed.java new file mode 100644 index 0000000000..6de84eeea1 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/MarkAsUnreviewed.java @@ -0,0 +1,77 @@ +// Copyright (C) 2017 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.change; + +import com.google.gerrit.extensions.restapi.Response; +import com.google.gerrit.extensions.restapi.RestModifyView; +import com.google.gerrit.extensions.webui.UiAction; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.StarredChangesUtil; +import com.google.gerrit.server.StarredChangesUtil.IllegalLabelException; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Singleton +public class MarkAsUnreviewed + implements RestModifyView, UiAction { + private static final Logger log = LoggerFactory.getLogger(MarkAsUnreviewed.class); + + public static class Input {} + + private final Provider dbProvider; + private final ChangeData.Factory changeDataFactory; + private final StarredChangesUtil stars; + + @Inject + MarkAsUnreviewed( + Provider dbProvider, + ChangeData.Factory changeDataFactory, + StarredChangesUtil stars) { + this.dbProvider = dbProvider; + this.changeDataFactory = changeDataFactory; + this.stars = stars; + } + + @Override + public Description getDescription(ChangeResource rsrc) { + return new UiAction.Description() + .setLabel("Mark Unreviewed") + .setTitle("Mark the change as unreviewed to highlight it in the dashboard") + .setVisible(isReviewed(rsrc)); + } + + @Override + public Response apply(ChangeResource rsrc, Input input) + throws OrmException, IllegalLabelException { + stars.markAsUnreviewed(rsrc); + return Response.ok(""); + } + + private boolean isReviewed(ChangeResource rsrc) { + try { + return changeDataFactory + .create(dbProvider.get(), rsrc.getNotes()) + .isReviewedBy(rsrc.getUser().asIdentifiedUser().getAccountId()); + } catch (OrmException e) { + log.error("failed to check if change is reviewed", e); + } + return false; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java index f3a6c66a20..107716efc9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java @@ -92,6 +92,8 @@ public class Module extends RestApiModule { put(CHANGE_KIND, "unignore").to(Unignore.class); put(CHANGE_KIND, "mute").to(Mute.class); put(CHANGE_KIND, "unmute").to(Unmute.class); + put(CHANGE_KIND, "reviewed").to(MarkAsReviewed.class); + put(CHANGE_KIND, "unreviewed").to(MarkAsUnreviewed.class); post(CHANGE_KIND, "wip").to(SetWorkInProgress.class); post(CHANGE_KIND, "ready").to(SetReadyForReview.class); put(CHANGE_KIND, "message").to(PutMessage.class); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index 40b384db7d..2a712587cc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -1084,6 +1084,22 @@ public class ChangeData { return draftsByUser; } + public boolean isReviewedBy(Account.Id accountId) throws OrmException { + Collection stars = stars(accountId); + + if (stars.contains( + StarredChangesUtil.REVIEWED_LABEL + "/" + currentPatchSet().getPatchSetId())) { + return true; + } + + if (stars.contains( + StarredChangesUtil.UNREVIEWED_LABEL + "/" + currentPatchSet().getPatchSetId())) { + return false; + } + + return reviewedBy().contains(accountId); + } + public Set reviewedBy() throws OrmException { if (reviewedBy == null) { if (!lazyLoad) {