opendev
/
gerrit
Code Issues Proposed changes

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

Remove the Mute/Unmute functionality, it is replaced by the new
MarkAsReviewed/MarkAsUnreviewed REST endpoints (added by predecessor
change).

A schema migration takes care to rename existing mute labels. Since
the star labels are also stored in the change index the affected
changes must be reindexed. Since we can't reindex changes from a
schema migration a new change schema version is added which enforces a
reindex of all changes on upgrade to the new index version (either by
online or offline reindex).

The mute functionality was added by change I83085033f which is not
part of any release yet. Hence we can still change the API for this
without breaking backwards compatibility.

Since the 'mute' namespace is now free again we may use this name
later to implement functionality which is similar to Gmail's mute.
E.g. remove the change from the dashboard until a new patch is
uploaded.

The muted() method is removed from the extension API since the
information whether a change is reviewed or not is already provided as
part of ChangeInfo (see "reviewed" field). This method was anyway bad
since this functionality didn't exist as REST endpoint and the
extension API is normally only a wrapper around the REST API.

The 'mute' field from ChangeInfo is removed since it wasn't used on
client-side. The client rather relied on the "reviewed" field to
highlight changes in dashboards.

Bug: Issue 7237
Change-Id: Ia21e6031908dd0c722f3839ab186724043b460d3
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin 2017-10-01 12:39:36 +02:00 committed by Paladox none
parent dc0f294b80
commit 614f983a71
15 changed files with 83 additions and 336 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
Documentation/dev-stars.txt
View File

@ -61,19 +61,6 @@ request. They can then decide to remove the ignore star.
The ignore star is represented by the special star label 'ignore'.
[[mute-star]]
== Mute Star
If the "mute/<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.
The ChangeInfo muted-field will show if the change is currently in a
mute state.
[[reviewed-star]]
== Reviewed Star

36
Documentation/rest-api-changes.txt
View File

@ -2329,40 +2329,6 @@ Un-marks a change as ignored.
PUT /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/unignore HTTP/1.0
----
[[mute]]
=== Mute
--
'PUT /changes/link:#change-id[\{change-id\}]/mute'
--
Marks a change as muted.
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/mute HTTP/1.0
----
[[unmute]]
=== Unmute
--
'PUT /changes/link:#change-id[\{change-id\}]/unmute'
--
Unmutes a change.
.Request
----
PUT /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/unmute HTTP/1.0
----
[[mark-as-reviewed]]
=== Mark as Reviewed
--
@ -5677,8 +5643,6 @@ A list of star labels that are applied by the calling user to this
change. The labels are lexicographically sorted.
|`reviewed` |not set if `false`|
Whether the change was reviewed by the calling user.
|`muted` |not set if `false`|
Whether the change has been link:#mute[muted] by the calling user.
Only set if link:#reviewed[reviewed] is requested.
|`submit_type` |optional|
The link:project-configuration.html#submit_type[submit type] of the change. +

36
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@ -3373,42 +3373,6 @@ public class ChangeIT extends AbstractDaemonTest {
gApi.accounts().self().starChange(changeId);
}
@Test
public void mute() 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()).muted()).isFalse();
gApi.changes().id(r.getChangeId()).mute(true);
assertThat(gApi.changes().id(r.getChangeId()).muted()).isTrue();
setApiUser(user2);
sender.clear();
amendChange(r.getChangeId());
setApiUser(user);
assertThat(gApi.changes().id(r.getChangeId()).muted()).isFalse();
List<Message> messages = sender.getMessages();
assertThat(messages).hasSize(1);
assertThat(messages.get(0).rcpt()).containsExactly(user.emailAddress);
}
@Test
public void cannotMuteOwnChange() throws Exception {
String changeId = createChange().getChangeId();
exception.expect(BadRequestException.class);
exception.expectMessage("cannot mute own change");
gApi.changes().id(changeId).mute(true);
}
@Test
public void markAsReviewed() throws Exception {
TestAccount user2 = accountCreator.user2();

24
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
View File

@ -118,20 +118,6 @@ public interface ChangeApi {
*/
boolean ignored() throws RestApiException;
/**
* Mute or un-mute this change.
*
* @param mute mute the change if true
*/
void mute(boolean mute) throws RestApiException;
/**
* Check if this change is muted.
*
* @return true if the change is muted.
*/
boolean muted() throws RestApiException;
/**
* Mark this change as reviewed/unreviewed.
*
@ -590,16 +576,6 @@ public interface ChangeApi {
throw new NotImplementedException();
}
@Override
public void mute(boolean mute) throws RestApiException {
throw new NotImplementedException();
}
@Override
public boolean muted() throws RestApiException {
throw new NotImplementedException();
}
@Override
public void markAsReviewed(boolean reviewed) throws RestApiException {
throw new NotImplementedException();

1
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java
View File

@ -37,7 +37,6 @@ public class ChangeInfo {
public Timestamp submitted;
public AccountInfo submitter;
public Boolean starred;
public Boolean muted;
public Collection<String> stars;
public Boolean reviewed;
public SubmitType submitType;

2
gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java
View File

@ -138,8 +138,6 @@ public class ChangeInfo extends JavaScriptObject {
public final native boolean starred() /*-{ return this.starred ? true : false; }-*/;
public final native boolean muted() /*-{ return this.muted ? true : false; }-*/;
public final native boolean reviewed() /*-{ return this.reviewed ? true : false; }-*/;
public final native boolean isPrivate() /*-{ return this.is_private ? true : false; }-*/;

33
gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java
View File

@ -157,7 +157,6 @@ 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 =
@ -335,36 +334,6 @@ public class StarredChangesUtil {
return isIgnoredBy(rsrc.getChange().getId(), rsrc.getUser().asIdentifiedUser().getAccountId());
}
private static String getMuteLabel(Change change) {
return MUTE_LABEL + "/" + change.currentPatchSetId().get();
}
public void mute(ChangeResource rsrc) throws OrmException, IllegalLabelException {
star(
rsrc.getUser().asIdentifiedUser().getAccountId(),
rsrc.getProject(),
rsrc.getChange().getId(),
ImmutableSet.of(getMuteLabel(rsrc.getChange())),
ImmutableSet.of());
}
public void unmute(ChangeResource rsrc) throws OrmException, IllegalLabelException {
star(
rsrc.getUser().asIdentifiedUser().getAccountId(),
rsrc.getProject(),
rsrc.getChange().getId(),
ImmutableSet.of(),
ImmutableSet.of(getMuteLabel(rsrc.getChange())));
}
public boolean isMutedBy(Change change, Account.Id accountId) throws OrmException {
return getLabels(accountId, change.getId()).contains(getMuteLabel(change));
}
public boolean isMuted(ChangeResource rsrc) throws OrmException {
return isMutedBy(rsrc.getChange(), rsrc.getUser().asIdentifiedUser().getAccountId());
}
private static String getReviewedLabel(Change change) {
return getReviewedLabel(change.currentPatchSetId().get());
}
@ -399,7 +368,7 @@ public class StarredChangesUtil {
ImmutableSet.of(getReviewedLabel(rsrc.getChange())));
}
private static StarRef readLabels(Repository repo, String refName) throws IOException {
public static StarRef readLabels(Repository repo, String refName) throws IOException {
Ref ref = repo.exactRef(refName);
if (ref == null) {
return StarRef.MISSING;

32
gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
View File

@ -72,7 +72,6 @@ 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;
import com.google.gerrit.server.change.PostPrivate;
import com.google.gerrit.server.change.PostReviewers;
@ -90,7 +89,6 @@ import com.google.gerrit.server.change.SetWorkInProgress;
import com.google.gerrit.server.change.SubmittedTogether;
import com.google.gerrit.server.change.SuggestChangeReviewers;
import com.google.gerrit.server.change.Unignore;
import com.google.gerrit.server.change.Unmute;
import com.google.gerrit.server.change.WorkInProgressOp;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@ -142,8 +140,6 @@ class ChangeApiImpl implements ChangeApi {
private final DeletePrivate deletePrivate;
private final Ignore ignore;
private final Unignore unignore;
private final Mute mute;
private final Unmute unmute;
private final MarkAsReviewed markAsReviewed;
private final MarkAsUnreviewed markAsUnreviewed;
private final SetWorkInProgress setWip;
@ -189,8 +185,6 @@ class ChangeApiImpl implements ChangeApi {
DeletePrivate deletePrivate,
Ignore ignore,
Unignore unignore,
Mute mute,
Unmute unmute,
MarkAsReviewed markAsReviewed,
MarkAsUnreviewed markAsUnreviewed,
SetWorkInProgress setWip,
@ -234,8 +228,6 @@ class ChangeApiImpl implements ChangeApi {
this.deletePrivate = deletePrivate;
this.ignore = ignore;
this.unignore = unignore;
this.mute = mute;
this.unmute = unmute;
this.markAsReviewed = markAsReviewed;
this.markAsUnreviewed = markAsUnreviewed;
this.setWip = setWip;
@ -684,30 +676,6 @@ class ChangeApiImpl implements ChangeApi {
}
}
@Override
public void mute(boolean mute) throws RestApiException {
// TODO(dborowitz): Convert to RetryingRestModifyView. Needs to plumb BatchUpdate.Factory into
// StarredChangesUtil.
try {
if (mute) {
this.mute.apply(change, new Mute.Input());
} else {
unmute.apply(change, new Unmute.Input());
}
} catch (OrmException | IllegalLabelException e) {
throw asRestApiException("Cannot mute change", e);
}
}
@Override
public boolean muted() throws RestApiException {
try {
return stars.isMuted(change);
} catch (OrmException e) {
throw asRestApiException("Cannot check if muted", e);
}
}
@Override
public void markAsReviewed(boolean reviewed) throws RestApiException {
// TODO(dborowitz): Convert to RetryingRestModifyView. Needs to plumb BatchUpdate.Factory into

10
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
View File

@ -551,21 +551,13 @@ public class ChangeJson {
if (user.isIdentifiedUser()) {
Collection<String> stars = cd.stars(user.getAccountId());
out.starred = stars.contains(StarredChangesUtil.DEFAULT_LABEL) ? true : null;
out.muted =
stars.contains(StarredChangesUtil.MUTE_LABEL + "/" + cd.currentPatchSet().getPatchSetId())
? true
: null;
if (!stars.isEmpty()) {
out.stars = stars;
}
}
if (in.getStatus().isOpen() && has(REVIEWED) && user.isIdentifiedUser()) {
if (out.muted != null) {
out.reviewed = true;
} else {
out.reviewed = cd.isReviewedBy(user.getAccountId()) ? true : null;
}
out.reviewed = cd.isReviewedBy(user.getAccountId()) ? true : null;
}
out.labels = labelsFor(perm, cd, has(LABELS), has(DETAILED_LABELS));

2
gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
View File

@ -90,8 +90,6 @@ public class Module extends RestApiModule {
delete(CHANGE_KIND, "private").to(DeletePrivate.class);
put(CHANGE_KIND, "ignore").to(Ignore.class);
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);

80
gerrit-server/src/main/java/com/google/gerrit/server/change/Mute.java
View File

@ -1,80 +0,0 @@
// 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.BadRequestException;
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.server.StarredChangesUtil;
import com.google.gerrit.server.StarredChangesUtil.IllegalLabelException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@Singleton
public class Mute implements RestModifyView<ChangeResource, Mute.Input>, UiAction<ChangeResource> {
private static final Logger log = LoggerFactory.getLogger(Mute.class);
public static class Input {}
private final StarredChangesUtil stars;
@Inject
Mute(StarredChangesUtil stars) {
this.stars = stars;
}
@Override
public Description getDescription(ChangeResource rsrc) {
return new UiAction.Description()
.setLabel("Mute")
.setTitle("Mute the change to unhighlight it in the dashboard")
.setVisible(isMuteable(rsrc));
}
@Override
public Response<String> apply(ChangeResource rsrc, Input input)
throws RestApiException, OrmException, IllegalLabelException {
if (rsrc.isUserOwner()) {
throw new BadRequestException("cannot mute own change");
}
if (!isMuted(rsrc)) {
stars.mute(rsrc);
}
return Response.ok("");
}
private boolean isMuted(ChangeResource rsrc) {
try {
return stars.isMuted(rsrc);
} catch (OrmException e) {
log.error("failed to check muted star", e);
}
return false;
}
private boolean isMuteable(ChangeResource rsrc) {
try {
return !rsrc.isUserOwner() && !isMuted(rsrc) && !stars.isIgnored(rsrc);
} catch (OrmException e) {
log.error("failed to check ignored star", e);
}
return false;
}
}

67
gerrit-server/src/main/java/com/google/gerrit/server/change/Unmute.java
View File

@ -1,67 +0,0 @@
// 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.server.StarredChangesUtil;
import com.google.gerrit.server.StarredChangesUtil.IllegalLabelException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@Singleton
public class Unmute
implements RestModifyView<ChangeResource, Unmute.Input>, UiAction<ChangeResource> {
private static final Logger log = LoggerFactory.getLogger(Unmute.class);
public static class Input {}
private final StarredChangesUtil stars;
@Inject
Unmute(StarredChangesUtil stars) {
this.stars = stars;
}
@Override
public Description getDescription(ChangeResource rsrc) {
return new UiAction.Description()
.setLabel("Unmute")
.setTitle("Unmute the change")
.setVisible(isMuted(rsrc));
}
@Override
public Response<String> apply(ChangeResource rsrc, Input input)
throws OrmException, IllegalLabelException {
if (isMuted(rsrc)) {
stars.unmute(rsrc);
}
return Response.ok("");
}
private boolean isMuted(ChangeResource rsrc) {
try {
return stars.isMuted(rsrc);
} catch (OrmException e) {
log.error("failed to check muted star", e);
}
return false;
}
}

5
gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
View File

@ -91,7 +91,10 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions<ChangeData> {
@Deprecated static final Schema<ChangeData> V46 = schema(V45);
// Removal of draft change workflow requires reindexing
static final Schema<ChangeData> V47 = schema(V46);
@Deprecated static final Schema<ChangeData> V47 = schema(V46);
// Rename of star label 'mute' to 'reviewed' requires reindexing
static final Schema<ChangeData> V48 = schema(V47);
public static final String NAME = "changes";
public static final ChangeSchemaDefinitions INSTANCE = new ChangeSchemaDefinitions();

2
gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java
View File

@ -35,7 +35,7 @@ import java.util.concurrent.TimeUnit;
/** A version of the database schema. */
public abstract class SchemaVersion {
/** The current schema version. */
public static final Class<Schema_160> C = Schema_160.class;
public static final Class<Schema_161> C = Schema_161.class;
public static int getBinaryVersion() {
return guessVersion(C);

76
gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_161.java Normal file
View File

@ -0,0 +1,76 @@
// 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.schema;
import static java.util.stream.Collectors.toList;
import com.google.gerrit.reviewdb.client.RefNames;
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.StarredChangesUtil.StarRef;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.lib.TextProgressMonitor;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
public class Schema_161 extends SchemaVersion {
private static final String MUTE_LABEL = "mute";
private final GitRepositoryManager repoManager;
private final AllUsersName allUsersName;
@Inject
Schema_161(
Provider<Schema_160> prior, GitRepositoryManager repoManager, AllUsersName allUsersName) {
super(prior);
this.repoManager = repoManager;
this.allUsersName = allUsersName;
}
@Override
protected void migrateData(ReviewDb db, UpdateUI ui) throws OrmException {
try (Repository git = repoManager.openRepository(allUsersName);
RevWalk rw = new RevWalk(git)) {
BatchRefUpdate bru = git.getRefDatabase().newBatchUpdate();
for (Ref ref : git.getRefDatabase().getRefs(RefNames.REFS_STARRED_CHANGES).values()) {
StarRef starRef = StarredChangesUtil.readLabels(git, ref.getName());
if (starRef.labels().contains(MUTE_LABEL)) {
ObjectId id =
StarredChangesUtil.writeLabels(
git,
starRef
.labels()
.stream()
.map(l -> l.equals(MUTE_LABEL) ? StarredChangesUtil.REVIEWED_LABEL : l)
.collect(toList()));
bru.addCommand(new ReceiveCommand(ObjectId.zeroId(), id, ref.getName()));
}
}
bru.execute(rw, new TextProgressMonitor());
} catch (IOException | IllegalLabelException ex) {
throw new OrmException(ex);
}
}
}