From a59bbe91cf9eb7313c2dd94f05934e3a2fc007ab Mon Sep 17 00:00:00 2001 From: Kasper Nilsson Date: Tue, 16 Jan 2018 16:43:50 -0800 Subject: [PATCH] Move file list fetch logic into rest API interface Consolidating the logic for which API request to make in order to get a fully populated file list into the API interface simplifies things. Making UI components like the file list patchset-agnostic follows a pattern set in previous edit related changes and centralizes fetching logic. Also removes some spurious/redundant tests. Bug: Issue 4437 Change-Id: I21361ef3af8ab50c085248292b43bd4f8325afab --- .../change/gr-file-list/gr-file-list.js | 4 - .../gr-file-list/gr-file-list_test.html | 76 ------------------- .../gr-rest-api-interface.js | 13 ++-- .../gr-rest-api-interface_test.html | 17 +++++ 4 files changed, 23 insertions(+), 87 deletions(-) diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js index 44c7e2dfce..66e3db73f9 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js @@ -408,10 +408,6 @@ }, _getFiles() { - if (this.editLoaded) { - return this.$.restAPI.getChangeEditFilesAsSpeciallySortedArray( - this.changeNum, this.patchRange); - } return this.$.restAPI.getChangeFilesAsSpeciallySortedArray( this.changeNum, this.patchRange); }, diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html index 06799ff3c3..381a30432a 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html @@ -105,82 +105,6 @@ limitations under the License. element.numFilesShown); }); - test('get file list', done => { - const getChangeFilesStub = sandbox.stub(element.$.restAPI, 'getChangeFiles', - () => { - return Promise.resolve({ - '/COMMIT_MSG': {lines_inserted: 9}, - 'tags.html': {lines_deleted: 123}, - 'about.txt': {}, - }); - }); - - element._getFiles().then(files => { - const filenames = files.map(f => { return f.__path; }); - assert.deepEqual(filenames, ['/COMMIT_MSG', 'about.txt', 'tags.html']); - assert.deepEqual(files[0], { - lines_inserted: 9, - lines_deleted: 0, - __path: '/COMMIT_MSG', - }); - assert.deepEqual(files[1], { - lines_inserted: 0, - lines_deleted: 0, - __path: 'about.txt', - }); - assert.deepEqual(files[2], { - lines_inserted: 0, - lines_deleted: 123, - __path: 'tags.html', - }); - - getChangeFilesStub.restore(); - done(); - }); - }); - - test('get file list with change edit', done => { - element.editLoaded = true; - - sandbox.stub(element.$.restAPI, - 'getChangeEditFiles', () => { - return Promise.resolve({ - commit: {}, - files: { - '/COMMIT_MSG': { - lines_inserted: 9, - }, - 'tags.html': { - lines_deleted: 123, - }, - 'about.txt': {}, - }, - }); - }); - - element._getFiles().then(files => { - const filenames = files.map(f => { return f.__path; }); - assert.deepEqual(filenames, ['/COMMIT_MSG', 'about.txt', 'tags.html']); - assert.deepEqual(files[0], { - lines_inserted: 9, - lines_deleted: 0, - __path: '/COMMIT_MSG', - }); - assert.deepEqual(files[1], { - lines_inserted: 0, - lines_deleted: 0, - __path: 'about.txt', - }); - assert.deepEqual(files[2], { - lines_inserted: 0, - lines_deleted: 123, - __path: 'tags.html', - }); - - done(); - }); - }); - test('calculate totals for patch number', () => { element._files = [ {__path: '/COMMIT_MSG', lines_inserted: 9}, diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js index 397f9e89fd..13c67bf14d 100644 --- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js @@ -1002,12 +1002,12 @@ /** * @param {number|string} changeNum - * @param {!Promise} patchRange + * @param {Defs.patchRange} patchRange */ getChangeEditFiles(changeNum, patchRange) { let endpoint = '/edit?list'; if (patchRange.basePatchNum !== 'PARENT') { - endpoint += '&base=' + encodeURIComponent(patchRange.basePatchNum); + endpoint += '&base=' + encodeURIComponent(patchRange.basePatchNum + ''); } return this._getChangeURLAndFetch(changeNum, endpoint); }, @@ -1029,15 +1029,14 @@ * @return {!Promise>} */ getChangeFilesAsSpeciallySortedArray(changeNum, patchRange) { + if (this.patchNumEquals(patchRange.patchNum, this.EDIT_NAME)) { + return this.getChangeEditFiles(changeNum, patchRange).then(res => + this._normalizeChangeFilesResponse(res.files)); + } return this.getChangeFiles(changeNum, patchRange).then( this._normalizeChangeFilesResponse.bind(this)); }, - getChangeEditFilesAsSpeciallySortedArray(changeNum, patchRange) { - return this.getChangeEditFiles(changeNum, patchRange).then(files => - this._normalizeChangeFilesResponse(files.files)); - }, - /** * The closure compiler doesn't realize this.specialFilePathCompare is * valid. diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html index 96fd8303fa..efe70a9ad0 100644 --- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html @@ -1276,5 +1276,22 @@ limitations under the License. return Promise.all([edit, normal]); }); + + test('getChangeFilesAsSpeciallySortedArray is edit-sensitive', () => { + const fn = element.getChangeFilesAsSpeciallySortedArray.bind(element); + const getChangeFilesStub = sandbox.stub(element, 'getChangeFiles') + .returns(Promise.resolve({})); + const getChangeEditFilesStub = sandbox.stub(element, 'getChangeEditFiles') + .returns(Promise.resolve({})); + + return fn('1', {patchNum: 'edit'}).then(() => { + assert.isTrue(getChangeEditFilesStub.calledOnce); + assert.isFalse(getChangeFilesStub.called); + return fn('1', {patchNum: '1'}).then(() => { + assert.isTrue(getChangeEditFilesStub.calledOnce); + assert.isTrue(getChangeFilesStub.calledOnce); + }); + }); + }); });