Merge changes I160d92d6,Ia70dfc06,Icd55739b,I9d52653c * changes: Convert gr-repo-list to lit Convert gr-repo-list_test.js to typescript Migrate gr-create-repo-dialog to lit Fix template problems with gr-create-repo-dialog
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts index b10c17e..536ef92 100644 --- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts +++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -99,7 +99,6 @@ getVotingRange, StandardLabels, } from '../../../utils/label-util'; -import {CommentThread} from '../../../utils/comment-util'; import {ShowAlertEventDetail} from '../../../types/events'; import { ActionPriority, @@ -448,9 +447,6 @@ @property({type: String}) _actionLoadingMessage = ''; - @property({type: Array}) - commentThreads: CommentThread[] = []; - @property({ type: Array, computed:
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_html.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_html.ts index d21c29f..17ca7cf 100644 --- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_html.ts +++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_html.ts
@@ -213,11 +213,9 @@ <gr-confirm-submit-dialog id="confirmSubmitDialog" class="confirmDialog" - change="[[change]]" action="[[_revisionSubmitAction]]" on-cancel="_handleConfirmDialogCancel" on-confirm="_handleSubmitConfirm" - comment-threads="[[commentThreads]]" hidden="" ></gr-confirm-submit-dialog> <gr-dialog
diff --git a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts index e4b466b..c4ccb7d 100644 --- a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts +++ b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
@@ -15,7 +15,7 @@ * limitations under the License. */ import {LitElement, css, html} from 'lit'; -import {customElement, property} from 'lit/decorators'; +import {customElement, property, state} from 'lit/decorators'; import {subscribe} from '../../lit/subscription-controller'; import {sharedStyles} from '../../../styles/shared-styles'; import {appContext} from '../../../services/app-context'; @@ -65,6 +65,11 @@ import {modifierPressed} from '../../../utils/dom-util'; import {DropdownLink} from '../../shared/gr-dropdown/gr-dropdown'; import {fontStyles} from '../../../styles/gr-font-styles'; +import {account$} from '../../../services/user/user-model'; +import { + changeComments$, + threads$, +} from '../../../services/comments/comments-model'; export enum SummaryChipStyles { INFO = 'info', @@ -378,31 +383,31 @@ @customElement('gr-change-summary') export class GrChangeSummary extends LitElement { - @property({type: Object}) + @state() changeComments?: ChangeComments; - @property({type: Array}) + @state() commentThreads?: CommentThread[]; - @property({type: Object}) + @state() selfAccount?: AccountInfo; - @property() + @state() runs: CheckRun[] = []; - @property() + @state() showChecksSummary = false; - @property() + @state() someProvidersAreLoading = false; - @property() + @state() errorMessages: ErrorMessages = {}; - @property() + @state() loginCallback?: () => void; - @property() + @state() actions: Action[] = []; private showAllChips = new Map<RunStatus | Category, boolean>(); @@ -421,6 +426,9 @@ subscribe(this, errorMessagesLatest$, x => (this.errorMessages = x)); subscribe(this, loginCallbackLatest$, x => (this.loginCallback = x)); subscribe(this, topLevelActionsLatest$, x => (this.actions = x)); + subscribe(this, changeComments$, x => (this.changeComments = x)); + subscribe(this, threads$, x => (this.commentThreads = x)); + subscribe(this, account$, x => (this.selfAccount = x)); } static override get styles() {
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 8d64bc0..8a28cb1 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
@@ -18,7 +18,6 @@ import '../../../styles/gr-a11y-styles'; import '../../../styles/gr-paper-styles'; import '../../../styles/shared-styles'; -import '../../diff/gr-comment-api/gr-comment-api'; import '../../plugins/gr-endpoint-decorator/gr-endpoint-decorator'; import '../../plugins/gr-endpoint-param/gr-endpoint-param'; import '../../shared/gr-account-link/gr-account-link'; @@ -129,10 +128,7 @@ import {GrIncludedInDialog} from '../gr-included-in-dialog/gr-included-in-dialog'; import {GrDownloadDialog} from '../gr-download-dialog/gr-download-dialog'; import {GrChangeMetadata} from '../gr-change-metadata/gr-change-metadata'; -import { - ChangeComments, - GrCommentApi, -} from '../../diff/gr-comment-api/gr-comment-api'; +import {ChangeComments} from '../../diff/gr-comment-api/gr-comment-api'; import { assertIsDefined, hasOwnProperty, @@ -230,7 +226,6 @@ export interface GrChangeView { $: { - commentAPI: GrCommentApi; applyFixDialog: GrApplyFixDialog; fileList: GrFileList & Element; fileListHeader: GrFileListHeader; @@ -2026,23 +2021,6 @@ }); } - /** - * Fetches a new changeComment object, and data for all types of comments - * (comments, robot comments, draft comments) is requested. - */ - _reloadComments() { - // We are resetting all comment related properties, because we want to avoid - // a new change being loaded and then paired with outdated comments. - this._changeComments = undefined; - this._commentThreads = undefined; - this._draftCommentThreads = undefined; - this._robotCommentThreads = undefined; - if (!this._changeNum) - throw new Error('missing required changeNum property'); - - this.commentsService.loadAll(this._changeNum, this._patchRange?.patchNum); - } - @observe('_changeComments') changeCommentsChanged(comments?: ChangeComments) { if (!comments) return; @@ -2124,8 +2102,6 @@ }); allDataPromises.push(projectConfigLoaded); - this._reloadComments(); - let coreDataPromise; // If the patch number is specified @@ -2230,13 +2206,20 @@ assertIsDefined(this._changeNum, '_changeNum'); if (!this._patchRange?.patchNum) throw new Error('missing patchNum'); const promises = [this._getCommitInfo(), this.$.fileList.reload()]; - if (patchNumChanged) + if (patchNumChanged) { promises.push( - this.$.commentAPI.reloadPortedComments( + this.commentsService.reloadPortedComments( this._changeNum, this._patchRange?.patchNum ) ); + promises.push( + this.commentsService.reloadPortedDrafts( + this._changeNum, + this._patchRange?.patchNum + ) + ); + } return Promise.all(promises); }
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts index 9341b18..9181ca0 100644 --- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts +++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts
@@ -381,7 +381,6 @@ on-stop-edit-tap="_handleStopEditTap" on-download-tap="_handleOpenDownloadDialog" on-included-tap="_handleOpenIncludedInDialog" - comment-threads="[[_commentThreads]]" ></gr-change-actions> </div> <!-- end commit actions --> @@ -439,12 +438,7 @@ </gr-editable-content> </div> <h3 class="assistive-tech-only">Comments and Checks Summary</h3> - <gr-change-summary - change-comments="[[_changeComments]]" - comment-threads="[[_commentThreads]]" - self-account="[[_account]]" - > - </gr-change-summary> + <gr-change-summary></gr-change-summary> <gr-endpoint-decorator name="commit-container"> <gr-endpoint-param name="change" value="[[_change]]"> </gr-endpoint-param> @@ -528,7 +522,6 @@ change="[[_change]]" change-num="[[_changeNum]]" revision-info="[[_revisionInfo]]" - change-comments="[[_changeComments]]" commit-info="[[_commitInfo]]" change-url="[[_computeChangeUrl(_change)]]" edit-mode="[[_editMode]]" @@ -720,5 +713,4 @@ </gr-reply-dialog> </template> </gr-overlay> - <gr-comment-api id="commentAPI"></gr-comment-api> `;
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 ab17f47..72cd91e 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
@@ -40,6 +40,7 @@ import { mockPromise, queryAndAssert, + stubComments, stubRestApi, stubUsers, waitQueryAndAssert, @@ -1346,10 +1347,8 @@ sinon.stub(element, '_getCommitInfo'); sinon.stub(element.$.fileList, 'reload'); flush(); - const reloadPortedCommentsStub = sinon.stub( - element.$.commentAPI, - 'reloadPortedComments' - ); + const reloadPortedCommentsStub = stubComments('reloadPortedComments'); + const reloadPortedDraftsStub = stubComments('reloadPortedDrafts'); sinon.stub(element.$.fileList, 'collapseAllDiffs'); const value: AppElementChangeViewParams = { @@ -1374,6 +1373,7 @@ element.params = {...value}; await flush(); assert.isTrue(reloadPortedCommentsStub.calledOnce); + assert.isTrue(reloadPortedDraftsStub.calledOnce); }); test('do not reload entire page when patchRange doesnt change', async () => {
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.ts index 9d371d3..a9b7b81 100644 --- a/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.ts +++ b/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.ts
@@ -20,14 +20,18 @@ import '../../plugins/gr-endpoint-decorator/gr-endpoint-decorator'; import '../../plugins/gr-endpoint-param/gr-endpoint-param'; import '../gr-thread-list/gr-thread-list'; -import {ChangeInfo, ActionInfo} from '../../../types/common'; +import {ActionInfo} from '../../../types/common'; import {GrDialog} from '../../shared/gr-dialog/gr-dialog'; import {pluralize} from '../../../utils/string-util'; import {CommentThread, isUnresolved} from '../../../utils/comment-util'; import {sharedStyles} from '../../../styles/shared-styles'; import {LitElement, css, html} from 'lit'; -import {customElement, property, query} from 'lit/decorators'; +import {customElement, property, query, state} from 'lit/decorators'; import {fontStyles} from '../../../styles/gr-font-styles'; +import {subscribe} from '../../lit/subscription-controller'; +import {change$} from '../../../services/change/change-model'; +import {threads$} from '../../../services/comments/comments-model'; +import {ParsedChangeInfo} from '../../../types/types'; @customElement('gr-confirm-submit-dialog') export class GrConfirmSubmitDialog extends LitElement { @@ -47,16 +51,16 @@ */ @property({type: Object}) - change?: ChangeInfo; - - @property({type: Object}) action?: ActionInfo; - @property({type: Array}) - commentThreads?: CommentThread[] = []; + @state() + change?: ParsedChangeInfo; - @property({type: Boolean}) - _initialised = false; + @state() + unresolvedThreads: CommentThread[] = []; + + @state() + initialised = false; static override get styles() { return [ @@ -84,6 +88,16 @@ ]; } + constructor() { + super(); + subscribe(this, change$, x => (this.change = x)); + subscribe( + this, + threads$, + x => (this.unresolvedThreads = x.filter(isUnresolved)) + ); + } + private renderPrivate() { if (!this.change?.is_private) return ''; return html` @@ -106,11 +120,11 @@ icon="gr-icons:warning" class="warningBeforeSubmit" ></iron-icon> - ${this._computeUnresolvedCommentsWarning(this.change)} + ${this.computeUnresolvedCommentsWarning()} </p> <gr-thread-list id="commentList" - .threads="${this._computeUnresolvedThreads(this.commentThreads)}" + .threads="${this.unresolvedThreads}" .change="${this.change}" .changeNum="${this.change?._number}" logged-in @@ -121,7 +135,7 @@ } private renderChangeEdit() { - if (!this._computeHasChangeEdit(this.change)) return ''; + if (!this.computeHasChangeEdit()) return ''; return html` <iron-icon icon="gr-icons:warning" @@ -133,7 +147,7 @@ } private renderInitialised() { - if (!this._initialised) return ''; + if (!this.initialised) return ''; return html` <div class="header" slot="header">${this.action?.label}</div> <div class="main" slot="main"> @@ -159,48 +173,43 @@ id="dialog" confirm-label="Continue" confirm-on-enter="" - @cancel=${this._handleCancelTap} - @confirm=${this._handleConfirmTap} + @cancel=${this.handleCancelTap} + @confirm=${this.handleConfirmTap} > ${this.renderInitialised()} </gr-dialog>`; } init() { - this._initialised = true; + this.initialised = true; } resetFocus() { this.dialog?.resetFocus(); } - _computeHasChangeEdit(change?: ChangeInfo) { - return ( - !!change && - !!change.revisions && - Object.values(change.revisions).some(rev => rev._number === 'edit') + // Private method, but visible for testing. + computeHasChangeEdit() { + return Object.values(this.change?.revisions ?? {}).some( + rev => rev._number === 'edit' ); } - _computeUnresolvedThreads(commentThreads?: CommentThread[]) { - if (!commentThreads) return []; - return commentThreads.filter(thread => isUnresolved(thread)); - } - - _computeUnresolvedCommentsWarning(change?: ChangeInfo) { - if (!change) return ''; - const unresolvedCount = change.unresolved_comment_count; + // Private method, but visible for testing. + computeUnresolvedCommentsWarning() { + if (!this.change) return ''; + const unresolvedCount = this.change.unresolved_comment_count; if (!unresolvedCount) throw new Error('unresolved comments undefined or 0'); return `Heads Up! ${pluralize(unresolvedCount, 'unresolved comment')}.`; } - _handleConfirmTap(e: Event) { + private handleConfirmTap(e: Event) { e.preventDefault(); e.stopPropagation(); this.dispatchEvent(new CustomEvent('confirm', {bubbles: false})); } - _handleCancelTap(e: Event) { + private handleCancelTap(e: Event) { e.preventDefault(); e.stopPropagation(); this.dispatchEvent(new CustomEvent('cancel', {bubbles: false}));
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog_test.ts index e1823b1..0426cb6 100644 --- a/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog_test.ts +++ b/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog_test.ts
@@ -16,7 +16,10 @@ */ import '../../../test/common-test-setup-karma'; -import {createChange, createRevision} from '../../../test/test-data-generators'; +import { + createParsedChange, + createRevision, +} from '../../../test/test-data-generators'; import {queryAndAssert} from '../../../test/test-utils'; import {PatchSetNum} from '../../../types/common'; import {GrConfirmSubmitDialog} from './gr-confirm-submit-dialog'; @@ -28,13 +31,13 @@ setup(() => { element = basicFixture.instantiate(); - element._initialised = true; + element.initialised = true; }); test('display', async () => { element.action = {label: 'my-label'}; element.change = { - ...createChange(), + ...createParsedChange(), subject: 'my-subject', revisions: {}, }; @@ -47,23 +50,23 @@ assert.notEqual(message.textContent!.indexOf('my-subject'), -1); }); - test('_computeUnresolvedCommentsWarning', () => { - const change = {...createChange(), unresolved_comment_count: 1}; + test('computeUnresolvedCommentsWarning', () => { + element.change = {...createParsedChange(), unresolved_comment_count: 1}; assert.equal( - element._computeUnresolvedCommentsWarning(change), + element.computeUnresolvedCommentsWarning(), 'Heads Up! 1 unresolved comment.' ); - const change2 = {...createChange(), unresolved_comment_count: 2}; + element.change = {...createParsedChange(), unresolved_comment_count: 2}; assert.equal( - element._computeUnresolvedCommentsWarning(change2), + element.computeUnresolvedCommentsWarning(), 'Heads Up! 2 unresolved comments.' ); }); - test('_computeHasChangeEdit', () => { - const change = { - ...createChange(), + test('computeHasChangeEdit', () => { + element.change = { + ...createParsedChange(), revisions: { d442ff05d6c4f2a3af0eeca1f67374b39f9dc3d8: { ...createRevision(), @@ -73,10 +76,10 @@ unresolved_comment_count: 0, }; - assert.isTrue(element._computeHasChangeEdit(change)); + assert.isTrue(element.computeHasChangeEdit()); - const change2 = { - ...createChange(), + element.change = { + ...createParsedChange(), revisions: { d442ff05d6c4f2a3af0eeca1f67374b39f9dc3d8: { ...createRevision(), @@ -84,6 +87,6 @@ }, }, }; - assert.isFalse(element._computeHasChangeEdit(change2)); + assert.isFalse(element.computeHasChangeEdit()); }); });
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts index 1b44e35..920844b 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts +++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
@@ -39,7 +39,6 @@ BasePatchSetNum, } from '../../../types/common'; import {DiffPreferencesInfo} from '../../../types/diff'; -import {ChangeComments} from '../../diff/gr-comment-api/gr-comment-api'; import {GrDiffModeSelector} from '../../diff/gr-diff-mode-selector/gr-diff-mode-selector'; import {GrButton} from '../../shared/gr-button/gr-button'; import {fireEvent} from '../../../utils/event-util'; @@ -101,9 +100,6 @@ changeUrl?: string; @property({type: Object}) - changeComments?: ChangeComments; - - @property({type: Object}) commitInfo?: CommitInfo; @property({type: Boolean})
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.ts b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.ts index 5a85531..fbba2fc 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.ts +++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.ts
@@ -132,7 +132,6 @@ <div class="patchInfoContent"> <gr-patch-range-select id="rangeSelect" - change-comments="[[changeComments]]" change-num="[[changeNum]]" patch-num="[[patchNum]]" base-patch-num="[[basePatchNum]]"
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts index 95984b8..d02f09b 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -18,7 +18,6 @@ import '../../../styles/shared-styles'; import '../../diff/gr-diff-cursor/gr-diff-cursor'; import '../../diff/gr-diff-host/gr-diff-host'; -import '../../diff/gr-comment-api/gr-comment-api'; import '../../diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog'; import '../../edit/gr-edit-file-controls/gr-edit-file-controls'; import '../../shared/gr-button/gr-button';
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js index 0db7690..ee65837 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
@@ -16,7 +16,6 @@ */ import '../../../test/common-test-setup-karma.js'; -import '../../diff/gr-comment-api/gr-comment-api.js'; import '../../shared/gr-date-formatter/gr-date-formatter.js'; import {getMockDiffResponse} from '../../../test/mocks/diff-response.js'; import './gr-file-list.js'; @@ -49,7 +48,6 @@ 'gr-file-list-comment-api-mock', html` <gr-file-list id="fileList" change-comments="[[_changeComments]]"></gr-file-list> - <gr-comment-api id="commentAPI"></gr-comment-api> `); const basicFixture = fixtureFromElement(commentApiMock.is);
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.js b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.js index 0939daa..a3b8873 100644 --- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.js +++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.js
@@ -16,7 +16,6 @@ */ import '../../../test/common-test-setup-karma.js'; -import '../../diff/gr-comment-api/gr-comment-api.js'; import './gr-messages-list.js'; import {createCommentApiMockWithTemplateElement} from '../../../test/mocks/comment-api.js'; import {TEST_ONLY} from './gr-messages-list.js'; @@ -30,7 +29,6 @@ <gr-messages-list id="messagesList" change-comments="[[_changeComments]]"></gr-messages-list> - <gr-comment-api id="commentAPI"></gr-comment-api> `); const basicFixture = fixtureFromTemplate(html`
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts index 7e7e507..32c732e 100644 --- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts +++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
@@ -14,16 +14,12 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import {PolymerElement} from '@polymer/polymer/polymer-element'; -import {htmlTemplate} from './gr-comment-api_html'; -import {customElement, property} from '@polymer/decorators'; import { CommentBasics, PatchRange, PatchSetNum, RobotCommentInfo, UrlEncodedCommentId, - NumericChangeId, PathToCommentsInfoMap, FileInfo, ParentPatchSetNum, @@ -45,7 +41,6 @@ addPath, } from '../../../utils/comment-util'; import {PatchSetFile, PatchNumOnly, isPatchSetFile} from '../../../types/types'; -import {appContext} from '../../../services/app-context'; import {CommentSide, Side} from '../../../constants/constants'; import {pluralize} from '../../../utils/string-util'; import {NormalizedFileInfo} from '../../change/gr-file-list/gr-file-list'; @@ -611,38 +606,3 @@ return createCommentThreads(comments); } } - -@customElement('gr-comment-api') -export class GrCommentApi extends PolymerElement { - static get template() { - return htmlTemplate; - } - - @property({type: Object}) - _changeComments?: ChangeComments; - - private readonly restApiService = appContext.restApiService; - - private readonly commentsService = appContext.commentsService; - - reloadPortedComments(changeNum: NumericChangeId, patchNum: PatchSetNum) { - if (!this._changeComments) { - this.commentsService.loadAll(changeNum); - return Promise.resolve(); - } - return Promise.all([ - this.restApiService.getPortedComments(changeNum, patchNum), - this.restApiService.getPortedDrafts(changeNum, patchNum), - ]).then(res => { - if (!this._changeComments) return; - this._changeComments = - this._changeComments.cloneWithUpdatedPortedComments(res[0], res[1]); - }); - } -} - -declare global { - interface HTMLElementTagNameMap { - 'gr-comment-api': GrCommentApi; - } -}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts index 3e292fe..a47db20 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts +++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts
@@ -408,6 +408,7 @@ range: Text | Element | Range ) { if (startLine > 1) { + actionBox.positionBelow = false; actionBox.placeAbove(range); return; }
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js index 6facdca..8d75230 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
@@ -20,7 +20,6 @@ import {dom} from '@polymer/polymer/lib/legacy/polymer.dom.js'; import {createDefaultDiffPrefs, Side} from '../../../constants/constants.js'; -import {_testOnly_resetState} from '../../../services/comments/comments-model.js'; import {createChange, createComment, createCommentThread} from '../../../test/test-data-generators.js'; import {addListenerForTest, mockPromise, stubRestApi} from '../../../test/test-utils.js'; import {EditPatchSetNum, ParentPatchSetNum} from '../../../types/common.js'; @@ -43,7 +42,6 @@ element.path = 'some/path'; sinon.stub(element.reporting, 'time'); sinon.stub(element.reporting, 'timeEnd'); - _testOnly_resetState(); await flush(); });
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts index 0ffe61f..64186f4 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -65,7 +65,7 @@ GrDropdownList, } from '../../shared/gr-dropdown-list/gr-dropdown-list'; import {GrOverlay} from '../../shared/gr-overlay/gr-overlay'; -import {ChangeComments, GrCommentApi} from '../gr-comment-api/gr-comment-api'; +import {ChangeComments} from '../gr-comment-api/gr-comment-api'; import {GrDiffModeSelector} from '../gr-diff-mode-selector/gr-diff-mode-selector'; import { BasePatchSetNum, @@ -134,7 +134,6 @@ export interface GrDiffView { $: { - commentAPI: GrCommentApi; diffHost: GrDiffHost; reviewed: HTMLInputElement; dropdown: GrDropdownList; @@ -348,8 +347,6 @@ private readonly userService = appContext.userService; - private readonly commentsService = appContext.commentsService; - private readonly shortcuts = appContext.shortcutsService; _throttledToggleFileReviewed?: (e: KeyboardEvent) => void; @@ -1074,8 +1071,6 @@ if (!this._change) promises.push(this._getChangeDetail(this._changeNum)); - if (!this._changeComments) this._loadComments(value.patchNum); - promises.push(this._getChangeEdit()); this.$.diffHost.cancel(); @@ -1464,11 +1459,6 @@ return url; } - _loadComments(patchSet?: PatchSetNum) { - assertIsDefined(this._changeNum, '_changeNum'); - return this.commentsService.loadAll(this._changeNum, patchSet); - } - @observe( '_changeComments', '_files.changeFilesByPath',
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts index 16adb45..d87d192 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
@@ -426,5 +426,4 @@ on-reload-diff-preference="_handleReloadingDiffPreference" > </gr-diff-preferences-dialog> - <gr-comment-api id="commentAPI"></gr-comment-api> `;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js index 46ec5d0..5c04ab0 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
@@ -30,7 +30,7 @@ import {EditPatchSetNum} from '../../../types/common.js'; import {CursorMoveResult} from '../../../api/core.js'; import {EventType} from '../../../types/events.js'; -import {_testOnly_resetState, _testOnly_setState} from '../../../services/browser/browser-model.js'; +import {_testOnly_setState} from '../../../services/browser/browser-model.js'; const basicFixture = fixtureFromElement('gr-diff-view'); @@ -69,8 +69,6 @@ stubRestApi('getDiffDrafts').returns(Promise.resolve({})); stubRestApi('getPortedComments').returns(Promise.resolve({})); - _testOnly_resetState(); - element = basicFixture.instantiate(); element._changeNum = '42'; element._path = 'some/path.txt'; @@ -2040,7 +2038,6 @@ Promise.resolve([])); element = basicFixture.instantiate(); element._changeNum = '42'; - return element._loadComments(); }); test('_getFiles add files with comments without changes', () => {
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts index 501f688..5d4405f 100644 --- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts +++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts
@@ -50,10 +50,16 @@ import {sharedStyles} from '../../../styles/shared-styles'; import {LitElement, PropertyValues, css, html} from 'lit'; import {customElement, property, query, state} from 'lit/decorators'; +import {subscribe} from '../../lit/subscription-controller'; +import {changeComments$} from '../../../services/comments/comments-model'; // Maximum length for patch set descriptions. const PATCH_DESC_MAX_LENGTH = 500; +function getShaForPatch(patch: PatchSet) { + return patch.sha.substring(0, 10); +} + export interface PatchRangeChangeDetail { patchNum?: PatchSetNum; basePatchNum?: BasePatchSetNum; @@ -95,9 +101,6 @@ changeNum?: string; @property({type: Object}) - changeComments?: ChangeComments; - - @property({type: Object}) filesWeblinks?: FilesWebLinks; @property({type: String}) @@ -106,18 +109,28 @@ @property({type: String}) basePatchNum?: BasePatchSetNum; + /** Not used directly. Translated into `sortedRevisions` in willUpdate(). */ @property({type: Object}) - revisions?: RevisionInfo[]; + revisions: (RevisionInfo | EditRevisionInfo)[] = []; @property({type: Object}) revisionInfo?: RevisionInfoClass; - @property({type: Array}) + /** Private internal state, derived from `revisions` in willUpdate(). */ @state() - protected sortedRevisions?: RevisionInfo[]; + private sortedRevisions: (RevisionInfo | EditRevisionInfo)[] = []; + + /** Private internal state, visible for testing. */ + @state() + changeComments?: ChangeComments; private readonly reporting: ReportingService = appContext.reportingService; + constructor() { + super(); + subscribe(this, changeComments$, x => (this.changeComments = x)); + } + static override get styles() { return [ a11yStyles, @@ -152,20 +165,6 @@ ]; } - private renderWeblinks(fileLink?: GeneratedWebLink[]) { - if (!fileLink) return; - - return html`<span class="filesWeblinks"> - ${fileLink.map( - weblink => html` - <a target="_blank" rel="noopener" href="${weblink.url}"> - ${weblink.name} - </a> - ` - )}</span - > `; - } - override render() { return html` <h3 class="assistive-tech-only">Patchset Range Selection</h3> @@ -173,14 +172,8 @@ <gr-dropdown-list id="basePatchDropdown" .value="${convertToString(this.basePatchNum)}" - .items="${this._computeBaseDropdownContent( - this.availablePatches, - this.patchNum, - this.sortedRevisions, - this.changeComments, - this.revisionInfo - )}" - @value-change=${this._handlePatchChange} + .items="${this.computeBaseDropdownContent()}" + @value-change=${this.handlePatchChange} > </gr-dropdown-list> </span> @@ -190,13 +183,8 @@ <gr-dropdown-list id="patchNumDropdown" .value="${convertToString(this.patchNum)}" - .items="${this._computePatchDropdownContent( - this.availablePatches, - this.basePatchNum, - this.sortedRevisions, - this.changeComments - )}" - @value-change=${this._handlePatchChange} + .items="${this.computePatchDropdownContent()}" + @value-change=${this.handlePatchChange} > </gr-dropdown-list> ${this.renderWeblinks(this.filesWeblinks?.meta_b)} @@ -204,63 +192,54 @@ `; } - override updated(changedProperties: PropertyValues) { + private renderWeblinks(fileLinks?: GeneratedWebLink[]) { + if (!fileLinks) return; + return html`<span class="filesWeblinks"> + ${fileLinks.map( + weblink => html` + <a target="_blank" rel="noopener" href="${weblink.url}"> + ${weblink.name} + </a> + ` + )}</span + > `; + } + + override willUpdate(changedProperties: PropertyValues) { if (changedProperties.has('revisions')) { - this._updateSortedRevisions(this.revisions); + this.sortedRevisions = sortRevisions(Object.values(this.revisions || {})); } } - _updateSortedRevisions(revisions?: RevisionInfo[]) { - if (!revisions) return; - this.sortedRevisions = sortRevisions(Object.values(revisions)); - } - - _getShaForPatch(patch: PatchSet) { - return patch.sha.substring(0, 10); - } - - _computeBaseDropdownContent( - availablePatches?: PatchSet[], - patchNum?: PatchSetNum, - sortedRevisions?: (RevisionInfo | EditRevisionInfo)[], - changeComments?: ChangeComments, - revisionInfo?: RevisionInfoClass - ): DropdownItem[] { - // Polymer 2: check for undefined + // Private method, but visible for testing. + computeBaseDropdownContent(): DropdownItem[] { if ( - availablePatches === undefined || - patchNum === undefined || - sortedRevisions === undefined || - changeComments === undefined || - revisionInfo === undefined + this.availablePatches === undefined || + this.patchNum === undefined || + this.changeComments === undefined || + this.revisionInfo === undefined ) { return []; } - const parentCounts = revisionInfo.getParentCountMap(); - const currentParentCount = hasOwnProperty(parentCounts, patchNum) - ? parentCounts[patchNum as number] + const parentCounts = this.revisionInfo.getParentCountMap(); + const currentParentCount = hasOwnProperty(parentCounts, this.patchNum) + ? parentCounts[this.patchNum as number] : 1; - const maxParents = revisionInfo.getMaxParents(); + const maxParents = this.revisionInfo.getMaxParents(); const isMerge = currentParentCount > 1; const dropdownContent: DropdownItem[] = []; - for (const basePatch of availablePatches) { + for (const basePatch of this.availablePatches) { const basePatchNum = basePatch.num; - const entry: DropdownItem = this._createDropdownEntry( + const entry: DropdownItem = this.createDropdownEntry( basePatchNum, 'Patchset ', - sortedRevisions, - changeComments, - this._getShaForPatch(basePatch) + getShaForPatch(basePatch) ); dropdownContent.push({ ...entry, - disabled: this._computeLeftDisabled( - basePatch.num, - patchNum, - sortedRevisions - ), + disabled: this.computeLeftDisabled(basePatch.num, this.patchNum), }); } @@ -282,91 +261,61 @@ return dropdownContent; } - _computeMobileText( - patchNum: PatchSetNum, - changeComments: ChangeComments, - revisions: (RevisionInfo | EditRevisionInfo)[] - ) { + private computeMobileText(patchNum: PatchSetNum) { return ( `${patchNum}` + - `${this._computePatchSetCommentsString(changeComments, patchNum)}` + - `${this._computePatchSetDescription(revisions, patchNum, true)}` + `${this.computePatchSetCommentsString(patchNum)}` + + `${this.computePatchSetDescription(patchNum, true)}` ); } - _computePatchDropdownContent( - availablePatches?: PatchSet[], - basePatchNum?: BasePatchSetNum, - sortedRevisions?: (RevisionInfo | EditRevisionInfo)[], - changeComments?: ChangeComments - ): DropdownItem[] { - // Polymer 2: check for undefined + // Private method, but visible for testing. + computePatchDropdownContent(): DropdownItem[] { if ( - availablePatches === undefined || - basePatchNum === undefined || - sortedRevisions === undefined || - changeComments === undefined + this.availablePatches === undefined || + this.basePatchNum === undefined || + this.changeComments === undefined ) { return []; } const dropdownContent: DropdownItem[] = []; - for (const patch of availablePatches) { + for (const patch of this.availablePatches) { const patchNum = patch.num; - const entry = this._createDropdownEntry( + const entry = this.createDropdownEntry( patchNum, patchNum === 'edit' ? '' : 'Patchset ', - sortedRevisions, - changeComments, - this._getShaForPatch(patch) + getShaForPatch(patch) ); dropdownContent.push({ ...entry, - disabled: this._computeRightDisabled( - basePatchNum, - patchNum, - sortedRevisions - ), + disabled: this.computeRightDisabled(this.basePatchNum, patchNum), }); } return dropdownContent; } - _computeText( - patchNum: PatchSetNum, - prefix: string, - changeComments: ChangeComments, - sha: string - ) { + private computeText(patchNum: PatchSetNum, prefix: string, sha: string) { return ( `${prefix}${patchNum}` + - `${this._computePatchSetCommentsString(changeComments, patchNum)}` + + `${this.computePatchSetCommentsString(patchNum)}` + ` | ${sha}` ); } - _createDropdownEntry( + private createDropdownEntry( patchNum: PatchSetNum, prefix: string, - sortedRevisions: (RevisionInfo | EditRevisionInfo)[], - changeComments: ChangeComments, sha: string ) { const entry: DropdownItem = { triggerText: `${prefix}${patchNum}`, - text: this._computeText(patchNum, prefix, changeComments, sha), - mobileText: this._computeMobileText( - patchNum, - changeComments, - sortedRevisions - ), - bottomText: `${this._computePatchSetDescription( - sortedRevisions, - patchNum - )}`, + text: this.computeText(patchNum, prefix, sha), + mobileText: this.computeMobileText(patchNum), + bottomText: `${this.computePatchSetDescription(patchNum)}`, value: patchNum, }; - const date = this._computePatchSetDate(sortedRevisions, patchNum); + const date = this.computePatchSetDate(patchNum); if (date) { entry.date = date; } @@ -378,17 +327,18 @@ * is sorted in reverse order (higher patchset nums first), invalid base * patch nums have an index greater than the index of patchNum. * + * Private method, but visible for testing. + * * @param basePatchNum The possible base patch num. * @param patchNum The current selected patch num. */ - _computeLeftDisabled( + computeLeftDisabled( basePatchNum: PatchSetNum, - patchNum: PatchSetNum, - sortedRevisions: (RevisionInfo | EditRevisionInfo)[] + patchNum: PatchSetNum ): boolean { return ( - findSortedIndex(basePatchNum, sortedRevisions) <= - findSortedIndex(patchNum, sortedRevisions) + findSortedIndex(basePatchNum, this.sortedRevisions) <= + findSortedIndex(patchNum, this.sortedRevisions) ); } @@ -403,13 +353,14 @@ * If the current basePatchNum is a parent index, then only patches that have * at least that many parents are valid. * + * Private method, but visible for testing. + * * @param basePatchNum The current selected base patch num. * @param patchNum The possible patch num. */ - _computeRightDisabled( + computeRightDisabled( basePatchNum: PatchSetNum, - patchNum: PatchSetNum, - sortedRevisions: (RevisionInfo | EditRevisionInfo)[] + patchNum: PatchSetNum ): boolean { if (basePatchNum === ParentPatchSetNum) { return false; @@ -427,21 +378,17 @@ } return ( - findSortedIndex(basePatchNum, sortedRevisions) <= - findSortedIndex(patchNum, sortedRevisions) + findSortedIndex(basePatchNum, this.sortedRevisions) <= + findSortedIndex(patchNum, this.sortedRevisions) ); } // TODO(dhruvsri): have ported comments contribute to this count - _computePatchSetCommentsString( - changeComments: ChangeComments, - patchNum: PatchSetNum - ) { - if (!changeComments) { - return; - } + // Private method, but visible for testing. + computePatchSetCommentsString(patchNum: PatchSetNum): string { + if (!this.changeComments) return ''; - const commentThreadCount = changeComments.computeCommentThreadCount( + const commentThreadCount = this.changeComments.computeCommentThreadCount( { patchNum, }, @@ -449,7 +396,7 @@ ); const commentThreadString = pluralize(commentThreadCount, 'comment'); - const unresolvedCount = changeComments.computeUnresolvedNum( + const unresolvedCount = this.changeComments.computeUnresolvedNum( {patchNum}, true ); @@ -468,23 +415,19 @@ ); } - _computePatchSetDescription( - revisions: (RevisionInfo | EditRevisionInfo)[], + private computePatchSetDescription( patchNum: PatchSetNum, addFrontSpace?: boolean ) { - const rev = getRevisionByPatchNum(revisions, patchNum); + const rev = getRevisionByPatchNum(this.sortedRevisions, patchNum); return rev?.description ? (addFrontSpace ? ' ' : '') + rev.description.substring(0, PATCH_DESC_MAX_LENGTH) : ''; } - _computePatchSetDate( - revisions: (RevisionInfo | EditRevisionInfo)[], - patchNum: PatchSetNum - ): Timestamp | undefined { - const rev = getRevisionByPatchNum(revisions, patchNum); + private computePatchSetDate(patchNum: PatchSetNum): Timestamp | undefined { + const rev = getRevisionByPatchNum(this.sortedRevisions, patchNum); return rev ? rev.created : undefined; } @@ -492,7 +435,7 @@ * Catches value-change events from the patchset dropdowns and determines * whether or not a patch change event should be fired. */ - _handlePatchChange(e: DropDownValueChangeEvent) { + private handlePatchChange(e: DropDownValueChangeEvent) { const detail: PatchRangeChangeDetail = { patchNum: this.patchNum, basePatchNum: this.basePatchNum,
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts index a47b685..342fe3a 100644 --- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts +++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts
@@ -55,7 +55,7 @@ suite('gr-patch-range-select tests', () => { let element: GrPatchRangeSelect; - function getInfo(revisions: RevisionInfo[]) { + function getInfo(revisions: (RevisionInfo | EditRevisionInfo)[]) { const revisionObj: Partial<RevIdToRevisionInfo> = {}; for (let i = 0; i < revisions.length; i++) { revisionObj[i] = revisions[i]; @@ -78,97 +78,54 @@ await element.updateComplete; }); - test('enabled/disabled options', () => { - const patchRange = { - basePatchNum: 'PARENT' as PatchSetNum, - patchNum: 3 as PatchSetNum, - }; - const sortedRevisions = [ + test('enabled/disabled options', async () => { + element.revisions = [ createRevision(3) as RevisionInfo, createEditRevision(2) as EditRevisionInfo, createRevision(2) as RevisionInfo, createRevision(1) as RevisionInfo, ]; + await element.updateComplete; + + const parent = 'PARENT' as PatchSetNum; + const edit = EditPatchSetNum; + for (const patchNum of [1, 2, 3]) { assert.isFalse( - element._computeRightDisabled( - patchRange.basePatchNum, - patchNum as PatchSetNum, - sortedRevisions - ) + element.computeRightDisabled(parent, patchNum as PatchSetNum) ); } for (const basePatchNum of [1, 2]) { - assert.isFalse( - element._computeLeftDisabled( - basePatchNum as PatchSetNum, - patchRange.patchNum, - sortedRevisions - ) - ); + const base = basePatchNum as PatchSetNum; + assert.isFalse(element.computeLeftDisabled(base, 3 as PatchSetNum)); } assert.isTrue( - element._computeLeftDisabled(3 as PatchSetNum, patchRange.patchNum, []) + element.computeLeftDisabled(3 as PatchSetNum, 3 as PatchSetNum) ); - patchRange.basePatchNum = EditPatchSetNum; assert.isTrue( - element._computeLeftDisabled( - 3 as PatchSetNum, - patchRange.patchNum, - sortedRevisions - ) + element.computeLeftDisabled(3 as PatchSetNum, 3 as PatchSetNum) ); - assert.isTrue( - element._computeRightDisabled( - patchRange.basePatchNum, - 1 as PatchSetNum, - sortedRevisions - ) - ); - assert.isTrue( - element._computeRightDisabled( - patchRange.basePatchNum, - 2 as PatchSetNum, - sortedRevisions - ) - ); - assert.isFalse( - element._computeRightDisabled( - patchRange.basePatchNum, - 3 as PatchSetNum, - sortedRevisions - ) - ); - assert.isTrue( - element._computeRightDisabled( - patchRange.basePatchNum, - EditPatchSetNum, - sortedRevisions - ) - ); + assert.isTrue(element.computeRightDisabled(edit, 1 as PatchSetNum)); + assert.isTrue(element.computeRightDisabled(edit, 2 as PatchSetNum)); + assert.isFalse(element.computeRightDisabled(edit, 3 as PatchSetNum)); + assert.isTrue(element.computeRightDisabled(edit, edit)); }); - test('_computeBaseDropdownContent', () => { - const availablePatches = [ + test('computeBaseDropdownContent', async () => { + element.availablePatches = [ {num: 'edit', sha: '1'} as PatchSet, {num: 3, sha: '2'} as PatchSet, {num: 2, sha: '3'} as PatchSet, {num: 1, sha: '4'} as PatchSet, ]; - const revisions: RevisionInfo[] = [ + element.revisions = [ createRevision(2), createRevision(3), createRevision(1), createRevision(4), ]; - element.revisionInfo = getInfo(revisions); - const sortedRevisions = [ - createRevision(3) as RevisionInfo, - createEditRevision(2) as EditRevisionInfo, - createRevision(2) as RevisionInfo, - createRevision(1) as RevisionInfo, - ]; + element.revisionInfo = getInfo(element.revisions); const expectedResult: DropdownItem[] = [ { disabled: true, @@ -210,19 +167,14 @@ value: 'PARENT', } as DropdownItem, ]; - assert.deepEqual( - element._computeBaseDropdownContent( - availablePatches, - 1 as PatchSetNum, - sortedRevisions, - element.changeComments, - element.revisionInfo - ), - expectedResult - ); + element.patchNum = 1 as PatchSetNum; + element.basePatchNum = 'PARENT' as BasePatchSetNum; + await element.updateComplete; + + assert.deepEqual(element.computeBaseDropdownContent(), expectedResult); }); - test('_computeBaseDropdownContent called when patchNum updates', async () => { + test('computeBaseDropdownContent called when patchNum updates', async () => { element.revisions = [ createRevision(2), createRevision(3), @@ -240,7 +192,7 @@ element.basePatchNum = 'PARENT' as BasePatchSetNum; await element.updateComplete; - const baseDropDownStub = sinon.stub(element, '_computeBaseDropdownContent'); + const baseDropDownStub = sinon.stub(element, 'computeBaseDropdownContent'); // Should be recomputed for each available patch element.patchNum = 1 as PatchSetNum; @@ -248,7 +200,7 @@ assert.equal(baseDropDownStub.callCount, 1); }); - test('_computeBaseDropdownContent called when changeComments update', async () => { + test('computeBaseDropdownContent called when changeComments update', async () => { element.revisions = [ createRevision(2), createRevision(3), @@ -266,14 +218,14 @@ await element.updateComplete; // Should be recomputed for each available patch - const baseDropDownStub = sinon.stub(element, '_computeBaseDropdownContent'); + const baseDropDownStub = sinon.stub(element, 'computeBaseDropdownContent'); assert.equal(baseDropDownStub.callCount, 0); element.changeComments = new ChangeComments(); await element.updateComplete; assert.equal(baseDropDownStub.callCount, 1); }); - test('_computePatchDropdownContent called when basePatchNum updates', async () => { + test('computePatchDropdownContent called when basePatchNum updates', async () => { element.revisions = [ createRevision(2), createRevision(3), @@ -292,29 +244,27 @@ await element.updateComplete; // Should be recomputed for each available patch - const baseDropDownStub = sinon.stub( - element, - '_computePatchDropdownContent' - ); + const baseDropDownStub = sinon.stub(element, 'computePatchDropdownContent'); element.basePatchNum = 1 as BasePatchSetNum; await element.updateComplete; assert.equal(baseDropDownStub.callCount, 1); }); - test('_computePatchDropdownContent', () => { - const availablePatches: PatchSet[] = [ + test('computePatchDropdownContent', async () => { + element.availablePatches = [ {num: 'edit', sha: '1'} as PatchSet, {num: 3, sha: '2'} as PatchSet, {num: 2, sha: '3'} as PatchSet, {num: 1, sha: '4'} as PatchSet, ]; - const basePatchNum = 1; - const sortedRevisions = [ + element.basePatchNum = 1 as BasePatchSetNum; + element.revisions = [ createRevision(3) as RevisionInfo, createEditRevision(2) as EditRevisionInfo, createRevision(2, 'description') as RevisionInfo, createRevision(1) as RevisionInfo, ]; + await element.updateComplete; const expectedResult: DropdownItem[] = [ { @@ -354,15 +304,7 @@ } as DropdownItem, ]; - assert.deepEqual( - element._computePatchDropdownContent( - availablePatches, - basePatchNum as BasePatchSetNum, - sortedRevisions, - element.changeComments - ), - expectedResult - ); + assert.deepEqual(element.computePatchDropdownContent(), expectedResult); }); test('filesWeblinks', async () => { @@ -391,7 +333,7 @@ ); }); - test('_computePatchSetCommentsString', () => { + test('computePatchSetCommentsString', () => { // Test string with unresolved comments. const comments: PathToCommentsInfoMap = { foo: [ @@ -432,10 +374,7 @@ element.changeComments = new ChangeComments(comments); assert.equal( - element._computePatchSetCommentsString( - element.changeComments, - 1 as PatchSetNum - ), + element.computePatchSetCommentsString(1 as PatchSetNum), ' (3 comments, 1 unresolved)' ); @@ -443,23 +382,14 @@ delete comments['foo']; element.changeComments = new ChangeComments(comments); assert.equal( - element._computePatchSetCommentsString( - element.changeComments, - 1 as PatchSetNum - ), + element.computePatchSetCommentsString(1 as PatchSetNum), ' (2 comments)' ); // Test string with no comments. delete comments['bar']; element.changeComments = new ChangeComments(comments); - assert.equal( - element._computePatchSetCommentsString( - element.changeComments, - 1 as PatchSetNum - ), - '' - ); + assert.equal(element.computePatchSetCommentsString(1 as PatchSetNum), ''); }); test('patch-range-change fires', () => {
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts index 551889f..0f64d9e 100644 --- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts +++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
@@ -18,7 +18,6 @@ import '../../shared/gr-tooltip/gr-tooltip'; import {GrTooltip} from '../../shared/gr-tooltip/gr-tooltip'; import {customElement, property} from '@polymer/decorators'; -import {flush} from '@polymer/polymer/lib/legacy/polymer.dom'; import {PolymerElement} from '@polymer/polymer/polymer-element'; import {htmlTemplate} from './gr-selection-action-box_html'; import {fireEvent} from '../../../utils/event-util'; @@ -56,8 +55,8 @@ this.addEventListener('mousedown', e => this._handleMouseDown(e)); } - placeAbove(el: Text | Element | Range) { - flush(); + async placeAbove(el: Text | Element | Range) { + await this.$.tooltip.updateComplete; const rect = this._getTargetBoundingRect(el); const boxRect = this.$.tooltip.getBoundingClientRect(); const parentRect = this._getParentBoundingClientRect(); @@ -70,8 +69,8 @@ }px`; } - placeBelow(el: Text | Element | Range) { - flush(); + async placeBelow(el: Text | Element | Range) { + await this.$.tooltip.updateComplete; const rect = this._getTargetBoundingRect(el); const boxRect = this.$.tooltip.getBoundingClientRect(); const parentRect = this._getParentBoundingClientRect();
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_html.ts b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_html.ts index 24d63b3..558742a 100644 --- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_html.ts +++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_html.ts
@@ -23,6 +23,9 @@ font-family: var(--font-family); position: absolute; white-space: nowrap; + /* This prevents the mouse over the tooltip from interfering with the + selection. */ + pointer-events: none; } </style> <gr-tooltip
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.js b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.js index 81cf0d6..c978c37 100644 --- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.js +++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.js
@@ -83,35 +83,35 @@ {width: 10, height: 10}); }); - test('placeAbove for Element argument', () => { - element.placeAbove(target); + test('placeAbove for Element argument', async () => { + await element.placeAbove(target); assert.equal(element.style.top, '25px'); assert.equal(element.style.left, '72px'); }); - test('placeAbove for Text Node argument', () => { - element.placeAbove(target.firstChild); + test('placeAbove for Text Node argument', async () => { + await element.placeAbove(target.firstChild); assert.equal(element.style.top, '25px'); assert.equal(element.style.left, '72px'); }); - test('placeBelow for Element argument', () => { - element.placeBelow(target); + test('placeBelow for Element argument', async () => { + await element.placeBelow(target); assert.equal(element.style.top, '45px'); assert.equal(element.style.left, '72px'); }); - test('placeBelow for Text Node argument', () => { - element.placeBelow(target.firstChild); + test('placeBelow for Text Node argument', async () => { + await element.placeBelow(target.firstChild); assert.equal(element.style.top, '45px'); assert.equal(element.style.left, '72px'); }); - test('uses document.createRange', () => { + test('uses document.createRange', async () => { sinon.spy(document, 'createRange'); element._getTargetBoundingRect.restore(); sinon.spy(element, '_getTargetBoundingRect'); - element.placeAbove(target.firstChild); + await element.placeAbove(target.firstChild); assert.isTrue(document.createRange.called); }); });
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts index 595de34..9e0027a 100644 --- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts +++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
@@ -50,7 +50,6 @@ stubReporting, stubRestApi, } from '../../../test/test-utils'; -import {_testOnly_resetState} from '../../../services/comments/comments-model'; import {SinonStub} from 'sinon'; const basicFixture = fixtureFromElement('gr-comment-thread'); @@ -63,7 +62,6 @@ setup(() => { stubRestApi('getLoggedIn').returns(Promise.resolve(false)); - _testOnly_resetState(); element = basicFixture.instantiate(); element.patchNum = 3 as PatchSetNum; element.changeNum = 1 as NumericChangeId;
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts index 00edc07..09ac95b 100644 --- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts +++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts
@@ -384,11 +384,7 @@ test('delete comment', async () => { const stub = stubRestApi('deleteComment').returns( - Promise.resolve({ - id: '1' as UrlEncodedCommentId, - updated: '1' as Timestamp, - ...createComment(), - }) + Promise.resolve(createComment()) ); const openSpy = sinon.spy(element.confirmDeleteOverlay!, 'open'); element.changeNum = 42 as NumericChangeId; @@ -1183,6 +1179,7 @@ test('draft prevent save when disabled', async () => { const saveStub = sinon.stub(element, 'save').returns(Promise.resolve()); + sinon.stub(element, '_fireEdit'); element.showActions = true; element.draft = true; await flush();
diff --git a/polygerrit-ui/app/services/browser/browser-model.ts b/polygerrit-ui/app/services/browser/browser-model.ts index db790f6..8cb6575 100644 --- a/polygerrit-ui/app/services/browser/browser-model.ts +++ b/polygerrit-ui/app/services/browser/browser-model.ts
@@ -33,11 +33,13 @@ const initialState: BrowserState = {}; -// Mutable for testing -let privateState$ = new BehaviorSubject(initialState); +const privateState$ = new BehaviorSubject(initialState); export function _testOnly_resetState() { - privateState$ = new BehaviorSubject(initialState); + // We cannot assign a new subject to privateState$, because all the selectors + // have already subscribed to the original subject. So we have to emit the + // initial state on the existing subject. + privateState$.next({...initialState}); } export function _testOnly_setState(state: BrowserState) {
diff --git a/polygerrit-ui/app/services/change/change-model.ts b/polygerrit-ui/app/services/change/change-model.ts index 962ef4d..7df0c22 100644 --- a/polygerrit-ui/app/services/change/change-model.ts +++ b/polygerrit-ui/app/services/change/change-model.ts
@@ -40,6 +40,13 @@ const privateState$ = new BehaviorSubject(initialState); +export function _testOnly_resetState() { + // We cannot assign a new subject to privateState$, because all the selectors + // have already subscribed to the original subject. So we have to emit the + // initial state on the existing subject. + privateState$.next({...initialState}); +} + // Re-exporting as Observable so that you can only subscribe, but not emit. export const changeState$: Observable<ChangeState> = privateState$;
diff --git a/polygerrit-ui/app/services/checks/checks-model.ts b/polygerrit-ui/app/services/checks/checks-model.ts index 75c24b6d..6435252 100644 --- a/polygerrit-ui/app/services/checks/checks-model.ts +++ b/polygerrit-ui/app/services/checks/checks-model.ts
@@ -126,11 +126,13 @@ pluginStateSelected: {}, }; -// Mutable for testing -let privateState$ = new BehaviorSubject(initialState); +const privateState$ = new BehaviorSubject(initialState); export function _testOnly_resetState() { - privateState$ = new BehaviorSubject(initialState); + // We cannot assign a new subject to privateState$, because all the selectors + // have already subscribed to the original subject. So we have to emit the + // initial state on the existing subject. + privateState$.next({...initialState}); } export function _testOnly_setState(state: ChecksState) {
diff --git a/polygerrit-ui/app/services/checks/checks-model_test.ts b/polygerrit-ui/app/services/checks/checks-model_test.ts index dbd3f86..0be0451 100644 --- a/polygerrit-ui/app/services/checks/checks-model_test.ts +++ b/polygerrit-ui/app/services/checks/checks-model_test.ts
@@ -18,7 +18,6 @@ import './checks-model'; import { _testOnly_getState, - _testOnly_resetState, ChecksPatchset, updateStateSetLoading, updateStateSetProvider, @@ -52,7 +51,6 @@ suite('checks-model tests', () => { test('updateStateSetProvider', () => { - _testOnly_resetState(); updateStateSetProvider(PLUGIN_NAME, ChecksPatchset.LATEST); assert.deepEqual(current(), { pluginName: PLUGIN_NAME, @@ -65,7 +63,6 @@ }); test('loading and first time load', () => { - _testOnly_resetState(); updateStateSetProvider(PLUGIN_NAME, ChecksPatchset.LATEST); assert.isFalse(current().loading); assert.isTrue(current().firstTimeLoad); @@ -84,14 +81,12 @@ }); test('updateStateSetResults', () => { - _testOnly_resetState(); updateStateSetResults(PLUGIN_NAME, RUNS, [], [], ChecksPatchset.LATEST); assert.lengthOf(current().runs, 1); assert.lengthOf(current().runs[0].results!, 1); }); test('updateStateUpdateResult', () => { - _testOnly_resetState(); updateStateSetResults(PLUGIN_NAME, RUNS, [], [], ChecksPatchset.LATEST); assert.equal( current().runs[0].results![0].summary,
diff --git a/polygerrit-ui/app/services/comments/comments-model.ts b/polygerrit-ui/app/services/comments/comments-model.ts index 850acbc..5b32465 100644 --- a/polygerrit-ui/app/services/comments/comments-model.ts +++ b/polygerrit-ui/app/services/comments/comments-model.ts
@@ -51,7 +51,10 @@ const privateState$ = new BehaviorSubject(initialState); export function _testOnly_resetState() { - privateState$.next(initialState); + // We cannot assign a new subject to privateState$, because all the selectors + // have already subscribed to the original subject. So we have to emit the + // initial state on the existing subject. + privateState$.next({...initialState}); } // Re-exporting as Observable so that you can only subscribe, but not emit. @@ -65,11 +68,21 @@ privateState$.next(state); } +export const comments$ = commentState$.pipe( + map(commentState => commentState.comments), + distinctUntilChanged() +); + export const drafts$ = commentState$.pipe( map(commentState => commentState.drafts), distinctUntilChanged() ); +export const portedComments$ = commentState$.pipe( + map(commentState => commentState.portedComments), + distinctUntilChanged() +); + export const discardedDrafts$ = commentState$.pipe( map(commentState => commentState.discardedDrafts), distinctUntilChanged() @@ -87,14 +100,22 @@ commentState.portedComments, commentState.portedDrafts ) - ), - distinctUntilChanged() + ) +); + +export const threads$ = changeComments$.pipe( + map(changeComments => changeComments.getAllThreadsForChange()) ); function publishState(state: CommentState) { privateState$.next(state); } +/** Called when the change number changes. Wipes out all data from the state. */ +export function updateStateReset() { + publishState({...initialState}); +} + export function updateStateComments(comments?: { [path: string]: CommentInfo[]; }) {
diff --git a/polygerrit-ui/app/services/comments/comments-model_test.ts b/polygerrit-ui/app/services/comments/comments-model_test.ts index e389254..30fc7cf 100644 --- a/polygerrit-ui/app/services/comments/comments-model_test.ts +++ b/polygerrit-ui/app/services/comments/comments-model_test.ts
@@ -22,13 +22,11 @@ import { updateStateDeleteDraft, _testOnly_getState, - _testOnly_resetState, _testOnly_setState, } from './comments-model'; suite('comments model tests', () => { test('updateStateDeleteDraft', () => { - _testOnly_resetState(); const draft = createDraft(); draft.id = '1' as UrlEncodedCommentId; _testOnly_setState({
diff --git a/polygerrit-ui/app/services/comments/comments-service.ts b/polygerrit-ui/app/services/comments/comments-service.ts index 16ee2f7..5896b52 100644 --- a/polygerrit-ui/app/services/comments/comments-service.ts +++ b/polygerrit-ui/app/services/comments/comments-service.ts
@@ -31,7 +31,10 @@ updateStatePortedDrafts, updateStateUndoDiscardedDraft, discardedDrafts$, + updateStateReset, } from './comments-model'; +import {changeNum$, currentPatchNum$} from '../change/change-model'; +import {combineLatest} from 'rxjs'; export class CommentsService { private discardedDrafts?: UIDraft[] = []; @@ -40,31 +43,55 @@ discardedDrafts$.subscribe( discardedDrafts => (this.discardedDrafts = discardedDrafts) ); + changeNum$.subscribe(changeNum => { + updateStateReset(); + if (!changeNum) return; + this.reloadComments(changeNum); + this.reloadRobotComments(changeNum); + this.reloadDrafts(changeNum); + }); + combineLatest([changeNum$, currentPatchNum$]).subscribe( + ([changeNum, currentPatchNum]) => { + if (!changeNum || !currentPatchNum) return; + this.reloadPortedComments(changeNum, currentPatchNum); + this.reloadPortedDrafts(changeNum, currentPatchNum); + } + ); } - /** - * Load all comments (with drafts and robot comments) for the given change - * number. The returned promise resolves when the comments have loaded, but - * does not yield the comment data. - */ - // TODO(dhruvsri): listen to changeNum changes or reload event to update - // automatically - loadAll(changeNum: NumericChangeId, patchNum = CURRENT as RevisionId) { - const revision = patchNum; - this.restApiService + reloadComments(changeNum: NumericChangeId): Promise<void> { + return this.restApiService .getDiffComments(changeNum) .then(comments => updateStateComments(comments)); - this.restApiService + } + + reloadRobotComments(changeNum: NumericChangeId): Promise<void> { + return this.restApiService .getDiffRobotComments(changeNum) .then(robotComments => updateStateRobotComments(robotComments)); - this.restApiService + } + + reloadDrafts(changeNum: NumericChangeId): Promise<void> { + return this.restApiService .getDiffDrafts(changeNum) .then(drafts => updateStateDrafts(drafts)); - this.restApiService - .getPortedComments(changeNum, revision) + } + + reloadPortedComments( + changeNum: NumericChangeId, + patchNum = CURRENT as RevisionId + ): Promise<void> { + return this.restApiService + .getPortedComments(changeNum, patchNum) .then(portedComments => updateStatePortedComments(portedComments)); - this.restApiService - .getPortedDrafts(changeNum, revision) + } + + reloadPortedDrafts( + changeNum: NumericChangeId, + patchNum = CURRENT as RevisionId + ): Promise<void> { + return this.restApiService + .getPortedDrafts(changeNum, patchNum) .then(portedDrafts => updateStatePortedDrafts(portedDrafts)); }
diff --git a/polygerrit-ui/app/services/comments/comments-service_test.ts b/polygerrit-ui/app/services/comments/comments-service_test.ts index 604b5c4..a35768e 100644 --- a/polygerrit-ui/app/services/comments/comments-service_test.ts +++ b/polygerrit-ui/app/services/comments/comments-service_test.ts
@@ -18,109 +18,63 @@ import '../../test/common-test-setup-karma'; import { createComment, - createFixSuggestionInfo, + createParsedChange, + TEST_NUMERIC_CHANGE_ID, } from '../../test/test-data-generators'; -import {stubRestApi} from '../../test/test-utils'; -import { - NumericChangeId, - RobotId, - RobotRunId, - Timestamp, - UrlEncodedCommentId, -} from '../../types/common'; +import {stubRestApi, waitUntil, waitUntilCalled} from '../../test/test-utils'; import {appContext} from '../app-context'; import {CommentsService} from './comments-service'; +import {updateState as updateChangeState} from '../change/change-model'; +import { + GerritView, + updateState as updateRouterState, +} from '../router/router-model'; +import {comments$, portedComments$} from './comments-model'; +import {PathToCommentsInfoMap} from '../../types/common'; suite('change service tests', () => { - let commentsService: CommentsService; - - test('loads logged-out', () => { - const changeNum = 1234 as NumericChangeId; - commentsService = new CommentsService(appContext.restApiService); - stubRestApi('getLoggedIn').returns(Promise.resolve(false)); + test('loads comments', async () => { + new CommentsService(appContext.restApiService); const diffCommentsSpy = stubRestApi('getDiffComments').returns( - Promise.resolve({ - 'foo.c': [ - { - ...createComment(), - id: '123' as UrlEncodedCommentId, - message: 'Done', - updated: '2017-02-08 16:40:49' as Timestamp, - }, - ], - }) + Promise.resolve({'foo.c': [createComment()]}) ); const diffRobotCommentsSpy = stubRestApi('getDiffRobotComments').returns( - Promise.resolve({ - 'foo.c': [ - { - ...createComment(), - id: '321' as UrlEncodedCommentId, - message: 'Done', - updated: '2017-02-08 16:40:49' as Timestamp, - robot_id: 'robot_1' as RobotId, - robot_run_id: 'run_1' as RobotRunId, - properties: {}, - fix_suggestions: [ - createFixSuggestionInfo('fix_1'), - createFixSuggestionInfo('fix_2'), - ], - }, - ], - }) + Promise.resolve({}) ); const diffDraftsSpy = stubRestApi('getDiffDrafts').returns( Promise.resolve({}) ); - - commentsService.loadAll(changeNum); - assert.isTrue(diffCommentsSpy.calledWithExactly(changeNum)); - assert.isTrue(diffRobotCommentsSpy.calledWithExactly(changeNum)); - assert.isTrue(diffDraftsSpy.calledWithExactly(changeNum)); - }); - - test('loads logged-in', () => { - const changeNum = 1234 as NumericChangeId; - - stubRestApi('getLoggedIn').returns(Promise.resolve(true)); - const diffCommentsSpy = stubRestApi('getDiffComments').returns( - Promise.resolve({ - 'foo.c': [ - { - ...createComment(), - id: '123' as UrlEncodedCommentId, - message: 'Done', - updated: '2017-02-08 16:40:49' as Timestamp, - }, - ], - }) + const portedCommentsSpy = stubRestApi('getPortedComments').returns( + Promise.resolve({'foo.c': [createComment()]}) ); - const diffRobotCommentsSpy = stubRestApi('getDiffRobotComments').returns( - Promise.resolve({ - 'foo.c': [ - { - ...createComment(), - id: '321' as UrlEncodedCommentId, - message: 'Done', - updated: '2017-02-08 16:40:49' as Timestamp, - robot_id: 'robot_1' as RobotId, - robot_run_id: 'run_1' as RobotRunId, - properties: {}, - fix_suggestions: [ - createFixSuggestionInfo('fix_1'), - createFixSuggestionInfo('fix_2'), - ], - }, - ], - }) - ); - const diffDraftsSpy = stubRestApi('getDiffDrafts').returns( + const portedDraftsSpy = stubRestApi('getPortedDrafts').returns( Promise.resolve({}) ); + let comments: PathToCommentsInfoMap = {}; + comments$.subscribe(c => (comments = c)); + let portedComments: PathToCommentsInfoMap = {}; + portedComments$.subscribe(c => (portedComments = c)); - commentsService.loadAll(changeNum); - assert.isTrue(diffCommentsSpy.calledWithExactly(changeNum)); - assert.isTrue(diffRobotCommentsSpy.calledWithExactly(changeNum)); - assert.isTrue(diffDraftsSpy.calledWithExactly(changeNum)); + updateRouterState(GerritView.CHANGE, TEST_NUMERIC_CHANGE_ID); + updateChangeState(createParsedChange()); + + await waitUntilCalled(diffCommentsSpy, 'diffCommentsSpy'); + await waitUntilCalled(diffRobotCommentsSpy, 'diffRobotCommentsSpy'); + await waitUntilCalled(diffDraftsSpy, 'diffDraftsSpy'); + await waitUntilCalled(portedCommentsSpy, 'portedCommentsSpy'); + await waitUntilCalled(portedDraftsSpy, 'portedDraftsSpy'); + await waitUntil( + () => Object.keys(comments).length > 0, + 'comment in model not set' + ); + await waitUntil( + () => Object.keys(portedComments).length > 0, + 'ported comment in model not set' + ); + + assert.equal(comments['foo.c'].length, 1); + assert.equal(comments['foo.c'][0].id, '12345'); + assert.equal(portedComments['foo.c'].length, 1); + assert.equal(portedComments['foo.c'][0].id, '12345'); }); });
diff --git a/polygerrit-ui/app/services/router/router-model.ts b/polygerrit-ui/app/services/router/router-model.ts index b3cdf9e..ae3d848 100644 --- a/polygerrit-ui/app/services/router/router-model.ts +++ b/polygerrit-ui/app/services/router/router-model.ts
@@ -47,6 +47,13 @@ const privateState$ = new BehaviorSubject<RouterState>(initialState); +export function _testOnly_resetState() { + // We cannot assign a new subject to privateState$, because all the selectors + // have already subscribed to the original subject. So we have to emit the + // initial state on the existing subject. + privateState$.next({...initialState}); +} + // Re-exporting as Observable so that you can only subscribe, but not emit. export const routerState$: Observable<RouterState> = privateState$;
diff --git a/polygerrit-ui/app/services/user/user-model.ts b/polygerrit-ui/app/services/user/user-model.ts index 000887c..df307d6 100644 --- a/polygerrit-ui/app/services/user/user-model.ts +++ b/polygerrit-ui/app/services/user/user-model.ts
@@ -37,11 +37,13 @@ diffPreferences: createDefaultDiffPrefs(), }; -// Mutable for testing -let privateState$ = new BehaviorSubject(initialState); +const privateState$ = new BehaviorSubject(initialState); export function _testOnly_resetState() { - privateState$ = new BehaviorSubject(initialState); + // We cannot assign a new subject to privateState$, because all the selectors + // have already subscribed to the original subject. So we have to emit the + // initial state on the existing subject. + privateState$.next({...initialState}); } export function _testOnly_setState(state: UserState) {
diff --git a/polygerrit-ui/app/test/common-test-setup.ts b/polygerrit-ui/app/test/common-test-setup.ts index bd5504a..05adb41 100644 --- a/polygerrit-ui/app/test/common-test-setup.ts +++ b/polygerrit-ui/app/test/common-test-setup.ts
@@ -45,6 +45,12 @@ import {updatePreferences} from '../services/user/user-model'; import {createDefaultPreferences} from '../constants/constants'; import {appContext} from '../services/app-context'; +import {_testOnly_resetState as resetBrowserState} from '../services/browser/browser-model'; +import {_testOnly_resetState as resetChangeState} from '../services/change/change-model'; +import {_testOnly_resetState as resetChecksState} from '../services/checks/checks-model'; +import {_testOnly_resetState as resetCommentsState} from '../services/comments/comments-model'; +import {_testOnly_resetState as resetRouterState} from '../services/router/router-model'; +import {_testOnly_resetState as resetUserState} from '../services/user/user-model'; declare global { interface Window { @@ -106,6 +112,14 @@ // tests. initGlobalVariables(); _testOnly_initGerritPluginApi(); + + resetBrowserState(); + resetChangeState(); + resetChecksState(); + resetCommentsState(); + resetRouterState(); + resetUserState(); + const shortcuts = appContext.shortcutsService; assert.isTrue(shortcuts._testOnly_isEmpty()); const selection = document.getSelection();
diff --git a/polygerrit-ui/app/test/test-data-generators.ts b/polygerrit-ui/app/test/test-data-generators.ts index dd56ce2..91cd2f3 100644 --- a/polygerrit-ui/app/test/test-data-generators.ts +++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -95,7 +95,12 @@ import {AppElementChangeViewParams} from '../elements/gr-app-types'; import {CommitInfoWithRequiredCommit} from '../elements/change/gr-change-metadata/gr-change-metadata'; import {WebLinkInfo} from '../types/diff'; -import {createCommentThreads, UIComment, UIDraft} from '../utils/comment-util'; +import { + createCommentThreads, + UIComment, + UIDraft, + UIHuman, +} from '../utils/comment-util'; import {GerritView} from '../services/router/router-model'; import {ChangeComments} from '../elements/diff/gr-comment-api/gr-comment-api'; import {EditRevisionInfo, ParsedChangeInfo} from '../types/types'; @@ -488,7 +493,7 @@ }; } -export function createComment(): UIComment { +export function createComment(): UIHuman { return { patch_set: 1 as PatchSetNum, id: '12345' as UrlEncodedCommentId,
diff --git a/polygerrit-ui/app/test/test-utils.ts b/polygerrit-ui/app/test/test-utils.ts index 4a513f8..63c125e 100644 --- a/polygerrit-ui/app/test/test-utils.ts +++ b/polygerrit-ui/app/test/test-utils.ts
@@ -19,7 +19,7 @@ import {_testOnly_resetEndpoints} from '../elements/shared/gr-js-api-interface/gr-plugin-endpoints'; import {appContext} from '../services/app-context'; import {RestApiService} from '../services/gr-rest-api/gr-rest-api'; -import {SinonSpy} from 'sinon'; +import {SinonSpy, SinonStub} from 'sinon'; import {StorageService} from '../services/storage/gr-storage'; import {AuthService} from '../services/gr-auth/gr-auth'; import {ReportingService} from '../services/gr-reporting/gr-reporting'; @@ -185,6 +185,7 @@ ): Promise<void> { const start = Date.now(); let sleep = 0; + if (predicate()) return Promise.resolve(); return new Promise((resolve, reject) => { const waiter = () => { if (predicate()) { @@ -200,6 +201,10 @@ }); } +export function waitUntilCalled(stub: SinonStub, name: string) { + return waitUntil(() => stub.called, `${name} was not called`); +} + /** * Promisify an event callback to simplify async...await tests. *
diff --git a/polygerrit-ui/app/utils/patch-set-util.ts b/polygerrit-ui/app/utils/patch-set-util.ts index ee4ed8b..921850a 100644 --- a/polygerrit-ui/app/utils/patch-set-util.ts +++ b/polygerrit-ui/app/utils/patch-set-util.ts
@@ -103,7 +103,7 @@ return rev; } } - console.warn('no revision found'); + if (revisions.length > 0) console.warn('no revision found'); return; }