Replace Mute/Unmute by Mark as Reviewed/Unreviewed (Part 1)

Muting a change de-highlights the change on the dashboard until a new
patch set is uploaded. Most users were confused about mute since they
had different expections of what mute would do. E.g. they expected
that mute would behave more similar to how mute in Gmail works, and
that the change would disappear from the dashboard until a new patch
is uploaded (see issue 7237).

Mute as it is implemented now behaves more as "Mark as Read" in email
clients. To make this function more intuitive for users this change
adds "Mark as Reviewed" as a replacement for "Mute". It's named
"Mark as Reviewed" as opposed to "Mark as Read" to be consistent with
the existing "reviewed" field in ChangeInfo. For muted changes the
"reviewed" flag is always set to true. The Mute / Unmute REST
endpoints are still kept to allow a migrating Gerrit hosts at
*.googlesource.com.

The REST endpoint for setting a change as reviewed is:

  PUT /changes/<change-id>/reviewed

This name was chosen to stay consistent with the REST endpoint for
setting reviewed flags on files which is:

  PUT /changes/<change>/revisions/<revision>/files/<file>/reviewed

Mute is represented by a star label called 'mute'. The new
MarkAsReviewed REST endpoint uses the new 'reviewed' label instead.
A follow-up change will add a schema migration that takes care about
renaming the existing mute labels.

The Unmute action removes the "mute" label from the change, so that
the "reviewed" flag is no longer overwritten with true. Without mute
star label the "reviewed" flag is again computed based on the change
state. This means renaming "Unmute" to "Mark as Unreviewed" would be
confusing since users would expect from "Mark as Unreviewed" that the
change is non-reviewed afterwards (and not automatically computed
which can result in reviewed = true).

This is why "Mark as Unreviewed" sets a "unreviewed" star label on the
change (in addition to removing the "reviewed" label). This way we
have 3 states:

1. neither "reviewed" nor "unreviewed" label: whether the change is
   highlighted in the dashboard is automatically computed based on the
   change state
2. "reviewed" label: change is not highlighted in the dashboard
3. "unreviewed" label: change is highlighted in the dashboard

Having the "reviewed" and "unreviewed" labels on a change (for the
same patch set) at the same time is invalid and is rejected by the
server.

This means MarkAsReviewed moves the change from state 1 or 3 to state
2, MarkAsUnreviewed moves the change from state 1 or 2 to state 3.

We could also support DELETE requests on /changes/<change>/reviewed
and /changes/<change>/unreviewed for moving the change from state 2
and 3 back to state 1 but we don't have a need for this at the moment,
hence this is not implemented in this change. The change will
automatically move back to state 1 when a new patch set is added. If
manually going back to state 1 is necessary one can always use the
generic stars API to remove the "reviewed" / "unreviewed" star.

Mute is not allowed on own changes and on changes which are ignored.
This limitation is removed for "Mark as Reviewed" so that this funtion
is completely orthogonal to the ignore functionality.

The migration to replace Mute/Unmute by Mark as Reviewed/Unreviewed is
done in multiple steps:

1. Add new REST endpoints for Mark As Reviewed/Unreviewed (this
   change)
2. Adapt PolyGerrit to use Mark As Reviewed/Unreviewed instead of
   Mute/Unmute
3. Run migration for Gerrit hosts at *.googlesource.com that renames
   existing mute labels
4. Remove Mute/Unmute functionality and add schema migration that
   renames existing mute labels

Bug: Issue 7237
Change-Id: I59f4156688dbf1582a33cb4e18f0adb54a73fe35
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2017-10-01 12:29:05 +02:00
parent 512eb25dcd
commit ceb673ecff
11 changed files with 421 additions and 2 deletions

View File

@@ -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/<patchset_id>"-star is set by a user, and <patchset_id>
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/<patchset_id>"-star is set by a user, and <patchset_id>
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

View File

@@ -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
--

View File

@@ -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<Message> 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();

View File

@@ -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();

View File

@@ -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<String> 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<Integer> reviewedPatchSets =
labels
.stream()
.filter(l -> l.startsWith(REVIEWED_LABEL))
.map(l -> Integer.valueOf(l.substring(REVIEWED_LABEL.length() + 1)))
.collect(toSet());
Set<Integer> unreviewedPatchSets =
labels
.stream()
.filter(l -> l.startsWith(UNREVIEWED_LABEL))
.map(l -> Integer.valueOf(l.substring(UNREVIEWED_LABEL.length() + 1)))
.collect(toSet());
Optional<Integer> ps =
Sets.intersection(reviewedPatchSets, unreviewedPatchSets).stream().findFirst();
if (ps.isPresent()) {
throw new MutuallyExclusiveLabelsException(
getReviewedLabel(ps.get()), getUnreviewedLabel(ps.get()));
}
}
private static void validateLabels(Collection<String> labels) throws InvalidLabelsException {

View File

@@ -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);

View File

@@ -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;
}
}

View File

@@ -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<ChangeResource, MarkAsReviewed.Input>, UiAction<ChangeResource> {
private static final Logger log = LoggerFactory.getLogger(MarkAsReviewed.class);
public static class Input {}
private final Provider<ReviewDb> dbProvider;
private final ChangeData.Factory changeDataFactory;
private final StarredChangesUtil stars;
@Inject
MarkAsReviewed(
Provider<ReviewDb> 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<String> 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;
}
}

View File

@@ -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<ChangeResource, MarkAsUnreviewed.Input>, UiAction<ChangeResource> {
private static final Logger log = LoggerFactory.getLogger(MarkAsUnreviewed.class);
public static class Input {}
private final Provider<ReviewDb> dbProvider;
private final ChangeData.Factory changeDataFactory;
private final StarredChangesUtil stars;
@Inject
MarkAsUnreviewed(
Provider<ReviewDb> 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<String> 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;
}
}

View File

@@ -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);

View File

@@ -1084,6 +1084,22 @@ public class ChangeData {
return draftsByUser;
}
public boolean isReviewedBy(Account.Id accountId) throws OrmException {
Collection<String> 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<Account.Id> reviewedBy() throws OrmException {
if (reviewedBy == null) {
if (!lazyLoad) {