Require REVIEWED option to retrieve reviewed flag from /changes/ Setting the reviewed flag in the ChangeInfo JSON requires loading all ChangeMessage entities for the current patch set, sorting them by the date, and filtering to find an entry by the calling user. This information is only valid for the caller. When viewing another user's dashboard changes are marked bold if the *caller* has not reviewed the change, not if the owner of the displayed dashboard has. This is outright misleading. On dashboards that are not the callers highlighting is now disabled and the reviewed flag is not computed. The reviewed flag and highlighting is not used in search results, such as All > Open. Computing the reviewed flag for the signed in caller is a waste of server CPU resources. As the queries are performed sequentially during JSON result construction, this is also very slow on slower database environments like gerrit-review. Disable the recursive query logic behind an option in the request, significantly cutting costs for common queries. Change-Id: If8acbe430e8803e1d9cfcb747dfe28713c1b6da4
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 6835670..97ce2a1 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt
@@ -48,7 +48,6 @@ "status": "NEW", "created": "2012-07-17 07:18:30.854000000", "updated": "2012-07-17 07:19:27.766000000", - "reviewed": true, "mergeable": true, "_sortkey": "001e7057000006dc", "_number": 1756, @@ -120,7 +119,6 @@ "status": "NEW", "created": "2012-07-17 07:18:30.854000000", "updated": "2012-07-17 07:19:27.766000000", - "reviewed": true, "mergeable": true, "_sortkey": "001e7057000006dc", "_number": 1756, @@ -220,6 +218,12 @@ authenticated to obtain the available actions. -- +[[reviewed]] +-- +* `REVIEWED`: include the `reviewed` field if the caller is + authenticated and has commented on the current revision. +-- + .Request ---- GET /changes/?q=97&o=CURRENT_REVISION&o=CURRENT_COMMIT&o=CURRENT_FILES HTTP/1.0 @@ -351,7 +355,6 @@ "status": "NEW", "created": "2013-02-01 09:59:32.126000000", "updated": "2013-02-21 11:16:36.775000000", - "reviewed": true, "mergeable": true, "_sortkey": "0023412400000f7d", "_number": 3965, @@ -401,7 +404,6 @@ "status": "NEW", "created": "2013-02-01 09:59:32.126000000", "updated": "2013-02-21 11:16:36.775000000", - "reviewed": true, "mergeable": true, "_sortkey": "0023412400000f7d", "_number": 3965, @@ -640,7 +642,6 @@ "status": "ABANDONED", "created": "2013-02-01 09:59:32.126000000", "updated": "2013-02-21 11:16:36.775000000", - "reviewed": true, "mergeable": true, "_sortkey": "0023412400000f7d", "_number": 3965, @@ -699,7 +700,6 @@ "status": "NEW", "created": "2013-02-01 09:59:32.126000000", "updated": "2013-02-21 11:16:36.775000000", - "reviewed": true, "mergeable": true, "_sortkey": "0023412400000f7d", "_number": 3965, @@ -847,7 +847,6 @@ "status": "NEW", "created": "2013-02-01 09:59:32.126000000", "updated": "2013-02-21 11:16:36.775000000", - "reviewed": true, "mergeable": true, "_sortkey": "0023412400000f7d", "_number": 3965, @@ -912,7 +911,6 @@ "status": "MERGED", "created": "2013-02-01 09:59:32.126000000", "updated": "2013-02-21 11:16:36.775000000", - "reviewed": true, "_sortkey": "0023412400000f7d", "_number": 3965, "owner": { @@ -1220,7 +1218,6 @@ "status": "NEW", "created": "2013-02-01 09:59:32.126000000", "updated": "2013-02-21 11:16:36.775000000", - "reviewed": true, "mergeable": true, "_sortkey": "0023412400000f7d", "_number": 3965, @@ -2271,7 +2268,6 @@ "status": "NEW", "created": "2013-02-01 09:59:32.126000000", "updated": "2013-02-21 11:16:36.775000000", - "reviewed": true, "mergeable": true, "_sortkey": "0023412400000f7d", "_number": 3965, @@ -2322,7 +2318,6 @@ "status": "NEW", "created": "2013-02-01 09:59:32.126000000", "updated": "2013-02-21 11:16:36.775000000", - "reviewed": true, "mergeable": true, "_sortkey": "0023412400000f7d", "_number": 3965, @@ -2503,6 +2498,7 @@ Whether the calling user has starred this change. |`reviewed` |not set if `false`| Whether the change was reviewed by the calling user. +Only set if link:#reviewed[reviewed] is requested. |`mergeable` |optional| Whether the change is mergeable. + Not set for merged changes. @@ -2529,7 +2525,7 @@ link:rest-api-accounts.html#account-info[AccountInfo] entities. + Only set if link:#detailed-labels[detailed labels] are requested. |`messages`|optional| -Messages associated with the change as a list of +Messages associated with the change as a list of link:#change-message-info[ChangeMessageInfo] entities. + Only set if link:#messages[messages] are requested. |`current_revision` |optional|
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/changes/ListChangesOption.java b/gerrit-common/src/main/java/com/google/gerrit/common/changes/ListChangesOption.java index 6db0847..94d8389 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/changes/ListChangesOption.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/changes/ListChangesOption.java
@@ -40,7 +40,10 @@ MESSAGES(9), /** Include allowed actions client could perform. */ - CURRENT_ACTIONS(10); + CURRENT_ACTIONS(10), + + /** Set the reviewed boolean for the caller. */ + REVIEWED(11); private final int value;
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountDashboardScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountDashboardScreen.java index 6273717..5f89bf0 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountDashboardScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountDashboardScreen.java
@@ -19,6 +19,7 @@ import com.google.gerrit.client.rpc.Natives; import com.google.gerrit.client.rpc.ScreenLoadCallback; import com.google.gerrit.client.ui.Screen; +import com.google.gerrit.common.changes.ListChangesOption; import com.google.gerrit.reviewdb.client.Account; import com.google.gwt.core.client.JsArray; import com.google.gwt.event.dom.client.KeyPressEvent; @@ -26,6 +27,7 @@ import java.util.Collections; import java.util.Comparator; +import java.util.EnumSet; public class AccountDashboardScreen extends Screen implements ChangeListScreen { private final Account.Id ownerId; @@ -61,7 +63,7 @@ outgoing.setTitleText(Util.C.outgoingReviews()); incoming.setTitleText(Util.C.incomingReviews()); - incoming.setHighlightUnreviewed(true); + incoming.setHighlightUnreviewed(mine); closed.setTitleText(Util.C.recentlyClosed()); table.addSection(outgoing); @@ -83,6 +85,9 @@ display(result); } }, + mine + ? EnumSet.of(ListChangesOption.REVIEWED) + : EnumSet.noneOf(ListChangesOption.class), "is:open owner:" + who, "is:open reviewer:" + who + " -owner:" + who, "is:closed owner:" + who + " -age:4w limit:10");
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeList.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeList.java index fea117f..773313a 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeList.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeList.java
@@ -28,13 +28,18 @@ /** Run 2 or more queries in a single remote invocation. */ public static void query( - AsyncCallback<JsArray<ChangeList>> callback, String... queries) { + AsyncCallback<JsArray<ChangeList>> callback, + EnumSet<ListChangesOption> options, + String... queries) { assert queries.length >= 2; // At least 2 is required for correct result. RestApi call = new RestApi(URI); for (String q : queries) { call.addParameterRaw("q", KeyUtil.encode(q)); } - addOptions(call, EnumSet.of(ListChangesOption.LABELS)); + + EnumSet<ListChangesOption> o = EnumSet.of(ListChangesOption.LABELS); + o.addAll(options); + addOptions(call, o); call.get(callback); }
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/DashboardTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/DashboardTable.java index 7b387ab..3002e48 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/DashboardTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/DashboardTable.java
@@ -19,10 +19,12 @@ import com.google.gerrit.client.rpc.Natives; import com.google.gerrit.client.ui.InlineHyperlink; import com.google.gerrit.common.PageLinks; +import com.google.gerrit.common.changes.ListChangesOption; import com.google.gwt.core.client.JsArray; import com.google.gwt.http.client.URL; import java.util.ArrayList; +import java.util.EnumSet; import java.util.List; import java.util.ListIterator; @@ -103,6 +105,7 @@ finishDisplay(); } }, + EnumSet.noneOf(ListChangesOption.class), queries.toArray(new String[queries.size()])); } }
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 90ff799..9a33bfb 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
@@ -25,6 +25,7 @@ import static com.google.gerrit.common.changes.ListChangesOption.DETAILED_LABELS; import static com.google.gerrit.common.changes.ListChangesOption.LABELS; import static com.google.gerrit.common.changes.ListChangesOption.MESSAGES; +import static com.google.gerrit.common.changes.ListChangesOption.REVIEWED; import com.google.common.base.Joiner; import com.google.common.base.Objects; @@ -260,7 +261,9 @@ out.starred = userProvider.get().getStarredChanges().contains(in.getId()) ? true : null; - out.reviewed = in.getStatus().isOpen() && isChangeReviewed(cd) ? true : null; + out.reviewed = in.getStatus().isOpen() + && has(REVIEWED) + && isChangeReviewed(cd) ? true : null; out.labels = labelsFor(cd, has(LABELS), has(DETAILED_LABELS)); Collection<PatchSet.Id> limited = cd.getLimitedPatchSets();