Move loading of `mergeable` from change-view into change-model
Release-Notes: skip
Google-Bug-Id: b/247042673
Change-Id: I9b8e34e18e8082404fdebe26b25cae084a302c7b
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index a80c058..7e28af9 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -408,9 +408,7 @@
);
}
- // Private but used in tests.
- @state()
- mergeable: boolean | null = null;
+ @state() mergeable?: boolean;
/**
* Plugins can provide (multiple) tabs. For each plugin tab we render an
@@ -721,6 +719,11 @@
);
subscribe(
this,
+ () => this.getChangeModel().mergeable$,
+ mergeable => (this.mergeable = mergeable)
+ );
+ subscribe(
+ this,
() => this.getChangeModel().revision$,
revision => (this.revision = revision)
);
@@ -843,6 +846,8 @@
changedProperties.has('mergeable') ||
changedProperties.has('currentRevisionActions')
) {
+ // TODO: Just compute `changeStatuses` on the fly. No need for it to be a
+ // @state object.
this.changeStatuses = this.computeChangeStatusChips();
}
}
@@ -1421,7 +1426,6 @@
<div class="relatedChanges">
<gr-related-changes-list
id="relatedChanges"
- .mergeable=${this.mergeable}
></gr-related-changes-list>
</div>
<div class="emptySpace"></div>
@@ -1764,18 +1768,11 @@
}
private computeChangeStatusChips() {
- if (!this.change) {
- return [];
- }
-
- // Show no chips until mergeability is loaded.
- if (this.mergeable === null) {
- return [];
- }
+ if (!this.change || this.mergeable === undefined) return [];
const options = {
includeDerived: true,
- mergeable: !!this.mergeable,
+ mergeable: this.mergeable,
submitEnabled: !!this.isSubmitEnabled(),
};
return changeStatuses(this.change as ChangeInfo, options);
@@ -2130,7 +2127,7 @@
this.getPluginLoader().jsApiService.handleShowChange({
change: this.change,
patchNum: this.patchRange.patchNum,
- info: {mergeable: this.mergeable},
+ info: {mergeable: this.mergeable ?? null},
});
}
@@ -2697,11 +2694,6 @@
});
const coreDataPromise = loadingFlagSet;
- const mergeabilityLoaded = coreDataPromise.then(() =>
- this.getMergeability()
- );
- allDataPromises.push(mergeabilityLoaded);
-
coreDataPromise.then(() => {
fire(this, 'change-details-loaded', {});
this.reporting.timeEnd(Timing.CHANGE_RELOAD);
@@ -2769,43 +2761,6 @@
);
}
- // Private but used in tests
- getMergeability(): Promise<void> {
- if (!this.change) {
- this.mergeable = null;
- return Promise.resolve();
- }
- // If the change is closed, it is not mergeable. Note: already merged
- // changes are obviously not mergeable, but the mergeability API will not
- // answer for abandoned changes.
- if (
- this.change.status === ChangeStatus.MERGED ||
- this.change.status === ChangeStatus.ABANDONED
- ) {
- this.mergeable = false;
- return Promise.resolve();
- }
-
- if (!this.changeNum) {
- return Promise.reject(new Error('missing required changeNum property'));
- }
-
- // If mergeable bit was already returned in detail REST endpoint, use it.
- if (this.change.mergeable !== undefined) {
- this.mergeable = this.change.mergeable;
- return Promise.resolve();
- }
-
- this.mergeable = null;
- return this.restApiService
- .getMergeable(this.changeNum)
- .then(mergableInfo => {
- if (mergableInfo) {
- this.mergeable = mergableInfo.mergeable;
- }
- });
- }
-
/**
* Returns the text to be copied when
* click the copy icon next to change subject
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
index 44a8428..6ea19c7 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
@@ -35,7 +35,6 @@
createChange,
createChangeMessages,
createCommit,
- createMergeable,
createPreferences,
createRevision,
createRevisions,
@@ -75,8 +74,7 @@
SavingState,
} from '../../../types/common';
import {GrEditControls} from '../../edit/gr-edit-controls/gr-edit-controls';
-import {SinonFakeTimers, SinonStubbedMember} from 'sinon';
-import {RestApiService} from '../../../services/gr-rest-api/gr-rest-api';
+import {SinonFakeTimers} from 'sinon';
import {GerritView} from '../../../services/router/router-model';
import {ParsedChangeInfo} from '../../../types/types';
import {GrRelatedChangesList} from '../gr-related-changes-list/gr-related-changes-list';
@@ -800,7 +798,6 @@
Promise.resolve(createParsedChange())
);
sinon.stub(element, 'performPostChangeLoadTasks');
- sinon.stub(element, 'getMergeability');
const change = {
...createChangeViewChange(),
revisions: createRevisions(1),
@@ -1539,9 +1536,6 @@
'#relatedChanges'
) as GrRelatedChangesList;
const reloadStub = sinon.stub(relatedChanges, 'reload');
- stubRestApi('getMergeable').returns(
- Promise.resolve({...createMergeable(), mergeable: true})
- );
element.viewState = createChangeViewState();
changeModel.setState({
@@ -2103,42 +2097,6 @@
});
});
- suite('getMergeability', () => {
- let getMergeableStub: SinonStubbedMember<RestApiService['getMergeable']>;
- setup(() => {
- element.change = {...createChangeViewChange(), labels: {}};
- getMergeableStub = stubRestApi('getMergeable').returns(
- Promise.resolve({...createMergeable(), mergeable: true})
- );
- });
-
- test('merged change', () => {
- element.mergeable = null;
- element.change!.status = ChangeStatus.MERGED;
- return element.getMergeability().then(() => {
- assert.isFalse(element.mergeable);
- assert.isFalse(getMergeableStub.called);
- });
- });
-
- test('abandoned change', () => {
- element.mergeable = null;
- element.change!.status = ChangeStatus.ABANDONED;
- return element.getMergeability().then(() => {
- assert.isFalse(element.mergeable);
- assert.isFalse(getMergeableStub.called);
- });
- });
-
- test('open change', () => {
- element.mergeable = null;
- return element.getMergeability().then(() => {
- assert.isTrue(element.mergeable);
- assert.isTrue(getMergeableStub.called);
- });
- });
- });
-
test('handleToggleStar called when star is tapped', async () => {
element.change = {
...createChangeViewChange(),
@@ -2162,7 +2120,6 @@
patchNum: 1 as RevisionPatchSetNum,
};
sinon.stub(element, 'performPostChangeLoadTasks');
- sinon.stub(element, 'getMergeability').returns(Promise.resolve());
});
test("don't report changeDisplayed on reply", async () => {
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
index 05acb29..9d03d15 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
@@ -11,7 +11,7 @@
import '../../shared/gr-icon/gr-icon';
import {classMap} from 'lit/directives/class-map.js';
import {LitElement, css, html, TemplateResult} from 'lit';
-import {customElement, property, state} from 'lit/decorators.js';
+import {customElement, state} from 'lit/decorators.js';
import {sharedStyles} from '../../../styles/shared-styles';
import {
ChangeInfo,
@@ -54,7 +54,7 @@
@customElement('gr-related-changes-list')
export class GrRelatedChangesList extends LitElement {
- @property({type: Boolean})
+ @state()
mergeable?: boolean;
@state()
@@ -97,6 +97,11 @@
() => this.getChangeModel().latestPatchNum$,
x => (this.latestPatchNum = x)
);
+ subscribe(
+ this,
+ () => this.getChangeModel().mergeable$,
+ x => (this.mergeable = x)
+ );
}
static override get styles() {
@@ -616,8 +621,6 @@
}),
];
- // Get conflicts if change is open and is mergeable.
- // Mergeable is output of restApiServict.getMergeable from gr-change-view
if (changeIsOpen(change) && this.mergeable) {
promises.push(
this.restApiService
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts
index 6d575ce..da88bfc 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -15,7 +15,7 @@
PatchSetNumber,
CommitId,
} from '../../types/common';
-import {DefaultBase} from '../../constants/constants';
+import {ChangeStatus, DefaultBase} from '../../constants/constants';
import {combineLatest, from, fromEvent, Observable, forkJoin, of} from 'rxjs';
import {
map,
@@ -71,6 +71,13 @@
* Undefined means it's still loading and empty set means no files reviewed.
*/
reviewedFiles?: string[];
+ /**
+ * Either filled from `change.mergeable`, or from a dedicated REST API call.
+ * Is initially `undefined`, such that you can identify whether this
+ * information has already been loaded once for this change or not. Will never
+ * go back to `undefined` after being set for a change.
+ */
+ mergeable?: boolean;
}
/**
@@ -201,6 +208,11 @@
changeState => changeState?.reviewedFiles
);
+ public readonly mergeable$ = select(
+ this.state$,
+ changeState => changeState.mergeable
+ );
+
public readonly changeNum$ = select(this.change$, change => change?._number);
public readonly repo$ = select(this.change$, change => change?.project);
@@ -353,6 +365,21 @@
// helps with that.
this.updateStateChange(change ?? undefined);
}),
+ this.change$
+ .pipe(
+ switchMap(change => {
+ if (change?._number === undefined) return of(undefined);
+ if (change.mergeable !== undefined) return of(change.mergeable);
+ if (change.status === ChangeStatus.MERGED) return of(false);
+ if (change.status === ChangeStatus.ABANDONED) return of(false);
+ return from(
+ this.restApiService
+ .getMergeable(change._number)
+ .then(mergableInfo => mergableInfo?.mergeable ?? false)
+ );
+ })
+ )
+ .subscribe(mergeable => this.updateState({mergeable})),
this.change$.subscribe(change => (this.change = change)),
this.patchNum$.subscribe(patchNum => (this.patchNum = patchNum)),
this.basePatchNum$.subscribe(
diff --git a/polygerrit-ui/app/models/change/change-model_test.ts b/polygerrit-ui/app/models/change/change-model_test.ts
index 2ec0b20..5ce848d 100644
--- a/polygerrit-ui/app/models/change/change-model_test.ts
+++ b/polygerrit-ui/app/models/change/change-model_test.ts
@@ -11,6 +11,7 @@
createChangeMessageInfo,
createChangeViewState,
createEditInfo,
+ createMergeable,
createParsedChange,
createRevision,
} from '../../test/test-data-generators';
@@ -41,6 +42,7 @@
import {userModelToken} from '../user/user-model';
import {ChangeViewModel, changeViewModelToken} from '../views/change';
import {navigationToken} from '../../elements/core/gr-navigation/gr-navigation';
+import {SinonStub} from 'sinon';
suite('updateRevisionsWithCommitShas() tests', () => {
test('undefined edit', async () => {
@@ -123,6 +125,93 @@
changeModel.finalize();
});
+ suite('mergeability', async () => {
+ let getMergeableStub: SinonStub;
+ let mergeableApiResponse = false;
+
+ setup(() => {
+ getMergeableStub = stubRestApi('getMergeable').callsFake(() =>
+ Promise.resolve(createMergeable(mergeableApiResponse))
+ );
+ });
+
+ test('mergeability initially undefined', async () => {
+ waitUntilObserved(
+ changeModel.mergeable$,
+ mergeable => mergeable === undefined
+ );
+ assert.isFalse(getMergeableStub.called);
+ });
+
+ test('mergeability true from change', async () => {
+ changeModel.updateStateChange({...knownChange, mergeable: true});
+
+ waitUntilObserved(
+ changeModel.mergeable$,
+ mergeable => mergeable === true
+ );
+ assert.isFalse(getMergeableStub.called);
+ });
+
+ test('mergeability false from change', async () => {
+ changeModel.updateStateChange({...knownChange, mergeable: false});
+
+ waitUntilObserved(
+ changeModel.mergeable$,
+ mergeable => mergeable === true
+ );
+ assert.isFalse(getMergeableStub.called);
+ });
+
+ test('mergeability false for MERGED change', async () => {
+ changeModel.updateStateChange({
+ ...knownChange,
+ status: ChangeStatus.MERGED,
+ });
+
+ waitUntilObserved(
+ changeModel.mergeable$,
+ mergeable => mergeable === false
+ );
+ assert.isFalse(getMergeableStub.called);
+ });
+
+ test('mergeability false for ABANDONED change', async () => {
+ changeModel.updateStateChange({
+ ...knownChange,
+ status: ChangeStatus.ABANDONED,
+ });
+
+ waitUntilObserved(
+ changeModel.mergeable$,
+ mergeable => mergeable === false
+ );
+ assert.isFalse(getMergeableStub.called);
+ });
+
+ test('mergeability true from API', async () => {
+ mergeableApiResponse = true;
+ changeModel.updateStateChange(knownChange);
+
+ waitUntilObserved(
+ changeModel.mergeable$,
+ mergeable => mergeable === true
+ );
+ assert.isTrue(getMergeableStub.calledOnce);
+ });
+
+ test('mergeability false from API', async () => {
+ mergeableApiResponse = false;
+ changeModel.updateStateChange(knownChange);
+
+ waitUntilObserved(
+ changeModel.mergeable$,
+ mergeable => mergeable === false
+ );
+ assert.isTrue(getMergeableStub.calledOnce);
+ });
+ });
+
test('load a change', async () => {
const promise = mockPromise<ParsedChangeInfo | undefined>();
const stub = stubRestApi('getChangeDetail').callsFake(() => promise);
diff --git a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
index b60ace5..f8e4160 100644
--- a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
+++ b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
@@ -67,6 +67,7 @@
createChange,
createCommit,
createConfig,
+ createMergeable,
createPreferences,
createServerInfo,
createSubmittedTogetherInfo,
@@ -368,7 +369,7 @@
return Promise.resolve(true);
},
getMergeable(): Promise<MergeableInfo | undefined> {
- throw new Error('getMergeable() not implemented by RestApiMock.');
+ return Promise.resolve(createMergeable());
},
getPlugins(): Promise<{[p: string]: PluginInfo} | undefined> {
return Promise.resolve({});
diff --git a/polygerrit-ui/app/test/test-data-generators.ts b/polygerrit-ui/app/test/test-data-generators.ts
index a92e932..dce8e4b 100644
--- a/polygerrit-ui/app/test/test-data-generators.ts
+++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -672,10 +672,10 @@
};
}
-export function createMergeable(): MergeableInfo {
+export function createMergeable(mergeable = false): MergeableInfo {
return {
submit_type: SubmitType.MERGE_IF_NECESSARY,
- mergeable: false,
+ mergeable,
};
}
