Move basic change state from router to change view model Release-Notes: skip Google-Bug-Id: b/247042673 Change-Id: I3c59896837b18709fbc4bb8584d3abb7354492f1
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts index 8282a3f..eaaf697 100644 --- a/polygerrit-ui/app/models/change/change-model.ts +++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -22,7 +22,6 @@ startWith, switchMap, } from 'rxjs/operators'; -import {RouterModel} from '../../services/router/router-model'; import { computeAllPatchSets, computeLatestPatchNum, @@ -38,6 +37,7 @@ import {UserModel} from '../user/user-model'; import {define} from '../dependency'; import {isOwner} from '../../utils/change-util'; +import {ChangeViewModel} from '../views/change'; export enum LoadingStatus { NOT_LOADED = 'NOT_LOADED', @@ -76,7 +76,7 @@ export function updateChangeWithEdit( change?: ParsedChangeInfo, edit?: EditInfo, - routerPatchNum?: PatchSetNum + viewModelPatchNum?: PatchSetNum ): ParsedChangeInfo | undefined { if (!change || !edit) return change; assertIsDefined(edit.commit.commit, 'edit.commit.commit'); @@ -95,7 +95,7 @@ // which is still done in change-view. `_patchRange.patchNum` should // eventually also be model managed, so we can reconcile these two code // snippets into one location. - if (routerPatchNum === undefined) { + if (viewModelPatchNum === undefined) { change.current_revision = edit.commit.commit; } return change; @@ -103,20 +103,20 @@ /** * Derives the base patchset number from all the data that can potentially - * influence it. Mostly just returns `routerBasePatchNum` or PARENT, but has + * influence it. Mostly just returns `viewModelBasePatchNum` or PARENT, but has * some special logic when looking at merge commits. * - * NOTE: At the moment this returns just `routerBasePatchNum ?? PARENT`, see + * NOTE: At the moment this returns just `viewModelBasePatchNum ?? PARENT`, see * TODO below. */ function computeBase( - routerBasePatchNum: BasePatchSetNum | undefined, + viewModelBasePatchNum: BasePatchSetNum | undefined, patchNum: RevisionPatchSetNum | undefined, change: ParsedChangeInfo | undefined, preferences: PreferencesInfo ): BasePatchSetNum { - if (routerBasePatchNum && routerBasePatchNum !== PARENT) { - return routerBasePatchNum; + if (viewModelBasePatchNum && viewModelBasePatchNum !== PARENT) { + return viewModelBasePatchNum; } if (!change || !patchNum) return PARENT; @@ -129,7 +129,7 @@ // but we are not sure whether this was ever 100% working correctly. A // major challenge is being able to select PARENT explicitly even if your // preference for the default choice is FIRST_PARENT. <gr-file-list-header> - // just uses `navigation.setUrl()` and the router does not have any + // just uses `navigation.setUrl()` and the view model does not have any // way of forcing the basePatchSetNum to stick to PARENT without being // altered back to FIRST_PARENT here. // See also corresponding TODO in gr-settings-view. @@ -192,57 +192,57 @@ public readonly patchNum$: Observable<RevisionPatchSetNum | undefined> = select( combineLatest([ - this.routerModel.state$, + this.viewModel.state$, this.state$, this.latestPatchNum$, ]).pipe( /** - * If you depend on both, router and change state, then you want to - * filter out inconsistent state, e.g. router changeNum already updated, - * change not yet reset to undefined. + * If you depend on both, view model and change state, then you want to + * filter out inconsistent state, e.g. view model changeNum already + * updated, change not yet reset to undefined. */ - filter(([routerState, changeState, _latestPatchN]) => { + filter(([viewModelState, changeState, _latestPatchN]) => { const changeNum = changeState.change?._number; - const routerChangeNum = routerState.changeNum; - return changeNum === undefined || changeNum === routerChangeNum; + const viewModelChangeNum = viewModelState?.changeNum; + return changeNum === undefined || changeNum === viewModelChangeNum; }) ), - ([routerState, _changeState, latestPatchN]) => - routerState?.patchNum || latestPatchN + ([viewModelState, _changeState, latestPatchN]) => + viewModelState?.patchNum || latestPatchN ); /** * Emits the base patchset number. This is identical to the - * `routerBasePatchNum$`, but has some special logic for merges. + * `viewModel.basePatchNum$`, but has some special logic for merges. * * Note that this selector can emit without the change being available! */ public readonly basePatchNum$: Observable<BasePatchSetNum> = /** - * If you depend on both, router and change state, then you want to filter - * out inconsistent state, e.g. router changeNum already updated, change not - * yet reset to undefined. + * If you depend on both, view model and change state, then you want to + * filter out inconsistent state, e.g. view model changeNum already + * updated, change not yet reset to undefined. */ select( combineLatest([ - this.routerModel.state$, + this.viewModel.state$, this.state$, this.userModel.state$, ]).pipe( - filter(([routerState, changeState, _]) => { + filter(([viewModelState, changeState, _]) => { const changeNum = changeState.change?._number; - const routerChangeNum = routerState.changeNum; - return changeNum === undefined || changeNum === routerChangeNum; + const viewModelChangeNum = viewModelState?.changeNum; + return changeNum === undefined || changeNum === viewModelChangeNum; }), withLatestFrom( - this.routerModel.routerBasePatchNum$, + this.viewModel.basePatchNum$, this.patchNum$, this.change$, this.userModel.preferences$ ) ), - ([_, routerBasePatchNum, patchNum, change, preferences]) => - computeBase(routerBasePatchNum, patchNum, change, preferences) + ([_, viewModelBasePatchNum, patchNum, change, preferences]) => + computeBase(viewModelBasePatchNum, patchNum, change, preferences) ); public readonly isOwner$: Observable<boolean> = select( @@ -257,13 +257,13 @@ ); constructor( - private readonly routerModel: RouterModel, + private readonly viewModel: ChangeViewModel, private readonly restApiService: RestApiService, private readonly userModel: UserModel ) { super(initialState); this.subscriptions = [ - combineLatest([this.routerModel.routerChangeNum$, this.reload$]) + combineLatest([this.viewModel.changeNum$, this.reload$]) .pipe( map(([changeNum, _]) => changeNum), switchMap(changeNum => { @@ -272,7 +272,7 @@ const edit = from(this.restApiService.getChangeEdit(changeNum)); return forkJoin([change, edit]); }), - withLatestFrom(this.routerModel.routerPatchNum$), + withLatestFrom(this.viewModel.patchNum$), map(([[change, edit], patchNum]) => updateChangeWithEdit(change, edit, patchNum) )
diff --git a/polygerrit-ui/app/models/change/change-model_test.ts b/polygerrit-ui/app/models/change/change-model_test.ts index a2fc7c9..4bc23d8 100644 --- a/polygerrit-ui/app/models/change/change-model_test.ts +++ b/polygerrit-ui/app/models/change/change-model_test.ts
@@ -9,6 +9,7 @@ import { createChange, createChangeMessageInfo, + createChangeViewState, createEditInfo, createParsedChange, createRevision, @@ -28,12 +29,12 @@ } from '../../types/common'; import {ParsedChangeInfo} from '../../types/types'; import {getAppContext} from '../../services/app-context'; -import {GerritView, routerModelToken} from '../../services/router/router-model'; import {ChangeState, LoadingStatus, updateChangeWithEdit} from './change-model'; import {ChangeModel} from './change-model'; import {assert} from '@open-wc/testing'; import {testResolver} from '../../test/common-test-setup'; import {userModelToken} from '../user/user-model'; +import {changeViewModelToken} from '../views/change'; suite('updateChangeWithEdit() tests', () => { test('undefined change', async () => { @@ -83,7 +84,7 @@ setup(() => { changeModel = new ChangeModel( - testResolver(routerModelToken), + testResolver(changeViewModelToken), getAppContext().restApiService, testResolver(userModelToken) ); @@ -121,10 +122,7 @@ assert.equal(stub.callCount, 0); assert.isUndefined(state?.change); - testResolver(routerModelToken).setState({ - view: GerritView.CHANGE, - changeNum: knownChange._number, - }); + testResolver(changeViewModelToken).setState(createChangeViewState()); state = await waitForLoadingStatus(LoadingStatus.LOADING); assert.equal(stub.callCount, 1); assert.isUndefined(state?.change); @@ -140,10 +138,7 @@ const promise = mockPromise<ParsedChangeInfo | undefined>(); const stub = stubRestApi('getChangeDetail').callsFake(() => promise); let state: ChangeState; - testResolver(routerModelToken).setState({ - view: GerritView.CHANGE, - changeNum: knownChange._number, - }); + testResolver(changeViewModelToken).setState(createChangeViewState()); promise.resolve(knownChange); state = await waitForLoadingStatus(LoadingStatus.LOADED); @@ -164,10 +159,7 @@ let promise = mockPromise<ParsedChangeInfo | undefined>(); const stub = stubRestApi('getChangeDetail').callsFake(() => promise); let state: ChangeState; - testResolver(routerModelToken).setState({ - view: GerritView.CHANGE, - changeNum: knownChange._number, - }); + testResolver(changeViewModelToken).setState(createChangeViewState()); promise.resolve(knownChange); state = await waitForLoadingStatus(LoadingStatus.LOADED); @@ -178,8 +170,8 @@ _number: 123 as NumericChangeId, }; promise = mockPromise<ParsedChangeInfo | undefined>(); - testResolver(routerModelToken).setState({ - view: GerritView.CHANGE, + testResolver(changeViewModelToken).setState({ + ...createChangeViewState(), changeNum: otherChange._number, }); state = await waitForLoadingStatus(LoadingStatus.LOADING); @@ -197,10 +189,7 @@ let promise = mockPromise<ParsedChangeInfo | undefined>(); const stub = stubRestApi('getChangeDetail').callsFake(() => promise); let state: ChangeState; - testResolver(routerModelToken).setState({ - view: GerritView.CHANGE, - changeNum: knownChange._number, - }); + testResolver(changeViewModelToken).setState(createChangeViewState()); promise.resolve(knownChange); state = await waitForLoadingStatus(LoadingStatus.LOADED); @@ -208,10 +197,7 @@ promise = mockPromise<ParsedChangeInfo | undefined>(); promise.resolve(undefined); - testResolver(routerModelToken).setState({ - view: GerritView.CHANGE, - changeNum: undefined, - }); + testResolver(changeViewModelToken).setState(undefined); state = await waitForLoadingStatus(LoadingStatus.NOT_LOADED); assert.equal(stub.callCount, 2); assert.isUndefined(state?.change); @@ -220,10 +206,7 @@ promise = mockPromise<ParsedChangeInfo | undefined>(); promise.resolve(knownChange); - testResolver(routerModelToken).setState({ - view: GerritView.CHANGE, - changeNum: knownChange._number, - }); + testResolver(changeViewModelToken).setState(createChangeViewState()); state = await waitForLoadingStatus(LoadingStatus.LOADED); assert.equal(stub.callCount, 3); assert.equal(state?.change, knownChange); @@ -299,7 +282,7 @@ assert.equal(spy.lastCall.firstArg, PARENT); // test update - testResolver(routerModelToken).updateState({ + testResolver(changeViewModelToken).updateState({ basePatchNum: 1 as PatchSetNumber, }); assert.equal(spy.callCount, 2);
diff --git a/polygerrit-ui/app/models/comments/comments-model.ts b/polygerrit-ui/app/models/comments/comments-model.ts index 51b4591..1fdf342 100644 --- a/polygerrit-ui/app/models/comments/comments-model.ts +++ b/polygerrit-ui/app/models/comments/comments-model.ts
@@ -29,7 +29,6 @@ } from '../../utils/comment-util'; import {deepEqual} from '../../utils/deep-util'; import {select} from '../../utils/observable-util'; -import {RouterModel} from '../../services/router/router-model'; import {define} from '../dependency'; import {combineLatest, forkJoin, from, Observable, of} from 'rxjs'; import {fire, fireAlert, fireEvent} from '../../utils/event-util'; @@ -54,6 +53,7 @@ switchMap, } from 'rxjs/operators'; import {isDefined} from '../../types/types'; +import {ChangeViewModel} from '../views/change'; export interface CommentState { /** undefined means 'still loading' */ @@ -415,7 +415,7 @@ private discardedDrafts: DraftInfo[] = []; constructor( - private readonly routerModel: RouterModel, + private readonly changeViewModel: ChangeViewModel, private readonly changeModel: ChangeModel, private readonly accountsModel: AccountsModel, private readonly restApiService: RestApiService, @@ -432,7 +432,7 @@ this.changeModel.patchNum$.subscribe(x => (this.patchNum = x)) ); this.subscriptions.push( - this.routerModel.routerChangeNum$.subscribe(changeNum => { + this.changeViewModel.changeNum$.subscribe(changeNum => { this.changeNum = changeNum; this.setState({...initialState}); this.reloadAllComments();
diff --git a/polygerrit-ui/app/models/comments/comments-model_test.ts b/polygerrit-ui/app/models/comments/comments-model_test.ts index cab36d2..a689e42 100644 --- a/polygerrit-ui/app/models/comments/comments-model_test.ts +++ b/polygerrit-ui/app/models/comments/comments-model_test.ts
@@ -6,6 +6,7 @@ import '../../test/common-test-setup'; import { createAccountWithEmail, + createChangeViewState, createDraft, } from '../../test/test-data-generators'; import { @@ -20,17 +21,16 @@ import { createComment, createParsedChange, - TEST_NUMERIC_CHANGE_ID, } from '../../test/test-data-generators'; import {stubRestApi, waitUntil, waitUntilCalled} from '../../test/test-utils'; import {getAppContext} from '../../services/app-context'; -import {GerritView, routerModelToken} from '../../services/router/router-model'; import {PathToCommentsInfoMap} from '../../types/common'; import {changeModelToken} from '../change/change-model'; import {assert} from '@open-wc/testing'; import {testResolver} from '../../test/common-test-setup'; import {accountsModelToken} from '../accounts-model/accounts-model'; import {ChangeComments} from '../../elements/diff/gr-comment-api/gr-comment-api'; +import {changeViewModelToken} from '../views/change'; suite('comments model tests', () => { test('updateStateDeleteDraft', () => { @@ -72,7 +72,7 @@ test('loads comments', async () => { const model = new CommentsModel( - testResolver(routerModelToken), + testResolver(changeViewModelToken), testResolver(changeModelToken), testResolver(accountsModelToken), getAppContext().restApiService, @@ -100,10 +100,7 @@ model.portedComments$.subscribe(c => (portedComments = c ?? {})) ); - testResolver(routerModelToken).setState({ - view: GerritView.CHANGE, - changeNum: TEST_NUMERIC_CHANGE_ID, - }); + testResolver(changeViewModelToken).setState(createChangeViewState()); testResolver(changeModelToken).updateStateChange(createParsedChange()); await waitUntilCalled(diffCommentsSpy, 'diffCommentsSpy'); @@ -133,7 +130,7 @@ }; stubRestApi('getAccountDetails').returns(Promise.resolve(account)); const model = new CommentsModel( - testResolver(routerModelToken), + testResolver(changeViewModelToken), testResolver(changeModelToken), testResolver(accountsModelToken), getAppContext().restApiService, @@ -161,7 +158,7 @@ }; stubRestApi('getAccountDetails').returns(Promise.resolve(account)); const model = new CommentsModel( - testResolver(routerModelToken), + testResolver(changeViewModelToken), testResolver(changeModelToken), testResolver(accountsModelToken), getAppContext().restApiService, @@ -199,7 +196,7 @@ }) ); const model = new CommentsModel( - testResolver(routerModelToken), + testResolver(changeViewModelToken), testResolver(changeModelToken), testResolver(accountsModelToken), getAppContext().restApiService,
diff --git a/polygerrit-ui/app/models/views/change.ts b/polygerrit-ui/app/models/views/change.ts index 9701b54..3f3c76b 100644 --- a/polygerrit-ui/app/models/views/change.ts +++ b/polygerrit-ui/app/models/views/change.ts
@@ -200,6 +200,15 @@ define<ChangeViewModel>('change-view-model'); export class ChangeViewModel extends Model<ChangeViewState | undefined> { + public readonly changeNum$ = select(this.state$, state => state?.changeNum); + + public readonly patchNum$ = select(this.state$, state => state?.patchNum); + + public readonly basePatchNum$ = select( + this.state$, + state => state?.basePatchNum + ); + public readonly childView$ = select(this.state$, state => state?.childView); public readonly tab$ = select(this.state$, state => state?.tab);