Implement patchset picker for new Checks UI
Change-Id: Ie57b8c8c39d9350cec475d5936da42bcf137da0b
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-tab.ts b/polygerrit-ui/app/elements/checks/gr-checks-tab.ts
index e5386f4..9376a03 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-tab.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-tab.ts
@@ -28,18 +28,23 @@
allActions$,
allResults$,
allRuns$,
+ checksPatchsetNumber$,
someProvidersAreLoading$,
} from '../../services/checks/checks-model';
import './gr-checks-runs';
import './gr-checks-results';
import {sharedStyles} from '../../styles/shared-styles';
-import {changeNum$, currentPatchNum$} from '../../services/change/change-model';
-import {NumericChangeId, PatchSetNum} from '../../types/common';
+import {changeNum$, latestPatchNum$} from '../../services/change/change-model';
+import {NumericChangeId, PatchSetNumber} from '../../types/common';
import {
ActionTriggeredEvent,
fireActionTriggered,
} from '../../services/checks/checks-util';
-import {checkRequiredProperty} from '../../utils/common-util';
+import {
+ assertIsDefined,
+ check,
+ checkRequiredProperty,
+} from '../../utils/common-util';
import {RunSelectedEvent} from './gr-checks-runs';
import {ChecksTabState} from '../../types/events';
import {fireAlert} from '../../utils/event-util';
@@ -64,7 +69,10 @@
tabState?: ChecksTabState;
@property()
- currentPatchNum: PatchSetNum | undefined = undefined;
+ checksPatchsetNumber: PatchSetNumber | undefined = undefined;
+
+ @property()
+ latestPatchsetNumber: PatchSetNumber | undefined = undefined;
@property()
changeNum: NumericChangeId | undefined = undefined;
@@ -82,7 +90,8 @@
this.subscribe('runs', allRuns$);
this.subscribe('actions', allActions$);
this.subscribe('results', allResults$);
- this.subscribe('currentPatchNum', currentPatchNum$);
+ this.subscribe('checksPatchsetNumber', checksPatchsetNumber$);
+ this.subscribe('latestPatchsetNumber', latestPatchNum$);
this.subscribe('changeNum', changeNum$);
this.subscribe('someProvidersAreLoading', someProvidersAreLoading$);
@@ -124,7 +133,6 @@
}
render() {
- const ps = `Patchset ${this.currentPatchNum} (Latest)`;
const filteredRuns = this.runs.filter(
r =>
this.selectedRuns.length === 0 ||
@@ -134,13 +142,9 @@
<div class="header">
<div class="left">
<gr-dropdown-list
- value="${ps}"
- .items="${[
- {
- value: `${ps}`,
- text: `${ps}`,
- },
- ]}"
+ value="${this.checksPatchsetNumber}"
+ .items="${this.createPatchsetDropdownItems()}"
+ @value-change="${this.onPatchsetSelected}"
></gr-dropdown-list>
<span ?hidden="${!this.someProvidersAreLoading}">Loading...</span>
</div>
@@ -163,6 +167,24 @@
`;
}
+ private onPatchsetSelected(e: CustomEvent<{value: string}>) {
+ const patchset = Number(e.detail.value);
+ check(!isNaN(patchset), 'selected patchset must be a number');
+ this.checksService.setPatchset(patchset as PatchSetNumber);
+ }
+
+ private createPatchsetDropdownItems() {
+ return Array.from(Array(this.latestPatchsetNumber), (_, i) => {
+ assertIsDefined(this.latestPatchsetNumber, 'latestPatchsetNumber');
+ const index = this.latestPatchsetNumber - i;
+ const postfix = index === this.latestPatchsetNumber ? ' (latest)' : '';
+ return {
+ value: `${index}`,
+ text: `Patchset ${index}${postfix}`,
+ };
+ });
+ }
+
protected updated(changedProperties: PropertyValues) {
super.updated(changedProperties);
if (changedProperties.has('tabState')) {
@@ -181,10 +203,10 @@
handleActionTriggered(action: Action, run?: CheckRun) {
if (!this.changeNum) return;
- if (!this.currentPatchNum) return;
+ if (!this.checksPatchsetNumber) return;
const promise = action.callback(
this.changeNum,
- this.currentPatchNum as number,
+ this.checksPatchsetNumber,
run?.attempt,
run?.externalId,
run?.checkName,
diff --git a/polygerrit-ui/app/services/change/change-model.ts b/polygerrit-ui/app/services/change/change-model.ts
index b2bdcfe..e7472de 100644
--- a/polygerrit-ui/app/services/change/change-model.ts
+++ b/polygerrit-ui/app/services/change/change-model.ts
@@ -46,10 +46,18 @@
// Must only be used by the change service or whatever is in control of this
// model.
export function updateState(change?: ParsedChangeInfo) {
- privateState$.next({
- ...privateState$.getValue(),
- change,
- });
+ const current = privateState$.getValue();
+ // We want to make it easy for subscribers to react to change changes, so we
+ // are explicitly emitting and additional `undefined` when the change number
+ // changes. So if you are subscribed to the latestPatchsetNumber for example,
+ // then you can rely on emissions even if the old and the new change have the
+ // same latestPatchsetNumber.
+ if (change !== undefined && current.change !== undefined) {
+ if (change._number !== current.change._number) {
+ privateState$.next({...current, change: undefined});
+ }
+ }
+ privateState$.next({...current, change});
}
/**
@@ -91,9 +99,6 @@
*
* Note that this selector can emit a patchNum without the change being
* available!
- *
- * TODO: It would be good to assert/enforce somehow that currentPatchNum$ cannot
- * emit 'PARENT'.
*/
export const currentPatchNum$: Observable<
PatchSetNum | undefined
diff --git a/polygerrit-ui/app/services/change/change-service.ts b/polygerrit-ui/app/services/change/change-service.ts
index 4524813..c292fb5 100644
--- a/polygerrit-ui/app/services/change/change-service.ts
+++ b/polygerrit-ui/app/services/change/change-service.ts
@@ -16,21 +16,16 @@
*/
import {routerChangeNum$} from '../router/router-model';
import {updateState} from './change-model';
-import {tap} from 'rxjs/operators';
import {ParsedChangeInfo} from '../../types/types';
export class ChangeService {
- // TODO: In the future we will want to make restApiService.getChangeDetail()
- // calls from a switchMap() here. For now just make sure to invalidate the
- // change when no changeNum is set.
- private routerChangeNumEffect = routerChangeNum$.pipe(
- tap(changeNum => {
- if (!changeNum) updateState(undefined);
- })
- );
-
constructor() {
- this.routerChangeNumEffect.subscribe();
+ // TODO: In the future we will want to make restApiService.getChangeDetail()
+ // calls from a switchMap() here. For now just make sure to invalidate the
+ // change when no changeNum is set.
+ routerChangeNum$.subscribe(changeNum => {
+ if (!changeNum) updateState(undefined);
+ });
}
/**
diff --git a/polygerrit-ui/app/services/checks/checks-model.ts b/polygerrit-ui/app/services/checks/checks-model.ts
index d8dc688..1c5b862 100644
--- a/polygerrit-ui/app/services/checks/checks-model.ts
+++ b/polygerrit-ui/app/services/checks/checks-model.ts
@@ -26,6 +26,7 @@
RunStatus,
} from '../../api/checks';
import {distinctUntilChanged, map} from 'rxjs/operators';
+import {PatchSetNumber} from '../../types/common';
// This is a convenience type for working with results, because when working
// with a bunch of results you will typically also want to know about the run
@@ -41,29 +42,44 @@
}
interface ChecksState {
- [name: string]: ChecksProviderState;
+ patchsetNumber?: PatchSetNumber;
+ providerNameToState: {
+ [name: string]: ChecksProviderState;
+ };
}
-const initialState: ChecksState = {};
+const initialState: ChecksState = {
+ providerNameToState: {},
+};
const privateState$ = new BehaviorSubject(initialState);
// Re-exporting as Observable so that you can only subscribe, but not emit.
export const checksState$: Observable<ChecksState> = privateState$;
-export const aPluginHasRegistered$ = checksState$.pipe(
+export const checksPatchsetNumber$ = checksState$.pipe(
+ map(state => state.patchsetNumber),
+ distinctUntilChanged()
+);
+
+export const checksProviderState$ = checksState$.pipe(
+ map(state => state.providerNameToState),
+ distinctUntilChanged()
+);
+
+export const aPluginHasRegistered$ = checksProviderState$.pipe(
map(state => Object.keys(state).length > 0),
distinctUntilChanged()
);
-export const someProvidersAreLoading$ = checksState$.pipe(
+export const someProvidersAreLoading$ = checksProviderState$.pipe(
map(state => {
return Object.values(state).some(providerState => providerState.loading);
}),
distinctUntilChanged()
);
-export const allActions$ = checksState$.pipe(
+export const allActions$ = checksProviderState$.pipe(
map(state => {
return Object.values(state).reduce(
(allActions: Action[], providerState: ChecksProviderState) => [
@@ -75,7 +91,7 @@
})
);
-export const allRuns$ = checksState$.pipe(
+export const allRuns$ = checksProviderState$.pipe(
map(state => {
return Object.values(state).reduce(
(allRuns: CheckRun[], providerState: ChecksProviderState) => [
@@ -87,7 +103,7 @@
})
);
-export const checkToPluginMap$ = checksState$.pipe(
+export const checkToPluginMap$ = checksProviderState$.pipe(
map(state => {
const map = new Map<string, string>();
for (const [pluginName, providerState] of Object.entries(state)) {
@@ -99,7 +115,7 @@
})
);
-export const allResults$ = checksState$.pipe(
+export const allResults$ = checksProviderState$.pipe(
map(state => {
return Object.values(state)
.reduce(
@@ -124,7 +140,8 @@
config?: ChecksApiConfig
) {
const nextState = {...privateState$.getValue()};
- nextState[pluginName] = {
+ nextState.providerNameToState = {...nextState.providerNameToState};
+ nextState.providerNameToState[pluginName] = {
pluginName,
loading: false,
config,
@@ -200,8 +217,9 @@
export function updateStateSetLoading(pluginName: string) {
const nextState = {...privateState$.getValue()};
- nextState[pluginName] = {
- ...nextState[pluginName],
+ nextState.providerNameToState = {...nextState.providerNameToState};
+ nextState.providerNameToState[pluginName] = {
+ ...nextState.providerNameToState[pluginName],
loading: true,
};
privateState$.next(nextState);
@@ -213,11 +231,18 @@
actions: Action[] = []
) {
const nextState = {...privateState$.getValue()};
- nextState[pluginName] = {
- ...nextState[pluginName],
+ nextState.providerNameToState = {...nextState.providerNameToState};
+ nextState.providerNameToState[pluginName] = {
+ ...nextState.providerNameToState[pluginName],
loading: false,
runs: [...runs],
actions: [...actions],
};
privateState$.next(nextState);
}
+
+export function updateStateSetPatchset(patchsetNumber?: PatchSetNumber) {
+ const nextState = {...privateState$.getValue()};
+ nextState.patchsetNumber = patchsetNumber;
+ privateState$.next(nextState);
+}
diff --git a/polygerrit-ui/app/services/checks/checks-service.ts b/polygerrit-ui/app/services/checks/checks-service.ts
index d2a4775..3ce714a 100644
--- a/polygerrit-ui/app/services/checks/checks-service.ts
+++ b/polygerrit-ui/app/services/checks/checks-service.ts
@@ -18,7 +18,6 @@
import {
switchMap,
takeWhile,
- tap,
throttleTime,
withLatestFrom,
} from 'rxjs/operators';
@@ -29,12 +28,14 @@
FetchResponse,
ResponseCode,
} from '../../api/checks';
-import {change$, currentPatchNum$} from '../change/change-model';
+import {change$, changeNum$, latestPatchNum$} from '../change/change-model';
import {
updateStateSetLoading,
checkToPluginMap$,
updateStateSetProvider,
updateStateSetResults,
+ checksPatchsetNumber$,
+ updateStateSetPatchset,
} from './checks-model';
import {
BehaviorSubject,
@@ -44,18 +45,26 @@
of,
Subject,
} from 'rxjs';
+import {PatchSetNumber} from '../../types/common';
export class ChecksService {
private readonly providers: {[name: string]: ChecksProvider} = {};
private readonly reloadSubjects: {[name: string]: Subject<void>} = {};
- private changeAndPatchNum$ = change$.pipe(withLatestFrom(currentPatchNum$));
-
private checkToPluginMap = new Map<string, string>();
constructor() {
- checkToPluginMap$.subscribe(x => (this.checkToPluginMap = x));
+ checkToPluginMap$.subscribe(map => {
+ this.checkToPluginMap = map;
+ });
+ latestPatchNum$.subscribe(num => {
+ updateStateSetPatchset(num);
+ });
+ }
+
+ setPatchset(num: PatchSetNumber) {
+ updateStateSetPatchset(num);
}
reload(pluginName: string) {
@@ -82,37 +91,43 @@
updateStateSetProvider(pluginName, config);
// Both, changed numbers and and announceUpdate request should trigger.
combineLatest([
- this.changeAndPatchNum$,
+ changeNum$,
+ checksPatchsetNumber$,
this.reloadSubjects[pluginName].pipe(throttleTime(1000)),
])
.pipe(
takeWhile(_ => !!this.providers[pluginName]),
+ withLatestFrom(change$),
switchMap(
- ([[change, patchNum], _]): Observable<FetchResponse> => {
- if (!change || !patchNum || typeof patchNum !== 'number') {
+ ([[changeNum, patchNum, _], change]): Observable<FetchResponse> => {
+ if (
+ !change ||
+ !changeNum ||
+ !patchNum ||
+ typeof patchNum !== 'number'
+ ) {
return of({
responseCode: ResponseCode.OK,
runs: [],
});
}
const data: ChangeData = {
- changeNumber: change._number,
+ changeNumber: changeNum,
patchsetNumber: patchNum,
repo: change.project,
};
updateStateSetLoading(pluginName);
return from(this.providers[pluginName].fetch(data));
}
- ),
- tap(response => {
- updateStateSetResults(
- pluginName,
- response.runs ?? [],
- response.actions
- );
- })
+ )
)
- .subscribe(() => {});
+ .subscribe(response => {
+ updateStateSetResults(
+ pluginName,
+ response.runs ?? [],
+ response.actions
+ );
+ });
this.reload(pluginName);
}
}
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts
index 711b11b..1bfabf8 100644
--- a/polygerrit-ui/app/types/common.ts
+++ b/polygerrit-ui/app/types/common.ts
@@ -76,6 +76,7 @@
export type ParsedJSON = BrandType<unknown, '_parsedJSON'>;
export type PatchSetNum = BrandType<'PARENT' | 'edit' | number, '_patchSet'>;
+export type PatchSetNumber = BrandType<number, '_patchSet'>;
export const EditPatchSetNum = 'edit' as PatchSetNum;
// TODO(TS): This is not correct, it is better to have a separate ApiPatchSetNum
diff --git a/polygerrit-ui/app/utils/patch-set-util.ts b/polygerrit-ui/app/utils/patch-set-util.ts
index 40e3eef..af56798 100644
--- a/polygerrit-ui/app/utils/patch-set-util.ts
+++ b/polygerrit-ui/app/utils/patch-set-util.ts
@@ -3,11 +3,12 @@
ChangeInfo,
PatchSetNum,
EditPatchSetNum,
- BrandType,
ParentPatchSetNum,
+ PatchSetNumber,
} from '../types/common';
import {RestApiService} from '../services/gr-rest-api/gr-rest-api';
import {EditRevisionInfo, ParsedChangeInfo} from '../types/types';
+import {check} from './common-util';
/**
* @license
@@ -82,9 +83,7 @@
return patchset as PatchSetNum;
}
-export function isNumber(
- psn: PatchSetNum
-): psn is BrandType<number, '_patchSet'> {
+export function isNumber(psn: PatchSetNum): psn is PatchSetNumber {
return typeof psn === 'number';
}
@@ -250,14 +249,16 @@
export function computeLatestPatchNum(
allPatchSets?: PatchSet[]
-): PatchSetNum | undefined {
+): PatchSetNumber | undefined {
if (!allPatchSets || !allPatchSets.length) {
return undefined;
}
- if (allPatchSets[0].num === EditPatchSetNum) {
- return allPatchSets[1].num;
+ let latest = allPatchSets[0].num;
+ if (latest === EditPatchSetNum) {
+ latest = allPatchSets[1].num;
}
- return allPatchSets[0].num;
+ check(isNumber(latest), 'Latest patchset cannot be EDIT or PARENT.');
+ return latest;
}
export function computePredecessor(
