Merge "Move behavior from component into view model for change-list-view"
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts
index a58c7bb..6b74921 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts
@@ -7,7 +7,6 @@
import '../gr-repo-header/gr-repo-header';
import '../gr-user-header/gr-user-header';
import {page} from '../../../utils/page-wrapper-utils';
-import {navigationToken} from '../../core/gr-navigation/gr-navigation';
import {
AccountDetailInfo,
AccountId,
@@ -26,20 +25,9 @@
import {
createSearchUrl,
searchViewModelToken,
- SearchViewState,
} from '../../../models/views/search';
import {resolve} from '../../../models/dependency';
import {subscribe} from '../../lit/subscription-controller';
-import {createChangeUrl} from '../../../models/views/change';
-import {debounce, DelayedTask} from '../../../utils/async-util';
-
-const GET_CHANGES_DEBOUNCE_INTERVAL_MS = 10;
-
-const LOOKUP_QUERY_PATTERNS: RegExp[] = [
- /^\s*i?[0-9a-f]{7,40}\s*$/i, // CHANGE_ID
- /^\s*[1-9][0-9]*\s*$/g, // CHANGE_NUM
- /[0-9a-f]{40}/, // COMMIT
-];
const USER_QUERY_PATTERN = /^owner:\s?("[^"]+"|[^ ]+)$/;
@@ -60,21 +48,6 @@
@query('#nextArrow') protected nextArrow?: HTMLAnchorElement;
- private _viewState?: SearchViewState;
-
- @state()
- get viewState() {
- return this._viewState;
- }
-
- set viewState(viewState: SearchViewState | undefined) {
- if (this._viewState === viewState) return;
- const oldViewState = this._viewState;
- this._viewState = viewState;
- this.viewStateChanged();
- this.requestUpdate('viewState', oldViewState);
- }
-
// private but used in test
@state() account?: AccountDetailInfo;
@@ -91,19 +64,19 @@
@state() query = '';
// private but used in test
- @state() offset?: number;
+ @state() offset = 0;
// private but used in test
- @state() changes?: ChangeInfo[];
+ @state() changes: ChangeInfo[] = [];
// private but used in test
@state() loading = true;
// private but used in test
- @state() userId: AccountId | EmailAddress | null = null;
+ @state() userId?: AccountId | EmailAddress;
// private but used in test
- @state() repo: RepoName | null = null;
+ @state() repo?: RepoName;
@state() selectedIndex = 0;
@@ -115,17 +88,30 @@
private readonly getViewModel = resolve(this, searchViewModelToken);
- private readonly getNavigation = resolve(this, navigationToken);
-
constructor() {
super();
this.addEventListener('next-page', () => this.handleNextPage());
this.addEventListener('previous-page', () => this.handlePreviousPage());
- this.addEventListener('reload', () => this.reload());
+
subscribe(
this,
- () => this.getViewModel().state$,
- x => (this.viewState = x)
+ () => this.getViewModel().query$,
+ x => (this.query = x)
+ );
+ subscribe(
+ this,
+ () => this.getViewModel().offsetNumber$,
+ x => (this.offset = x)
+ );
+ subscribe(
+ this,
+ () => this.getViewModel().loading$,
+ x => (this.loading = x)
+ );
+ subscribe(
+ this,
+ () => this.getViewModel().changes$,
+ x => (this.changes = x)
);
subscribe(
this,
@@ -139,20 +125,14 @@
);
subscribe(
this,
- () => this.userModel.preferences$,
- x => {
- this.preferences = x;
- if (this.changesPerPage !== x.changes_per_page) {
- this.changesPerPage = x.changes_per_page;
- this.debouncedGetChanges();
- }
- }
+ () => this.userModel.preferenceChangesPerPage$,
+ x => (this.changesPerPage = x)
);
- }
-
- override disconnectedCallback() {
- this.getChangesTask?.flush();
- super.disconnectedCallback();
+ subscribe(
+ this,
+ () => this.userModel.preferences$,
+ x => (this.preferences = x)
+ );
}
static override get styles() {
@@ -282,63 +262,10 @@
}
}
- reload() {
- if (!this.loading) this.debouncedGetChanges();
- }
-
- // private, but visible for testing
- viewStateChanged() {
- if (!this.viewState) return;
-
- let offset = Number(this.viewState.offset);
- if (isNaN(offset)) offset = 0;
- const query = this.viewState.query ?? '';
-
- if (this.query !== query) this.selectedIndex = 0;
- this.loading = true;
- this.query = query;
- this.offset = offset;
-
- // NOTE: This method may be called before attachment. Fire title-change
- // in an async so that attachment to the DOM can take place first.
- setTimeout(() => fireTitleChange(this, this.query));
-
- this.debouncedGetChanges(true);
- }
-
- private getChangesTask?: DelayedTask;
-
- private debouncedGetChanges(shouldSingleMatchRedirect = false) {
- this.getChangesTask = debounce(
- this.getChangesTask,
- () => {
- this.getChanges(shouldSingleMatchRedirect);
- },
- GET_CHANGES_DEBOUNCE_INTERVAL_MS
- );
- }
-
- async getChanges(shouldSingleMatchRedirect = false) {
- this.loading = true;
- const changes =
- (await this.restApiService.getChanges(
- this.changesPerPage,
- this.query,
- this.offset
- )) ?? [];
- if (shouldSingleMatchRedirect && this.query && changes.length === 1) {
- for (const queryPattern of LOOKUP_QUERY_PATTERNS) {
- if (this.query.match(queryPattern)) {
- // "Back"/"Forward" buttons work correctly only with replaceUrl()
- this.getNavigation().replaceUrl(
- createChangeUrl({change: changes[0]})
- );
- return;
- }
- }
+ override updated(changedProperties: PropertyValues) {
+ if (changedProperties.has('query')) {
+ fireTitleChange(this, this.query);
}
- this.changes = changes;
- this.loading = false;
}
// private but used in test
@@ -372,14 +299,12 @@
}
private changesChanged() {
- this.userId = null;
- this.repo = null;
- const changes = this.changes;
- if (!changes || !changes.length) {
- return;
- }
+ this.selectedIndex = 0;
+ this.userId = undefined;
+ this.repo = undefined;
+ if (this.changes.length === 0) return;
if (USER_QUERY_PATTERN.test(this.query)) {
- const owner = changes[0].owner;
+ const owner = this.changes[0].owner;
const userId = owner._account_id ? owner._account_id : owner.email;
if (userId) {
this.userId = userId;
@@ -387,7 +312,7 @@
}
}
if (REPO_QUERY_PATTERN.test(this.query)) {
- this.repo = changes[0].project;
+ this.repo = this.changes[0].project;
}
}
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view_test.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view_test.ts
index b003b66..4dc3701 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view_test.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view_test.ts
@@ -7,45 +7,20 @@
import './gr-change-list-view';
import {GrChangeListView} from './gr-change-list-view';
import {page} from '../../../utils/page-wrapper-utils';
-import {navigationToken} from '../../core/gr-navigation/gr-navigation';
-import {
- query,
- stubRestApi,
- queryAndAssert,
- stubFlags,
-} from '../../../test/test-utils';
+import {query, queryAndAssert} from '../../../test/test-utils';
import {createChange} from '../../../test/test-data-generators';
-import {
- ChangeInfo,
- EmailAddress,
- NumericChangeId,
- RepoName,
-} from '../../../api/rest-api';
+import {ChangeInfo, EmailAddress, RepoName} from '../../../api/rest-api';
import {fixture, html, waitUntil, assert} from '@open-wc/testing';
-import {GerritView} from '../../../services/router/router-model';
-import {testResolver} from '../../../test/common-test-setup';
-import {SinonFakeTimers, SinonStub} from 'sinon';
import {GrChangeList} from '../gr-change-list/gr-change-list';
import {GrChangeListSection} from '../gr-change-list-section/gr-change-list-section';
import {GrChangeListItem} from '../gr-change-list-item/gr-change-list-item';
-const CHANGE_ID = 'IcA3dAB3edAB9f60B8dcdA6ef71A75980e4B7127';
-const COMMIT_HASH = '12345678';
-
suite('gr-change-list-view tests', () => {
let element: GrChangeListView;
- let changes: ChangeInfo[] | undefined = [];
- let clock: SinonFakeTimers;
setup(async () => {
- clock = sinon.useFakeTimers();
- stubRestApi('getChanges').callsFake(() => Promise.resolve(changes));
element = await fixture(html`<gr-change-list-view></gr-change-list-view>`);
- element.viewState = {
- view: GerritView.SEARCH,
- query: 'test-query',
- offset: '0',
- };
+ element.query = 'test-query';
await element.updateComplete;
});
@@ -75,11 +50,9 @@
suite('bulk actions', () => {
setup(async () => {
- stubFlags('isEnabled').returns(true);
- changes = [createChange()];
element.loading = false;
- element.reload();
- clock.tick(100);
+ element.changes = [createChange()];
+ await element.updateComplete;
await element.updateComplete;
await waitUntil(() => element.loading === false);
});
@@ -107,8 +80,9 @@
checkbox.click();
await waitUntil(() => checkbox.checked);
- element.reload();
+ element.changes = [createChange()];
await element.updateComplete;
+
checkbox = queryAndAssert<HTMLInputElement>(
query(
query(query(element, 'gr-change-list'), 'gr-change-list-section'),
@@ -222,7 +196,7 @@
});
test('userId query', async () => {
- assert.isNull(element.userId);
+ assert.isUndefined(element.userId);
element.query = 'owner: foo@bar';
element.changes = [
{...createChange(), owner: {email: 'foo@bar' as EmailAddress}},
@@ -235,19 +209,19 @@
{...createChange(), owner: {email: 'foo@bar' as EmailAddress}},
];
await element.updateComplete;
- assert.isNull(element.userId);
+ assert.isUndefined(element.userId);
});
test('userId query without email', async () => {
- assert.isNull(element.userId);
+ assert.isUndefined(element.userId);
element.query = 'owner: foo@bar';
element.changes = [{...createChange(), owner: {}}];
await element.updateComplete;
- assert.isNull(element.userId);
+ assert.isUndefined(element.userId);
});
test('repo query', async () => {
- assert.isNull(element.repo);
+ assert.isUndefined(element.repo);
element.query = 'project: test-repo';
element.changes = [
{
@@ -264,11 +238,11 @@
{...createChange(), owner: {email: 'foo@bar' as EmailAddress}},
];
await element.updateComplete;
- assert.isNull(element.repo);
+ assert.isUndefined(element.repo);
});
test('repo query with open status', async () => {
- assert.isNull(element.repo);
+ assert.isUndefined(element.repo);
element.query = 'project:test-repo status:open';
element.changes = [
{
@@ -285,74 +259,6 @@
{...createChange(), owner: {email: 'foo@bar' as EmailAddress}},
];
await element.updateComplete;
- assert.isNull(element.repo);
- });
-
- suite('query based navigation', () => {
- let replaceUrlStub: SinonStub;
- setup(() => {
- replaceUrlStub = sinon.stub(testResolver(navigationToken), 'replaceUrl');
- });
-
- teardown(async () => {
- await element.updateComplete;
- sinon.restore();
- });
-
- test('Searching for a change ID redirects to change', async () => {
- const change = {...createChange(), _number: 1 as NumericChangeId};
- changes = [change];
-
- element.viewState = {view: GerritView.SEARCH, query: CHANGE_ID};
- clock.tick(100);
- await element.updateComplete;
-
- assert.isTrue(replaceUrlStub.called);
- assert.equal(replaceUrlStub.lastCall.firstArg, '/c/test-project/+/1');
- });
-
- test('Searching for a change num redirects to change', async () => {
- const change = {...createChange(), _number: 1 as NumericChangeId};
- changes = [change];
-
- element.viewState = {view: GerritView.SEARCH, query: '1'};
- clock.tick(100);
- await element.updateComplete;
-
- assert.isTrue(replaceUrlStub.called);
- assert.equal(replaceUrlStub.lastCall.firstArg, '/c/test-project/+/1');
- });
-
- test('Commit hash redirects to change', async () => {
- const change = {...createChange(), _number: 1 as NumericChangeId};
- changes = [change];
-
- element.viewState = {view: GerritView.SEARCH, query: COMMIT_HASH};
- clock.tick(100);
- await element.updateComplete;
-
- assert.isTrue(replaceUrlStub.called);
- assert.equal(replaceUrlStub.lastCall.firstArg, '/c/test-project/+/1');
- });
-
- test('Searching for an invalid change ID searches', async () => {
- changes = [];
-
- element.viewState = {view: GerritView.SEARCH, query: CHANGE_ID};
- clock.tick(100);
- await element.updateComplete;
-
- assert.isFalse(replaceUrlStub.called);
- });
-
- test('Change ID with multiple search results searches', async () => {
- changes = undefined;
-
- element.viewState = {view: GerritView.SEARCH, query: CHANGE_ID};
- clock.tick(100);
- await element.updateComplete;
-
- assert.isFalse(replaceUrlStub.called);
- });
+ assert.isUndefined(element.repo);
});
});
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index 762f6b7..e40a161 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -1292,6 +1292,8 @@
view: GerritView.SEARCH,
query: ctx.params[0],
offset: ctx.params[2],
+ changes: [],
+ loading: false,
};
this.setState(state);
this.searchViewModel.setState(state);
@@ -1304,6 +1306,8 @@
const state: SearchViewState = {
view: GerritView.SEARCH,
query: ctx.params[0],
+ changes: [],
+ loading: false,
};
this.setState(state);
this.searchViewModel.setState(state);
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
index ce3ed0d..6c29336 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
@@ -403,6 +403,8 @@
view: GerritView.SEARCH,
query: 'project:foo/bar/baz',
offset: undefined,
+ changes: [],
+ loading: false,
});
ctx.params[1] = '123';
@@ -411,6 +413,8 @@
view: GerritView.SEARCH,
query: 'project:foo/bar/baz',
offset: '123',
+ changes: [],
+ loading: false,
});
});
@@ -429,6 +433,8 @@
assertctxToParams(ctx, 'handleChangeIdQueryRoute', {
view: GerritView.SEARCH,
query: 'I0123456789abcdef0123456789abcdef01234567',
+ changes: [],
+ loading: false,
});
});
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts
index 5cfe670..b4f16ab 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -14,15 +14,7 @@
RevisionPatchSetNum,
} from '../../types/common';
import {DefaultBase} from '../../constants/constants';
-import {
- combineLatest,
- from,
- fromEvent,
- Observable,
- Subscription,
- forkJoin,
- of,
-} from 'rxjs';
+import {combineLatest, from, fromEvent, Observable, forkJoin, of} from 'rxjs';
import {
map,
filter,
@@ -261,8 +253,6 @@
([change, account]) => isOwner(change, account)
);
- private subscriptions: Subscription[] = [];
-
// For usage in `combineLatest` we need `startWith` such that reload$ has an
// initial value.
readonly reload$: Observable<unknown> = fromEvent(document, 'reload').pipe(
@@ -316,13 +306,6 @@
];
}
- override finalize() {
- for (const s of this.subscriptions) {
- s.unsubscribe();
- }
- this.subscriptions = [];
- }
-
// Temporary workaround until path is derived in the model itself.
updatePath(diffPath?: string) {
this.updateState({diffPath});
diff --git a/polygerrit-ui/app/models/change/files-model.ts b/polygerrit-ui/app/models/change/files-model.ts
index 31e42a6..6922f6d 100644
--- a/polygerrit-ui/app/models/change/files-model.ts
+++ b/polygerrit-ui/app/models/change/files-model.ts
@@ -12,7 +12,7 @@
PatchSetNumber,
RevisionPatchSetNum,
} from '../../types/common';
-import {combineLatest, Subscription, of, from} from 'rxjs';
+import {combineLatest, of, from} from 'rxjs';
import {switchMap, map} from 'rxjs/operators';
import {RestApiService} from '../../services/gr-rest-api/gr-rest-api';
import {Finalizable} from '../../services/registry';
@@ -131,8 +131,6 @@
state => state.filesRightBase
);
- private subscriptions: Subscription[] = [];
-
constructor(
readonly changeModel: ChangeModel,
readonly commentsModel: CommentsModel,
@@ -198,11 +196,4 @@
this.updateState(state);
});
}
-
- override finalize() {
- for (const s of this.subscriptions) {
- s.unsubscribe();
- }
- this.subscriptions = [];
- }
}
diff --git a/polygerrit-ui/app/models/checks/checks-model.ts b/polygerrit-ui/app/models/checks/checks-model.ts
index bb46f27..61c2f77 100644
--- a/polygerrit-ui/app/models/checks/checks-model.ts
+++ b/polygerrit-ui/app/models/checks/checks-model.ts
@@ -20,7 +20,6 @@
Observable,
of,
Subject,
- Subscription,
timer,
} from 'rxjs';
import {
@@ -208,8 +207,6 @@
private readonly visibilityChangeListener: () => void;
- private subscriptions: Subscription[] = [];
-
public checksSelectedPatchsetNumber$ = select(
this.state$,
state => state.patchsetNumberSelected
@@ -483,10 +480,7 @@
'visibilitychange',
this.visibilityChangeListener
);
- for (const s of this.subscriptions) {
- s.unsubscribe();
- }
- this.subscriptions = [];
+ super.finalize();
}
// Must only be used by the checks service or whatever is in control of this
diff --git a/polygerrit-ui/app/models/comments/comments-model.ts b/polygerrit-ui/app/models/comments/comments-model.ts
index 39d9eaa..8ed4b69 100644
--- a/polygerrit-ui/app/models/comments/comments-model.ts
+++ b/polygerrit-ui/app/models/comments/comments-model.ts
@@ -30,7 +30,7 @@
import {RouterModel} from '../../services/router/router-model';
import {Finalizable} from '../../services/registry';
import {define} from '../dependency';
-import {combineLatest, forkJoin, from, Observable, Subscription} from 'rxjs';
+import {combineLatest, forkJoin, from, Observable} from 'rxjs';
import {fire, fireAlert, fireEvent} from '../../utils/event-util';
import {CURRENT} from '../../utils/patch-set-util';
import {RestApiService} from '../../services/gr-rest-api/gr-rest-api';
@@ -370,8 +370,6 @@
private readonly reloadListener: () => void;
- private readonly subscriptions: Subscription[] = [];
-
private drafts: {[path: string]: DraftInfo[]} = {};
private draftToastTask?: DelayedTask;
@@ -421,10 +419,7 @@
override finalize() {
document.removeEventListener('reload', this.reloadListener);
- for (const s of this.subscriptions) {
- s.unsubscribe();
- }
- this.subscriptions.splice(0, this.subscriptions.length);
+ super.finalize();
}
// Note that this does *not* reload ported comments.
diff --git a/polygerrit-ui/app/models/config/config-model.ts b/polygerrit-ui/app/models/config/config-model.ts
index c4bd637..6e374d1 100644
--- a/polygerrit-ui/app/models/config/config-model.ts
+++ b/polygerrit-ui/app/models/config/config-model.ts
@@ -4,7 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {ConfigInfo, RepoName, ServerInfo} from '../../types/common';
-import {from, of, Subscription} from 'rxjs';
+import {from, of} from 'rxjs';
import {switchMap} from 'rxjs/operators';
import {Finalizable} from '../../services/registry';
import {RestApiService} from '../../services/gr-rest-api/gr-rest-api';
@@ -50,8 +50,6 @@
url => url
);
- private subscriptions: Subscription[];
-
constructor(
readonly changeModel: ChangeModel,
readonly restApiService: RestApiService
@@ -83,11 +81,4 @@
updateServerConfig(serverConfig?: ServerInfo) {
this.updateState({serverConfig});
}
-
- override finalize() {
- for (const s of this.subscriptions) {
- s.unsubscribe();
- }
- this.subscriptions = [];
- }
}
diff --git a/polygerrit-ui/app/models/model.ts b/polygerrit-ui/app/models/model.ts
index 7cd8b2a..c44c0bf 100644
--- a/polygerrit-ui/app/models/model.ts
+++ b/polygerrit-ui/app/models/model.ts
@@ -3,7 +3,7 @@
* Copyright 2021 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {BehaviorSubject, Observable} from 'rxjs';
+import {BehaviorSubject, Observable, Subscription} from 'rxjs';
import {Finalizable} from '../services/registry';
/**
@@ -25,6 +25,8 @@
public state$: Observable<T>;
+ protected subscriptions: Subscription[] = [];
+
constructor(initialState: T) {
this.subject$ = new BehaviorSubject(initialState);
this.state$ = this.subject$.asObservable();
@@ -44,5 +46,9 @@
finalize() {
this.subject$.complete();
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions = [];
}
}
diff --git a/polygerrit-ui/app/models/user/user-model.ts b/polygerrit-ui/app/models/user/user-model.ts
index b69c715..bb607e9 100644
--- a/polygerrit-ui/app/models/user/user-model.ts
+++ b/polygerrit-ui/app/models/user/user-model.ts
@@ -3,8 +3,8 @@
* Copyright 2021 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {from, of, Observable, Subscription} from 'rxjs';
-import {switchMap} from 'rxjs/operators';
+import {from, of, Observable} from 'rxjs';
+import {filter, switchMap} from 'rxjs/operators';
import {
DiffPreferencesInfo as DiffPreferencesInfoAPI,
DiffViewMode,
@@ -26,24 +26,62 @@
import {Finalizable} from '../../services/registry';
import {select} from '../../utils/observable-util';
import {Model} from '../model';
+import {isDefined} from '../../utils/common-util';
export interface UserState {
/**
* Keeps being defined even when credentials have expired.
+ *
+ * `undefined` can mean that the app is still starting up and we have not
+ * tried loading an account object yet. If you want to wait until the
+ * `account` is known, then use `accountLoaded` below.
*/
account?: AccountDetailInfo;
- preferences: PreferencesInfo;
- diffPreferences: DiffPreferencesInfo;
- editPreferences: EditPreferencesInfo;
+ /**
+ * Starts as `false` and switches to `true` after the first `getAccount` call.
+ * A common use case for this is to wait with loading or doing something until
+ * we know whether the user is logged in or not, see `loadedAccount$` below.
+ *
+ * This value cannot change back to `false` once it has become `true`.
+ *
+ * This value does *not* indicate whether the user is logged in or whether an
+ * `account` object is available. If the first `getAccount()` call returns
+ * `undefined`, then `accountLoaded` still becomes true, even if `account`
+ * stays `undefined`.
+ */
+ accountLoaded: boolean;
+ preferences?: PreferencesInfo;
+ diffPreferences?: DiffPreferencesInfo;
+ editPreferences?: EditPreferencesInfo;
capabilities?: AccountCapabilityInfo;
}
export class UserModel extends Model<UserState> implements Finalizable {
+ /**
+ * Note that the initially emitted `undefined` value can mean "not loaded
+ * the account into object yet" or "user is not logged in". Consider using
+ * `loadedAccount$` below.
+ *
+ * TODO: Maybe consider changing all usages to `loadedAccount$`.
+ */
readonly account$: Observable<AccountDetailInfo | undefined> = select(
this.state$,
userState => userState.account
);
+ /**
+ * Only emits once we have tried to actually load the account. Note that
+ * this does not initially emit a value.
+ *
+ * So if this emits `undefined`, then you actually know that the user is not
+ * logged in. And for logged in users you will never get an initial
+ * `undefined` emission.
+ */
+ readonly loadedAccount$: Observable<AccountDetailInfo | undefined> = select(
+ this.state$.pipe(filter(s => s.accountLoaded)),
+ userState => userState.account
+ );
+
/** Note that this may still be true, even if credentials have expired. */
readonly loggedIn$: Observable<boolean> = select(
this.account$,
@@ -61,17 +99,17 @@
readonly preferences$: Observable<PreferencesInfo> = select(
this.state$,
userState => userState.preferences
- );
+ ).pipe(filter(isDefined));
readonly diffPreferences$: Observable<DiffPreferencesInfo> = select(
this.state$,
userState => userState.diffPreferences
- );
+ ).pipe(filter(isDefined));
readonly editPreferences$: Observable<EditPreferencesInfo> = select(
this.state$,
userState => userState.editPreferences
- );
+ ).pipe(filter(isDefined));
readonly preferenceDiffViewMode$: Observable<DiffViewMode> = select(
this.preferences$,
@@ -83,13 +121,14 @@
preference => preference.theme
);
- private subscriptions: Subscription[] = [];
+ readonly preferenceChangesPerPage$: Observable<number> = select(
+ this.preferences$,
+ preference => preference.changes_per_page
+ );
constructor(readonly restApiService: RestApiService) {
super({
- preferences: createDefaultPreferences(),
- diffPreferences: createDefaultDiffPrefs(),
- editPreferences: createDefaultEditPrefs(),
+ accountLoaded: false,
});
this.subscriptions = [
from(this.restApiService.getAccount()).subscribe(
@@ -97,7 +136,7 @@
this.setAccount(account);
}
),
- this.account$
+ this.loadedAccount$
.pipe(
switchMap(account => {
if (!account) return of(createDefaultPreferences());
@@ -107,7 +146,7 @@
.subscribe((preferences?: PreferencesInfo) => {
this.setPreferences(preferences ?? createDefaultPreferences());
}),
- this.account$
+ this.loadedAccount$
.pipe(
switchMap(account => {
if (!account) return of(createDefaultDiffPrefs());
@@ -117,7 +156,7 @@
.subscribe((diffPrefs?: DiffPreferencesInfoAPI) => {
this.setDiffPreferences(diffPrefs ?? createDefaultDiffPrefs());
}),
- this.account$
+ this.loadedAccount$
.pipe(
switchMap(account => {
if (!account) return of(createDefaultEditPrefs());
@@ -127,7 +166,7 @@
.subscribe((editPrefs?: EditPreferencesInfo) => {
this.setEditPreferences(editPrefs ?? createDefaultEditPrefs());
}),
- this.account$
+ this.loadedAccount$
.pipe(
switchMap(account => {
if (!account) return of(undefined);
@@ -140,13 +179,6 @@
];
}
- override finalize() {
- for (const s of this.subscriptions) {
- s.unsubscribe();
- }
- this.subscriptions = [];
- }
-
updatePreferences(prefs: Partial<PreferencesInfo>) {
return this.restApiService
.savePreferences(prefs)
@@ -205,6 +237,6 @@
}
setAccount(account?: AccountDetailInfo) {
- this.updateState({account});
+ this.updateState({account, accountLoaded: true});
}
}
diff --git a/polygerrit-ui/app/models/views/search.ts b/polygerrit-ui/app/models/views/search.ts
index 58cb8f7..d601488 100644
--- a/polygerrit-ui/app/models/views/search.ts
+++ b/polygerrit-ui/app/models/views/search.ts
@@ -3,18 +3,61 @@
* Copyright 2022 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {RepoName, BranchName, TopicName} from '../../api/rest-api';
+import {combineLatest, fromEvent, Observable} from 'rxjs';
+import {
+ filter,
+ map,
+ startWith,
+ switchMap,
+ tap,
+ withLatestFrom,
+} from 'rxjs/operators';
+import {RepoName, BranchName, TopicName, ChangeInfo} from '../../api/rest-api';
+import {NavigationService} from '../../elements/core/gr-navigation/gr-navigation';
+import {RestApiService} from '../../services/gr-rest-api/gr-rest-api';
import {GerritView} from '../../services/router/router-model';
+import {select} from '../../utils/observable-util';
import {addQuotesWhen} from '../../utils/string-util';
import {encodeURL} from '../../utils/url-util';
import {define} from '../dependency';
import {Model} from '../model';
+import {UserModel} from '../user/user-model';
import {ViewState} from './base';
+import {createChangeUrl} from './change';
+
+const LOOKUP_QUERY_PATTERNS: RegExp[] = [
+ /^\s*i?[0-9a-f]{7,40}\s*$/i, // CHANGE_ID
+ /^\s*[1-9][0-9]*\s*$/g, // CHANGE_NUM
+ /[0-9a-f]{40}/, // COMMIT
+];
export interface SearchViewState extends ViewState {
view: GerritView.SEARCH;
- query?: string;
+ /**
+ * The query for searching changes.
+ *
+ * Changing this to something non-empty will trigger search.
+ */
+ query: string;
+ /**
+ * How many initial search results should be skipped? This is for showing
+ * more than one search result page. This must be a non-negative number.
+ * If the string is not provided or cannot be parsed as expected, then the
+ * offset falls back to 0.
+ *
+ * TODO: Consider converting from string to number before writing to the
+ * state object.
+ */
offset?: string;
+
+ /**
+ * Is a search API call currrently in progress?
+ */
+ loading: boolean;
+ /**
+ * The search results for the current query.
+ */
+ changes: ChangeInfo[];
}
export interface SearchUrlOptions {
@@ -86,8 +129,108 @@
export const searchViewModelToken =
define<SearchViewModel>('search-view-model');
+/**
+ * This is the view model for the search page.
+ *
+ * It keeps track of the overall search view state and provides selectors for
+ * subscribing to certain slices of the state.
+ *
+ * It manages loading the changes to be shown on the search page by providing
+ * `changes` in its state. Changes to the view state or certain user preferences
+ * will automatically trigger reloading the changes.
+ */
export class SearchViewModel extends Model<SearchViewState | undefined> {
- constructor() {
+ public readonly query$ = select(this.state$, s => s?.query ?? '');
+
+ private readonly offset$ = select(this.state$, s => s?.offset ?? '0');
+
+ /**
+ * Convenience selector for getting the `offset` as a number.
+ *
+ * TODO: Consider changing the type of `offset$` and `state.offset` to
+ * `number`.
+ */
+ public readonly offsetNumber$ = select(this.offset$, offset => {
+ const offsetNumber = Number(offset);
+ return Number.isFinite(offsetNumber) ? offsetNumber : 0;
+ });
+
+ public readonly changes$ = select(this.state$, s => s?.changes ?? []);
+
+ public readonly loading$ = select(this.state$, s => s?.loading ?? false);
+
+ // For usage in `combineLatest` we need `startWith` such that reload$ has an
+ // initial value.
+ private readonly reload$: Observable<unknown> = fromEvent(
+ document,
+ 'reload'
+ ).pipe(startWith(undefined));
+
+ private readonly reloadChangesTrigger$ = combineLatest([
+ this.reload$,
+ this.query$,
+ this.offsetNumber$,
+ this.userModel.preferenceChangesPerPage$,
+ ]).pipe(
+ map(([_reload, query, offsetNumber, changesPerPage]) => {
+ const params: [string, number, number] = [
+ query,
+ offsetNumber,
+ changesPerPage,
+ ];
+ return params;
+ })
+ );
+
+ constructor(
+ private readonly restApiService: RestApiService,
+ private readonly userModel: UserModel,
+ private readonly getNavigation: () => NavigationService
+ ) {
super(undefined);
+ this.subscriptions = [
+ this.reloadChangesTrigger$
+ .pipe(
+ switchMap(a => this.reloadChanges(a)),
+ tap(changes => this.updateState({changes, loading: false}))
+ )
+ .subscribe(),
+ this.changes$
+ .pipe(
+ filter(changes => changes.length === 1),
+ withLatestFrom(this.query$)
+ )
+ .subscribe(([changes, query]) =>
+ this.redirectSingleResult(query, changes)
+ ),
+ ];
+ }
+
+ private async reloadChanges([query, offset, changesPerPage]: [
+ string,
+ number,
+ number
+ ]): Promise<ChangeInfo[]> {
+ if (this.getState() === undefined) return [];
+ if (query.trim().length === 0) return [];
+ this.updateState({loading: true});
+ const changes = await this.restApiService.getChanges(
+ changesPerPage,
+ query,
+ offset
+ );
+ return changes ?? [];
+ }
+
+ // visible for testing
+ redirectSingleResult(query: string, changes: ChangeInfo[]): void {
+ if (changes.length !== 1) return;
+ for (const queryPattern of LOOKUP_QUERY_PATTERNS) {
+ if (query.match(queryPattern)) {
+ // "Back"/"Forward" buttons work correctly only with replaceUrl()
+ this.getNavigation().replaceUrl(createChangeUrl({change: changes[0]}));
+ return;
+ }
+ }
}
}
diff --git a/polygerrit-ui/app/models/views/search_test.ts b/polygerrit-ui/app/models/views/search_test.ts
index 138ce1e..bb2c0e3 100644
--- a/polygerrit-ui/app/models/views/search_test.ts
+++ b/polygerrit-ui/app/models/views/search_test.ts
@@ -4,9 +4,26 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {assert} from '@open-wc/testing';
-import {BranchName, RepoName, TopicName} from '../../api/rest-api';
+import {SinonStub} from 'sinon';
+import {
+ BranchName,
+ NumericChangeId,
+ RepoName,
+ TopicName,
+} from '../../api/rest-api';
+import {navigationToken} from '../../elements/core/gr-navigation/gr-navigation';
import '../../test/common-test-setup';
-import {createSearchUrl, SearchUrlOptions} from './search';
+import {testResolver} from '../../test/common-test-setup';
+import {createChange} from '../../test/test-data-generators';
+import {
+ createSearchUrl,
+ SearchUrlOptions,
+ SearchViewModel,
+ searchViewModelToken,
+} from './search';
+
+const CHANGE_ID = 'IcA3dAB3edAB9f60B8dcdA6ef71A75980e4B7127';
+const COMMIT_HASH = '12345678';
suite('search view state tests', () => {
test('createSearchUrl', () => {
@@ -53,4 +70,60 @@
options = {topic: 'test:test' as TopicName};
assert.equal(createSearchUrl(options), '/q/topic:"test:test"');
});
+
+ suite('query based navigation', () => {
+ let replaceUrlStub: SinonStub;
+ let model: SearchViewModel;
+
+ setup(() => {
+ model = testResolver(searchViewModelToken);
+ replaceUrlStub = sinon.stub(testResolver(navigationToken), 'replaceUrl');
+ });
+
+ teardown(() => {
+ model.finalize();
+ });
+
+ test('Searching for a change ID redirects to change', async () => {
+ const change = {...createChange(), _number: 1 as NumericChangeId};
+
+ model.redirectSingleResult(CHANGE_ID, [change]);
+
+ assert.isTrue(replaceUrlStub.called);
+ assert.equal(replaceUrlStub.lastCall.firstArg, '/c/test-project/+/1');
+ });
+
+ test('Searching for a change num redirects to change', async () => {
+ const change = {...createChange(), _number: 1 as NumericChangeId};
+
+ model.redirectSingleResult('1', [change]);
+
+ assert.isTrue(replaceUrlStub.called);
+ assert.equal(replaceUrlStub.lastCall.firstArg, '/c/test-project/+/1');
+ });
+
+ test('Commit hash redirects to change', async () => {
+ const change = {...createChange(), _number: 1 as NumericChangeId};
+
+ model.redirectSingleResult(COMMIT_HASH, [change]);
+
+ assert.isTrue(replaceUrlStub.called);
+ assert.equal(replaceUrlStub.lastCall.firstArg, '/c/test-project/+/1');
+ });
+
+ test('No results: no redirect', async () => {
+ model.redirectSingleResult(CHANGE_ID, []);
+
+ assert.isFalse(replaceUrlStub.called);
+ });
+
+ test('More than 1 result: no redirect', async () => {
+ const change1 = {...createChange(), _number: 1 as NumericChangeId};
+ const change2 = {...createChange(), _number: 2 as NumericChangeId};
+
+ model.redirectSingleResult(CHANGE_ID, [change1, change2]);
+
+ assert.isFalse(replaceUrlStub.called);
+ });
+ });
});
diff --git a/polygerrit-ui/app/services/app-context-init.ts b/polygerrit-ui/app/services/app-context-init.ts
index 9f7a0c0..f74cfdf 100644
--- a/polygerrit-ui/app/services/app-context-init.ts
+++ b/polygerrit-ui/app/services/app-context-init.ts
@@ -57,7 +57,10 @@
import {PluginViewModel, pluginViewModelToken} from '../models/views/plugin';
import {RepoViewModel, repoViewModelToken} from '../models/views/repo';
import {SearchViewModel, searchViewModelToken} from '../models/views/search';
-import {navigationToken} from '../elements/core/gr-navigation/gr-navigation';
+import {
+ NavigationService,
+ navigationToken,
+} from '../elements/core/gr-navigation/gr-navigation';
/**
* The AppContext lazy initializator for all services
@@ -131,7 +134,11 @@
dependencies.set(pluginViewModelToken, pluginViewModel);
const repoViewModel = new RepoViewModel();
dependencies.set(repoViewModelToken, repoViewModel);
- const searchViewModel = new SearchViewModel();
+ const searchViewModel = new SearchViewModel(
+ appContext.restApiService,
+ appContext.userModel,
+ () => dependencies.get(navigationToken) as unknown as NavigationService
+ );
dependencies.set(searchViewModelToken, searchViewModel);
const settingsViewModel = new SettingsViewModel();
dependencies.set(settingsViewModelToken, settingsViewModel);
diff --git a/polygerrit-ui/app/test/test-app-context-init.ts b/polygerrit-ui/app/test/test-app-context-init.ts
index e34773e..eec5446 100644
--- a/polygerrit-ui/app/test/test-app-context-init.ts
+++ b/polygerrit-ui/app/test/test-app-context-init.ts
@@ -138,7 +138,10 @@
dependencies.set(pluginViewModelToken, pluginViewModelCreator);
const repoViewModelCreator = () => new RepoViewModel();
dependencies.set(repoViewModelToken, repoViewModelCreator);
- const searchViewModelCreator = () => new SearchViewModel();
+ const searchViewModelCreator = () =>
+ new SearchViewModel(appContext.restApiService, appContext.userModel, () =>
+ resolver(navigationToken)
+ );
dependencies.set(searchViewModelToken, searchViewModelCreator);
const settingsViewModelCreator = () => new SettingsViewModel();
dependencies.set(settingsViewModelToken, settingsViewModelCreator);
diff --git a/polygerrit-ui/app/test/test-data-generators.ts b/polygerrit-ui/app/test/test-data-generators.ts
index 9337500..fc865a3 100644
--- a/polygerrit-ui/app/test/test-data-generators.ts
+++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -706,6 +706,8 @@
view: GerritView.SEARCH,
query: TEST_NUMERIC_CHANGE_ID.toString(),
offset: '0',
+ changes: [],
+ loading: false,
};
}
diff --git a/polygerrit-ui/app/utils/common-util.ts b/polygerrit-ui/app/utils/common-util.ts
index 05c054b..6c2774e 100644
--- a/polygerrit-ui/app/utils/common-util.ts
+++ b/polygerrit-ui/app/utils/common-util.ts
@@ -47,6 +47,10 @@
if (!condition) throw new Error(errorMessage);
}
+export function isDefined<T>(x: T): x is NonNullable<T> {
+ return x !== undefined && x !== null;
+}
+
/**
* Throws an error if the property is not defined.
*/
