Merge "Avoid that comments autoclose."
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 6b74921..a58c7bb 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,6 +7,7 @@
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,
@@ -25,9 +26,20 @@
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?("[^"]+"|[^ ]+)$/;
@@ -48,6 +60,21 @@
@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;
@@ -64,19 +91,19 @@
@state() query = '';
// private but used in test
- @state() offset = 0;
+ @state() offset?: number;
// 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;
+ @state() userId: AccountId | EmailAddress | null = null;
// private but used in test
- @state() repo?: RepoName;
+ @state() repo: RepoName | null = null;
@state() selectedIndex = 0;
@@ -88,30 +115,17 @@
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().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)
+ () => this.getViewModel().state$,
+ x => (this.viewState = x)
);
subscribe(
this,
@@ -125,16 +139,22 @@
);
subscribe(
this,
- () => this.userModel.preferenceChangesPerPage$,
- x => (this.changesPerPage = x)
- );
- subscribe(
- this,
() => this.userModel.preferences$,
- x => (this.preferences = x)
+ x => {
+ this.preferences = x;
+ if (this.changesPerPage !== x.changes_per_page) {
+ this.changesPerPage = x.changes_per_page;
+ this.debouncedGetChanges();
+ }
+ }
);
}
+ override disconnectedCallback() {
+ this.getChangesTask?.flush();
+ super.disconnectedCallback();
+ }
+
static override get styles() {
return [
sharedStyles,
@@ -262,10 +282,63 @@
}
}
- 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
@@ -299,12 +372,14 @@
}
private changesChanged() {
- this.selectedIndex = 0;
- this.userId = undefined;
- this.repo = undefined;
- if (this.changes.length === 0) return;
+ this.userId = null;
+ this.repo = null;
+ const changes = this.changes;
+ if (!changes || !changes.length) {
+ return;
+ }
if (USER_QUERY_PATTERN.test(this.query)) {
- const owner = this.changes[0].owner;
+ const owner = changes[0].owner;
const userId = owner._account_id ? owner._account_id : owner.email;
if (userId) {
this.userId = userId;
@@ -312,7 +387,7 @@
}
}
if (REPO_QUERY_PATTERN.test(this.query)) {
- this.repo = this.changes[0].project;
+ this.repo = 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 4dc3701..b003b66 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,20 +7,45 @@
import './gr-change-list-view';
import {GrChangeListView} from './gr-change-list-view';
import {page} from '../../../utils/page-wrapper-utils';
-import {query, queryAndAssert} from '../../../test/test-utils';
+import {navigationToken} from '../../core/gr-navigation/gr-navigation';
+import {
+ query,
+ stubRestApi,
+ queryAndAssert,
+ stubFlags,
+} from '../../../test/test-utils';
import {createChange} from '../../../test/test-data-generators';
-import {ChangeInfo, EmailAddress, RepoName} from '../../../api/rest-api';
+import {
+ ChangeInfo,
+ EmailAddress,
+ NumericChangeId,
+ 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.query = 'test-query';
+ element.viewState = {
+ view: GerritView.SEARCH,
+ query: 'test-query',
+ offset: '0',
+ };
await element.updateComplete;
});
@@ -50,9 +75,11 @@
suite('bulk actions', () => {
setup(async () => {
+ stubFlags('isEnabled').returns(true);
+ changes = [createChange()];
element.loading = false;
- element.changes = [createChange()];
- await element.updateComplete;
+ element.reload();
+ clock.tick(100);
await element.updateComplete;
await waitUntil(() => element.loading === false);
});
@@ -80,9 +107,8 @@
checkbox.click();
await waitUntil(() => checkbox.checked);
- element.changes = [createChange()];
+ element.reload();
await element.updateComplete;
-
checkbox = queryAndAssert<HTMLInputElement>(
query(
query(query(element, 'gr-change-list'), 'gr-change-list-section'),
@@ -196,7 +222,7 @@
});
test('userId query', async () => {
- assert.isUndefined(element.userId);
+ assert.isNull(element.userId);
element.query = 'owner: foo@bar';
element.changes = [
{...createChange(), owner: {email: 'foo@bar' as EmailAddress}},
@@ -209,19 +235,19 @@
{...createChange(), owner: {email: 'foo@bar' as EmailAddress}},
];
await element.updateComplete;
- assert.isUndefined(element.userId);
+ assert.isNull(element.userId);
});
test('userId query without email', async () => {
- assert.isUndefined(element.userId);
+ assert.isNull(element.userId);
element.query = 'owner: foo@bar';
element.changes = [{...createChange(), owner: {}}];
await element.updateComplete;
- assert.isUndefined(element.userId);
+ assert.isNull(element.userId);
});
test('repo query', async () => {
- assert.isUndefined(element.repo);
+ assert.isNull(element.repo);
element.query = 'project: test-repo';
element.changes = [
{
@@ -238,11 +264,11 @@
{...createChange(), owner: {email: 'foo@bar' as EmailAddress}},
];
await element.updateComplete;
- assert.isUndefined(element.repo);
+ assert.isNull(element.repo);
});
test('repo query with open status', async () => {
- assert.isUndefined(element.repo);
+ assert.isNull(element.repo);
element.query = 'project:test-repo status:open';
element.changes = [
{
@@ -259,6 +285,74 @@
{...createChange(), owner: {email: 'foo@bar' as EmailAddress}},
];
await element.updateComplete;
- assert.isUndefined(element.repo);
+ 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/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index e40a161..762f6b7 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -1292,8 +1292,6 @@
view: GerritView.SEARCH,
query: ctx.params[0],
offset: ctx.params[2],
- changes: [],
- loading: false,
};
this.setState(state);
this.searchViewModel.setState(state);
@@ -1306,8 +1304,6 @@
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 6c29336..ce3ed0d 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,8 +403,6 @@
view: GerritView.SEARCH,
query: 'project:foo/bar/baz',
offset: undefined,
- changes: [],
- loading: false,
});
ctx.params[1] = '123';
@@ -413,8 +411,6 @@
view: GerritView.SEARCH,
query: 'project:foo/bar/baz',
offset: '123',
- changes: [],
- loading: false,
});
});
@@ -433,8 +429,6 @@
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 b4f16ab..5cfe670 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -14,7 +14,15 @@
RevisionPatchSetNum,
} from '../../types/common';
import {DefaultBase} from '../../constants/constants';
-import {combineLatest, from, fromEvent, Observable, forkJoin, of} from 'rxjs';
+import {
+ combineLatest,
+ from,
+ fromEvent,
+ Observable,
+ Subscription,
+ forkJoin,
+ of,
+} from 'rxjs';
import {
map,
filter,
@@ -253,6 +261,8 @@
([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(
@@ -306,6 +316,13 @@
];
}
+ 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 6922f6d..31e42a6 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, of, from} from 'rxjs';
+import {combineLatest, Subscription, 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,6 +131,8 @@
state => state.filesRightBase
);
+ private subscriptions: Subscription[] = [];
+
constructor(
readonly changeModel: ChangeModel,
readonly commentsModel: CommentsModel,
@@ -196,4 +198,11 @@
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 61c2f77..bb46f27 100644
--- a/polygerrit-ui/app/models/checks/checks-model.ts
+++ b/polygerrit-ui/app/models/checks/checks-model.ts
@@ -20,6 +20,7 @@
Observable,
of,
Subject,
+ Subscription,
timer,
} from 'rxjs';
import {
@@ -207,6 +208,8 @@
private readonly visibilityChangeListener: () => void;
+ private subscriptions: Subscription[] = [];
+
public checksSelectedPatchsetNumber$ = select(
this.state$,
state => state.patchsetNumberSelected
@@ -480,7 +483,10 @@
'visibilitychange',
this.visibilityChangeListener
);
- super.finalize();
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions = [];
}
// 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 8ed4b69..39d9eaa 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} from 'rxjs';
+import {combineLatest, forkJoin, from, Observable, Subscription} 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,6 +370,8 @@
private readonly reloadListener: () => void;
+ private readonly subscriptions: Subscription[] = [];
+
private drafts: {[path: string]: DraftInfo[]} = {};
private draftToastTask?: DelayedTask;
@@ -419,7 +421,10 @@
override finalize() {
document.removeEventListener('reload', this.reloadListener);
- super.finalize();
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions.splice(0, this.subscriptions.length);
}
// 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 6e374d1..c4bd637 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} from 'rxjs';
+import {from, of, Subscription} from 'rxjs';
import {switchMap} from 'rxjs/operators';
import {Finalizable} from '../../services/registry';
import {RestApiService} from '../../services/gr-rest-api/gr-rest-api';
@@ -50,6 +50,8 @@
url => url
);
+ private subscriptions: Subscription[];
+
constructor(
readonly changeModel: ChangeModel,
readonly restApiService: RestApiService
@@ -81,4 +83,11 @@
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 c44c0bf..7cd8b2a 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, Subscription} from 'rxjs';
+import {BehaviorSubject, Observable} from 'rxjs';
import {Finalizable} from '../services/registry';
/**
@@ -25,8 +25,6 @@
public state$: Observable<T>;
- protected subscriptions: Subscription[] = [];
-
constructor(initialState: T) {
this.subject$ = new BehaviorSubject(initialState);
this.state$ = this.subject$.asObservable();
@@ -46,9 +44,5 @@
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 bb607e9..b69c715 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} from 'rxjs';
-import {filter, switchMap} from 'rxjs/operators';
+import {from, of, Observable, Subscription} from 'rxjs';
+import {switchMap} from 'rxjs/operators';
import {
DiffPreferencesInfo as DiffPreferencesInfoAPI,
DiffViewMode,
@@ -26,62 +26,24 @@
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;
- /**
- * 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;
+ 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$,
@@ -99,17 +61,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$,
@@ -121,14 +83,13 @@
preference => preference.theme
);
- readonly preferenceChangesPerPage$: Observable<number> = select(
- this.preferences$,
- preference => preference.changes_per_page
- );
+ private subscriptions: Subscription[] = [];
constructor(readonly restApiService: RestApiService) {
super({
- accountLoaded: false,
+ preferences: createDefaultPreferences(),
+ diffPreferences: createDefaultDiffPrefs(),
+ editPreferences: createDefaultEditPrefs(),
});
this.subscriptions = [
from(this.restApiService.getAccount()).subscribe(
@@ -136,7 +97,7 @@
this.setAccount(account);
}
),
- this.loadedAccount$
+ this.account$
.pipe(
switchMap(account => {
if (!account) return of(createDefaultPreferences());
@@ -146,7 +107,7 @@
.subscribe((preferences?: PreferencesInfo) => {
this.setPreferences(preferences ?? createDefaultPreferences());
}),
- this.loadedAccount$
+ this.account$
.pipe(
switchMap(account => {
if (!account) return of(createDefaultDiffPrefs());
@@ -156,7 +117,7 @@
.subscribe((diffPrefs?: DiffPreferencesInfoAPI) => {
this.setDiffPreferences(diffPrefs ?? createDefaultDiffPrefs());
}),
- this.loadedAccount$
+ this.account$
.pipe(
switchMap(account => {
if (!account) return of(createDefaultEditPrefs());
@@ -166,7 +127,7 @@
.subscribe((editPrefs?: EditPreferencesInfo) => {
this.setEditPreferences(editPrefs ?? createDefaultEditPrefs());
}),
- this.loadedAccount$
+ this.account$
.pipe(
switchMap(account => {
if (!account) return of(undefined);
@@ -179,6 +140,13 @@
];
}
+ override finalize() {
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions = [];
+ }
+
updatePreferences(prefs: Partial<PreferencesInfo>) {
return this.restApiService
.savePreferences(prefs)
@@ -237,6 +205,6 @@
}
setAccount(account?: AccountDetailInfo) {
- this.updateState({account, accountLoaded: true});
+ this.updateState({account});
}
}
diff --git a/polygerrit-ui/app/models/views/search.ts b/polygerrit-ui/app/models/views/search.ts
index d601488..58cb8f7 100644
--- a/polygerrit-ui/app/models/views/search.ts
+++ b/polygerrit-ui/app/models/views/search.ts
@@ -3,61 +3,18 @@
* Copyright 2022 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-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 {RepoName, BranchName, TopicName} from '../../api/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;
- /**
- * 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.
- */
+ query?: string;
offset?: string;
-
- /**
- * Is a search API call currrently in progress?
- */
- loading: boolean;
- /**
- * The search results for the current query.
- */
- changes: ChangeInfo[];
}
export interface SearchUrlOptions {
@@ -129,108 +86,8 @@
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> {
- 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
- ) {
+ constructor() {
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 bb2c0e3..138ce1e 100644
--- a/polygerrit-ui/app/models/views/search_test.ts
+++ b/polygerrit-ui/app/models/views/search_test.ts
@@ -4,26 +4,9 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {assert} from '@open-wc/testing';
-import {SinonStub} from 'sinon';
-import {
- BranchName,
- NumericChangeId,
- RepoName,
- TopicName,
-} from '../../api/rest-api';
-import {navigationToken} from '../../elements/core/gr-navigation/gr-navigation';
+import {BranchName, RepoName, TopicName} from '../../api/rest-api';
import '../../test/common-test-setup';
-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';
+import {createSearchUrl, SearchUrlOptions} from './search';
suite('search view state tests', () => {
test('createSearchUrl', () => {
@@ -70,60 +53,4 @@
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 f74cfdf..9f7a0c0 100644
--- a/polygerrit-ui/app/services/app-context-init.ts
+++ b/polygerrit-ui/app/services/app-context-init.ts
@@ -57,10 +57,7 @@
import {PluginViewModel, pluginViewModelToken} from '../models/views/plugin';
import {RepoViewModel, repoViewModelToken} from '../models/views/repo';
import {SearchViewModel, searchViewModelToken} from '../models/views/search';
-import {
- NavigationService,
- navigationToken,
-} from '../elements/core/gr-navigation/gr-navigation';
+import {navigationToken} from '../elements/core/gr-navigation/gr-navigation';
/**
* The AppContext lazy initializator for all services
@@ -134,11 +131,7 @@
dependencies.set(pluginViewModelToken, pluginViewModel);
const repoViewModel = new RepoViewModel();
dependencies.set(repoViewModelToken, repoViewModel);
- const searchViewModel = new SearchViewModel(
- appContext.restApiService,
- appContext.userModel,
- () => dependencies.get(navigationToken) as unknown as NavigationService
- );
+ const searchViewModel = new SearchViewModel();
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 eec5446..e34773e 100644
--- a/polygerrit-ui/app/test/test-app-context-init.ts
+++ b/polygerrit-ui/app/test/test-app-context-init.ts
@@ -138,10 +138,7 @@
dependencies.set(pluginViewModelToken, pluginViewModelCreator);
const repoViewModelCreator = () => new RepoViewModel();
dependencies.set(repoViewModelToken, repoViewModelCreator);
- const searchViewModelCreator = () =>
- new SearchViewModel(appContext.restApiService, appContext.userModel, () =>
- resolver(navigationToken)
- );
+ const searchViewModelCreator = () => new SearchViewModel();
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 fc865a3..9337500 100644
--- a/polygerrit-ui/app/test/test-data-generators.ts
+++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -706,8 +706,6 @@
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 6c2774e..05c054b 100644
--- a/polygerrit-ui/app/utils/common-util.ts
+++ b/polygerrit-ui/app/utils/common-util.ts
@@ -47,10 +47,6 @@
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.
*/
