Move behavior from component into view model for change-list-view
Rolling forward change 346616. It was reverted, because of nested
state updates resulting in an empty file list when not logged in.
This was addressed by change 348076: Avoid nested setState() calls.
Compared to the original change we have extracted this refactoring
into a separate change 348254: Move `subscriptions` into `Model` base class.
The responsibility for handling the view state and loading changes
was moved from `GrChangeListView` into `SearchViewModel`.
`GrChangeListView` is much simpler than before and just displays the
state of the `SearchViewModel`.
Change-Id: I30dcb9a37c49afe7f97d51adc7e00836c5c6e0bf
Release-Notes: skip
Google-Bug-Id: b/248247819
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..8000c22 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,
@@ -22,29 +21,12 @@
import {sharedStyles} from '../../../styles/shared-styles';
import {LitElement, PropertyValues, html, css, nothing} from 'lit';
import {customElement, state, query} from 'lit/decorators.js';
-import {ValueChangedEvent} from '../../../types/events';
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?("[^"]+"|[^ ]+)$/;
-
-const REPO_QUERY_PATTERN =
- /^project:\s?("[^"]+"|[^ ]+)(\sstatus\s?:(open|"open"))?$/;
const LIMIT_OPERATOR_PATTERN = /\blimit:(\d+)/i;
@@ -60,21 +42,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,21 +58,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() selectedIndex = 0;
+ @state() repo?: RepoName;
private readonly restApiService = getAppContext().restApiService;
@@ -115,17 +80,40 @@
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,
+ () => this.getViewModel().userId$,
+ x => (this.userId = x)
+ );
+ subscribe(
+ this,
+ () => this.getViewModel().repo$,
+ x => (this.repo = x)
);
subscribe(
this,
@@ -139,20 +127,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() {
@@ -211,10 +193,6 @@
.changes=${this.changes}
.preferences=${this.preferences}
.showStar=${this.loggedIn}
- .selectedIndex=${this.selectedIndex}
- @selected-index-changed=${(e: ValueChangedEvent<number>) => {
- this.selectedIndex = e.detail.value;
- }}
@toggle-star=${(e: CustomEvent<ChangeStarToggleStarDetail>) => {
this.handleToggleStar(e);
}}
@@ -276,71 +254,12 @@
`;
}
- override willUpdate(changedProperties: PropertyValues) {
- if (changedProperties.has('changes')) {
- this.changesChanged();
+ override updated(changedProperties: PropertyValues) {
+ if (changedProperties.has('query')) {
+ fireTitleChange(this, this.query);
}
}
- 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;
- }
- }
- }
- this.changes = changes;
- this.loading = false;
- }
-
// private but used in test
limitFor(query: string, defaultLimit?: number) {
if (defaultLimit === undefined) return 0;
@@ -371,26 +290,6 @@
page.show(this.computeNavLink(-1));
}
- private changesChanged() {
- this.userId = null;
- this.repo = null;
- const changes = this.changes;
- if (!changes || !changes.length) {
- return;
- }
- if (USER_QUERY_PATTERN.test(this.query)) {
- const owner = changes[0].owner;
- const userId = owner._account_id ? owner._account_id : owner.email;
- if (userId) {
- this.userId = userId;
- return;
- }
- }
- if (REPO_QUERY_PATTERN.test(this.query)) {
- this.repo = changes[0].project;
- }
- }
-
// private but used in test
computePage() {
if (this.offset === undefined || this.changesPerPage === undefined) return;
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..399631e 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} 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'),
@@ -220,139 +194,4 @@
element.handlePreviousPage();
assert.isTrue(showStub.called);
});
-
- test('userId query', async () => {
- assert.isNull(element.userId);
- element.query = 'owner: foo@bar';
- element.changes = [
- {...createChange(), owner: {email: 'foo@bar' as EmailAddress}},
- ];
- await element.updateComplete;
- assert.equal(element.userId, 'foo@bar' as EmailAddress);
-
- element.query = 'foo bar baz';
- element.changes = [
- {...createChange(), owner: {email: 'foo@bar' as EmailAddress}},
- ];
- await element.updateComplete;
- assert.isNull(element.userId);
- });
-
- test('userId query without email', async () => {
- assert.isNull(element.userId);
- element.query = 'owner: foo@bar';
- element.changes = [{...createChange(), owner: {}}];
- await element.updateComplete;
- assert.isNull(element.userId);
- });
-
- test('repo query', async () => {
- assert.isNull(element.repo);
- element.query = 'project: test-repo';
- element.changes = [
- {
- ...createChange(),
- owner: {email: 'foo@bar' as EmailAddress},
- project: 'test-repo' as RepoName,
- },
- ];
- await element.updateComplete;
- assert.equal(element.repo, 'test-repo' as RepoName);
-
- element.query = 'foo bar baz';
- element.changes = [
- {...createChange(), owner: {email: 'foo@bar' as EmailAddress}},
- ];
- await element.updateComplete;
- assert.isNull(element.repo);
- });
-
- test('repo query with open status', async () => {
- assert.isNull(element.repo);
- element.query = 'project:test-repo status:open';
- element.changes = [
- {
- ...createChange(),
- owner: {email: 'foo@bar' as EmailAddress},
- project: 'test-repo' as RepoName,
- },
- ];
- await element.updateComplete;
- assert.equal(element.repo, 'test-repo' as RepoName);
-
- element.query = 'foo bar baz';
- element.changes = [
- {...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);
- });
- });
});
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.ts b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.ts
index 7b79893..48cef64 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.ts
@@ -54,6 +54,7 @@
></gr-file-list-header>`
);
element.diffPrefs = createDefaultDiffPrefs();
+ await element.updateComplete;
});
test('render', () => {
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 f829313..33fd2ee 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -1356,6 +1356,8 @@
view: GerritView.SEARCH,
query: ctx.params[0],
offset: ctx.params[2],
+ changes: [],
+ loading: false,
};
// Note that router model view must be updated before view models.
this.setState(state);
@@ -1369,6 +1371,8 @@
const state: SearchViewState = {
view: GerritView.SEARCH,
query: ctx.params[0],
+ changes: [],
+ loading: false,
};
// Note that router model view must be updated before view models.
this.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 119ba48..b550ea6 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_test.ts b/polygerrit-ui/app/models/change/change-model_test.ts
index fdf9e04..4b51d5b 100644
--- a/polygerrit-ui/app/models/change/change-model_test.ts
+++ b/polygerrit-ui/app/models/change/change-model_test.ts
@@ -285,6 +285,10 @@
// And the missing `replay` led to a bug that was hard to find. That is why
// we are testing this explicitly here.
test('basePatchNum$ selector', async () => {
+ // Let's first wait for the selector to emit. Then we can test the replay
+ // below.
+ await waitUntilObserved(changeModel.basePatchNum$, x => x === PARENT);
+
const spy = sinon.spy();
changeModel.basePatchNum$.subscribe(spy);
diff --git a/polygerrit-ui/app/models/user/user-model.ts b/polygerrit-ui/app/models/user/user-model.ts
index fa00a0b..2f2fe7d 100644
--- a/polygerrit-ui/app/models/user/user-model.ts
+++ b/polygerrit-ui/app/models/user/user-model.ts
@@ -4,7 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {from, of, Observable} from 'rxjs';
-import {switchMap} from 'rxjs/operators';
+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 {notUndefined} from '../../types/types';
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(notUndefined));
readonly diffPreferences$: Observable<DiffPreferencesInfo> = select(
this.state$,
userState => userState.diffPreferences
- );
+ ).pipe(filter(notUndefined));
readonly editPreferences$: Observable<EditPreferencesInfo> = select(
this.state$,
userState => userState.editPreferences
- );
+ ).pipe(filter(notUndefined));
readonly preferenceDiffViewMode$: Observable<DiffViewMode> = select(
this.preferences$,
@@ -83,11 +121,14 @@
preference => preference.theme
);
+ 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(
@@ -95,7 +136,7 @@
this.setAccount(account);
}
),
- this.account$
+ this.loadedAccount$
.pipe(
switchMap(account => {
if (!account) return of(createDefaultPreferences());
@@ -105,7 +146,7 @@
.subscribe((preferences?: PreferencesInfo) => {
this.setPreferences(preferences ?? createDefaultPreferences());
}),
- this.account$
+ this.loadedAccount$
.pipe(
switchMap(account => {
if (!account) return of(createDefaultDiffPrefs());
@@ -115,7 +156,7 @@
.subscribe((diffPrefs?: DiffPreferencesInfoAPI) => {
this.setDiffPreferences(diffPrefs ?? createDefaultDiffPrefs());
}),
- this.account$
+ this.loadedAccount$
.pipe(
switchMap(account => {
if (!account) return of(createDefaultEditPrefs());
@@ -125,7 +166,7 @@
.subscribe((editPrefs?: EditPreferencesInfo) => {
this.setEditPreferences(editPrefs ?? createDefaultEditPrefs());
}),
- this.account$
+ this.loadedAccount$
.pipe(
switchMap(account => {
if (!account) return of(undefined);
@@ -196,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 13de8f3..78f2d8f 100644
--- a/polygerrit-ui/app/models/views/search.ts
+++ b/polygerrit-ui/app/models/views/search.ts
@@ -3,18 +3,69 @@
* 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, getBaseUrl} from '../../utils/url-util';
-import {define} from '../dependency';
+import {define, Provider} from '../dependency';
import {Model} from '../model';
+import {UserModel} from '../user/user-model';
import {ViewState} from './base';
+import {createChangeUrl} from './change';
+
+const USER_QUERY_PATTERN = /^owner:\s?("[^"]+"|[^ ]+)$/;
+
+const REPO_QUERY_PATTERN =
+ /^project:\s?("[^"]+"|[^ ]+)(\sstatus\s?:(open|"open"))?$/;
+
+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 +137,127 @@
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 userId$ = select(
+ combineLatest([this.query$, this.changes$]),
+ ([query, changes]) => {
+ if (changes.length === 0) return undefined;
+ if (!USER_QUERY_PATTERN.test(query)) return undefined;
+ const owner = changes[0].owner;
+ return owner?._account_id ?? owner?.email;
+ }
+ );
+
+ public readonly repo$ = select(
+ combineLatest([this.query$, this.changes$]),
+ ([query, changes]) => {
+ if (changes.length === 0) return undefined;
+ if (!REPO_QUERY_PATTERN.test(query)) return undefined;
+ return changes[0].project;
+ }
+ );
+
+ 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: Provider<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 d48667b..9017f2e 100644
--- a/polygerrit-ui/app/models/views/search_test.ts
+++ b/polygerrit-ui/app/models/views/search_test.ts
@@ -4,9 +4,28 @@
* 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 {
+ AccountId,
+ BranchName,
+ EmailAddress,
+ 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', () => {
@@ -57,4 +76,130 @@
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);
+ });
+ });
+
+ suite('selectors', () => {
+ let model: SearchViewModel;
+ let userId: AccountId | EmailAddress | undefined;
+ let repo: RepoName | undefined;
+
+ setup(() => {
+ model = testResolver(searchViewModelToken);
+ model.userId$.subscribe(x => (userId = x));
+ model.repo$.subscribe(x => (repo = x));
+ });
+
+ teardown(() => {
+ model.finalize();
+ });
+
+ test('userId', async () => {
+ assert.isUndefined(userId);
+
+ model.updateState({
+ query: 'owner: foo@bar',
+ changes: [
+ {...createChange(), owner: {email: 'foo@bar' as EmailAddress}},
+ ],
+ });
+ assert.equal(userId, 'foo@bar' as EmailAddress);
+
+ model.updateState({
+ query: 'foo bar baz',
+ changes: [
+ {...createChange(), owner: {email: 'foo@bar' as EmailAddress}},
+ ],
+ });
+ assert.isUndefined(userId);
+
+ model.updateState({
+ query: 'owner: foo@bar',
+ changes: [{...createChange(), owner: {}}],
+ });
+ assert.isUndefined(userId);
+ });
+
+ test('repo', async () => {
+ assert.isUndefined(repo);
+
+ model.updateState({
+ query: 'foo bar baz',
+ changes: [{...createChange(), project: 'test-repo' as RepoName}],
+ });
+ assert.isUndefined(repo);
+
+ model.updateState({
+ query: 'foo bar baz',
+ changes: [{...createChange()}],
+ });
+ assert.isUndefined(repo);
+
+ model.updateState({
+ query: 'project: test-repo',
+ changes: [{...createChange(), project: 'test-repo' as RepoName}],
+ });
+ assert.equal(repo, 'test-repo' as RepoName);
+
+ model.updateState({
+ query: 'project:test-repo status:open',
+ changes: [{...createChange(), project: 'test-repo' as RepoName}],
+ });
+ assert.equal(repo, 'test-repo' as RepoName);
+ });
+ });
});
diff --git a/polygerrit-ui/app/services/app-context-init.ts b/polygerrit-ui/app/services/app-context-init.ts
index 2e1b817..e662d5f 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 2cdd4a5..0693570 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 4e29f53..ab3064f 100644
--- a/polygerrit-ui/app/test/test-data-generators.ts
+++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -707,6 +707,8 @@
view: GerritView.SEARCH,
query: TEST_NUMERIC_CHANGE_ID.toString(),
offset: '0',
+ changes: [],
+ loading: false,
};
}
diff --git a/polygerrit-ui/app/types/types.ts b/polygerrit-ui/app/types/types.ts
index 320ac66..04052e2 100644
--- a/polygerrit-ui/app/types/types.ts
+++ b/polygerrit-ui/app/types/types.ts
@@ -20,8 +20,8 @@
} from './common';
import {AuthRequestInit} from '../services/gr-auth/gr-auth';
-export function notUndefined<T>(x: T | undefined): x is T {
- return x !== undefined;
+export function notUndefined<T>(x: T): x is NonNullable<T> {
+ return x !== undefined && x !== null;
}
export interface FixIronA11yAnnouncer extends IronA11yAnnouncer {
