Move status and revert loading from change view into models
Release-Notes: skip
Google-Bug-Id: b/247042673
Change-Id: I1f372b2ae62ba8fa1d978c726c29ede8754a5d2f
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 6892549..00e9550 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
@@ -87,7 +87,6 @@
ServerInfo,
UrlEncodedCommentId,
isRobot,
- ChangeStates,
} from '../../../types/common';
import {FocusTarget, GrReplyDialog} from '../gr-reply-dialog/gr-reply-dialog';
import {GrIncludedInDialog} from '../gr-included-in-dialog/gr-included-in-dialog';
@@ -130,7 +129,6 @@
until,
} from '../../../utils/async-util';
import {Interaction, Timing} from '../../../constants/reporting';
-import {getRevertCreatedChangeIds} from '../../../utils/message-util';
import {
getAddedByReason,
getRemovedByReason,
@@ -146,7 +144,7 @@
import {resolve} from '../../../models/dependency';
import {checksModelToken} from '../../../models/checks/checks-model';
import {changeModelToken} from '../../../models/change/change-model';
-import {css, html, LitElement, nothing, PropertyValues} from 'lit';
+import {css, html, LitElement, nothing} from 'lit';
import {a11yStyles} from '../../../styles/gr-a11y-styles';
import {paperStyles} from '../../../styles/gr-paper-styles';
import {sharedStyles} from '../../../styles/shared-styles';
@@ -170,6 +168,7 @@
import {userModelToken} from '../../../models/user/user-model';
import {pluginLoaderToken} from '../../shared/gr-js-api-interface/gr-plugin-loader';
import {modalStyles} from '../../../styles/gr-modal-styles';
+import {relatedChangesModelToken} from '../../../models/change/related-changes-model';
const MIN_LINES_FOR_COMMIT_COLLAPSE = 18;
@@ -381,10 +380,6 @@
@state()
replyDisabled = true;
- // Private but used in tests.
- @state()
- changeStatuses: ChangeStates[] = [];
-
@state()
private updateCheckTimerHandle?: number | null;
@@ -468,7 +463,7 @@
private tabState?: TabState;
@state()
- private revertedChange?: ChangeInfo;
+ private revertingChange?: ChangeInfo;
// Private but used in tests.
@state()
@@ -499,6 +494,11 @@
private readonly getViewModel = resolve(this, changeViewModelToken);
+ private readonly getRelatedChangesModel = resolve(
+ this,
+ relatedChangesModelToken
+ );
+
private readonly getShortcutsService = resolve(this, shortcutsServiceToken);
private scrollTask?: DelayedTask;
@@ -758,6 +758,13 @@
this.projectConfig = config;
}
);
+ subscribe(
+ this,
+ () => this.getRelatedChangesModel().revertingChange$,
+ revertingChange => {
+ this.revertingChange = revertingChange;
+ }
+ );
}
override connectedCallback() {
@@ -833,18 +840,6 @@
super.disconnectedCallback();
}
- protected override willUpdate(changedProperties: PropertyValues): void {
- if (
- changedProperties.has('change') ||
- 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();
- }
- }
-
static override get styles() {
return [
a11yStyles,
@@ -1224,13 +1219,14 @@
}
private renderHeaderTitle() {
+ const changeStatuses = this.computeChangeStatusChips();
const resolveWeblinks =
this.revision?.commit?.resolve_conflicts_web_links ?? [];
return html` <div class="headerTitle">
<div class="changeStatuses">
- ${this.changeStatuses.map(
+ ${changeStatuses.map(
status => html` <gr-change-status
- .revertedChange=${this.revertedChange}
+ .revertedChange=${this.revertingChange}
.status=${status}
.resolveWeblinks=${resolveWeblinks}
></gr-change-status>`
@@ -1358,7 +1354,7 @@
<gr-change-metadata
id="metadata"
.change=${this.change}
- .revertedChange=${this.revertedChange}
+ .revertedChange=${this.revertingChange}
.account=${this.account}
.revision=${this.revision}
.commitInfo=${this.revision?.commit}
@@ -1761,9 +1757,9 @@
if (!this.change || this.mergeable === undefined) return [];
const options = {
- includeDerived: true,
mergeable: this.mergeable,
submitEnabled: !!this.isSubmitEnabled(),
+ revertingChangeStatus: this.revertingChange?.status,
};
return changeStatuses(this.change as ChangeInfo, options);
}
@@ -2557,41 +2553,6 @@
}
}
- computeRevertSubmitted(change?: ChangeInfo | ParsedChangeInfo) {
- if (!change?.messages) return;
- Promise.all(
- getRevertCreatedChangeIds(change.messages).map(changeId =>
- this.restApiService.getChange(changeId)
- )
- ).then(changes => {
- // if a change is deleted then getChanges returns null for that changeId
- changes = changes.filter(
- change => change && change.status !== ChangeStatus.ABANDONED
- );
- if (!changes.length) return;
- const submittedRevert = changes.find(
- change => change?.status === ChangeStatus.MERGED
- );
- if (!this.changeStatuses) return;
- // Protect against `computeRevertSubmitted()` being called twice.
- // TODO: Convert this to be rxjs based, so computeRevertSubmitted() is not
- // actively called, but instead we can subscribe to something.
- if (this.changeStatuses.includes(ChangeStates.REVERT_SUBMITTED)) return;
- if (this.changeStatuses.includes(ChangeStates.REVERT_CREATED)) return;
- if (submittedRevert) {
- this.revertedChange = submittedRevert;
- this.changeStatuses = this.changeStatuses.concat([
- ChangeStates.REVERT_SUBMITTED,
- ]);
- } else {
- if (changes[0]) this.revertedChange = changes[0];
- this.changeStatuses = this.changeStatuses.concat([
- ChangeStates.REVERT_CREATED,
- ]);
- }
- });
- }
-
private async untilModelLoaded() {
// NOTE: Wait until this page is connected before determining whether the
// model is loaded. This can happen when viewState changes when setting up
@@ -2632,8 +2593,6 @@
if (!this.change.reviewer_updates) {
this.change.reviewer_updates = null as unknown as undefined;
}
-
- this.computeRevertSubmitted(this.change);
}
private isParentCurrent() {
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 736b5c5..845ab65 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
@@ -12,7 +12,6 @@
CommentSide,
DefaultBase,
DiffViewMode,
- MessageTag,
createDefaultPreferences,
Tab,
} from '../../../constants/constants';
@@ -32,7 +31,6 @@
import {
createChangeViewState,
createApproval,
- createChange,
createChangeMessages,
createCommit,
createPreferences,
@@ -56,7 +54,6 @@
EDIT,
NumericChangeId,
PARENT,
- ReviewInputTag,
RevisionPatchSetNum,
RobotId,
RobotCommentInfo,
@@ -67,7 +64,6 @@
QuickLabelInfo,
PatchSetNumber,
CommentThread,
- ChangeStates,
SavingState,
} from '../../../types/common';
import {GrEditControls} from '../../edit/gr-edit-controls/gr-edit-controls';
@@ -1126,183 +1122,6 @@
);
});
- test('changeStatuses', async () => {
- element.loading = false;
- element.change = {
- ...createChangeViewChange(),
- revisions: {
- rev2: createRevision(2),
- rev1: createRevision(1),
- rev13: createRevision(13),
- rev3: createRevision(3),
- },
- current_revision: 'rev3' as CommitId,
- status: ChangeStatus.MERGED,
- labels: {
- test: {
- all: [],
- default_value: 0,
- values: {},
- approved: {},
- },
- },
- };
- element.mergeable = true;
- await element.updateComplete;
- const expectedStatuses = [ChangeStates.MERGED];
- assert.deepEqual(element.changeStatuses, expectedStatuses);
- const statusChips =
- element.shadowRoot!.querySelectorAll('gr-change-status');
- assert.equal(statusChips.length, 1);
- });
-
- suite('ChangeStatus revert', () => {
- test('do not show any chip if no revert created', async () => {
- const change = {
- ...createParsedChange(),
- messages: createChangeMessages(2),
- };
- const getChangeStub = stubRestApi('getChange');
- getChangeStub.onFirstCall().returns(
- Promise.resolve({
- ...createChange(),
- })
- );
- getChangeStub.onSecondCall().returns(
- Promise.resolve({
- ...createChange(),
- })
- );
- element.change = change;
- element.mergeable = true;
- element.currentRevisionActions = {submit: {enabled: true}};
- assert.isTrue(element.isSubmitEnabled());
- await element.updateComplete;
- element.computeRevertSubmitted(element.change);
- await element.updateComplete;
- assert.isFalse(
- element.changeStatuses?.includes(ChangeStates.REVERT_SUBMITTED)
- );
- assert.isFalse(
- element.changeStatuses?.includes(ChangeStates.REVERT_CREATED)
- );
- });
-
- test('do not show any chip if all reverts are abandoned', async () => {
- const change = {
- ...createParsedChange(),
- messages: createChangeMessages(2),
- };
- change.messages[0].message = 'Created a revert of this change as 12345';
- change.messages[0].tag = MessageTag.TAG_REVERT as ReviewInputTag;
-
- change.messages[1].message = 'Created a revert of this change as 23456';
- change.messages[1].tag = MessageTag.TAG_REVERT as ReviewInputTag;
-
- const getChangeStub = stubRestApi('getChange');
- getChangeStub.onFirstCall().returns(
- Promise.resolve({
- ...createChange(),
- status: ChangeStatus.ABANDONED,
- })
- );
- getChangeStub.onSecondCall().returns(
- Promise.resolve({
- ...createChange(),
- status: ChangeStatus.ABANDONED,
- })
- );
- element.change = change;
- element.mergeable = true;
- element.currentRevisionActions = {submit: {enabled: true}};
- assert.isTrue(element.isSubmitEnabled());
- await element.updateComplete;
- element.computeRevertSubmitted(element.change);
- await element.updateComplete;
- assert.isFalse(
- element.changeStatuses?.includes(ChangeStates.REVERT_SUBMITTED)
- );
- assert.isFalse(
- element.changeStatuses?.includes(ChangeStates.REVERT_CREATED)
- );
- });
-
- test('show revert created if no revert is merged', async () => {
- const change = {
- ...createParsedChange(),
- messages: createChangeMessages(2),
- };
- change.messages[0].message = 'Created a revert of this change as 12345';
- change.messages[0].tag = MessageTag.TAG_REVERT as ReviewInputTag;
-
- change.messages[1].message = 'Created a revert of this change as 23456';
- change.messages[1].tag = MessageTag.TAG_REVERT as ReviewInputTag;
-
- const getChangeStub = stubRestApi('getChange');
- getChangeStub.onFirstCall().returns(
- Promise.resolve({
- ...createChange(),
- })
- );
- getChangeStub.onSecondCall().returns(
- Promise.resolve({
- ...createChange(),
- })
- );
- element.change = change;
- element.mergeable = true;
- element.currentRevisionActions = {submit: {enabled: true}};
- assert.isTrue(element.isSubmitEnabled());
- await element.updateComplete;
- element.computeRevertSubmitted(element.change);
- // Wait for promises to settle.
- await waitEventLoop();
- await element.updateComplete;
- assert.isFalse(
- element.changeStatuses?.includes(ChangeStates.REVERT_SUBMITTED)
- );
- assert.isTrue(
- element.changeStatuses?.includes(ChangeStates.REVERT_CREATED)
- );
- });
-
- test('show revert submitted if revert is merged', async () => {
- const change = {
- ...createParsedChange(),
- messages: createChangeMessages(2),
- };
- change.messages[0].message = 'Created a revert of this change as 12345';
- change.messages[0].tag = MessageTag.TAG_REVERT as ReviewInputTag;
- const getChangeStub = stubRestApi('getChange');
- getChangeStub.onFirstCall().returns(
- Promise.resolve({
- ...createChange(),
- status: ChangeStatus.MERGED,
- })
- );
- getChangeStub.onSecondCall().returns(
- Promise.resolve({
- ...createChange(),
- })
- );
- element.change = change;
- element.mergeable = true;
- element.currentRevisionActions = {submit: {enabled: true}};
- assert.isTrue(element.isSubmitEnabled());
- await element.updateComplete;
- element.computeRevertSubmitted(element.change);
- // Wait for promises to settle.
- await waitEventLoop();
- await element.updateComplete;
- assert.isFalse(
- element.changeStatuses?.includes(ChangeStates.REVERT_CREATED)
- );
- assert.isTrue(
- element.changeStatuses?.includes(ChangeStates.REVERT_SUBMITTED)
- );
- });
- });
-
test('diff preferences open when open-diff-prefs is fired', async () => {
await element.updateComplete;
assertIsDefined(element.fileList);
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts
index 8be2c51..4863852 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -48,6 +48,7 @@
createEditUrl,
} from '../views/change';
import {NavigationService} from '../../elements/core/gr-navigation/gr-navigation';
+import {getRevertCreatedChangeIds} from '../../utils/message-util';
export enum LoadingStatus {
NOT_LOADED = 'NOT_LOADED',
@@ -332,6 +333,12 @@
([change, account]) => isOwner(change, account)
);
+ public readonly messages$ = select(this.change$, change => change?.messages);
+
+ public readonly revertingChangeIds$ = select(this.messages$, messages =>
+ getRevertCreatedChangeIds(messages ?? [])
+ );
+
// For usage in `combineLatest` we need `startWith` such that reload$ has an
// initial value.
readonly reload$: Observable<unknown> = fromEvent(document, 'reload').pipe(
diff --git a/polygerrit-ui/app/models/change/related-changes-model.ts b/polygerrit-ui/app/models/change/related-changes-model.ts
index 02d9f88..901999c 100644
--- a/polygerrit-ui/app/models/change/related-changes-model.ts
+++ b/polygerrit-ui/app/models/change/related-changes-model.ts
@@ -13,10 +13,11 @@
import {Model} from '../model';
import {define} from '../dependency';
import {ChangeModel} from './change-model';
-import {combineLatest, from, of} from 'rxjs';
-import {switchMap} from 'rxjs/operators';
+import {combineLatest, forkJoin, from, of} from 'rxjs';
+import {map, switchMap} from 'rxjs/operators';
import {ConfigModel} from '../config/config-model';
import {ChangeStatus} from '../../api/rest-api';
+import {isDefined} from '../../types/types';
export interface RelatedChangesState {
/** `undefined` means "not yet loaded". */
@@ -25,6 +26,7 @@
cherryPicks?: ChangeInfo[];
conflictingChanges?: ChangeInfo[];
sameTopicChanges?: ChangeInfo[];
+ revertingChanges: ChangeInfo[];
}
const initialState: RelatedChangesState = {
@@ -33,6 +35,7 @@
cherryPicks: undefined,
conflictingChanges: undefined,
sameTopicChanges: undefined,
+ revertingChanges: [],
};
export const relatedChangesModelToken = define<RelatedChangesModel>(
@@ -66,6 +69,32 @@
);
/**
+ * Emits all changes that have reverted the current change, based on
+ * information from parsed change messages. Abandoned changes are not
+ * included.
+ */
+ public readonly revertingChanges$ = select(
+ this.state$,
+ state => state.revertingChanges
+ );
+
+ /**
+ * Emits one reverting change (if there is any) from revertingChanges$.
+ * It prefers MERGED changes. Otherwise the choice is random.
+ */
+ public readonly revertingChange$ = select(
+ this.revertingChanges$,
+ revertingChanges => {
+ if (revertingChanges.length === 0) return undefined;
+ const submittedRevert = revertingChanges.find(
+ c => c.status === ChangeStatus.MERGED
+ );
+ if (submittedRevert) return submittedRevert;
+ return revertingChanges[0];
+ }
+ );
+
+ /**
* Determines whether the change has a parent change. If there
* is a relation chain, and the change id is not the last item of the
* relation chain, then there is a parent.
@@ -93,6 +122,7 @@
this.loadCherryPicks(),
this.loadConflictingChanges(),
this.loadSameTopicChanges(),
+ this.loadRevertingChanges(),
];
}
@@ -197,4 +227,26 @@
this.updateState({sameTopicChanges});
});
}
+
+ private loadRevertingChanges() {
+ return combineLatest([
+ this.changeModel.reload$,
+ this.changeModel.revertingChangeIds$,
+ ])
+ .pipe(
+ switchMap(([_, changeIds]) => {
+ if (!changeIds?.length) return of([]);
+ return forkJoin(
+ changeIds.map(changeId =>
+ from(this.restApiService.getChange(changeId))
+ )
+ );
+ }),
+ map(changes => changes.filter(isDefined)),
+ map(changes => changes.filter(c => c.status !== ChangeStatus.ABANDONED))
+ )
+ .subscribe(revertingChanges => {
+ this.updateState({revertingChanges});
+ });
+ }
}
diff --git a/polygerrit-ui/app/models/change/related-changes-model_test.ts b/polygerrit-ui/app/models/change/related-changes-model_test.ts
index 31c0026..295f284 100644
--- a/polygerrit-ui/app/models/change/related-changes-model_test.ts
+++ b/polygerrit-ui/app/models/change/related-changes-model_test.ts
@@ -22,8 +22,10 @@
createRelatedChangesInfo,
createRelatedChangeAndCommitInfo,
createChange,
+ createChangeMessage,
} from '../../test/test-data-generators';
-import {ChangeStatus, TopicName} from '../../api/rest-api';
+import {ChangeStatus, ReviewInputTag, TopicName} from '../../api/rest-api';
+import {MessageTag} from '../../constants/constants';
suite('related-changes-model tests', () => {
let model: RelatedChangesModel;
@@ -235,4 +237,50 @@
assert.isTrue(getChangesWithSameTopicStub.calledOnce);
});
});
+
+ suite('loadRevertingChanges', async () => {
+ let getChangeStub: SinonStub;
+
+ setup(() => {
+ getChangeStub = stubRestApi('getChange').callsFake(() =>
+ Promise.resolve(createChange())
+ );
+ });
+
+ test('revertingChanges initially empty', async () => {
+ await waitUntilObserved(
+ model.revertingChanges$,
+ revertingChanges => revertingChanges.length === 0
+ );
+ assert.isFalse(getChangeStub.called);
+ });
+
+ test('revertingChanges empty when change does not contain a revert message', async () => {
+ changeModel.updateStateChange(createParsedChange());
+ await waitUntilObserved(
+ model.revertingChanges$,
+ revertingChanges => revertingChanges.length === 0
+ );
+ assert.isFalse(getChangeStub.called);
+ });
+
+ test('revertingChanges emits', async () => {
+ changeModel.updateStateChange({
+ ...createParsedChange(),
+ messages: [
+ {
+ ...createChangeMessage(),
+ message: 'Created a revert of this change as 123',
+ tag: MessageTag.TAG_REVERT as ReviewInputTag,
+ },
+ ],
+ });
+
+ await waitUntilObserved(
+ model.revertingChanges$,
+ revertingChanges => revertingChanges?.length === 1
+ );
+ assert.isTrue(getChangeStub.calledOnce);
+ });
+ });
});
diff --git a/polygerrit-ui/app/utils/change-util.ts b/polygerrit-ui/app/utils/change-util.ts
index 5dda8d6..4490afa 100644
--- a/polygerrit-ui/app/utils/change-util.ts
+++ b/polygerrit-ui/app/utils/change-util.ts
@@ -20,6 +20,8 @@
interface ChangeStatusesOptions {
mergeable: boolean; // This can be wrong! See WARNING above
submitEnabled: boolean; // This can be wrong! See WARNING above
+ /** Is there a reverting change and if so, what status has it? */
+ revertingChangeStatus?: ChangeStatus;
}
export const ChangeDiffType = {
@@ -158,6 +160,12 @@
): ChangeStates[] {
const states = [];
if (change.status === ChangeStatus.MERGED) {
+ if (options?.revertingChangeStatus === ChangeStatus.MERGED) {
+ return [ChangeStates.MERGED, ChangeStates.REVERT_SUBMITTED];
+ }
+ if (options?.revertingChangeStatus !== undefined) {
+ return [ChangeStates.MERGED, ChangeStates.REVERT_CREATED];
+ }
return [ChangeStates.MERGED];
}
if (change.status === ChangeStatus.ABANDONED) {
diff --git a/polygerrit-ui/app/utils/change-util_test.ts b/polygerrit-ui/app/utils/change-util_test.ts
index ccaa1db..f768145 100644
--- a/polygerrit-ui/app/utils/change-util_test.ts
+++ b/polygerrit-ui/app/utils/change-util_test.ts
@@ -129,6 +129,32 @@
assert.deepEqual(changeStatuses(change), [ChangeStates.MERGED]);
});
+ test('Merged and Reverted status', () => {
+ const change = {
+ ...createChange(),
+ revisions: createRevisions(1),
+ current_revision: 'rev1' as CommitId,
+ status: ChangeStatus.MERGED,
+ };
+ assert.deepEqual(changeStatuses(change), [ChangeStates.MERGED]);
+ assert.deepEqual(
+ changeStatuses(change, {
+ revertingChangeStatus: ChangeStatus.NEW,
+ mergeable: true,
+ submitEnabled: true,
+ }),
+ [ChangeStates.MERGED, ChangeStates.REVERT_CREATED]
+ );
+ assert.deepEqual(
+ changeStatuses(change, {
+ revertingChangeStatus: ChangeStatus.MERGED,
+ mergeable: true,
+ submitEnabled: true,
+ }),
+ [ChangeStates.MERGED, ChangeStates.REVERT_SUBMITTED]
+ );
+ });
+
test('Abandoned status', () => {
const change = {
...createChange(),
diff --git a/polygerrit-ui/app/utils/message-util_test.ts b/polygerrit-ui/app/utils/message-util_test.ts
new file mode 100644
index 0000000..22a5e4d
--- /dev/null
+++ b/polygerrit-ui/app/utils/message-util_test.ts
@@ -0,0 +1,37 @@
+/**
+ * @license
+ * Copyright 2023 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+import {getRevertCreatedChangeIds} from './message-util';
+import {assert} from '@open-wc/testing';
+import {MessageTag} from '../constants/constants';
+import {ChangeId, ReviewInputTag} from '../api/rest-api';
+import {createChangeMessage} from '../test/test-data-generators';
+
+suite('message-util tests', () => {
+ test('getRevertCreatedChangeIds', () => {
+ const messages = [
+ {
+ ...createChangeMessage(),
+ message: 'Created a revert of this change as 123',
+ tag: MessageTag.TAG_REVERT as ReviewInputTag,
+ },
+ {
+ ...createChangeMessage(),
+ message: 'Created a revert of this change as xyz',
+ tag: MessageTag.TAG_REVERT as ReviewInputTag,
+ },
+ {
+ ...createChangeMessage(),
+ message: 'Created a revert of this change as abc',
+ tag: undefined,
+ },
+ ];
+
+ assert.deepEqual(getRevertCreatedChangeIds(messages), [
+ '123' as ChangeId,
+ 'xyz' as ChangeId,
+ ]);
+ });
+});
