Merge "Refactor keys transformation in comment context"
diff --git a/java/com/google/gerrit/server/approval/ApprovalInference.java b/java/com/google/gerrit/server/approval/ApprovalInference.java
index 2466b4f..1efbd37 100644
--- a/java/com/google/gerrit/server/approval/ApprovalInference.java
+++ b/java/com/google/gerrit/server/approval/ApprovalInference.java
@@ -30,7 +30,6 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.client.ChangeKind;
-import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.server.change.ChangeKindCache;
import com.google.gerrit.server.change.LabelNormalizer;
@@ -40,7 +39,6 @@
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffOperations;
-import com.google.gerrit.server.patch.PatchListKey;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
@@ -433,11 +431,6 @@
*/
private Map<String, FileDiffOutput> listModifiedFiles(
ProjectState project, PatchSet ps, Map.Entry<PatchSet.Id, PatchSet> priorPatchSet) {
- PatchListKey key =
- PatchListKey.againstCommit(
- priorPatchSet.getValue().commitId(),
- ps.commitId(),
- DiffPreferencesInfo.Whitespace.IGNORE_NONE);
try {
return diffOperations.listModifiedFiles(
project.getNameKey(), priorPatchSet.getValue().commitId(), ps.commitId());
diff --git a/java/com/google/gerrit/server/change/FileInfoJsonModule.java b/java/com/google/gerrit/server/change/FileInfoJsonModule.java
index f90bd14..de116bb 100644
--- a/java/com/google/gerrit/server/change/FileInfoJsonModule.java
+++ b/java/com/google/gerrit/server/change/FileInfoJsonModule.java
@@ -14,12 +14,31 @@
package com.google.gerrit.server.change;
+import com.google.gerrit.server.config.GerritServerConfig;
import com.google.inject.AbstractModule;
+import org.eclipse.jgit.lib.Config;
public class FileInfoJsonModule extends AbstractModule {
+ /** Use the new diff cache implementation {@link FileInfoJsonNewImpl}. */
+ private final boolean useNewDiffCache;
+
+ /** Used to dark launch the new diff cache with the list files endpoint. */
+ private final boolean runNewDiffCacheAsync;
+
+ public FileInfoJsonModule(@GerritServerConfig Config cfg) {
+ this.useNewDiffCache =
+ cfg.getBoolean("cache", "diff_cache", "runNewDiffCache_ListFiles", false);
+ this.runNewDiffCacheAsync =
+ cfg.getBoolean("cache", "diff_cache", "runNewDiffCacheAsync_listFiles", false);
+ }
@Override
public void configure() {
- bind(FileInfoJson.class).to(FileInfoJsonComparingImpl.class);
+ if (runNewDiffCacheAsync) {
+ bind(FileInfoJson.class).to(FileInfoJsonComparingImpl.class);
+ return;
+ }
+ bind(FileInfoJson.class)
+ .to(useNewDiffCache ? FileInfoJsonNewImpl.class : FileInfoJsonOldImpl.class);
}
}
diff --git a/java/com/google/gerrit/server/change/IncludedInResolver.java b/java/com/google/gerrit/server/change/IncludedInResolver.java
index 99a89bf..9216964 100644
--- a/java/com/google/gerrit/server/change/IncludedInResolver.java
+++ b/java/com/google/gerrit/server/change/IncludedInResolver.java
@@ -26,7 +26,6 @@
import com.google.common.collect.Lists;
import com.google.common.flogger.FluentLogger;
import java.io.IOException;
-import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
@@ -90,17 +89,6 @@
getMatchingRefNames(allMatchingTagsAndBranches, tags));
}
- private boolean includedInOne(Collection<Ref> refs) throws IOException {
- parseCommits(refs);
- List<RevCommit> before = new ArrayList<>();
- List<RevCommit> after = new ArrayList<>();
- partition(before, after);
- rw.reset();
- // It is highly likely that the target is reachable from the "after" set
- // Within the "before" set we are trying to handle cases arising from clock skew
- return !includedIn(after, 1).isEmpty() || !includedIn(before, 1).isEmpty();
- }
-
/** Resolves which tip refs include the target commit. */
private Set<String> includedIn(Collection<RevCommit> tips, int limit)
throws IOException, MissingObjectException, IncorrectObjectTypeException {
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 076ba46..5a74c78 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -271,7 +271,7 @@
install(new IgnoreSelfApprovalRule.Module());
install(new ReceiveCommitsModule());
install(new SshAddressesModule());
- install(new FileInfoJsonModule());
+ install(new FileInfoJsonModule(cfg));
install(ThreadLocalRequestContext.module());
install(new ApprovalModule());
diff --git a/java/com/google/gerrit/server/project/Reachable.java b/java/com/google/gerrit/server/project/Reachable.java
index 2bf9e54..2adebe7 100644
--- a/java/com/google/gerrit/server/project/Reachable.java
+++ b/java/com/google/gerrit/server/project/Reachable.java
@@ -31,6 +31,8 @@
import java.util.Collection;
import java.util.List;
import java.util.Optional;
+import org.eclipse.jgit.errors.IncorrectObjectTypeException;
+import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.ReachabilityChecker;
@@ -77,7 +79,19 @@
.filter(refs, repo, RefFilterOptions.defaults());
Collection<RevCommit> visible = new ArrayList<>();
for (Ref r : filtered) {
- visible.add(rw.parseCommit(r.getObjectId()));
+ try {
+ visible.add(rw.parseCommit(r.getObjectId()));
+ } catch (IncorrectObjectTypeException notCommit) {
+ // Its OK for a tag reference to point to a blob or a tree, this
+ // is common in the Linux kernel or git.git repository.
+ continue;
+ } catch (MissingObjectException notHere) {
+ // Log the problem with this branch, but keep processing.
+ logger.atWarning().log(
+ "Reference %s in %s points to dangling object %s",
+ r.getName(), repo.getDirectory(), r.getObjectId());
+ continue;
+ }
}
// The filtering above already produces a voluminous trace. To separate the permission check
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index cd5032a..7ca763a1 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -250,7 +250,7 @@
install(new RestApiModule());
install(new OAuthRestModule());
install(new DefaultProjectNameLockManager.Module());
- install(new FileInfoJsonModule());
+ install(new FileInfoJsonModule(cfg));
bind(ProjectOperations.class).to(ProjectOperationsImpl.class);
}
diff --git a/plugins/codemirror-editor b/plugins/codemirror-editor
index 42d5fe0..e0a6721 160000
--- a/plugins/codemirror-editor
+++ b/plugins/codemirror-editor
@@ -1 +1 @@
-Subproject commit 42d5fe041ee2ef6be579c0085396fa5e60889222
+Subproject commit e0a67217ae5359797570481cbb6e8aa1f5e0a7c3
diff --git a/polygerrit-ui/app/api/checks.ts b/polygerrit-ui/app/api/checks.ts
index 2682158..06f9509 100644
--- a/polygerrit-ui/app/api/checks.ts
+++ b/polygerrit-ui/app/api/checks.ts
@@ -28,6 +28,23 @@
* polling interval to pass.
*/
announceUpdate(): void;
+
+ /**
+ * Updates an individual result.
+ *
+ * This can be used for lazy loading detailed information. For example, if you
+ * are using the `check-result-expanded` endpoint, then you can load more
+ * result details when the user expands a result row.
+ *
+ * The parameter `run` is only used to *find* the correct run for updating the
+ * result. It will only be used for comparing `change`, `patchset`, `attempt`
+ * and `checkName`. Its properties other than `results` will not be updated.
+ *
+ * For us being able to identify the result that you want to update you have
+ * to set the `externalId` property. An undefined `externalId` will result in
+ * an error.
+ */
+ updateResult(run: CheckRun, result: CheckResult): void;
}
export declare interface ChecksApiConfig {
@@ -450,4 +467,5 @@
HELP_PAGE = 'help_page',
REPORT_BUG = 'report_bug',
CODE = 'code',
+ FILE_PRESENT = 'file_present',
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
index 251fbf7..c78d6c0 100644
--- a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
@@ -36,10 +36,11 @@
getResultsOf,
hasCompletedWithoutResults,
hasResultsOf,
- iconForCategory,
- iconForStatus,
+ iconFor,
isRunning,
isRunningOrHasCompleted,
+ isStatus,
+ labelFor,
} from '../../../services/checks/checks-util';
import {ChangeComments} from '../../diff/gr-comment-api/gr-comment-api';
import {
@@ -57,6 +58,7 @@
import {PrimaryTab} from '../../../constants/constants';
import {ChecksTabState, CommentTabState} from '../../../types/events';
import {spinnerStyles} from '../../../styles/gr-spinner-styles';
+import {modifierPressed} from '../../../utils/dom-util';
export enum SummaryChipStyles {
INFO = 'info',
@@ -106,6 +108,9 @@
background: var(--warning-background-hover);
box-shadow: var(--elevation-level-1);
}
+ .summaryChip.warning:focus-within {
+ background: var(--warning-background-focus);
+ }
.summaryChip.warning iron-icon {
color: var(--warning-foreground);
}
@@ -117,6 +122,9 @@
background: var(--gray-background-hover);
box-shadow: var(--elevation-level-1);
}
+ .summaryChip.check:focus-within {
+ background: var(--gray-background-focus);
+ }
.summaryChip.check iron-icon {
color: var(--gray-foreground);
}
@@ -148,7 +156,7 @@
@customElement('gr-checks-chip')
export class GrChecksChip extends GrLitElement {
@property()
- icon = '';
+ statusOrCategory?: Category | RunStatus;
@property()
text = '';
@@ -193,6 +201,9 @@
background: var(--error-background-hover);
box-shadow: var(--elevation-level-1);
}
+ .checksChip.error:focus-within {
+ background: var(--error-background-focus);
+ }
.checksChip.error iron-icon {
color: var(--error-foreground);
}
@@ -204,6 +215,9 @@
background: var(--warning-background-hover);
box-shadow: var(--elevation-level-1);
}
+ .checksChip.warning:focus-within {
+ background: var(--warning-background-focus);
+ }
.checksChip.warning iron-icon {
color: var(--warning-foreground);
}
@@ -215,6 +229,9 @@
background: var(--info-background-hover);
box-shadow: var(--elevation-level-1);
}
+ .checksChip.info-outline:focus-within {
+ background: var(--info-background-focus);
+ }
.checksChip.info-outline iron-icon {
color: var(--info-foreground);
}
@@ -226,6 +243,9 @@
background: var(--success-background-hover);
box-shadow: var(--elevation-level-1);
}
+ .checksChip.check-circle-outline:focus-within {
+ background: var(--success-background-focus);
+ }
.checksChip.check-circle-outline iron-icon {
color: var(--success-foreground);
}
@@ -239,6 +259,9 @@
background: var(--gray-background-hover);
box-shadow: var(--elevation-level-1);
}
+ .checksChip.timelapse:focus-within {
+ background: var(--gray-background-focus);
+ }
.checksChip.timelapse iron-icon {
color: var(--gray-foreground);
}
@@ -248,10 +271,25 @@
render() {
if (!this.text) return;
- const chipClass = `checksChip font-small ${this.icon}`;
- const grIcon = `gr-icons:${this.icon}`;
+ if (!this.statusOrCategory) return;
+ const icon = iconFor(this.statusOrCategory);
+ const label = labelFor(this.statusOrCategory);
+ const count = Number(this.text);
+ let ariaLabel = label;
+ if (!isNaN(count)) {
+ const type = isStatus(this.statusOrCategory) ? 'run' : 'result';
+ const plural = count > 1 ? 's' : '';
+ ariaLabel = `${this.text} ${label} ${type}${plural}`;
+ }
+ const chipClass = `checksChip font-small ${icon}`;
+ const grIcon = `gr-icons:${icon}`;
return html`
- <div class="${chipClass}" role="button">
+ <div
+ class="${chipClass}"
+ role="link"
+ tabindex="0"
+ aria-label="${ariaLabel}"
+ >
<iron-icon icon="${grIcon}"></iron-icon>
<div class="text">${this.text}</div>
<slot></slot>
@@ -411,18 +449,17 @@
if (this.errorMessage || this.loginCallback) return;
if (this.runs.some(isRunningOrHasCompleted)) return;
const msg = this.someProvidersAreLoading ? 'Loading results' : 'No results';
- return html`<span class="loading zeroState">${msg}</span>`;
+ return html`<span role="status" class="loading zeroState">${msg}</span>`;
}
renderChecksChipForCategory(category: Category) {
if (this.errorMessage || this.loginCallback) return;
- const icon = iconForCategory(category);
const runs = this.runs.filter(run => {
if (hasResultsOf(run, category)) return true;
return category === Category.SUCCESS && hasCompletedWithoutResults(run);
});
const count = (run: CheckRun) => getResultsOf(run, category);
- return this.renderChecksChip(icon, runs, category, count);
+ return this.renderChecksChip(runs, category, count);
}
renderChecksChipForStatus(
@@ -430,13 +467,11 @@
filter: (run: CheckRun) => boolean
) {
if (this.errorMessage || this.loginCallback) return;
- const icon = iconForStatus(status);
const runs = this.runs.filter(filter);
- return this.renderChecksChip(icon, runs, status, () => []);
+ return this.renderChecksChip(runs, status, () => []);
}
renderChecksChip(
- icon: string,
runs: CheckRun[],
statusOrCategory: RunStatus | Category,
resultFilter: (run: CheckRun) => CheckResult[]
@@ -462,13 +497,19 @@
const links = allPrimaryLinks.length === 1 ? allPrimaryLinks : [];
const text = `${run.checkName}`;
return html`<gr-checks-chip
- class="${icon}"
- .icon="${icon}"
+ .statusOrCategory="${statusOrCategory}"
.text="${text}"
@click="${() => this.onChipClick({checkName: run.checkName})}"
+ @keydown="${(e: KeyboardEvent) =>
+ this.onChipKeyDown(e, {checkName: run.checkName})}"
>${links.map(
link => html`
- <a href="${link.url}" target="_blank" @click="${this.onLinkClick}"
+ <a
+ href="${link.url}"
+ target="_blank"
+ @click="${this.onLinkClick}"
+ @keydown="${this.onLinkKeyDown}"
+ aria-label="Link to check details"
><iron-icon class="launch" icon="gr-icons:launch"></iron-icon
></a>
`
@@ -484,19 +525,34 @@
);
if (sum === 0) return;
return html`<gr-checks-chip
- class="${icon}"
- .icon="${icon}"
+ .statusOrCategory="${statusOrCategory}"
.text="${sum}"
@click="${() => this.onChipClick({statusOrCategory})}"
+ @keydown="${(e: KeyboardEvent) =>
+ this.onChipKeyDown(e, {statusOrCategory})}"
></gr-checks-chip>`;
}
+ private onChipKeyDown(e: KeyboardEvent, state: ChecksTabState) {
+ if (modifierPressed(e)) return;
+ // Only react to `return` and `space`.
+ if (e.keyCode !== 13 && e.keyCode !== 32) return;
+ e.preventDefault();
+ e.stopPropagation();
+ this.onChipClick(state);
+ }
+
private onChipClick(state: ChecksTabState) {
fireShowPrimaryTab(this, PrimaryTab.CHECKS, false, {
checksTab: state,
});
}
+ private onLinkKeyDown(e: KeyboardEvent) {
+ // Prevents onConChipKeyDown() from reacting to <a> link keyboard events.
+ e.stopPropagation();
+ }
+
private onLinkClick(e: MouseEvent) {
// Prevents onChipClick() from reacting to <a> link clicks.
e.stopPropagation();
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index 4304734..2ede186e 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -1254,16 +1254,37 @@
this.$.fileList.collapseAllDiffs();
}
+ /**
+ * ChangeView is never re-used for different changes. It is safer and simpler
+ * to just re-create another change view when the user switches to a new
+ * change page. Thus we need a reliable way to detect that the change view
+ * does not match the current change number anymore.
+ *
+ * If this method returns true, then the change view should not do anything
+ * anymore. The app element makes sure that an obsolete change view is not
+ * shown anymore, so if the change view is still and doing some update to
+ * itself, then that is not dangerous. But for example it should not call
+ * navigateToChange() anymore. That would very likely cause erroneous
+ * behavior.
+ */
+ private isChangeObsolete() {
+ // While this._changeNum is undefined the change view is fresh and has just
+ // not updated it to params.changeNum yet. Not obsolete in that case.
+ if (this._changeNum === undefined) return false;
+ // this.params reflects the current state of the URL. If this._changeNum
+ // does not match it anymore, then this view must be considered obsolete.
+ return this._changeNum !== this.params?.changeNum;
+ }
+
_paramsChanged(value: AppElementChangeViewParams) {
if (value.view !== GerritView.CHANGE) {
this._initialLoadComplete = false;
return;
}
- // Everything in the change view is tied to the change. It seems better to
- // force the re-creation of the change view when the change number changes.
- const changeChanged = this._changeNum !== value.changeNum;
- if (this._changeNum !== undefined && changeChanged) {
+ if (this.isChangeObsolete()) {
+ // Tell the app element that we are not going to handle the new change
+ // number and that they have to create a new change view.
fireEvent(this, EventType.RECREATE_CHANGE_VIEW);
return;
}
@@ -1298,7 +1319,7 @@
// If the change has already been loaded and the parameter change is only
// in the patch range, then don't do a full reload.
- if (!changeChanged && patchChanged && patchKnown) {
+ if (this._changeNum !== undefined && patchChanged && patchKnown) {
if (!patchRange.patchNum) {
patchRange.patchNum = computeLatestPatchNum(this._allPatchSets);
rightPatchNumChanged = true;
@@ -2116,6 +2137,7 @@
* promise resolves.
*/
loadData(isLocationChange?: boolean, clearPatchset?: boolean) {
+ if (this.isChangeObsolete()) return Promise.resolve([]);
if (clearPatchset && this._change) {
GerritNav.navigateToChange(this._change);
return Promise.resolve([]);
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
index 5128948..7d47abb 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
@@ -1614,7 +1614,7 @@
});
});
- test('don’t reload entire page when patchRange changes', () => {
+ test('don’t reload entire page when patchRange changes', async () => {
const reloadStub = sinon
.stub(element, 'loadData')
.callsFake(() => Promise.resolve([]));
@@ -1629,7 +1629,8 @@
view: GerritView.CHANGE,
patchNum: 1 as RevisionPatchSetNum,
};
- element._paramsChanged(value);
+ element.params = value;
+ await flush();
assert.isTrue(reloadStub.calledOnce);
element._initialLoadComplete = true;
@@ -1643,13 +1644,14 @@
value.basePatchNum = 1 as BasePatchSetNum;
value.patchNum = 2 as RevisionPatchSetNum;
- element._paramsChanged(value);
+ element.params = {...value};
+ await flush();
assert.isFalse(reloadStub.calledTwice);
assert.isTrue(reloadPatchDependentStub.calledOnce);
assert.isTrue(collapseStub.calledTwice);
});
- test('reload ported comments when patchNum changes', () => {
+ test('reload ported comments when patchNum changes', async () => {
sinon.stub(element, 'loadData').callsFake(() => Promise.resolve([]));
sinon.stub(element, '_getCommitInfo');
sinon.stub(element.$.fileList, 'reload');
@@ -1665,7 +1667,8 @@
view: GerritView.CHANGE,
patchNum: 1 as RevisionPatchSetNum,
};
- element._paramsChanged(value);
+ element.params = value;
+ await flush();
element._initialLoadComplete = true;
element._change = {
@@ -1678,24 +1681,42 @@
value.basePatchNum = 1 as BasePatchSetNum;
value.patchNum = 2 as RevisionPatchSetNum;
- element._paramsChanged(value);
+ element.params = {...value};
+ await flush();
assert.isTrue(reloadPortedCommentsStub.calledOnce);
});
- test('reload entire page when patchRange doesnt change', () => {
+ test('reload entire page when patchRange doesnt change', async () => {
const reloadStub = sinon
.stub(element, 'loadData')
.callsFake(() => Promise.resolve([]));
const collapseStub = sinon.stub(element.$.fileList, 'collapseAllDiffs');
const value: AppElementChangeViewParams = createAppElementChangeViewParams();
- element._paramsChanged(value);
+ element.params = value;
+ await flush();
assert.isTrue(reloadStub.calledOnce);
element._initialLoadComplete = true;
- element._paramsChanged(value);
+ element.params = {...value};
+ await flush();
assert.isTrue(reloadStub.calledTwice);
assert.isTrue(collapseStub.calledTwice);
});
+ test('do not handle new change numbers', async () => {
+ const recreateSpy = sinon.spy();
+ element.addEventListener('recreate-change-view', recreateSpy);
+
+ const value: AppElementChangeViewParams = createAppElementChangeViewParams();
+ element.params = value;
+ await flush();
+ assert.isFalse(recreateSpy.calledOnce);
+
+ value.changeNum = 555111333 as NumericChangeId;
+ element.params = {...value};
+ await flush();
+ assert.isTrue(recreateSpy.calledOnce);
+ });
+
test('related changes are not updated after other action', done => {
sinon.stub(element, 'loadData').callsFake(() => Promise.resolve([]));
flush();
@@ -2026,7 +2047,7 @@
element.handleScroll();
});
- test('scrollTop is set correctly', () => {
+ test('scrollTop is set correctly', async () => {
element.viewState = {scrollTop: TEST_SCROLL_TOP_PX};
sinon.stub(element, 'loadData').callsFake(() => {
@@ -2039,7 +2060,8 @@
// simulate reloading component, which is done when route
// changes to match a regex of change view type.
- element._paramsChanged({...createAppElementChangeViewParams()});
+ element.params = {...createAppElementChangeViewParams()};
+ await flush();
});
test('scrollTop is reset when new change is loaded', () => {
@@ -2591,7 +2613,7 @@
});
});
- test('report changeDisplayed on _paramsChanged', done => {
+ test('report changeDisplayed on _paramsChanged', async () => {
const changeDisplayStub = sinon.stub(
appContext.reportingService,
'changeDisplayed'
@@ -2600,16 +2622,14 @@
appContext.reportingService,
'changeFullyLoaded'
);
- element._paramsChanged({
+ element.params = {
...createAppElementChangeViewParams(),
changeNum: TEST_NUMERIC_CHANGE_ID,
project: TEST_PROJECT_NAME,
- });
- flush(() => {
- assert.isTrue(changeDisplayStub.called);
- assert.isTrue(changeFullyLoadedStub.called);
- done();
- });
+ };
+ await flush();
+ assert.isTrue(changeDisplayStub.called);
+ assert.isTrue(changeFullyLoadedStub.called);
});
});
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
index 8283be79..2e8e4ba 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
@@ -17,18 +17,20 @@
import '@polymer/paper-toggle-button/paper-toggle-button';
import '../../../styles/shared-styles';
import '../../shared/gr-comment-thread/gr-comment-thread';
+import '../../shared/gr-dropdown-list/gr-dropdown-list';
+
import {flush} from '@polymer/polymer/lib/legacy/polymer.dom';
import {PolymerElement} from '@polymer/polymer/polymer-element';
import {htmlTemplate} from './gr-thread-list_html';
import {parseDate} from '../../../utils/date-util';
import {CommentSide, SpecialFilePath} from '../../../constants/constants';
-import {customElement, observe, property} from '@polymer/decorators';
+import {computed, customElement, observe, property} from '@polymer/decorators';
import {
PolymerSpliceChange,
PolymerDeepPropertyChange,
} from '@polymer/polymer/interfaces';
-import {ChangeInfo} from '../../../types/common';
+import {AccountInfo, ChangeInfo} from '../../../types/common';
import {
CommentThread,
isDraft,
@@ -36,11 +38,14 @@
isDraftThread,
isRobotThread,
hasHumanReply,
+ getCommentAuthors,
} from '../../../utils/comment-util';
import {pluralize} from '../../../utils/string-util';
import {fireThreadListModifiedEvent} from '../../../utils/event-util';
-import {assertNever} from '../../../utils/common-util';
+import {assertIsDefined, assertNever} from '../../../utils/common-util';
import {CommentTabState} from '../../../types/events';
+import {DropdownItem} from '../../shared/gr-dropdown-list/gr-dropdown-list';
+import {GrAccountChip} from '../../shared/gr-account-chip/gr-account-chip';
interface CommentThreadWithInfo {
thread: CommentThread;
@@ -52,6 +57,13 @@
updated?: Date;
}
+enum SortDropdownState {
+ TIMESTAMP = 'Latest timestamp',
+ FILES = 'Files',
+}
+
+export const __testOnly_SortDropdownState = SortDropdownState;
+
@customElement('gr-thread-list')
export class GrThreadList extends PolymerElement {
static get template() {
@@ -79,7 +91,7 @@
@property({
computed:
'_computeDisplayedThreads(_sortedThreads.*, unresolvedOnly, ' +
- '_draftsOnly, onlyShowRobotCommentsWithHumanReply)',
+ '_draftsOnly, onlyShowRobotCommentsWithHumanReply, selectedAuthors)',
type: Array,
})
_displayedThreads: CommentThread[] = [];
@@ -102,6 +114,21 @@
@property({type: Object, observer: '_commentTabStateChange'})
commentTabState?: CommentTabState;
+ @property({type: Object})
+ sortDropdownValue: SortDropdownState = SortDropdownState.TIMESTAMP;
+
+ @property({type: Array, notify: true})
+ selectedAuthors: AccountInfo[] = [];
+
+ @computed('unresolvedOnly', '_draftsOnly')
+ get commentsDropdownValue() {
+ // set initial value and triggered when comment summary chips are clicked
+ if (this._draftsOnly) return CommentTabState.DRAFTS;
+ return this.unresolvedOnly
+ ? CommentTabState.UNRESOLVED
+ : CommentTabState.SHOW_ALL;
+ }
+
_showEmptyThreadsMessage(
threads: CommentThread[],
displayedThreads: CommentThread[],
@@ -146,7 +173,74 @@
this.unresolvedOnly = !this.unresolvedOnly;
}
+ getSortDropdownEntires() {
+ return [
+ {text: SortDropdownState.FILES, value: SortDropdownState.FILES},
+ {text: SortDropdownState.TIMESTAMP, value: SortDropdownState.TIMESTAMP},
+ ];
+ }
+
+ getCommentsDropdownEntires(threads: CommentThread[], loggedIn?: boolean) {
+ const items: DropdownItem[] = [
+ {
+ text: `Unresolved (${this._countUnresolved(threads)})`,
+ value: CommentTabState.UNRESOLVED,
+ },
+ {
+ text: `All (${this._countAllThreads(threads)})`,
+ value: CommentTabState.SHOW_ALL,
+ },
+ ];
+ if (loggedIn)
+ items.splice(1, 0, {
+ text: `Drafts (${this._countDrafts(threads)})`,
+ value: CommentTabState.DRAFTS,
+ });
+ return items;
+ }
+
+ getCommentAuthors(threads?: CommentThread[]) {
+ return getCommentAuthors(threads);
+ }
+
+ handleAccountClicked(e: MouseEvent) {
+ const account = (e.target as GrAccountChip).account;
+ assertIsDefined(account, 'account');
+ const index = this.selectedAuthors.findIndex(
+ author => author._account_id === account._account_id
+ );
+ if (index === -1) this.push('selectedAuthors', account);
+ else this.splice('selectedAuthors', index, 1);
+ // re-assign so that isSelected template method is called
+ this.selectedAuthors = [...this.selectedAuthors];
+ }
+
+ isSelected(author: AccountInfo, selectedAuthors: AccountInfo[]) {
+ return selectedAuthors.some(a => a._account_id === author._account_id);
+ }
+
+ handleSortDropdownValueChange(e: CustomEvent) {
+ this.sortDropdownValue = e.detail.value;
+ /*
+ * Ideally we would have updateSortedThreads observe on sortDropdownValue
+ * but the method triggered re-render only when the length of threads
+ * changes, hence keep the explicit resortThreads method
+ */
+ this.resortThreads(this.threads);
+ }
+
+ handleCommentsDropdownValueChange(e: CustomEvent) {
+ const value = e.detail.value;
+ if (value === CommentTabState.UNRESOLVED) this._handleOnlyUnresolved();
+ else if (value === CommentTabState.DRAFTS) this._handleOnlyDrafts();
+ else this._handleAllComments();
+ }
+
_compareThreads(c1: CommentThreadWithInfo, c2: CommentThreadWithInfo) {
+ if (this.sortDropdownValue === SortDropdownState.TIMESTAMP) {
+ if (c1.updated && c2.updated) return c1.updated > c2.updated ? -1 : 1;
+ }
+
if (c1.thread.path !== c2.thread.path) {
// '/PATCHSET' will not come before '/COMMIT' when sorting
// alphabetically so move it to the front explicitly
@@ -202,6 +296,15 @@
return 0;
}
+ resortThreads(threads: CommentThread[]) {
+ const threadsWithInfo = threads.map(thread =>
+ this._getThreadWithStatusInfo(thread)
+ );
+ this._sortedThreads = threadsWithInfo
+ .sort((t1, t2) => this._compareThreads(t1, t2))
+ .map(threadInfo => threadInfo.thread);
+ }
+
/**
* Observer on threads and update _sortedThreads when needed.
* Order as follows:
@@ -254,12 +357,7 @@
return;
}
- const threadsWithInfo = threads.map(thread =>
- this._getThreadWithStatusInfo(thread)
- );
- this._sortedThreads = threadsWithInfo
- .sort((t1, t2) => this._compareThreads(t1, t2))
- .map(threadInfo => threadInfo.thread);
+ this.resortThreads(threads);
}
_computeDisplayedThreads(
@@ -269,7 +367,8 @@
>,
unresolvedOnly?: boolean,
draftsOnly?: boolean,
- onlyShowRobotCommentsWithHumanReply?: boolean
+ onlyShowRobotCommentsWithHumanReply?: boolean,
+ selectedAuthors?: AccountInfo[]
) {
if (!sortedThreadsRecord || !sortedThreadsRecord.base) return [];
return sortedThreadsRecord.base.filter(t =>
@@ -277,7 +376,8 @@
t,
unresolvedOnly,
draftsOnly,
- onlyShowRobotCommentsWithHumanReply
+ onlyShowRobotCommentsWithHumanReply,
+ selectedAuthors
)
);
}
@@ -287,14 +387,16 @@
thread: CommentThread,
unresolvedOnly?: boolean,
draftsOnly?: boolean,
- onlyShowRobotCommentsWithHumanReply?: boolean
+ onlyShowRobotCommentsWithHumanReply?: boolean,
+ selectedAuthors?: AccountInfo[]
) {
const threads = displayedThreads.filter(t =>
this._shouldShowThread(
t,
unresolvedOnly,
draftsOnly,
- onlyShowRobotCommentsWithHumanReply
+ onlyShowRobotCommentsWithHumanReply,
+ selectedAuthors
)
);
const index = threads.findIndex(t => t.rootId === thread.rootId);
@@ -309,14 +411,16 @@
thread: CommentThread,
unresolvedOnly?: boolean,
draftsOnly?: boolean,
- onlyShowRobotCommentsWithHumanReply?: boolean
+ onlyShowRobotCommentsWithHumanReply?: boolean,
+ selectedAuthors?: AccountInfo[]
) {
const threads = displayedThreads.filter(t =>
this._shouldShowThread(
t,
unresolvedOnly,
draftsOnly,
- onlyShowRobotCommentsWithHumanReply
+ onlyShowRobotCommentsWithHumanReply,
+ selectedAuthors
)
);
const index = threads.findIndex(t => t.rootId === thread.rootId);
@@ -330,7 +434,8 @@
thread,
unresolvedOnly,
draftsOnly,
- onlyShowRobotCommentsWithHumanReply
+ onlyShowRobotCommentsWithHumanReply,
+ selectedAuthors
)
);
}
@@ -339,7 +444,8 @@
thread: CommentThread,
unresolvedOnly?: boolean,
draftsOnly?: boolean,
- onlyShowRobotCommentsWithHumanReply?: boolean
+ onlyShowRobotCommentsWithHumanReply?: boolean,
+ selectedAuthors?: AccountInfo[]
) {
if (
[
@@ -347,11 +453,26 @@
unresolvedOnly,
draftsOnly,
onlyShowRobotCommentsWithHumanReply,
+ selectedAuthors,
].includes(undefined)
) {
return false;
}
+ if (selectedAuthors!.length) {
+ if (
+ !thread.comments.some(
+ c =>
+ c.author &&
+ selectedAuthors!.some(
+ author => c.author!._account_id === author._account_id
+ )
+ )
+ ) {
+ return false;
+ }
+ }
+
if (
!draftsOnly &&
!unresolvedOnly &&
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.ts b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.ts
index da91095..41beed2 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.ts
@@ -33,7 +33,7 @@
border-top: 1px solid var(--border-color);
display: flex;
justify-content: left;
- padding: var(--spacing-m) var(--spacing-l);
+ padding: var(--spacing-s) var(--spacing-l);
}
.draftsOnly:not(.unresolvedOnly) gr-comment-thread[has-draft],
.unresolvedOnly:not(.draftsOnly) gr-comment-thread[unresolved],
@@ -48,54 +48,70 @@
box-shadow: none;
padding-left: var(--spacing-m);
}
- .header .categoryRadio {
- height: 18px;
- width: 18px;
- }
- .header label {
- padding-left: 8px;
- margin-right: 16px;
- }
.partypopper{
margin-right: var(--spacing-s);
}
+ gr-dropdown-list {
+ --trigger-style: {
+ color: var(--primary-text-color);
+ text-transform: none;
+ font-family: var(--font-family);
+ }
+ }
+ .filter-text, .sort-text, .author-text {
+ margin-right: var(--spacing-s);
+ color: var(--deemphasized-text-color);
+ }
+ .author-text {
+ margin-left: var(--spacing-m);
+ }
+ gr-account-label {
+ --account-max-length: 120px;
+ display: inline-block;
+ user-select: none;
+ --label-border-radius: 8px;
+ margin: 0 var(--spacing-xs);
+ padding: var(--spacing-xs) var(--spacing-m);
+ line-height: var(--line-height-normal);
+ cursor: pointer;
+ }
+ gr-account-label:focus {
+ outline: none;
+ }
+ gr-account-label:hover,
+ gr-account-label:hover {
+ box-shadow: var(--elevation-level-1);
+ cursor: pointer;
+ }
</style>
<template is="dom-if" if="[[!hideToggleButtons]]">
<div class="header">
- <input
- class="categoryRadio"
- id="unresolvedRadio"
- name="filterComments"
- type="radio"
- on-click="_handleOnlyUnresolved"
- checked="[[unresolvedOnly]]"
- />
- <label for="unresolvedRadio">
- Unresolved ([[_countUnresolved(threads)]])
- </label>
- <input
- class="categoryRadio"
- id="draftsRadio"
- name="filterComments"
- type="radio"
- on-click="_handleOnlyDrafts"
- checked="[[_draftsOnly]]"
- hidden$="[[!loggedIn]]"
- />
- <label for="draftsRadio" hidden$="[[!loggedIn]]">
- Drafts ([[_countDrafts(threads)]])
- </label>
- <input
- class="categoryRadio"
- id="allRadio"
- name="filterComments"
- type="radio"
- on-click="_handleAllComments"
- checked="[[_showAllComments(_draftsOnly, unresolvedOnly)]]"
- />
- <label for="allRadio">
- All ([[_countAllThreads(threads)]])
- </label>
+ <span class="sort-text">Sort By:</span>
+ <gr-dropdown-list
+ id="sortDropdown"
+ value="[[sortDropdownValue]]"
+ on-value-change="handleSortDropdownValueChange"
+ items="[[getSortDropdownEntires()]]"
+ >
+ </gr-dropdown-list>
+ <span class="separator"></span>
+ <span class="filter-text">Filter By:</span>
+ <gr-dropdown-list
+ id="filterDropdown"
+ value="[[commentsDropdownValue]]"
+ on-value-change="handleCommentsDropdownValueChange"
+ items="[[getCommentsDropdownEntires(threads, loggedIn)]]"
+ >
+ </gr-dropdown-list>
+ <span class="author-text">From:</span>
+ <template is="dom-repeat" items="[[getCommentAuthors(threads)]]">
+ <gr-account-label
+ account="[[item]]"
+ on-click="handleAccountClicked"
+ selection-chip-style
+ selected="[[isSelected(item, selectedAuthors)]]"
+ > </gr-account-label>
+ </template>
</div>
</template>
<div id="threads">
@@ -131,7 +147,7 @@
>
<template
is="dom-if"
- if="[[_shouldRenderSeparator(_displayedThreads, thread, unresolvedOnly, _draftsOnly, onlyShowRobotCommentsWithHumanReply)]]"
+ if="[[_shouldRenderSeparator(_displayedThreads, thread, unresolvedOnly, _draftsOnly, onlyShowRobotCommentsWithHumanReply, selectedAuthors)]]"
>
<div class="thread-separator"></div>
</template>
@@ -142,7 +158,7 @@
change-num="[[changeNum]]"
comments="[[thread.comments]]"
diff-side="[[thread.diffSide]]"
- show-file-name="[[_isFirstThreadWithFileName(_displayedThreads, thread, unresolvedOnly, _draftsOnly, onlyShowRobotCommentsWithHumanReply)]]"
+ show-file-name="[[_isFirstThreadWithFileName(_displayedThreads, thread, unresolvedOnly, _draftsOnly, onlyShowRobotCommentsWithHumanReply, selectedAuthors)]]"
project-name="[[change.project]]"
is-on-parent="[[_isOnParent(thread.commentSide)]]"
line-num="[[thread.line]]"
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.js b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.js
index 3d6cfd4..5c87c4b 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.js
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.js
@@ -19,6 +19,11 @@
import './gr-thread-list.js';
import {dom} from '@polymer/polymer/lib/legacy/polymer.dom.js';
import {SpecialFilePath} from '../../../constants/constants.js';
+import {CommentTabState} from '../../../types/events.js';
+import {__testOnly_SortDropdownState} from './gr-thread-list.js';
+import {queryAll} from '../../../test/test-utils.js';
+import {accountOrGroupKey} from '../../../utils/account-util.js';
+import {tap} from '@polymer/iron-test-helpers/mock-interactions';
const basicFixture = fixtureFromElement('gr-thread-list');
@@ -45,14 +50,14 @@
{
path: '/COMMIT_MSG',
author: {
- _account_id: 1000000,
+ _account_id: 1000001,
name: 'user',
username: 'user',
},
patch_set: 4,
id: 'ecf0b9fa_fe1a5f62',
line: 5,
- updated: '2018-02-08 18:49:18.000000000',
+ updated: '1',
message: 'test',
unresolved: true,
},
@@ -61,7 +66,7 @@
path: '/COMMIT_MSG',
line: 5,
in_reply_to: 'ecf0b9fa_fe1a5f62',
- updated: '2018-02-13 22:48:48.018000000',
+ updated: '1',
message: 'draft',
unresolved: true,
__draft: true,
@@ -74,21 +79,21 @@
path: '/COMMIT_MSG',
line: 5,
rootId: 'ecf0b9fa_fe1a5f62',
- start_datetime: '2018-02-08 18:49:18.000000000',
+ updated: '1',
},
{
comments: [
{
path: 'test.txt',
author: {
- _account_id: 1000000,
+ _account_id: 1000002,
name: 'user',
username: 'user',
},
patch_set: 3,
id: '09a9fb0a_1484e6cf',
side: 'PARENT',
- updated: '2018-02-13 22:47:19.000000000',
+ updated: '2',
message: 'Some comment on another patchset.',
unresolved: false,
},
@@ -96,7 +101,7 @@
patchNum: 3,
path: 'test.txt',
rootId: '09a9fb0a_1484e6cf',
- start_datetime: '2018-02-13 22:47:19.000000000',
+ updated: '2',
commentSide: 'PARENT',
},
{
@@ -104,13 +109,13 @@
{
path: '/COMMIT_MSG',
author: {
- _account_id: 1000000,
+ _account_id: 1000002,
name: 'user',
username: 'user',
},
patch_set: 2,
id: '8caddf38_44770ec1',
- updated: '2018-02-13 22:48:40.000000000',
+ updated: '3',
message: 'Another unresolved comment',
unresolved: false,
},
@@ -118,21 +123,21 @@
patchNum: 2,
path: '/COMMIT_MSG',
rootId: '8caddf38_44770ec1',
- start_datetime: '2018-02-13 22:48:40.000000000',
+ updated: '3',
},
{
comments: [
{
path: '/COMMIT_MSG',
author: {
- _account_id: 1000000,
+ _account_id: 1000003,
name: 'user',
username: 'user',
},
patch_set: 2,
id: 'scaddf38_44770ec1',
line: 4,
- updated: '2018-02-14 22:48:40.000000000',
+ updated: '4',
message: 'Yet another unresolved comment',
unresolved: true,
},
@@ -141,7 +146,7 @@
path: '/COMMIT_MSG',
line: 4,
rootId: 'scaddf38_44770ec1',
- start_datetime: '2018-02-14 22:48:40.000000000',
+ updated: '4',
},
{
comments: [
@@ -149,7 +154,7 @@
id: 'zcf0b9fa_fe1a5f62',
path: '/COMMIT_MSG',
line: 6,
- updated: '2018-02-15 22:48:48.018000000',
+ updated: '5',
message: 'resolved draft',
unresolved: false,
__draft: true,
@@ -162,14 +167,14 @@
path: '/COMMIT_MSG',
line: 6,
rootId: 'zcf0b9fa_fe1a5f62',
- start_datetime: '2018-02-09 18:49:18.000000000',
+ updated: '5',
},
{
comments: [
{
id: 'patchset_level_1',
path: SpecialFilePath.PATCHSET_LEVEL_COMMENTS,
- updated: '2018-02-15 22:48:48.018000000',
+ updated: '6',
message: 'patchset comment 1',
unresolved: false,
__editing: false,
@@ -179,14 +184,14 @@
patchNum: 2,
path: SpecialFilePath.PATCHSET_LEVEL_COMMENTS,
rootId: 'patchset_level_1',
- start_datetime: '2018-02-09 18:49:18.000000000',
+ updated: '6',
},
{
comments: [
{
id: 'patchset_level_2',
path: SpecialFilePath.PATCHSET_LEVEL_COMMENTS,
- updated: '2018-02-15 22:48:48.018000000',
+ updated: '7',
message: 'patchset comment 2',
unresolved: false,
__editing: false,
@@ -196,7 +201,7 @@
patchNum: 3,
path: SpecialFilePath.PATCHSET_LEVEL_COMMENTS,
rootId: 'patchset_level_2',
- start_datetime: '2018-02-09 18:49:18.000000000',
+ updated: '7',
},
{
comments: [
@@ -210,7 +215,7 @@
patch_set: 4,
id: 'rc1',
line: 5,
- updated: '2019-02-08 18:49:18.000000000',
+ updated: '8',
message: 'test',
unresolved: true,
robot_id: 'rc1',
@@ -220,7 +225,7 @@
path: '/COMMIT_MSG',
line: 5,
rootId: 'rc1',
- start_datetime: '2019-02-08 18:49:18.000000000',
+ updated: '8',
},
{
comments: [
@@ -234,7 +239,7 @@
patch_set: 4,
id: 'rc2',
line: 7,
- updated: '2019-03-08 18:49:18.000000000',
+ updated: '9',
message: 'test',
unresolved: true,
robot_id: 'rc2',
@@ -249,7 +254,7 @@
patch_set: 4,
id: 'c2_1',
line: 5,
- updated: '2019-03-08 18:49:18.000000000',
+ updated: '10',
message: 'test',
unresolved: true,
},
@@ -258,7 +263,7 @@
path: '/COMMIT_MSG',
line: 7,
rootId: 'rc2',
- start_datetime: '2019-03-08 18:49:18.000000000',
+ updated: '10',
},
];
@@ -270,15 +275,15 @@
});
});
- test('draft toggle only appears when logged in', () => {
+ test('draft dropdown item only appears when logged in', () => {
element.loggedIn = false;
- assert.equal(getComputedStyle(element.shadowRoot
- .querySelector('#draftsRadio')).display,
- 'none');
+ flush();
+ assert.equal(element.getCommentsDropdownEntires(element.threads,
+ element.loggedIn).length, 2);
element.loggedIn = true;
- assert.notEqual(getComputedStyle(element.shadowRoot
- .querySelector('#draftsRadio')).display,
- 'none');
+ flush();
+ assert.equal(element.getCommentsDropdownEntires(element.threads,
+ element.loggedIn).length, 3);
});
test('show all threads by default', () => {
@@ -298,13 +303,16 @@
});
test('showing file name takes visible threads into account', () => {
+ element.sortDropdownValue = __testOnly_SortDropdownState.FILES;
assert.equal(element._isFirstThreadWithFileName(element._sortedThreads,
element._sortedThreads[2], element.unresolvedOnly, element._draftsOnly,
- element.onlyShowRobotCommentsWithHumanReply), true);
+ element.onlyShowRobotCommentsWithHumanReply, element.selectedAuthors),
+ true);
element.unresolvedOnly = true;
assert.equal(element._isFirstThreadWithFileName(element._sortedThreads,
element._sortedThreads[2], element.unresolvedOnly, element._draftsOnly,
- element.onlyShowRobotCommentsWithHumanReply), false);
+ element.onlyShowRobotCommentsWithHumanReply, element.selectedAuthors),
+ false);
});
test('onlyShowRobotCommentsWithHumanReply ', () => {
@@ -317,6 +325,10 @@
});
suite('_compareThreads', () => {
+ setup(() => {
+ element.sortDropdownValue = __testOnly_SortDropdownState.FILES;
+ });
+
test('patchset comes before any other file', () => {
const t1 = {thread: {path: SpecialFilePath.PATCHSET_LEVEL_COMMENTS}};
const t2 = {thread: {path: SpecialFilePath.COMMIT_MESSAGE}};
@@ -448,6 +460,7 @@
});
test('_computeSortedThreads', () => {
+ element.sortDropdownValue = __testOnly_SortDropdownState.FILES;
assert.equal(element._sortedThreads.length, 9);
const expectedSortedRootIds = [
'patchset_level_2', // Posted on Patchset 3
@@ -465,7 +478,68 @@
});
});
+ test('_computeSortedThreads with timestamp', () => {
+ element.sortDropdownValue = __testOnly_SortDropdownState.TIMESTAMP;
+ element.resortThreads(element.threads);
+ assert.equal(element._sortedThreads.length, 9);
+ const expectedSortedRootIds = [
+ 'rc2',
+ 'rc1',
+ 'patchset_level_2',
+ 'patchset_level_1',
+ 'zcf0b9fa_fe1a5f62',
+ 'scaddf38_44770ec1',
+ '8caddf38_44770ec1',
+ '09a9fb0a_1484e6cf',
+ 'ecf0b9fa_fe1a5f62',
+ ];
+ element._sortedThreads.forEach((thread, index) => {
+ assert.equal(thread.rootId, expectedSortedRootIds[index]);
+ });
+ });
+
+ test('tapping single author chips', () => {
+ const chips = queryAll(element, 'gr-account-label');
+ const authors = Array.from(chips).map(
+ chip => accountOrGroupKey(chip.account))
+ .sort();
+ assert.deepEqual(authors, [1000000, 1000001, 1000002, 1000003]);
+ assert.equal(element.threads.length, 9);
+ assert.equal(element._displayedThreads.length, 9);
+
+ tap(chips[0]); // accountId 1000001
+ flush();
+
+ assert.equal(element.threads.length, 9);
+ assert.equal(element._displayedThreads.length, 1);
+ assert.equal(element._displayedThreads[0].comments[0].author._account_id,
+ 1000001);
+
+ tap(chips[0]); // tapping again resets
+ flush();
+ assert.equal(element.threads.length, 9);
+ assert.equal(element._displayedThreads.length, 9);
+ });
+
+ test('tapping multiple author chips', () => {
+ const chips = queryAll(element, 'gr-account-label');
+
+ tap(chips[0]); // accountId 1000001
+ tap(chips[1]); // accountId 1000002
+ flush();
+
+ assert.equal(element.threads.length, 9);
+ assert.equal(element._displayedThreads.length, 3);
+ assert.equal(element._displayedThreads[0].comments[0].author._account_id,
+ 1000002);
+ assert.equal(element._displayedThreads[1].comments[0].author._account_id,
+ 1000002);
+ assert.equal(element._displayedThreads[2].comments[0].author._account_id,
+ 1000001);
+ });
+
test('thread removal and sort again', () => {
+ element.sortDropdownValue = __testOnly_SortDropdownState.FILES;
threadElements[1].dispatchEvent(
new CustomEvent('thread-discard', {
detail: {rootId: 'rc2'},
@@ -489,6 +563,7 @@
});
test('modification on thread shold not trigger sort again', () => {
+ element.sortDropdownValue = __testOnly_SortDropdownState.FILES;
const currentSortedThreads = [...element._sortedThreads];
for (const thread of currentSortedThreads) {
thread.comments = [...thread.comments];
@@ -526,6 +601,7 @@
test('non-equal length of sortThreads and threads' +
' should trigger sort again', () => {
+ element.sortDropdownValue = __testOnly_SortDropdownState.FILES;
const modifiedThreads = [...element.threads];
const currentSortedThreads = [...element._sortedThreads];
element._sortedThreads = [];
@@ -549,22 +625,23 @@
});
});
- test('toggle all shows all all comments', () => {
- MockInteractions.tap(element.shadowRoot.querySelector(
- '#allRadio'));
+ test('show all comments', () => {
+ element.handleCommentsDropdownValueChange({detail: {
+ value: CommentTabState.SHOW_ALL}});
flush();
assert.equal(getVisibleThreads().length, 9);
});
- test('toggle unresolved shows all unresolved comments', () => {
- MockInteractions.tap(element.shadowRoot.querySelector(
- '#unresolvedRadio'));
+ test('unresolved shows all unresolved comments', () => {
+ element.handleCommentsDropdownValueChange({detail: {
+ value: CommentTabState.UNRESOLVED}});
flush();
assert.equal(getVisibleThreads().length, 4);
});
test('toggle drafts only shows threads with draft comments', () => {
- MockInteractions.tap(element.shadowRoot.querySelector('#draftsRadio'));
+ element.handleCommentsDropdownValueChange({detail: {
+ value: CommentTabState.DRAFTS}});
flush();
assert.equal(getVisibleThreads().length, 2);
});
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index 2ea6dd0..fd8cd80 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -52,7 +52,7 @@
fireActionTriggered,
firstPrimaryLink,
hasCompletedWithoutResults,
- iconForCategory,
+ iconFor,
iconForLink,
otherPrimaryLinks,
primaryRunAction,
@@ -60,7 +60,7 @@
tooltipForLink,
} from '../../services/checks/checks-util';
import {assertIsDefined, check} from '../../utils/common-util';
-import {toggleClass, whenVisible} from '../../utils/dom-util';
+import {modifierPressed, toggleClass, whenVisible} from '../../utils/dom-util';
import {durationString} from '../../utils/date-util';
import {charsOnly} from '../../utils/string-util';
import {isAttemptSelected} from './gr-checks-util';
@@ -83,6 +83,9 @@
@customElement('gr-result-row')
class GrResultRow extends GrLitElement {
+ @query('td.nameCol div.name')
+ nameEl?: HTMLElement;
+
@property()
result?: RunResult;
@@ -119,9 +122,11 @@
tr.container {
border-top: 1px solid var(--border-color);
}
+ a.link {
+ margin-right: var(--spacing-s);
+ }
iron-icon.link {
color: var(--link-color);
- margin-right: var(--spacing-s);
}
td.nameCol div.flex {
display: flex;
@@ -130,6 +135,7 @@
overflow: hidden;
text-overflow: ellipsis;
margin-right: var(--spacing-s);
+ outline-offset: var(--spacing-xs);
}
td.nameCol .space {
flex-grow: 1;
@@ -137,6 +143,7 @@
td.nameCol gr-checks-action {
display: none;
}
+ tr:focus-within td.nameCol gr-checks-action,
tr:hover td.nameCol gr-checks-action {
display: inline-block;
/* The button should fit into the 20px line-height. The negative
@@ -180,8 +187,13 @@
tr.container:hover {
background: var(--hover-background-color);
}
+ tr.container:focus-within {
+ background: var(--selection-background-color);
+ }
tr.container td .summary-cell .links,
tr.container td .summary-cell .actions,
+ tr.container.collapsed:focus-within td .summary-cell .links,
+ tr.container.collapsed:focus-within td .summary-cell .actions,
tr.container.collapsed:hover td .summary-cell .links,
tr.container.collapsed:hover td .summary-cell .actions,
:host(.dropdown-open) tr td .summary-cell .links,
@@ -278,6 +290,10 @@
super.update(changedProperties);
}
+ focus() {
+ if (this.nameEl) this.nameEl.focus();
+ }
+
firstUpdated() {
const loading = this.shadowRoot?.querySelector('.container');
assertIsDefined(loading, '"Loading" element');
@@ -299,10 +315,18 @@
}
return html`
<tr class="${classMap({container: true, collapsed: !this.isExpanded})}">
- <td class="nameCol" @click="${this.toggleExpanded}">
+ <td class="nameCol" @click="${this.toggleExpandedClick}">
<div class="flex">
<gr-hovercard-run .run="${this.result}"></gr-hovercard-run>
- <div class="name">${this.result.checkName}</div>
+ <div
+ class="name"
+ role="button"
+ tabindex="0"
+ @click="${this.toggleExpandedClick}"
+ @keydown="${this.toggleExpandedPress}"
+ >
+ ${this.result.checkName}
+ </div>
<div class="space"></div>
${this.renderPrimaryRunAction()}
</div>
@@ -311,7 +335,7 @@
<div class="summary-cell">
${this.renderLink(firstPrimaryLink(this.result))}
${this.renderSummary(this.result.summary)}
- <div class="message" @click="${this.toggleExpanded}">
+ <div class="message" @click="${this.toggleExpandedClick}">
${this.isExpanded ? '' : this.result.message}
</div>
${this.renderLinks()} ${this.renderActions()}
@@ -321,7 +345,7 @@
${this.renderLabel()}
</div>
</td>
- <td class="expanderCol" @click="${this.toggleExpanded}">
+ <td class="expanderCol" @click="${this.toggleExpandedClick}">
<div
class="show-hide"
role="switch"
@@ -331,7 +355,7 @@
aria-label="${this.isExpanded
? 'Collapse result row'
: 'Expand result row'}"
- @keydown="${this.toggleExpanded}"
+ @keydown="${this.toggleExpandedPress}"
>
<iron-icon
icon="${this.isExpanded
@@ -361,6 +385,23 @@
></gr-result-expanded>`;
}
+ private toggleExpandedClick(e: MouseEvent) {
+ if (!this.isExpandable) return;
+ e.preventDefault();
+ e.stopPropagation();
+ this.toggleExpanded();
+ }
+
+ private toggleExpandedPress(e: KeyboardEvent) {
+ if (!this.isExpandable) return;
+ if (modifierPressed(e)) return;
+ // Only react to `return` and `space`.
+ if (e.keyCode !== 13 && e.keyCode !== 32) return;
+ e.preventDefault();
+ e.stopPropagation();
+ this.toggleExpanded();
+ }
+
private toggleExpanded() {
if (!this.isExpandable) return;
this.isExpanded = !this.isExpanded;
@@ -383,10 +424,14 @@
if (!this.result?.isLatestAttempt) return;
const info = this.labels?.[label];
const status = getLabelStatus(info).toLowerCase();
- const value = valueString(getRepresentativeValue(info));
+ const value = getRepresentativeValue(info);
+ // A neutral vote is not interesting for the user to see and is just
+ // cluttering the UI.
+ if (value === 0) return;
+ const valueStr = valueString(value);
return html`
<div class="label ${status}">
- <span>${label} ${value}</span>
+ <span>${label} ${valueStr}</span>
<paper-tooltip offset="5" fit-to-visible-bounds="true">
The check result has (probably) influenced this label vote.
</paper-tooltip>
@@ -414,7 +459,7 @@
if (this.isExpanded) return;
if (!link) return;
const tooltipText = link.tooltip ?? tooltipForLink(link.icon);
- return html`<a href="${link.url}" target="_blank"
+ return html`<a href="${link.url}" class="link" target="_blank"
><iron-icon
aria-label="external link to details"
class="link"
@@ -897,7 +942,7 @@
) {
let cat = statusOrCategory.toString().toLowerCase();
if (statusOrCategory === RunStatus.COMPLETED) cat = 'success';
- this.scrollElIntoView(`.categoryHeader .${cat}`);
+ this.scrollElIntoView(`.categoryHeader.${cat} + table gr-result-row`);
} else if (checkName) {
this.scrollElIntoView(`gr-result-row.${charsOnly(checkName)}`);
}
@@ -910,6 +955,7 @@
// el might be a <gr-result-row> with an empty shadowRoot. Let's wait a
// moment before trying to find a child element in it.
setTimeout(() => {
+ if (el) (el as HTMLElement).focus();
// <gr-result-row> has display:contents and cannot be scrolled into view
// itself. Thus we are preferring to scroll the first child into view.
el = el?.shadowRoot?.firstElementChild ?? el;
@@ -1184,7 +1230,7 @@
<iron-icon class="expandIcon" icon="${icon}"></iron-icon>
<div class="statusIconWrapper">
<iron-icon
- icon="gr-icons:${iconForCategory(category)}"
+ icon="gr-icons:${iconFor(category)}"
class="statusIcon ${catString}"
></iron-icon>
<span class="title">${catString}</span>
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-runs.ts b/polygerrit-ui/app/elements/checks/gr-checks-runs.ts
index 4f9fd1d..0cf47cd 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-runs.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-runs.ts
@@ -33,7 +33,7 @@
AttemptDetail,
compareByWorstCategory,
fireActionTriggered,
- iconForCategory,
+ iconFor,
iconForRun,
PRIMARY_STATUS_ACTIONS,
primaryRunAction,
@@ -317,7 +317,7 @@
if (this.run.status !== RunStatus.RUNNING) return nothing;
const category = worstCategory(this.run);
if (!category) return nothing;
- const icon = iconForCategory(category);
+ const icon = iconFor(category);
return html`
<iron-icon class="${icon}" icon="gr-icons:${icon}"></iron-icon>
`;
diff --git a/polygerrit-ui/app/elements/checks/gr-hovercard-run.ts b/polygerrit-ui/app/elements/checks/gr-hovercard-run.ts
index 59b9de8..9d50b1c 100644
--- a/polygerrit-ui/app/elements/checks/gr-hovercard-run.ts
+++ b/polygerrit-ui/app/elements/checks/gr-hovercard-run.ts
@@ -22,8 +22,7 @@
import './gr-checks-action';
import {CheckRun} from '../../services/checks/checks-model';
import {
- iconForCategory,
- iconForStatus,
+ iconFor,
runActions,
worstCategory,
} from '../../services/checks/checks-util';
@@ -43,9 +42,9 @@
computeIcon(run?: CheckRun) {
if (!run) return '';
const category = worstCategory(run);
- if (category) return iconForCategory(category);
+ if (category) return iconFor(category);
return run.status === RunStatus.COMPLETED
- ? iconForStatus(RunStatus.COMPLETED)
+ ? iconFor(RunStatus.COMPLETED)
: '';
}
diff --git a/polygerrit-ui/app/elements/plugins/gr-checks-api/gr-checks-api.ts b/polygerrit-ui/app/elements/plugins/gr-checks-api/gr-checks-api.ts
index 39d3c8b..087779e 100644
--- a/polygerrit-ui/app/elements/plugins/gr-checks-api/gr-checks-api.ts
+++ b/polygerrit-ui/app/elements/plugins/gr-checks-api/gr-checks-api.ts
@@ -19,6 +19,8 @@
ChecksApiConfig,
ChecksProvider,
ChecksPluginApi,
+ CheckResult,
+ CheckRun,
} from '../../../api/checks';
import {appContext} from '../../../services/app-context';
@@ -54,6 +56,13 @@
this.checksService.reload(this.plugin.getPluginName());
}
+ updateResult(run: CheckRun, result: CheckResult) {
+ if (result.externalId === undefined) {
+ throw new Error('ChecksApi.updateResult() was called without externalId');
+ }
+ this.checksService.updateResult(this.plugin.getPluginName(), run, result);
+ }
+
register(provider: ChecksProvider, config?: ChecksApiConfig): void {
this.reporting.trackApi(this.plugin, 'checks', 'register');
if (this.state === State.REGISTERED)
diff --git a/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts b/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts
index 8968e18..519c24d 100644
--- a/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts
+++ b/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts
@@ -148,8 +148,10 @@
<g id="playArrow"><path d="M0 0h24v24H0z" fill="none"/><path d="M8 5v14l11-7z"/></g>
<!-- This SVG is a copy from material.io https://fonts.google.com/icons?selected=Material%20Icons%3Apause-->
<g id="pause"><path d="M0 0h24v24H0z" fill="none"/><path d="M6 19h4V5H6v14zm8-14v14h4V5h-4z"/></g>
- <!-- This SVG is a copy from material.io https://material.io/icons/#code-->
+ <!-- This SVG is a copy from material.io https://fonts.google.com/icons?selected=Material%20Icons%3Acode-->
<g id="code"><path d="M0 0h24v24H0V0z" fill="none"/><path d="M9.4 16.6L4.8 12l4.6-4.6L8 6l-6 6 6 6 1.4-1.4zm5.2 0l4.6-4.6-4.6-4.6L16 6l6 6-6 6-1.4-1.4z"/></g>
+ <!-- This SVG is a copy from material.io https://fonts.google.com/icons?selected=Material%20Icons%3Afile_present-->
+ <g id="file-present"><path d="M0 0h24v24H0V0z" fill="none"/><path d="M15 2H6c-1.1 0-2 .9-2 2v16c0 1.1.9 2 2 2h12c1.1 0 2-.9 2-2V7l-5-5zM6 20V4h8v4h4v12H6zm10-10v5c0 2.21-1.79 4-4 4s-4-1.79-4-4V8.5c0-1.47 1.26-2.64 2.76-2.49 1.3.13 2.24 1.32 2.24 2.63V15h-2V8.5c0-.28-.22-.5-.5-.5s-.5.22-.5.5V15c0 1.1.9 2 2 2s2-.9 2-2v-5h2z"/></g>
</defs>
</svg>
</iron-iconset-svg>`;
diff --git a/polygerrit-ui/app/services/checks/checks-model.ts b/polygerrit-ui/app/services/checks/checks-model.ts
index 570d80a..3a0bbf2 100644
--- a/polygerrit-ui/app/services/checks/checks-model.ts
+++ b/polygerrit-ui/app/services/checks/checks-model.ts
@@ -114,7 +114,20 @@
pluginStateSelected: {},
};
-const privateState$ = new BehaviorSubject(initialState);
+// Mutable for testing
+let privateState$ = new BehaviorSubject(initialState);
+
+export function _testOnly_resetState() {
+ privateState$ = new BehaviorSubject(initialState);
+}
+
+export function _testOnly_setState(state: ChecksState) {
+ privateState$.next(state);
+}
+
+export function _testOnly_getState() {
+ return privateState$.getValue();
+}
// Re-exporting as Observable so that you can only subscribe, but not emit.
export const checksState$: Observable<ChecksState> = privateState$;
@@ -533,6 +546,14 @@
internalResultId: 'f44r0',
category: Category.INFO,
summary: 'Dont be afraid. All TODOs will be eliminated.',
+ actions: [
+ {
+ name: 'Re-Run',
+ tooltip: 'More powerful run than before with a long tooltip, really.',
+ primary: true,
+ callback: () => Promise.resolve({message: 'fake "re-run" triggered'}),
+ },
+ ],
},
],
};
@@ -711,6 +732,42 @@
privateState$.next(nextState);
}
+export function updateStateUpdateResult(
+ pluginName: string,
+ updatedRun: CheckRunApi,
+ updatedResult: CheckResultApi,
+ patchset: ChecksPatchset
+) {
+ const nextState = {...privateState$.getValue()};
+ const pluginState = getPluginState(nextState, patchset);
+ let runUpdated = false;
+ const runs: CheckRun[] = pluginState[pluginName].runs.map(run => {
+ if (run.change !== updatedRun.change) return run;
+ if (run.patchset !== updatedRun.patchset) return run;
+ if (run.attempt !== updatedRun.attempt) return run;
+ if (run.checkName !== updatedRun.checkName) return run;
+ let resultUpdated = false;
+ const results: CheckResult[] = (run.results ?? []).map(result => {
+ if (result.externalId && result.externalId === updatedResult.externalId) {
+ runUpdated = true;
+ resultUpdated = true;
+ return {
+ ...updatedResult,
+ internalResultId: result.internalResultId,
+ };
+ }
+ return result;
+ });
+ return resultUpdated ? {...run, results} : run;
+ });
+ if (!runUpdated) return;
+ pluginState[pluginName] = {
+ ...pluginState[pluginName],
+ runs,
+ };
+ privateState$.next(nextState);
+}
+
export function updateStateSetPatchset(patchsetNumber?: PatchSetNumber) {
const nextState = {...privateState$.getValue()};
nextState.patchsetNumberSelected = patchsetNumber;
diff --git a/polygerrit-ui/app/services/checks/checks-model_test.ts b/polygerrit-ui/app/services/checks/checks-model_test.ts
new file mode 100644
index 0000000..f05facb
--- /dev/null
+++ b/polygerrit-ui/app/services/checks/checks-model_test.ts
@@ -0,0 +1,91 @@
+/**
+ * @license
+ * Copyright (C) 2021 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.
+ */
+import '../../test/common-test-setup-karma';
+import './checks-model';
+import {
+ _testOnly_getState,
+ _testOnly_resetState,
+ ChecksPatchset,
+ updateStateSetProvider,
+ updateStateSetResults,
+ updateStateUpdateResult,
+} from './checks-model';
+import {Category, CheckRun, RunStatus} from '../../api/checks';
+
+const PLUGIN_NAME = 'test-plugin';
+
+const RUNS: CheckRun[] = [
+ {
+ checkName: 'MacCheck',
+ change: 123,
+ patchset: 1,
+ attempt: 1,
+ status: RunStatus.COMPLETED,
+ results: [
+ {
+ externalId: 'id-314',
+ category: Category.WARNING,
+ summary: 'Meddle cheddle check and you are weg.',
+ },
+ ],
+ },
+];
+
+suite('checks-model tests', () => {
+ test('updateStateSetProvider', () => {
+ _testOnly_resetState();
+ updateStateSetProvider(PLUGIN_NAME, ChecksPatchset.LATEST);
+ const state = _testOnly_getState().pluginStateLatest[PLUGIN_NAME];
+ assert.deepEqual(state, {
+ pluginName: PLUGIN_NAME,
+ loading: false,
+ runs: [],
+ actions: [],
+ links: [],
+ });
+ });
+
+ test('updateStateSetResults', () => {
+ _testOnly_resetState();
+ updateStateSetResults(PLUGIN_NAME, RUNS, [], [], ChecksPatchset.LATEST);
+ const state = _testOnly_getState().pluginStateLatest[PLUGIN_NAME];
+ assert.lengthOf(state.runs, 1);
+ assert.lengthOf(state.runs[0].results!, 1);
+ });
+
+ test('updateStateUpdateResult', () => {
+ _testOnly_resetState();
+ updateStateSetResults(PLUGIN_NAME, RUNS, [], [], ChecksPatchset.LATEST);
+ let state = _testOnly_getState().pluginStateLatest[PLUGIN_NAME];
+ assert.equal(
+ state.runs[0].results![0].summary,
+ RUNS[0]!.results![0].summary
+ );
+ const result = RUNS[0].results![0];
+ const updatedResult = {...result, summary: 'new'};
+ updateStateUpdateResult(
+ PLUGIN_NAME,
+ RUNS[0],
+ updatedResult,
+ ChecksPatchset.LATEST
+ );
+ state = _testOnly_getState().pluginStateLatest[PLUGIN_NAME];
+ assert.lengthOf(state.runs, 1);
+ assert.lengthOf(state.runs[0].results!, 1);
+ assert.equal(state.runs[0].results![0].summary, 'new');
+ });
+});
diff --git a/polygerrit-ui/app/services/checks/checks-service.ts b/polygerrit-ui/app/services/checks/checks-service.ts
index bc05484..7844fa6 100644
--- a/polygerrit-ui/app/services/checks/checks-service.ts
+++ b/polygerrit-ui/app/services/checks/checks-service.ts
@@ -25,6 +25,8 @@
} from 'rxjs/operators';
import {
ChangeData,
+ CheckResult,
+ CheckRun,
ChecksApiConfig,
ChecksProvider,
FetchResponse,
@@ -41,6 +43,7 @@
updateStateSetPatchset,
updateStateSetProvider,
updateStateSetResults,
+ updateStateUpdateResult,
} from './checks-model';
import {
BehaviorSubject,
@@ -112,6 +115,11 @@
if (plugin) this.reload(plugin);
}
+ updateResult(pluginName: string, run: CheckRun, result: CheckResult) {
+ updateStateUpdateResult(pluginName, run, result, ChecksPatchset.LATEST);
+ updateStateUpdateResult(pluginName, run, result, ChecksPatchset.SELECTED);
+ }
+
register(
pluginName: string,
provider: ChecksProvider,
@@ -179,15 +187,16 @@
)
.subscribe(response => {
switch (response.responseCode) {
- case ResponseCode.ERROR:
- assertIsDefined(response.errorMessage, 'errorMessage');
+ case ResponseCode.ERROR: {
+ const message = response.errorMessage ?? '-';
this.reporting.reportExecution(Execution.CHECKS_API_ERROR, {
plugin: pluginName,
- message: response.errorMessage,
+ message,
});
- updateStateSetError(pluginName, response.errorMessage, patchset);
+ updateStateSetError(pluginName, message, patchset);
break;
- case ResponseCode.NOT_LOGGED_IN:
+ }
+ case ResponseCode.NOT_LOGGED_IN: {
assertIsDefined(response.loginCallback, 'loginCallback');
this.reporting.reportExecution(Execution.CHECKS_API_NOT_LOGGED_IN, {
plugin: pluginName,
@@ -198,7 +207,8 @@
patchset
);
break;
- case ResponseCode.OK:
+ }
+ case ResponseCode.OK: {
updateStateSetResults(
pluginName,
response.runs ?? [],
@@ -207,6 +217,7 @@
patchset
);
break;
+ }
}
});
}
diff --git a/polygerrit-ui/app/services/checks/checks-util.ts b/polygerrit-ui/app/services/checks/checks-util.ts
index 9381f30..897308e 100644
--- a/polygerrit-ui/app/services/checks/checks-util.ts
+++ b/polygerrit-ui/app/services/checks/checks-util.ts
@@ -17,11 +17,11 @@
import {
Action,
Category,
- CheckRun as CheckRunApi,
CheckResult as CheckResultApi,
+ CheckRun as CheckRunApi,
+ Link,
LinkIcon,
RunStatus,
- Link,
} from '../../api/checks';
import {assertNever} from '../../utils/common-util';
import {CheckResult, CheckRun} from './checks-model';
@@ -45,6 +45,8 @@
return 'bug';
case LinkIcon.CODE:
return 'code';
+ case LinkIcon.FILE_PRESENT:
+ return 'file-present';
default:
// We don't throw an assertion error here, because plugins don't have to
// be written in TypeScript, so we may encounter arbitrary strings for
@@ -70,6 +72,10 @@
return 'Link to help page';
case LinkIcon.REPORT_BUG:
return 'Link for reporting a problem';
+ case LinkIcon.CODE:
+ return 'Link to code';
+ case LinkIcon.FILE_PRESENT:
+ return 'Link to file';
default:
// We don't throw an assertion error here, because plugins don't have to
// be written in TypeScript, so we may encounter arbitrary strings for
@@ -86,8 +92,37 @@
return undefined;
}
-export function iconForCategory(category: Category) {
- switch (category) {
+export function isStatus(catStat: Category | RunStatus) {
+ return (
+ catStat === RunStatus.COMPLETED ||
+ catStat === RunStatus.RUNNABLE ||
+ catStat === RunStatus.RUNNING
+ );
+}
+
+export function labelFor(catStat: Category | RunStatus) {
+ switch (catStat) {
+ case Category.ERROR:
+ return 'error';
+ case Category.INFO:
+ return 'info';
+ case Category.WARNING:
+ return 'warning';
+ case Category.SUCCESS:
+ return 'success';
+ case RunStatus.COMPLETED:
+ return 'completed';
+ case RunStatus.RUNNABLE:
+ return 'runnable';
+ case RunStatus.RUNNING:
+ return 'running';
+ default:
+ assertNever(catStat, `Unsupported category/status: ${catStat}`);
+ }
+}
+
+export function iconFor(catStat: Category | RunStatus) {
+ switch (catStat) {
case Category.ERROR:
return 'error';
case Category.INFO:
@@ -96,8 +131,15 @@
return 'warning';
case Category.SUCCESS:
return 'check-circle-outline';
+ // Note that this is only for COMPLETED without results!
+ case RunStatus.COMPLETED:
+ return 'check-circle-outline';
+ case RunStatus.RUNNABLE:
+ return 'placeholder';
+ case RunStatus.RUNNING:
+ return 'timelapse';
default:
- assertNever(category, `Unsupported category: ${category}`);
+ assertNever(catStat, `Unsupported category/status: ${catStat}`);
}
}
@@ -145,24 +187,10 @@
export function iconForRun(run: CheckRun) {
if (run.status !== RunStatus.COMPLETED) {
- return iconForStatus(run.status);
+ return iconFor(run.status);
} else {
const category = worstCategory(run);
- return category ? iconForCategory(category) : iconForStatus(run.status);
- }
-}
-
-export function iconForStatus(status: RunStatus) {
- switch (status) {
- // Note that this is only for COMPLETED without results!
- case RunStatus.COMPLETED:
- return 'check-circle-outline';
- case RunStatus.RUNNABLE:
- return 'placeholder';
- case RunStatus.RUNNING:
- return 'timelapse';
- default:
- assertNever(status, `Unsupported status: ${status}`);
+ return category ? iconFor(category) : iconFor(run.status);
}
}
diff --git a/polygerrit-ui/app/utils/comment-util.ts b/polygerrit-ui/app/utils/comment-util.ts
index 2d5f66e..fd276c3 100644
--- a/polygerrit-ui/app/utils/comment-util.ts
+++ b/polygerrit-ui/app/utils/comment-util.ts
@@ -27,6 +27,7 @@
ContextLine,
BasePatchSetNum,
RevisionPatchSetNum,
+ AccountInfo,
} from '../types/common';
import {CommentSide, Side, SpecialFilePath} from '../constants/constants';
import {parseDate} from './date-util';
@@ -330,3 +331,18 @@
};
return diff;
}
+
+export function getCommentAuthors(threads?: CommentThread[]) {
+ if (!threads) return [];
+ const ids = new Set();
+ const authors: AccountInfo[] = [];
+ threads.forEach(t =>
+ t.comments.forEach(c => {
+ if (c.author && !ids.has(c.author._account_id)) {
+ ids.add(c.author._account_id);
+ authors.push(c.author);
+ }
+ })
+ );
+ return authors;
+}
diff --git a/polygerrit-ui/app/utils/dom-util.ts b/polygerrit-ui/app/utils/dom-util.ts
index 4f83881..e7cc956 100644
--- a/polygerrit-ui/app/utils/dom-util.ts
+++ b/polygerrit-ui/app/utils/dom-util.ts
@@ -294,6 +294,11 @@
}
}
+export function modifierPressed(e: KeyboardEvent) {
+ return e.altKey || e.ctrlKey || e.metaKey || e.shiftKey;
+}
+
+// Deprecated. Try using "normal" KeyboardEvent and modifierPressed() above.
export function isModifierPressed(event: CustomKeyboardEvent) {
const e = getKeyboardEvent(event);
return e.altKey || e.ctrlKey || e.metaKey || e.shiftKey;
