Merge "Fix error when try assign on getter with existing property"
diff --git a/Documentation/dev-design-docs.txt b/Documentation/dev-design-docs.txt
index ca5ff62..888b7dc 100644
--- a/Documentation/dev-design-docs.txt
+++ b/Documentation/dev-design-docs.txt
@@ -1,3 +1,4 @@
+:linkattrs:
= Gerrit Code Review - Design Docs
For the link:dev-contributing.html#design-driven-contribution-process[
@@ -98,6 +99,13 @@
Everyone in the link:dev-roles.html[Gerrit community] is welcome to
take part in the design review and comment on the design.
+Positive `Code-Review` votes on changes that add/modify design docs are
+sticky. This means any `Code-Review+1` and `Code-Review+2` vote is
+preserved when a new patch set is uploaded. If a new patch set makes
+significant changes, the uploader of the new patch set must start a new
+review round by removing all positive `Code-Review` votes from the
+change manually.
+
Ideas for alternative solutions should be uploaded as a change that
describes the solution (see link:#collaboration[above]).
diff --git a/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java b/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
index 6a66ba3..2ebe457 100644
--- a/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
+++ b/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
@@ -45,13 +45,15 @@
import java.util.regex.Pattern;
/** Helper for generating parts of {@code index.html}. */
+@UsedAt(Project.GOOGLE)
public class IndexHtmlUtil {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- static final String changeCanonicalUrl = ".*/c/(?<project>.+)/\\+/(?<changeNum>\\d+)";
- static final String basePatchNumUrlPart = "(/(-?\\d+|edit)(\\.\\.(\\d+|edit))?)";
- static final Pattern changeUrlPattern =
+ public static final String changeCanonicalUrl = ".*/c/(?<project>.+)/\\+/(?<changeNum>\\d+)";
+ public static final String basePatchNumUrlPart = "(/(-?\\d+|edit)(\\.\\.(\\d+|edit))?)";
+ public static final Pattern changeUrlPattern =
Pattern.compile(changeCanonicalUrl + basePatchNumUrlPart + "?" + "/?$");
- static final Pattern diffUrlPattern =
+
+ public static final Pattern diffUrlPattern =
Pattern.compile(changeCanonicalUrl + basePatchNumUrlPart + "(/(.+))" + "/?$");
public static String getDefaultChangeDetailHex() {
@@ -80,6 +82,18 @@
return ListOption.toHex(options);
}
+ public static String computeChangeRequestsPath(String requestedURL, Pattern pattern) {
+ Matcher matcher = pattern.matcher(requestedURL);
+ if (matcher.matches()) {
+ Integer changeId = Ints.tryParse(matcher.group("changeNum"));
+ if (changeId != null) {
+ return "changes/" + Url.encode(matcher.group("project")) + "~" + changeId;
+ }
+ }
+
+ return null;
+ }
+
/**
* Returns both static and dynamic parameters of {@code index.html}. The result is to be used when
* rendering the soy template.
@@ -107,7 +121,6 @@
}
/** Returns dynamic parameters of {@code index.html}. */
- @UsedAt(Project.GOOGLE)
public static Map<String, Map<String, SanitizedContent>> dynamicTemplateData(GerritApi gerritApi)
throws RestApiException {
Gson gson = OutputFormat.JSON_COMPACT.newGson();
@@ -201,18 +214,6 @@
return data.build();
}
- static String computeChangeRequestsPath(String requestedURL, Pattern pattern) {
- Matcher matcher = pattern.matcher(requestedURL);
- if (matcher.matches()) {
- Integer changeId = Ints.tryParse(matcher.group("changeNum"));
- if (changeId != null) {
- return "changes/" + Url.encode(matcher.group("project")) + "~" + changeId;
- }
- }
-
- return null;
- }
-
private static String computeCanonicalPath(@Nullable String canonicalURL)
throws URISyntaxException {
if (Strings.isNullOrEmpty(canonicalURL)) {
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 1fd0f82..4af7872 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -1716,7 +1716,7 @@
req.getMethod(), req.getRequestURI(), getParameterNames(req));
logger.atFinest().log("Calling user: %s", globals.currentUser.get().getLoggableName());
logger.atFinest().log(
- "Groups: %s", globals.currentUser.get().getEffectiveGroups().getKnownGroups());
+ "Groups: %s", lazy(() -> globals.currentUser.get().getEffectiveGroups().getKnownGroups()));
}
private boolean isDelete(HttpServletRequest req) {
diff --git a/java/com/google/gerrit/server/ApprovalInference.java b/java/com/google/gerrit/server/ApprovalInference.java
index a4c9f2a..cb9f10d 100644
--- a/java/com/google/gerrit/server/ApprovalInference.java
+++ b/java/com/google/gerrit/server/ApprovalInference.java
@@ -20,6 +20,7 @@
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Table;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.entities.Account;
@@ -54,6 +55,8 @@
*/
@Singleton
public class ApprovalInference {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
private final ProjectCache projectCache;
private final ChangeKindCache changeKindCache;
private final LabelNormalizer labelNormalizer;
@@ -95,29 +98,175 @@
checkArgument(n != psId.get());
LabelType type = project.getLabelTypes().byLabel(psa.labelId());
if (type == null) {
+ logger.atFine().log(
+ "approval %d on label %s of patch set %d of change %d cannot be copied"
+ + " to patch set %d because the label no longer exists on project %s",
+ psa.value(),
+ psa.label(),
+ n,
+ psa.key().patchSetId().changeId().get(),
+ psId.get(),
+ project.getName());
return false;
- } else if ((type.isCopyMinScore() && type.isMaxNegative(psa))
- || (type.isCopyMaxScore() && type.isMaxPositive(psa))) {
+ } else if (type.isCopyMinScore() && type.isMaxNegative(psa)) {
+ logger.atFine().log(
+ "veto approval %s on label %s of patch set %d of change %d can be copied"
+ + " to patch set %d because the label has set copyMinScore = true on project %s",
+ psa.value(),
+ psa.label(),
+ n,
+ psa.key().patchSetId().changeId().get(),
+ psId.get(),
+ project.getName());
+ return true;
+ } else if (type.isCopyMaxScore() && type.isMaxPositive(psa)) {
+ logger.atFine().log(
+ "max approval %s on label %s of patch set %d of change %d can be copied"
+ + " to patch set %d because the label has set copyMaxScore = true on project %s",
+ psa.value(),
+ psa.label(),
+ n,
+ psa.key().patchSetId().changeId().get(),
+ psId.get(),
+ project.getName());
return true;
} else if (type.isCopyAnyScore()) {
+ logger.atFine().log(
+ "approval %d on label %s of patch set %d of change %d can be copied"
+ + " to patch set %d because the label has set copyAnyScore = true on project %s",
+ psa.value(),
+ psa.label(),
+ n,
+ psa.key().patchSetId().changeId().get(),
+ psId.get(),
+ project.getName());
return true;
} else if (type.getCopyValues().contains(psa.value())) {
+ logger.atFine().log(
+ "approval %d on label %s of patch set %d of change %d can be copied"
+ + " to patch set %d because the label has set copyValue = %d on project %s",
+ psa.value(),
+ psa.label(),
+ n,
+ psa.key().patchSetId().changeId().get(),
+ psId.get(),
+ psa.value(),
+ project.getName());
return true;
}
switch (kind) {
case MERGE_FIRST_PARENT_UPDATE:
- return type.isCopyAllScoresOnMergeFirstParentUpdate();
+ if (type.isCopyAllScoresOnMergeFirstParentUpdate()) {
+ logger.atFine().log(
+ "approval %d on label %s of patch set %d of change %d can be copied"
+ + " to patch set %d because change kind is %s and the label has set"
+ + " copyAllScoresOnMergeFirstParentUpdate = true on project %s",
+ psa.value(),
+ psa.label(),
+ n,
+ psa.key().patchSetId().changeId().get(),
+ psId.get(),
+ kind,
+ project.getName());
+ return true;
+ }
+ return false;
case NO_CODE_CHANGE:
- return type.isCopyAllScoresIfNoCodeChange();
+ if (type.isCopyAllScoresIfNoCodeChange()) {
+ logger.atFine().log(
+ "approval %d on label %s of patch set %d of change %d can be copied"
+ + " to patch set %d because change kind is %s and the label has set"
+ + " copyAllScoresIfNoCodeChange = true on project %s",
+ psa.value(),
+ psa.label(),
+ n,
+ psa.key().patchSetId().changeId().get(),
+ psId.get(),
+ kind,
+ project.getName());
+ return true;
+ }
+ return false;
case TRIVIAL_REBASE:
- return type.isCopyAllScoresOnTrivialRebase();
+ if (type.isCopyAllScoresOnTrivialRebase()) {
+ logger.atFine().log(
+ "approval %d on label %s of patch set %d of change %d can be copied"
+ + " to patch set %d because change kind is %s and the label has set"
+ + " copyAllScoresOnTrivialRebase = true on project %s",
+ psa.value(),
+ psa.label(),
+ n,
+ psa.key().patchSetId().changeId().get(),
+ psId.get(),
+ kind,
+ project.getName());
+ return true;
+ }
+ return false;
case NO_CHANGE:
- return type.isCopyAllScoresIfNoChange()
- || type.isCopyAllScoresOnTrivialRebase()
- || type.isCopyAllScoresOnMergeFirstParentUpdate()
- || type.isCopyAllScoresIfNoCodeChange();
+ if (type.isCopyAllScoresIfNoChange()) {
+ logger.atFine().log(
+ "approval %d on label %s of patch set %d of change %d can be copied"
+ + " to patch set %d because change kind is %s and the label has set"
+ + " copyAllScoresIfNoCodeChange = true on project %s",
+ psa.value(),
+ psa.label(),
+ n,
+ psa.key().patchSetId().changeId().get(),
+ psId.get(),
+ kind,
+ project.getName());
+ return true;
+ }
+ if (type.isCopyAllScoresOnTrivialRebase()) {
+ logger.atFine().log(
+ "approval %d on label %s of patch set %d of change %d can be copied"
+ + " to patch set %d because change kind is %s and the label has set"
+ + " copyAllScoresOnTrivialRebase = true on project %s",
+ psa.value(),
+ psa.label(),
+ n,
+ psa.key().patchSetId().changeId().get(),
+ psId.get(),
+ kind,
+ project.getName());
+ return true;
+ }
+ if (type.isCopyAllScoresOnMergeFirstParentUpdate()) {
+ logger.atFine().log(
+ "approval %d on label %s of patch set %d of change %d can be copied"
+ + " to patch set %d because change kind is %s and the label has set"
+ + " copyAllScoresOnMergeFirstParentUpdate = true on project %s",
+ psa.value(),
+ psa.label(),
+ n,
+ psa.key().patchSetId().changeId().get(),
+ psId.get(),
+ kind,
+ project.getName());
+ return true;
+ }
+ if (type.isCopyAllScoresIfNoCodeChange()) {
+ logger.atFine().log(
+ "approval %d on label %s of patch set %d of change %d can be copied"
+ + " to patch set %d because change kind is %s and the label has set"
+ + " copyAllScoresIfNoCodeChange = true on project %s",
+ psa.value(),
+ psa.label(),
+ n,
+ psa.key().patchSetId().changeId().get(),
+ psId.get(),
+ kind,
+ project.getName());
+ return true;
+ }
+ return false;
case REWORK:
default:
+ logger.atFine().log(
+ "approval %d on label %s of patch set %d of change %d cannot be copied"
+ + " to patch set %d because change kind is %s",
+ psa.value(), psa.label(), n, psa.key().patchSetId().changeId().get(), psId.get(), kind);
return false;
}
}
@@ -179,6 +328,9 @@
repoConfig,
priorPatchSet.getValue().commitId(),
ps.commitId());
+ logger.atFine().log(
+ "change kind for patch set %d of change %d against prior patch set %s is %s",
+ ps.id().get(), ps.id().changeId().get(), priorPatchSet.getValue().id().changeId(), kind);
for (PatchSetApproval psa : priorApprovals) {
if (resultByUser.contains(psa.label(), psa.accountId())) {
continue;
diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
index 032f08e..b5035d8 100644
--- a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
+++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.restapi.change;
import static com.google.common.base.MoreObjects.firstNonNull;
+import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
import static com.google.gerrit.server.permissions.RefPermission.CREATE_CHANGE;
import static java.util.Objects.requireNonNull;
@@ -38,6 +39,7 @@
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.ChangeMessagesUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CurrentUser;
@@ -96,7 +98,8 @@
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
-public class RevertSubmission implements RestModifyView<ChangeResource, RevertInput> {
+public class RevertSubmission
+ implements RestModifyView<ChangeResource, RevertInput>, UiAction<ChangeResource> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final Provider<InternalChangeQuery> queryProvider;
@@ -527,6 +530,40 @@
potentialCommitToReturn.getName(), changeNotes.getChange().getChangeId()));
}
+ @Override
+ public Description getDescription(ChangeResource rsrc) {
+ Change change = rsrc.getChange();
+ boolean projectStatePermitsWrite = false;
+ try {
+ projectStatePermitsWrite = projectCache.checkedGet(rsrc.getProject()).statePermitsWrite();
+ } catch (IOException e) {
+ logger.atSevere().withCause(e).log(
+ "Failed to check if project state permits write: %s", rsrc.getProject());
+ }
+ return new UiAction.Description()
+ .setLabel("Revert submission")
+ .setTitle(
+ "Revert this change and all changes that have been submitted together with this change")
+ .setVisible(
+ and(
+ change.isMerged()
+ && change.getSubmissionId() != null
+ && isChangePartOfSubmission(change.getSubmissionId())
+ && projectStatePermitsWrite,
+ permissionBackend
+ .user(rsrc.getUser())
+ .ref(change.getDest())
+ .testCond(CREATE_CHANGE)));
+ }
+
+ /**
+ * @param submissionId the submission id of the change.
+ * @return True if the submission has more than one change, false otherwise.
+ */
+ private Boolean isChangePartOfSubmission(String submissionId) {
+ return (queryProvider.get().setLimit(2).bySubmissionId(submissionId).size() > 1);
+ }
+
private class CreateCherryPickOp implements BatchUpdateOp {
private final ObjectId revCommitId;
private final String topic;
diff --git a/java/com/google/gerrit/server/update/RetryHelper.java b/java/com/google/gerrit/server/update/RetryHelper.java
index e13cab1..2db625b 100644
--- a/java/com/google/gerrit/server/update/RetryHelper.java
+++ b/java/com/google/gerrit/server/update/RetryHelper.java
@@ -482,7 +482,8 @@
String traceId = "retry-on-failure-" + new RequestId();
traceContext.addTag(RequestId.Type.TRACE_ID, traceId).forceLogging();
logger.atFine().withCause(t).log(
- "AutoRetry: %s failed, retry with tracing enabled", actionName);
+ "AutoRetry: %s failed, retry with tracing enabled (cause = %s)",
+ actionName, cause);
opts.onAutoTrace().ifPresent(c -> c.accept(traceId));
metrics.autoRetryCount.increment(actionType, actionName, cause);
return true;
@@ -492,7 +493,7 @@
// enabled and it failed again. Log the failure so that admin can see if it
// differs from the failure that triggered the retry.
logger.atFine().withCause(t).log(
- "AutoRetry: auto-retry of %s has failed", actionName);
+ "AutoRetry: auto-retry of %s has failed (cause = %s)", actionName, cause);
metrics.failuresOnAutoRetryCount.increment(actionType, actionName, cause);
return false;
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
index 28b8cae..911a04d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
@@ -78,6 +78,22 @@
gApi.changes().id(changeId).current().submit();
Map<String, ActionInfo> actions = getChangeActions(changeId);
assertThat(actions).containsKey("revert");
+ assertThat(actions).doesNotContainKey("revert_submission");
+ }
+
+ @Test
+ public void changeActionTwoMergedChangesHaveReverts() throws Exception {
+ String changeId1 = createChangeWithTopic().getChangeId();
+ String changeId2 = createChangeWithTopic().getChangeId();
+ gApi.changes().id(changeId1).current().review(ReviewInput.approve());
+ gApi.changes().id(changeId2).current().review(ReviewInput.approve());
+ gApi.changes().id(changeId2).current().submit();
+ Map<String, ActionInfo> actions1 = getChangeActions(changeId1);
+ assertThat(actions1).containsKey("revert");
+ assertThat(actions1).containsKey("revert_submission");
+ Map<String, ActionInfo> actions2 = getChangeActions(changeId2);
+ assertThat(actions2).containsKey("revert");
+ assertThat(actions2).containsKey("revert_submission");
}
@Test
diff --git a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html
index 495f8ab..97c792a 100644
--- a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html
+++ b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html
@@ -269,17 +269,17 @@
_describe(Shortcut.NEXT_UNREVIEWED_FILE, ShortcutSection.DIFFS,
'Mark file as reviewed and go to next unreviewed file');
- _describe(Shortcut.NEXT_FILE, ShortcutSection.NAVIGATION, 'Select next file');
+ _describe(Shortcut.NEXT_FILE, ShortcutSection.NAVIGATION, 'Go to next file');
_describe(Shortcut.PREV_FILE, ShortcutSection.NAVIGATION,
- 'Select previous file');
+ 'Go to previous file');
_describe(Shortcut.NEXT_FILE_WITH_COMMENTS, ShortcutSection.NAVIGATION,
- 'Select next file that has comments');
+ 'Go to next file that has comments');
_describe(Shortcut.PREV_FILE_WITH_COMMENTS, ShortcutSection.NAVIGATION,
- 'Select previous file that has comments');
+ 'Go to previous file that has comments');
_describe(Shortcut.OPEN_FIRST_FILE, ShortcutSection.NAVIGATION,
- 'Show first file');
+ 'Go to first file');
_describe(Shortcut.OPEN_LAST_FILE, ShortcutSection.NAVIGATION,
- 'Show last file');
+ 'Go to last file');
_describe(Shortcut.UP_TO_DASHBOARD, ShortcutSection.NAVIGATION,
'Up to dashboard');
_describe(Shortcut.UP_TO_CHANGE, ShortcutSection.NAVIGATION, 'Up to change');
@@ -350,6 +350,17 @@
return this.listeners.delete(listener);
}
+ getDescription(section, shortcutName) {
+ const binding =
+ _help.get(section).find(binding => binding.shortcut == shortcutName);
+ return binding ? binding.text : '';
+ }
+
+ getShortcut(shortcutName) {
+ const binding = this.bindings.get(shortcutName);
+ return binding ? this.describeBinding(binding) : '';
+ }
+
activeShortcutsBySection() {
const activeShortcuts = new Set();
this.activeHosts.forEach(shortcuts => {
@@ -478,6 +489,8 @@
GO_KEY: GO_KEY,
// eslint-disable-next-line object-shorthand
Shortcut: Shortcut,
+ // eslint-disable-next-line object-shorthand
+ ShortcutSection: ShortcutSection,
properties: {
_shortcut_go_key_last_pressed: {
@@ -511,6 +524,11 @@
for (let i = 0; e.path && i < e.path.length; i++) {
if (e.path[i].tagName === 'GR-OVERLAY') { return true; }
}
+
+ this.fire('shortcut-triggered', {
+ event: e,
+ goKey: this._inGoKeyMode(),
+ });
return false;
},
@@ -528,6 +546,12 @@
shortcutManager.bindShortcut(shortcut, ...bindings);
},
+ createTitle(shortcutName, section) {
+ const desc = shortcutManager.getDescription(section, shortcutName);
+ const shortcut = shortcutManager.getShortcut(shortcutName);
+ return (desc && shortcut) ? `${desc} (shortcut: ${shortcut})` : '';
+ },
+
_addOwnKeyBindings(shortcut, handler) {
const bindings = shortcutManager.getBindingsForShortcut(shortcut);
if (!bindings) {
@@ -591,10 +615,14 @@
this._shortcut_go_key_last_pressed = null;
},
+ _inGoKeyMode() {
+ return this._shortcut_go_key_last_pressed &&
+ (Date.now() - this._shortcut_go_key_last_pressed <=
+ GO_KEY_TIMEOUT_MS);
+ },
+
_handleGoAction(e) {
- if (!this._shortcut_go_key_last_pressed ||
- (Date.now() - this._shortcut_go_key_last_pressed >
- GO_KEY_TIMEOUT_MS) ||
+ if (!this._inGoKeyMode() ||
!this._shortcut_go_table.has(e.detail.key) ||
this.shouldSuppressKeyboardShortcut(e)) {
return;
@@ -632,6 +660,11 @@
*/
Gerrit.KeyboardShortcutMixin = base =>
class extends base {
+ get Shortcut() { return undefined; }
+
+ get ShortcutSection() { return undefined; }
+
+ createTitle(shortcutName, section) {}
};
}
})(window);
diff --git a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html
index ba143ec..f1bbcb0 100644
--- a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html
+++ b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html
@@ -189,7 +189,7 @@
mapToObject(mgr.activeShortcutsBySection()),
{
[NAVIGATION]: [
- {shortcut: NEXT_FILE, text: 'Select next file'},
+ {shortcut: NEXT_FILE, text: 'Go to next file'},
],
});
@@ -207,7 +207,7 @@
{shortcut: NEXT_LINE, text: 'Go to next line'},
],
[NAVIGATION]: [
- {shortcut: NEXT_FILE, text: 'Select next file'},
+ {shortcut: NEXT_FILE, text: 'Go to next file'},
],
});
@@ -233,7 +233,7 @@
},
],
[NAVIGATION]: [
- {shortcut: NEXT_FILE, text: 'Select next file'},
+ {shortcut: NEXT_FILE, text: 'Go to next file'},
],
});
});
@@ -286,7 +286,7 @@
{binding: [['g', 'o']], text: 'Go to Opened Changes'},
],
[NAVIGATION]: [
- {binding: [[']']], text: 'Select next file'},
+ {binding: [[']']], text: 'Go to next file'},
],
});
});
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
index 11e2217..c62aba2 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
@@ -441,6 +441,8 @@
<gr-button
id="replyBtn"
class="reply"
+ title="[[createTitle(Shortcut.OPEN_REPLY_DIALOG,
+ ShortcutSection.ACTIONS)]]"
hidden$="[[!_loggedIn]]"
primary
disabled="[[_replyDisabled]]"
@@ -637,6 +639,7 @@
threads="[[_commentThreads]]"
change="[[_change]]"
change-num="[[_changeNum]]"
+ comment-tab="[[_currentView]]"
logged-in="[[_loggedIn]]"
only-show-robot-comments-with-human-reply
on-thread-list-modified="_handleReloadDiffComments"></gr-thread-list>
@@ -654,6 +657,7 @@
change="[[_change]]"
change-num="[[_changeNum]]"
logged-in="[[_loggedIn]]"
+ comment-tab="[[_currentView]]"
hide-toggle-buttons
on-thread-list-modified="_handleReloadDiffComments"></gr-thread-list>
</template>
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-move-dialog/gr-confirm-move-dialog.js b/polygerrit-ui/app/elements/change/gr-confirm-move-dialog/gr-confirm-move-dialog.js
index feb3d2f..8932af7 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-move-dialog/gr-confirm-move-dialog.js
+++ b/polygerrit-ui/app/elements/change/gr-confirm-move-dialog/gr-confirm-move-dialog.js
@@ -21,10 +21,12 @@
/**
* @appliesMixin Gerrit.FireMixin
+ * @appliesMixin Gerrit.KeyboardShortcutMixin
* @extends Polymer.Element
*/
class GrConfirmMoveDialog extends Polymer.mixinBehaviors( [
Gerrit.FireBehavior,
+ Gerrit.KeyboardShortcutBehavior,
], Polymer.GestureEventListeners(
Polymer.LegacyElementMixin(
Polymer.Element))) {
@@ -55,6 +57,12 @@
};
}
+ get keyBindings() {
+ return {
+ 'ctrl+enter meta+enter': '_handleConfirmTap',
+ };
+ }
+
_handleConfirmTap(e) {
e.preventDefault();
e.stopPropagation();
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html
index 9146125..6fb6746 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html
@@ -223,6 +223,8 @@
<span class="downloadContainer desktop">
<gr-button link
class="download"
+ title="[[createTitle(Shortcut.OPEN_DOWNLOAD_DIALOG,
+ ShortcutSection.ACTIONS)]]"
on-click="_handleDownloadTap">Download</gr-button>
</span>
<span class$="includedInContainer [[_hideIncludedIn(change)]] desktop">
@@ -235,6 +237,8 @@
<gr-button
id="expandBtn"
link
+ title="[[createTitle(Shortcut.EXPAND_ALL_DIFF_CONTEXT,
+ ShortcutSection.DIFFS)]]"
on-click="_expandAllDiffs">Expand All</gr-button>
<gr-button
id="collapseBtn"
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js
index 5828006..eb7c1f9 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js
@@ -24,11 +24,13 @@
/**
* @appliesMixin Gerrit.FireMixin
* @appliesMixin Gerrit.PatchSetMixin
+ * @appliesMixin Gerrit.KeyboardShortcutMixin
* @extends Polymer.Element
*/
class GrFileListHeader extends Polymer.mixinBehaviors( [
Gerrit.FireBehavior,
Gerrit.PatchSetBehavior,
+ Gerrit.KeyboardShortcutBehavior,
], Polymer.GestureEventListeners(
Polymer.LegacyElementMixin(
Polymer.Element))) {
diff --git a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.html b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.html
index 29b83a3..8a8a9d5 100644
--- a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.html
+++ b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.html
@@ -23,7 +23,7 @@
<dom-module id="gr-label-scores">
<template>
<style include="shared-styles">
- :host {
+ .scoresTable {
display: table;
width: 100%;
}
@@ -42,15 +42,17 @@
display: var(--label-no-access-display, table-row);
}
</style>
- <template is="dom-repeat" items="[[_labels]]" as="label">
- <gr-label-score-row
- class$="[[_computeLabelAccessClass(label.name, permittedLabels)]]"
- label="[[label]]"
- name="[[label.name]]"
- labels="[[change.labels]]"
- permitted-labels="[[permittedLabels]]"
- label-values="[[_labelValues]]"></gr-label-score-row>
- </template>
+ <div class="scoresTable">
+ <template is="dom-repeat" items="[[_labels]]" as="label">
+ <gr-label-score-row
+ class$="[[_computeLabelAccessClass(label.name, permittedLabels)]]"
+ label="[[label]]"
+ name="[[label.name]]"
+ labels="[[change.labels]]"
+ permitted-labels="[[permittedLabels]]"
+ label-values="[[_labelValues]]"></gr-label-score-row>
+ </template>
+ </div>
<div class="mergedMessage"
hidden$="[[!_changeIsMerged(change.status)]]">
Because this change has been merged, votes may not be decreased.
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.html b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.html
index 8d76510..1e57693 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.html
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.html
@@ -17,6 +17,7 @@
<link rel="import" href="/bower_components/polymer/polymer.html">
<link rel="import" href="/bower_components/paper-toggle-button/paper-toggle-button.html">
+<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
<link rel="import" href="../../core/gr-reporting/gr-reporting.html">
<link rel="import" href="../../shared/gr-button/gr-button.html">
<link rel="import" href="../gr-message/gr-message.html">
@@ -84,6 +85,7 @@
<gr-button
id="collapse-messages"
link
+ title="[[_expandCollapseTitle]]"
on-click="_handleExpandCollapseTap">
[[_computeExpandCollapseMessage(_expanded)]]
</gr-button>
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js
index 20680b5..c637870 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js
@@ -25,10 +25,15 @@
SHOW_MORE: 'show-more-messages',
};
- /** @extends Polymer.Element */
- class GrMessagesList extends Polymer.GestureEventListeners(
+ /**
+ * @appliesMixin Gerrit.KeyboardShortcutMixin
+ * @extends Polymer.Element
+ */
+ class GrMessagesList extends Polymer.mixinBehaviors( [
+ Gerrit.KeyboardShortcutBehavior,
+ ], Polymer.GestureEventListeners(
Polymer.LegacyElementMixin(
- Polymer.Element)) {
+ Polymer.Element))) {
static get is() { return 'gr-messages-list'; }
static get properties() {
@@ -55,6 +60,11 @@
value: false,
observer: '_expandedChanged',
},
+
+ _expandCollapseTitle: {
+ type: String,
+ },
+
_hideAutomated: {
type: Boolean,
value: false,
@@ -174,6 +184,14 @@
this.notifyPath(`_visibleMessages.${i}.expanded`);
}
}
+
+ if (this._expanded) {
+ this._expandCollapseTitle = this.createTitle(
+ this.Shortcut.COLLAPSE_ALL_MESSAGES, this.ShortcutSection.ACTIONS);
+ } else {
+ this._expandCollapseTitle = this.createTitle(
+ this.Shortcut.EXPAND_ALL_MESSAGES, this.ShortcutSection.ACTIONS);
+ }
}
_highlightEl(el) {
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.html b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.html
index a096aec..eacc0d0 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.html
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.html
@@ -76,7 +76,7 @@
</template>
<div id="threads">
<template is="dom-if" if="[[!threads.length]]">
- There are no inline comment threads on any diff for this change.
+ [[_computeNoThreadsMessage(commentTab)]]
</template>
<template
is="dom-repeat"
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js
index e449fad..d8c7b61 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js
@@ -23,6 +23,17 @@
* @event thread-list-modified
* @extends Polymer.Element
*/
+ const NO_THREADS_MESSAGE = 'There are no inline comment threads on any diff '
+ + 'for this change.';
+ const NO_ROBOT_COMMENTS_THREADS_MESSAGE = 'There are no findings for this ' +
+ 'patchset.';
+
+ const CommentTabs = {
+ CHANGE_LOG: 0,
+ COMMENT_THREADS: 1,
+ ROBOT_COMMENTS: 2,
+ };
+
class GrThreadList extends Polymer.GestureEventListeners(
Polymer.LegacyElementMixin(
Polymer.Element)) {
@@ -62,6 +73,7 @@
type: Boolean,
value: false,
},
+ commentTab: Number,
};
}
@@ -71,6 +83,13 @@
return loggedIn ? 'show' : '';
}
+ _computeNoThreadsMessage(commentTab) {
+ if (commentTab === CommentTabs.ROBOT_COMMENTS) {
+ return NO_ROBOT_COMMENTS_THREADS_MESSAGE;
+ }
+ return NO_THREADS_MESSAGE;
+ }
+
/**
* Order as follows:
* - Unresolved threads with drafts (reverse chronological)
diff --git a/polygerrit-ui/app/elements/core/gr-error-dialog/gr-error-dialog.html b/polygerrit-ui/app/elements/core/gr-error-dialog/gr-error-dialog.html
index 09b928e..ffd7f896 100644
--- a/polygerrit-ui/app/elements/core/gr-error-dialog/gr-error-dialog.html
+++ b/polygerrit-ui/app/elements/core/gr-error-dialog/gr-error-dialog.html
@@ -34,6 +34,9 @@
max-width: 50em;
}
}
+ .signInLink {
+ text-decoration: none;
+ }
</style>
<gr-dialog
id="dialog"
@@ -43,6 +46,14 @@
confirm-on-enter>
<div class="header" slot="header">An error occurred</div>
<div class="main" slot="main">[[text]]</div>
+ <gr-button
+ id="signIn"
+ class$="signInLink"
+ hidden$="[[!showSignInButton]]"
+ link
+ slot="footer">
+ <a href$="[[loginUrl]]" class="signInLink">Sign in</a>
+ </gr-button>
</gr-dialog>
</template>
<script src="gr-error-dialog.js"></script>
diff --git a/polygerrit-ui/app/elements/core/gr-error-dialog/gr-error-dialog.js b/polygerrit-ui/app/elements/core/gr-error-dialog/gr-error-dialog.js
index db70d57..63339c9 100644
--- a/polygerrit-ui/app/elements/core/gr-error-dialog/gr-error-dialog.js
+++ b/polygerrit-ui/app/elements/core/gr-error-dialog/gr-error-dialog.js
@@ -31,6 +31,20 @@
static get properties() {
return {
text: String,
+ /**
+ * loginUrl to open on "sign in" button click
+ */
+ loginUrl: {
+ type: String,
+ value: '/login',
+ },
+ /**
+ * Show/hide "Sign In" button in dialog
+ */
+ showSignInButton: {
+ type: Boolean,
+ value: false,
+ },
};
}
diff --git a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.html b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.html
index 03d8d6a..104d5b0 100644
--- a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.html
+++ b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.html
@@ -34,7 +34,9 @@
id="errorDialog"
on-dismiss="_handleDismissErrorDialog"
confirm-label="Dismiss"
- confirm-on-enter></gr-error-dialog>
+ confirm-on-enter
+ login-url="[[loginUrl]]"
+ ></gr-error-dialog>
</gr-overlay>
<gr-overlay
id="noInteractionOverlay"
diff --git a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.js b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.js
index 5a7e58d..b828774 100644
--- a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.js
+++ b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.js
@@ -62,6 +62,11 @@
type: Number,
value() { return Date.now(); },
},
+
+ loginUrl: {
+ type: String,
+ value: '/login',
+ },
};
}
@@ -139,23 +144,60 @@
this._authService.clearCache();
this.$.restAPI.getLoggedIn();
} else if (!this._shouldSuppressError(errorText)) {
- this._showErrorDialog(this._constructServerErrorMsg({
- status,
- statusText,
- errorText,
- url,
- }));
+ const trace =
+ response.headers && response.headers.get('X-Gerrit-Trace');
+ if (response.status === 404) {
+ this._showNotFoundMessageWithTip({
+ status,
+ statusText,
+ errorText,
+ url,
+ trace,
+ });
+ } else {
+ this._showErrorDialog(this._constructServerErrorMsg({
+ status,
+ statusText,
+ errorText,
+ url,
+ trace,
+ }));
+ }
}
console.log(`server error: ${errorText}`);
});
}
- _constructServerErrorMsg({errorText, status, statusText, url}) {
- let err = `Error ${status}`;
+ _showNotFoundMessageWithTip({status, statusText, errorText, url, trace}) {
+ this.$.restAPI.getLoggedIn().then(isLoggedIn => {
+ const tip = isLoggedIn ?
+ 'You might have not enough privileges.' :
+ 'You might have not enough privileges. Sign in and try again.';
+ this._showErrorDialog(this._constructServerErrorMsg({
+ status,
+ statusText,
+ errorText,
+ url,
+ trace,
+ tip,
+ }), {
+ showSignInButton: !isLoggedIn,
+ });
+ });
+ return;
+ }
+
+ _constructServerErrorMsg({errorText, status, statusText, url, trace, tip}) {
+ let err = '';
+ if (tip) {
+ err += `${tip}\n\n`;
+ }
+ err += `Error ${status}`;
if (statusText) { err += ` (${statusText})`; }
if (errorText || url) { err += ': '; }
if (errorText) { err += errorText; }
if (url) { err += `\nEndpoint: ${url}`; }
+ if (trace) { err += `\nTrace Id: ${trace}`; }
return err;
}
@@ -342,12 +384,14 @@
this.$.errorOverlay.close();
}
- _showErrorDialog(message) {
+ _showErrorDialog(message, opt_options) {
this.$.reporting.reportErrorDialog(message);
this.$.errorDialog.text = message;
+ this.$.errorDialog.showSignInButton =
+ opt_options && opt_options.showSignInButton;
this.$.errorOverlay.open();
}
}
customElements.define(GrErrorManager.is, GrErrorManager);
-})();
\ No newline at end of file
+})();
diff --git a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html
index 4f9b103..8296804 100644
--- a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html
+++ b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html
@@ -140,6 +140,36 @@
url,
}), 'Error 409 (Conflict): change conflicts' +
'\nEndpoint: /my/test/url');
+ assert.equal(element._constructServerErrorMsg({
+ status,
+ statusText,
+ errorText,
+ url,
+ trace: 'xxxxx',
+ }), 'Error 409 (Conflict): change conflicts' +
+ '\nEndpoint: /my/test/url\nTrace Id: xxxxx');
+ });
+
+ test('extract trace id from headers if exists', done => {
+ const textSpy = sandbox.spy(
+ () => Promise.resolve('500')
+ );
+ const headers = new Headers();
+ headers.set('X-Gerrit-Trace', 'xxxx');
+ element.fire('server-error', {
+ response: {
+ headers,
+ status: 500,
+ text: textSpy,
+ },
+ });
+ flush(() => {
+ assert.equal(
+ element.$.errorDialog.text,
+ 'Error 500: 500\nTrace Id: xxxx'
+ );
+ done();
+ });
});
test('suppress TOO_MANY_FILES error', done => {
diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.html b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.html
index e208148..c438047 100644
--- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.html
+++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.html
@@ -220,7 +220,7 @@
[[_registerText]]
</a>
</div>
- <a class="loginButton" href$="[[_loginURL]]">Sign in</a>
+ <a class="loginButton" href$="[[loginUrl]]">Sign in</a>
<a
class="settingsButton"
href$="[[_generateSettingsLink()]]"
diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js
index 53c5ef5..05765fb 100644
--- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js
+++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js
@@ -122,7 +122,7 @@
computed: '_computeLinks(_defaultLinks, _userLinks, _adminLinks, ' +
'_topMenus, _docBaseUrl)',
},
- _loginURL: {
+ loginUrl: {
type: String,
value: '/login',
},
@@ -162,36 +162,17 @@
super.attached();
this._loadAccount();
this._loadConfig();
- this.listen(window, 'location-change', '_handleLocationChange');
}
/** @override */
detached() {
super.detached();
- this.unlisten(window, 'location-change', '_handleLocationChange');
}
reload() {
this._loadAccount();
}
- _handleLocationChange(e) {
- const baseUrl = this.getBaseUrl();
- if (baseUrl) {
- // Strip the canonical path from the path since needing canonical in
- // the path is uneeded and breaks the url.
- this._loginURL = baseUrl + '/login/' + encodeURIComponent(
- '/' + window.location.pathname.substring(baseUrl.length) +
- window.location.search +
- window.location.hash);
- } else {
- this._loginURL = '/login/' + encodeURIComponent(
- window.location.pathname +
- window.location.search +
- window.location.hash);
- }
- }
-
_computeRelativeURL(path) {
return '//' + window.location.host + this.getBaseUrl() + path;
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
index 42fc4a6..6c20a6a 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
@@ -240,14 +240,20 @@
<span class="separator"></span>
</span>
<a class="navLink"
+ title="[[createTitle(Shortcut.PREV_FILE,
+ ShortcutSection.NAVIGATION)]]"
href$="[[_computeNavLinkURL(_change, _path, _fileList, -1, 1)]]">
Prev</a>
<span class="separator"></span>
<a class="navLink"
+ title="[[createTitle(Shortcut.UP_TO_CHANGE,
+ ShortcutSection.NAVIGATION)]]"
href$="[[_computeChangePath(_change, _patchRange.*, _change.revisions)]]">
Up</a>
<span class="separator"></span>
<a class="navLink"
+ title="[[createTitle(Shortcut.NEXT_FILE,
+ ShortcutSection.NAVIGATION)]]"
href$="[[_computeNavLinkURL(_change, _path, _fileList, 1, 1)]]">
Next</a>
</div>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
index cb89c0c..e91c8b0 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
@@ -117,14 +117,24 @@
// element for selected a file to view.
_formattedFiles: {
type: Array,
- computed: '_formatFilesForDropdown(_fileList, ' +
+ computed: '_formatFilesForDropdown(_files, ' +
'_patchRange.patchNum, _changeComments)',
},
// An sorted array of files, as returned by the rest API.
_fileList: {
type: Array,
- value() { return []; },
+ computed: '_getSortedFileList(_files)',
},
+ /**
+ * Contains information about files as returned by the rest API.
+ *
+ * @type {{ sortedFileList: Array<string>, changeFilesByPath: Object }}
+ */
+ _files: {
+ type: Object,
+ value() { return {sortedFileList: [], changeFilesByPath: {}}; },
+ },
+
_path: {
type: String,
observer: '_pathChanged',
@@ -212,7 +222,7 @@
static get observers() {
return [
'_getProjectConfig(_change.project)',
- '_getFiles(_changeNum, _patchRange.*)',
+ '_getFiles(_changeNum, _patchRange.*, _changeComments)',
'_setReviewedObserver(_loggedIn, params.*, _prefs)',
];
}
@@ -293,23 +303,31 @@
return this.$.restAPI.getChangeEdit(this._changeNum);
}
- _getFiles(changeNum, patchRangeRecord) {
+ _getSortedFileList(files) {
+ return files.sortedFileList;
+ }
+
+ _getFiles(changeNum, patchRangeRecord, changeComments) {
// Polymer 2: check for undefined
- if ([changeNum, patchRangeRecord, patchRangeRecord.base]
+ if ([changeNum, patchRangeRecord, patchRangeRecord.base, changeComments]
.some(arg => arg === undefined)) {
- return;
+ return Promise.resolve();
}
const patchRange = patchRangeRecord.base;
- return this.$.restAPI.getChangeFilePathsAsSpeciallySortedArray(
- changeNum, patchRange).then(files => {
- this._fileList = files;
-
- // in case current file is not in changed files
- // (file has no change but has comments)
- if (this._path && !this._fileList.includes(this._path)) {
- this._fileList.push(this._path);
- }
+ return this.$.restAPI.getChangeFiles(
+ changeNum, patchRange).then(changeFiles => {
+ if (!changeFiles) return;
+ const commentedPaths = changeComments.getPaths(patchRange);
+ const files = Object.assign({}, changeFiles);
+ Object.keys(commentedPaths).forEach(commentedPath => {
+ if (files.hasOwnProperty(commentedPath)) { return; }
+ files[commentedPath] = {status: 'U'};
+ });
+ this._files = {
+ sortedFileList: Object.keys(files).sort(this.specialFilePathCompare),
+ changeFilesByPath: files,
+ };
});
}
@@ -852,31 +870,31 @@
return this._getChangePath(change, patchRangeRecord.base, revisions);
}
- _formatFilesForDropdown(fileList, patchNum, changeComments) {
+ _formatFilesForDropdown(files, patchNum, changeComments) {
// Polymer 2: check for undefined
if ([
- fileList,
+ files,
patchNum,
changeComments,
].some(arg => arg === undefined)) {
return;
}
- if (!fileList) { return; }
+ if (!files) { return; }
const dropdownContent = [];
- for (const path of fileList) {
+ for (const path of files.sortedFileList) {
dropdownContent.push({
text: this.computeDisplayPath(path),
mobileText: this.computeTruncatedPath(path),
value: path,
bottomText: this._computeCommentString(changeComments, patchNum,
- path),
+ path, files.changeFilesByPath[path]),
});
}
return dropdownContent;
}
- _computeCommentString(changeComments, patchNum, path) {
+ _computeCommentString(changeComments, patchNum, path, changeFileInfo) {
const unresolvedCount = changeComments.computeUnresolvedNum(patchNum,
path);
const commentCount = changeComments.computeCommentCount(patchNum, path);
@@ -885,11 +903,13 @@
const unresolvedString = GrCountStringFormatter.computeString(
unresolvedCount, 'unresolved');
- return commentString +
- // Add a space if both comments and unresolved
- (commentString && unresolvedString ? ', ' : '') +
- // Add parentheses around unresolved if it exists.
- (unresolvedString ? `${unresolvedString}` : '');
+ const unmodifiedString = changeFileInfo.status === 'U' ? 'no changes': '';
+
+ return [
+ unmodifiedString,
+ commentString,
+ unresolvedString]
+ .filter(v => v && v.length > 0).join(', ');
}
_computePrefsButtonHidden(prefs, prefsDisabled) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
index bd172a5..1c32407 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
@@ -76,6 +76,17 @@
const PARENT = 'PARENT';
+ function getFilesFromFileList(fileList) {
+ const changeFilesByPath = fileList.reduce((files, path) => {
+ files[path] = {};
+ return files;
+ }, {});
+ return {
+ sortedFileList: fileList,
+ changeFilesByPath,
+ };
+ }
+
setup(() => {
sandbox = sinon.sandbox.create();
@@ -135,7 +146,8 @@
a: {_number: 10, commit: {parents: []}},
},
};
- element._fileList = ['chell.go', 'glados.txt', 'wheatley.md'];
+ element._files = getFilesFromFileList(
+ ['chell.go', 'glados.txt', 'wheatley.md']);
element._path = 'glados.txt';
element.changeViewState.selectedFileIndex = 1;
element._loggedIn = true;
@@ -242,7 +254,8 @@
b: {_number: 5, commit: {parents: []}},
},
};
- element._fileList = ['chell.go', 'glados.txt', 'wheatley.md'];
+ element._files = getFilesFromFileList(
+ ['chell.go', 'glados.txt', 'wheatley.md']);
element._path = 'glados.txt';
const diffNavStub = sandbox.stub(Gerrit.Nav, 'navigateToDiff');
@@ -309,7 +322,8 @@
b: {_number: 2, commit: {parents: []}},
},
};
- element._fileList = ['chell.go', 'glados.txt', 'wheatley.md'];
+ element._files = getFilesFromFileList(
+ ['chell.go', 'glados.txt', 'wheatley.md']);
element._path = 'glados.txt';
const diffNavStub = sandbox.stub(Gerrit.Nav, 'navigateToDiff');
@@ -422,34 +436,26 @@
unresolvedCountStub.withArgs(3, path).returns(2);
unresolvedCountStub.withArgs(4, path).returns(0);
- assert.equal(element._computeCommentString(comments, 1, path),
+ assert.equal(element._computeCommentString(comments, 1, path, {}),
'1 unresolved');
- assert.equal(element._computeCommentString(comments, 2, path),
+ assert.equal(
+ element._computeCommentString(comments, 2, path, {status: 'M'}),
'1 comment');
- assert.equal(element._computeCommentString(comments, 3, path),
+ assert.equal(
+ element._computeCommentString(comments, 2, path, {status: 'U'}),
+ 'no changes, 1 comment');
+ assert.equal(
+ element._computeCommentString(comments, 3, path, {status: 'A'}),
'2 comments, 2 unresolved');
- assert.equal(element._computeCommentString(comments, 4, path), '');
+ assert.equal(
+ element._computeCommentString(comments, 4, path, {status: 'M'}), '');
+ assert.equal(
+ element._computeCommentString(comments, 4, path, {status: 'U'}),
+ 'no changes');
done();
});
});
- suite('unchanged file', () => {
- test('unchanged file should be included in _fileList', done => {
- element._path = 'aaa.txt';
- // trigger reload on files
- element._changeNum = '123';
- element._patchRange = {
- basePatchNum: PARENT,
- patchNum: '1',
- };
- assert.isFalse(element._fileList.includes('aaa.txt'));
- flush(() => {
- assert.isTrue(element._fileList.includes('aaa.txt'));
- done();
- });
- });
- });
-
suite('url params', () => {
setup(() => {
sandbox.stub(
@@ -469,8 +475,9 @@
patchNum: '10',
};
element._change = {_number: 42};
- element._fileList = ['chell.go', 'glados.txt', 'wheatley.md',
- '/COMMIT_MSG', '/MERGE_LIST'];
+ element._files = getFilesFromFileList(
+ ['chell.go', 'glados.txt', 'wheatley.md',
+ '/COMMIT_MSG', '/MERGE_LIST']);
element._path = 'glados.txt';
const expectedFormattedFiles = [
{
@@ -519,7 +526,8 @@
a: {_number: 10, commit: {parents: []}},
},
};
- element._fileList = ['chell.go', 'glados.txt', 'wheatley.md'];
+ element._files = getFilesFromFileList(
+ ['chell.go', 'glados.txt', 'wheatley.md']);
element._path = 'glados.txt';
flushAsynchronousOperations();
const linkEls = Polymer.dom(element.root).querySelectorAll('.navLink');
@@ -561,7 +569,8 @@
b: {_number: 10, commit: {parents: []}},
},
};
- element._fileList = ['chell.go', 'glados.txt', 'wheatley.md'];
+ element._files = getFilesFromFileList(
+ ['chell.go', 'glados.txt', 'wheatley.md']);
element._path = 'glados.txt';
flushAsynchronousOperations();
const linkEls = Polymer.dom(element.root).querySelectorAll('.navLink');
@@ -988,9 +997,9 @@
setup(() => {
navToChangeStub = sandbox.stub(element, '_navToChangeView');
navToDiffStub = sandbox.stub(Gerrit.Nav, 'navigateToDiff');
- element._fileList = [
+ element._files = getFilesFromFileList([
'path/one.jpg', 'path/two.m4v', 'path/three.wav',
- ];
+ ]);
element._patchRange = {patchNum: '2', basePatchNum: '1'};
});
@@ -1141,7 +1150,7 @@
});
test('shift+m navigates to next unreviewed file', () => {
- element._fileList = ['file1', 'file2', 'file3'];
+ element._files = getFilesFromFileList(['file1', 'file2', 'file3']);
element._reviewedFiles = new Set(['file1', 'file2']);
element._path = 'file1';
const reviewedStub = sandbox.stub(element, '_setReviewed');
@@ -1158,7 +1167,7 @@
});
test('File change should trigger navigateToDiff once', () => {
- element._fileList = ['file1', 'file2', 'file3'];
+ element._files = getFilesFromFileList(['file1', 'file2', 'file3']);
sandbox.stub(element, '_getLineOfInterest');
sandbox.stub(element, '_initCursor');
sandbox.stub(Gerrit.Nav, 'navigateToDiff');
@@ -1285,4 +1294,54 @@
'/changes/test~12/revisions/1/patch?zip&path=index.php');
});
});
+
+ suite('gr-diff-view tests unmodified files with comments', () => {
+ setup(() => {
+ sandbox = sinon.sandbox.create();
+ const changedFiles = {
+ 'file1.txt': {},
+ 'a/b/test.c': {},
+ };
+ stub('gr-rest-api-interface', {
+ getConfig() { return Promise.resolve({change: {}}); },
+ getLoggedIn() { return Promise.resolve(false); },
+ getProjectConfig() { return Promise.resolve({}); },
+ getDiffChangeDetail() { return Promise.resolve({}); },
+ getChangeFiles() { return Promise.resolve(changedFiles); },
+ saveFileReviewed() { return Promise.resolve(); },
+ getDiffComments() { return Promise.resolve({}); },
+ getDiffRobotComments() { return Promise.resolve({}); },
+ getDiffDrafts() { return Promise.resolve({}); },
+ getReviewedFiles() { return Promise.resolve([]); },
+ });
+ element = fixture('basic');
+ return element._loadComments();
+ });
+
+ teardown(() => {
+ sandbox.restore();
+ });
+
+ test('_getFiles add files with comments without changes', () => {
+ const patchChangeRecord = {
+ base: {
+ basePatchNum: '5',
+ patchNum: '10',
+ },
+ };
+ const changeComments = {
+ getPaths: sandbox.stub().returns({'file2.txt': {}, 'file1.txt': {}}),
+ };
+ return element._getFiles(23, patchChangeRecord, changeComments).then(() => {
+ assert.deepEqual(element._files, {
+ sortedFileList: ['a/b/test.c', 'file1.txt', 'file2.txt'],
+ changeFilesByPath: {
+ 'file1.txt': {},
+ 'file2.txt': {status: 'U'},
+ 'a/b/test.c': {},
+ },
+ });
+ });
+ });
+ });
</script>
diff --git a/polygerrit-ui/app/elements/gr-app-element.html b/polygerrit-ui/app/elements/gr-app-element.html
index f758280..6c334b5 100644
--- a/polygerrit-ui/app/elements/gr-app-element.html
+++ b/polygerrit-ui/app/elements/gr-app-element.html
@@ -126,7 +126,9 @@
<gr-main-header
id="mainHeader"
search-query="{{params.query}}"
- on-mobile-search="_mobileSearchToggle">
+ on-mobile-search="_mobileSearchToggle"
+ login-url="[[_loginUrl]]"
+ >
</gr-main-header>
</gr-fixed-panel>
<main>
@@ -222,7 +224,7 @@
</gr-registration-dialog>
</gr-overlay>
<gr-endpoint-decorator name="plugin-overlay"></gr-endpoint-decorator>
- <gr-error-manager id="errorManager"></gr-error-manager>
+ <gr-error-manager id="errorManager" login-url="[[_loginUrl]]"></gr-error-manager>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
<gr-reporting id="reporting"></gr-reporting>
<gr-router id="router"></gr-router>
diff --git a/polygerrit-ui/app/elements/gr-app-element.js b/polygerrit-ui/app/elements/gr-app-element.js
index c1e317a..7e62b9d 100644
--- a/polygerrit-ui/app/elements/gr-app-element.js
+++ b/polygerrit-ui/app/elements/gr-app-element.js
@@ -94,6 +94,15 @@
type: Boolean,
value: false,
},
+
+ /**
+ * Other elements in app must open this URL when
+ * user login is required.
+ */
+ _loginUrl: {
+ type: String,
+ value: '/login',
+ },
};
}
@@ -127,11 +136,14 @@
e => this._handleLocationChange(e));
this.addEventListener('rpc-log',
e => this._handleRpcLog(e));
+ this.addEventListener('shortcut-triggered',
+ e => this._handleShortcutTriggered(e));
}
/** @override */
ready() {
super.ready();
+ this._updateLoginUrl();
this.$.reporting.appStarted();
this.$.router.start();
@@ -356,6 +368,22 @@
this.$.header.unfloat();
}
+ _handleShortcutTriggered(event) {
+ const {event: e, goKey} = event.detail;
+ // eg: {key: "k:keydown", ..., from: "gr-diff-view"}
+ let key = `${e.key}:${e.type}`;
+ if (goKey) key = 'g+' + key;
+ if (e.shiftKey) key = 'shift+' + key;
+ if (e.ctrlKey) key = 'ctrl+' + key;
+ if (e.metaKey) key = 'meta+' + key;
+ if (e.altKey) key = 'alt+' + key;
+ this.$.reporting.reportInteraction('shortcut-triggered', {
+ key,
+ from: event.path && event.path[0]
+ && event.path[0].nodeName || 'unknown',
+ });
+ }
+
_handlePageError(e) {
const props = [
'_showChangeListView',
@@ -385,6 +413,8 @@
}
_handleLocationChange(e) {
+ this._updateLoginUrl();
+
const hash = e.detail.hash.substring(1);
let pathname = e.detail.pathname;
if (pathname.startsWith('/c/') && parseInt(hash, 10) > 0) {
@@ -393,6 +423,23 @@
this.set('_path', pathname);
}
+ _updateLoginUrl() {
+ const baseUrl = this.getBaseUrl();
+ if (baseUrl) {
+ // Strip the canonical path from the path since needing canonical in
+ // the path is uneeded and breaks the url.
+ this._loginUrl = baseUrl + '/login/' + encodeURIComponent(
+ '/' + window.location.pathname.substring(baseUrl.length) +
+ window.location.search +
+ window.location.hash);
+ } else {
+ this._loginUrl = '/login/' + encodeURIComponent(
+ window.location.pathname +
+ window.location.search +
+ window.location.hash);
+ }
+ }
+
_paramsChanged(paramsRecord) {
const params = paramsRecord.base;
const viewsToCheck = [Gerrit.Nav.View.SEARCH, Gerrit.Nav.View.DASHBOARD];
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.html b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.html
index 333ca9d..31de1ce 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.html
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.html
@@ -88,7 +88,7 @@
span.date:hover {
text-decoration: underline;
}
- .actions {
+ .actions, .robotActions {
display: flex;
justify-content: flex-end;
padding-top: 0;
@@ -96,17 +96,6 @@
.action {
margin-left: var(--spacing-l);
}
- .robotActions {
- display: flex;
- justify-content: flex-start;
- padding-top: var(--spacing-m);
- border-top: 1px solid var(--border-color);
- }
- .robotActions .action {
- /* Keep button text lined up with output text */
- margin-left: -4px;
- margin-right: var(--spacing-l);
- }
.rightActions {
display: flex;
justify-content: flex-end;
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
index ce26bf6..f021bf0 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -1172,19 +1172,6 @@
return this.getChangeFiles(changeNum, patchRange);
}
- /**
- * The closure compiler doesn't realize this.specialFilePathCompare is
- * valid.
- *
- * @suppress {checkTypes}
- */
- getChangeFilePathsAsSpeciallySortedArray(changeNum, patchRange) {
- return this.getChangeFiles(changeNum, patchRange).then(files => {
- if (!files) return;
- return Object.keys(files).sort(this.specialFilePathCompare);
- });
- }
-
getChangeRevisionActions(changeNum, patchNum) {
const req = {
changeNum,
@@ -2770,4 +2757,4 @@
}
customElements.define(GrRestApiInterface.is, GrRestApiInterface);
-})();
\ No newline at end of file
+})();
diff --git a/tools/BUILD b/tools/BUILD
index 2d702fe..7f890b1 100644
--- a/tools/BUILD
+++ b/tools/BUILD
@@ -86,8 +86,8 @@
"-Xep:TypeParameterShadowing:ERROR",
"-Xep:TypeParameterUnusedInFormals:ERROR",
"-Xep:URLEqualsHashCode:ERROR",
- "-Xep:UnusedException:ERROR",
"-Xep:UnsynchronizedOverridesSynchronized:ERROR",
+ "-Xep:UnusedException:ERROR",
"-Xep:WaitNotInLoop:ERROR",
"-Xep:WildcardImport:ERROR",
],
