Remove ListChangesOption.DRAFT_COMMENTS
If this option is used the change details include for each patch set the information whether there are draft comments on it. The problem is that the draft comment status is not included into the ETag computation of the change and as result the change is not reloaded when the draft comment status has changed. Including the draft comment status into the ETag computation would be expensive since this requires one SQL statement per patch set to load the draft comments from the database. An easier and cheaper way to find out if there are draft comments on a change is to do a change query with the 'has:draft' operator. This query can be directly answered from the secondary index and no SQL statements need to be fired. Already now ListChangesOption.DRAFT_COMMENTS is no longer used by the Gerrit client. Also the name of the DRAFT_COMMENTS option was misleading. It didn't include the draft comments into change detail, but only for each patch set the information whether it has draft comments. Change-Id: I297fcf01256f02953aa0b5caee4f3aee4554bc59 Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>
This commit is contained in:
@@ -231,13 +231,6 @@ default. Optional fields are:
|
||||
`CURRENT_REVISION` or `ALL_REVISIONS` option is selected.
|
||||
--
|
||||
|
||||
[[draft_comments]]
|
||||
--
|
||||
* `DRAFT_COMMENTS`: include the `has_draft_comments` field for
|
||||
revisions. Only valid when the `CURRENT_REVISION` or `ALL_REVISIONS`
|
||||
option is selected.
|
||||
--
|
||||
|
||||
[[current-commit]]
|
||||
--
|
||||
* `CURRENT_COMMIT`: parse and output all header fields from the
|
||||
@@ -4198,9 +4191,6 @@ link:#list-changes[Query Changes].
|
||||
|===========================
|
||||
|Field Name ||Description
|
||||
|`draft` |not set if `false`|Whether the patch set is a draft.
|
||||
|`has_draft_comments` |not set if `false`|Whether the patch
|
||||
set has one or more draft comments by the calling user. Only set if
|
||||
link:#draft_comments[DRAFT_COMMENTS] option is requested.
|
||||
|`_number` ||The patch set number.
|
||||
|`created` ||
|
||||
The link:rest-api.html#timestamp[timestamp] of when the patch set was
|
||||
|
||||
@@ -46,9 +46,6 @@ public enum ListChangesOption {
|
||||
/** Set the reviewed boolean for the caller. */
|
||||
REVIEWED(11),
|
||||
|
||||
/** Include draft comments for the caller. */
|
||||
DRAFT_COMMENTS(12),
|
||||
|
||||
/** Include download commands for the caller. */
|
||||
DOWNLOAD_COMMANDS(13),
|
||||
|
||||
|
||||
@@ -20,7 +20,6 @@ import java.util.Map;
|
||||
public class RevisionInfo {
|
||||
public transient boolean isCurrent;
|
||||
public Boolean draft;
|
||||
public Boolean hasDraftComments;
|
||||
public int _number;
|
||||
public Timestamp created;
|
||||
public AccountInfo uploader;
|
||||
|
||||
@@ -307,7 +307,6 @@ public class ChangeInfo extends JavaScriptObject {
|
||||
public final native int _number() /*-{ return this._number; }-*/;
|
||||
public final native String name() /*-{ return this.name; }-*/;
|
||||
public final native boolean draft() /*-{ return this.draft || false; }-*/;
|
||||
public final native boolean hasDraftComments() /*-{ return this.has_draft_comments || false; }-*/;
|
||||
public final native boolean isEdit() /*-{ return this._number == 0; }-*/;
|
||||
public final native CommitInfo commit() /*-{ return this.commit; }-*/;
|
||||
public final native void setCommit(CommitInfo c) /*-{ this.commit = c; }-*/;
|
||||
|
||||
@@ -27,7 +27,6 @@ import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVI
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.DETAILED_ACCOUNTS;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.DETAILED_LABELS;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.DOWNLOAD_COMMANDS;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.DRAFT_COMMENTS;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.LABELS;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.MESSAGES;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWED;
|
||||
@@ -86,7 +85,6 @@ import com.google.gerrit.server.AnonymousUser;
|
||||
import com.google.gerrit.server.ChangeMessagesUtil;
|
||||
import com.google.gerrit.server.CurrentUser;
|
||||
import com.google.gerrit.server.IdentifiedUser;
|
||||
import com.google.gerrit.server.PatchLineCommentsUtil;
|
||||
import com.google.gerrit.server.WebLinks;
|
||||
import com.google.gerrit.server.account.AccountLoader;
|
||||
import com.google.gerrit.server.git.GitRepositoryManager;
|
||||
@@ -141,7 +139,6 @@ public class ChangeJson {
|
||||
private final WebLinks webLinks;
|
||||
private final EnumSet<ListChangesOption> options;
|
||||
private final ChangeMessagesUtil cmUtil;
|
||||
private final PatchLineCommentsUtil plcUtil;
|
||||
private final Provider<ConsistencyChecker> checkerProvider;
|
||||
private final ActionJson actionJson;
|
||||
private final RebaseChange rebaseChange;
|
||||
@@ -166,7 +163,6 @@ public class ChangeJson {
|
||||
DynamicMap<DownloadCommand> downloadCommands,
|
||||
WebLinks webLinks,
|
||||
ChangeMessagesUtil cmUtil,
|
||||
PatchLineCommentsUtil plcUtil,
|
||||
Provider<ConsistencyChecker> checkerProvider,
|
||||
ActionJson actionJson,
|
||||
RebaseChange rebaseChange) {
|
||||
@@ -185,7 +181,6 @@ public class ChangeJson {
|
||||
this.downloadCommands = downloadCommands;
|
||||
this.webLinks = webLinks;
|
||||
this.cmUtil = cmUtil;
|
||||
this.plcUtil = plcUtil;
|
||||
this.checkerProvider = checkerProvider;
|
||||
this.actionJson = actionJson;
|
||||
this.rebaseChange = rebaseChange;
|
||||
@@ -881,15 +876,6 @@ public class ChangeJson {
|
||||
new RevisionResource(new ChangeResource(ctl, rebaseChange), in));
|
||||
}
|
||||
|
||||
if (has(DRAFT_COMMENTS)
|
||||
&& userProvider.get().isIdentifiedUser()) {
|
||||
IdentifiedUser user = (IdentifiedUser)userProvider.get();
|
||||
out.hasDraftComments =
|
||||
plcUtil.draftByPatchSetAuthor(db.get(), in.getId(),
|
||||
user.getAccountId(), ctl.getNotes()).iterator().hasNext()
|
||||
? true
|
||||
: null;
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user