From 6fa5c33af4a214e695942e05bb50f564e09d1f44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Wed, 5 Jul 2017 14:54:26 +0200 Subject: [PATCH 1/2] Add acceptance tests for sorting of favorite files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- tests/acceptance/features/app-files.feature | 19 ++++ .../features/bootstrap/FilesAppContext.php | 86 +++++++++++++++++++ 2 files changed, 105 insertions(+) diff --git a/tests/acceptance/features/app-files.feature b/tests/acceptance/features/app-files.feature index 8d32508513a..c968ae53ce4 100644 --- a/tests/acceptance/features/app-files.feature +++ b/tests/acceptance/features/app-files.feature @@ -68,3 +68,22 @@ Feature: app-files And I see that the "Sharing" tab in the details view is eventually loaded When I open the input field for tags in the details view Then I see that the input field for tags in the details view is shown + + Scenario: marking a file as favorite causes the file list to be sorted again + Given I am logged in + And I create a new folder named "A name alphabetically lower than welcome.txt" + And I see that "A name alphabetically lower than welcome.txt" precedes "welcome.txt" in the file list + When I mark "welcome.txt" as favorite + Then I see that "welcome.txt" is marked as favorite + And I see that "welcome.txt" precedes "A name alphabetically lower than welcome.txt" in the file list + + Scenario: unmarking a file as favorite causes the file list to be sorted again + Given I am logged in + And I create a new folder named "A name alphabetically lower than welcome.txt" + And I see that "A name alphabetically lower than welcome.txt" precedes "welcome.txt" in the file list + And I mark "welcome.txt" as favorite + And I see that "welcome.txt" is marked as favorite + And I see that "welcome.txt" precedes "A name alphabetically lower than welcome.txt" in the file list + When I unmark "welcome.txt" as favorite + Then I see that "welcome.txt" is not marked as favorite + And I see that "A name alphabetically lower than welcome.txt" precedes "welcome.txt" in the file list diff --git a/tests/acceptance/features/bootstrap/FilesAppContext.php b/tests/acceptance/features/bootstrap/FilesAppContext.php index 52f69c66796..631eb60393d 100644 --- a/tests/acceptance/features/bootstrap/FilesAppContext.php +++ b/tests/acceptance/features/bootstrap/FilesAppContext.php @@ -213,6 +213,40 @@ class FilesAppContext implements Context, ActorAwareInterface { describedAs("Password protect working icon in the details view in Files app"); } + /** + * @return Locator + */ + public static function createMenuButton() { + return Locator::forThe()->css("#controls .button.new")-> + descendantOf(self::currentSectionMainView())-> + describedAs("Create menu button in Files app"); + } + + /** + * @return Locator + */ + public static function createNewFolderMenuItem() { + return self::createMenuItemFor("New folder"); + } + + /** + * @return Locator + */ + public static function createNewFolderMenuItemNameInput() { + return Locator::forThe()->css(".filenameform input")-> + descendantOf(self::createNewFolderMenuItem())-> + describedAs("Name input in create new folder menu item in Files app"); + } + + /** + * @return Locator + */ + private static function createMenuItemFor($newType) { + return Locator::forThe()->xpath("//div[contains(concat(' ', normalize-space(@class), ' '), ' newFileMenu ')]//span[normalize-space() = '$newType']/ancestor::li")-> + descendantOf(self::currentSectionMainView())-> + describedAs("Create $newType menu item in Files app"); + } + /** * @return Locator */ @@ -222,6 +256,15 @@ class FilesAppContext implements Context, ActorAwareInterface { describedAs("Row for file $fileName in Files app"); } + /** + * @return Locator + */ + public static function rowForFilePreceding($fileName1, $fileName2) { + return Locator::forThe()->xpath("//preceding-sibling::tr//span[contains(concat(' ', normalize-space(@class), ' '), ' nametext ') and normalize-space() = '$fileName1']/ancestor::tr")-> + descendantOf(self::rowForFile($fileName2))-> + describedAs("Row for file $fileName1 preceding $fileName2 in Files app"); + } + /** * @return Locator */ @@ -230,6 +273,14 @@ class FilesAppContext implements Context, ActorAwareInterface { describedAs("Favorite action for file $fileName in Files app"); } + /** + * @return Locator + */ + public static function notFavoritedStateIconForFile($fileName) { + return Locator::forThe()->css(".icon-star")->descendantOf(self::favoriteActionForFile($fileName))-> + describedAs("Not favorited state icon for file $fileName in Files app"); + } + /** * @return Locator */ @@ -293,6 +344,16 @@ class FilesAppContext implements Context, ActorAwareInterface { describedAs($itemText . " item in file actions menu in Files app"); } + /** + * @Given I create a new folder named :folderName + */ + public function iCreateANewFolderNamed($folderName) { + $this->actor->find(self::createMenuButton(), 10)->click(); + + $this->actor->find(self::createNewFolderMenuItem(), 2)->click(); + $this->actor->find(self::createNewFolderMenuItemNameInput(), 2)->setValue($folderName . "\r"); + } + /** * @Given I open the :section section */ @@ -327,6 +388,17 @@ class FilesAppContext implements Context, ActorAwareInterface { * @Given I mark :fileName as favorite */ public function iMarkAsFavorite($fileName) { + $this->iSeeThatIsNotMarkedAsFavorite($fileName); + + $this->actor->find(self::favoriteActionForFile($fileName), 10)->click(); + } + + /** + * @Given I unmark :fileName as favorite + */ + public function iUnmarkAsFavorite($fileName) { + $this->iSeeThatIsMarkedAsFavorite($fileName); + $this->actor->find(self::favoriteActionForFile($fileName), 10)->click(); } @@ -413,6 +485,13 @@ class FilesAppContext implements Context, ActorAwareInterface { } } + /** + * @Then I see that :fileName1 precedes :fileName2 in the file list + */ + public function iSeeThatPrecedesInTheFileList($fileName1, $fileName2) { + PHPUnit_Framework_Assert::assertNotNull($this->actor->find(self::rowForFilePreceding($fileName1, $fileName2), 10)); + } + /** * @Then I see that :fileName is marked as favorite */ @@ -420,6 +499,13 @@ class FilesAppContext implements Context, ActorAwareInterface { PHPUnit_Framework_Assert::assertNotNull($this->actor->find(self::favoritedStateIconForFile($fileName), 10)); } + /** + * @Then I see that :fileName is not marked as favorite + */ + public function iSeeThatIsNotMarkedAsFavorite($fileName) { + PHPUnit_Framework_Assert::assertNotNull($this->actor->find(self::notFavoritedStateIconForFile($fileName), 10)); + } + /** * @Then I see that the input field for tags in the details view is shown */ From be56374c517633d895e245916e047a72e6723038 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Wed, 5 Jul 2017 15:01:23 +0200 Subject: [PATCH 2/2] Fix sorting of favorite files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sort comparator checks the "isFavorite" property of the FileInfo objects to compare. That property is set when the file list is loaded and the response from the server is parsed, and thus a freshly loaded file list has the proper sorting for favorite files. However, the property is not set in other cases, like when the FileInfo objects are derived from FileInfoModels due to a file being marked as a favorite or a text editor being closed, which causes the file to be sorted in the wrong position. There is no need to add the property in those situations, though; in all cases the TagsPlugin adds a "tags" array property that contains an OC.TAG_FAVORITE tag, so that tag can be checked instead of "isFavorite". Moreover, although "isFavorite" was added by the main "_parseFileInfo" function it did not really belong there but to the "FileInfoParser" from the TagsPlugin; however, as that property now is not used anywhere it was removed altogether. A cleaner solution would have been to make the sort comparator extensible by plugins like other behaviours of the file list and then add the sorting logic related to favorite files to the TagsPlugin. However, right now only the TagsPlugin would need to alter the main sorting logic, and it seems like a corner case anyway. Even if it is implemented as a plugin, favorite files is a core feature, so for the time being it will be taken into account directly in the main sorting logic; making the sort comparator extensible by plugins is defered until there are other use cases for that. Fixes #5410 Signed-off-by: Daniel Calviño Sánchez --- apps/files/js/filelist.js | 9 ++- apps/files/tests/js/filelistSpec.js | 114 ++++++++++++++++++++++++++++ core/js/files/client.js | 7 -- core/js/files/fileinfo.js | 7 +- 4 files changed, 122 insertions(+), 15 deletions(-) diff --git a/apps/files/js/filelist.js b/apps/files/js/filelist.js index b1e7c3f5f8c..086e148e102 100644 --- a/apps/files/js/filelist.js +++ b/apps/files/js/filelist.js @@ -1566,11 +1566,16 @@ this._sort = sort; this._sortDirection = (direction === 'desc')?'desc':'asc'; this._sortComparator = function(fileInfo1, fileInfo2) { - if(fileInfo1.isFavorite && !fileInfo2.isFavorite) { + var isFavorite = function(fileInfo) { + return fileInfo.tags && fileInfo.tags.indexOf(OC.TAG_FAVORITE) >= 0; + }; + + if (isFavorite(fileInfo1) && !isFavorite(fileInfo2)) { return -1; - } else if(!fileInfo1.isFavorite && fileInfo2.isFavorite) { + } else if (!isFavorite(fileInfo1) && isFavorite(fileInfo2)) { return 1; } + return direction === 'asc' ? comparator(fileInfo1, fileInfo2) : -comparator(fileInfo1, fileInfo2); }; diff --git a/apps/files/tests/js/filelistSpec.js b/apps/files/tests/js/filelistSpec.js index b7ee9c8554e..ddd9a45d153 100644 --- a/apps/files/tests/js/filelistSpec.js +++ b/apps/files/tests/js/filelistSpec.js @@ -2476,6 +2476,120 @@ describe('OCA.Files.FileList tests', function() { sortStub.restore(); }); + describe('with favorites', function() { + it('shows favorite files on top', function() { + testFiles.push(new FileInfo({ + id: 5, + type: 'file', + name: 'ZZY Before last file in ascending order', + mimetype: 'text/plain', + mtime: 999999998, + size: 9999998, + // Tags would be added by TagsPlugin + tags: [OC.TAG_FAVORITE], + }), new FileInfo({ + id: 6, + type: 'file', + name: 'ZZZ Last file in ascending order', + mimetype: 'text/plain', + mtime: 999999999, + size: 9999999, + // Tags would be added by TagsPlugin + tags: [OC.TAG_FAVORITE], + })); + + fileList.setFiles(testFiles); + + // Sort by name in ascending order (default sorting is by name + // in ascending order, but setFiles does not trigger a sort, so + // the files must be sorted before being set or a sort must be + // triggered afterwards by clicking on the header). + fileList.$el.find('.column-name .columntitle').click(); + fileList.$el.find('.column-name .columntitle').click(); + + expect(fileList.findFileEl('ZZY Before last file in ascending order').index()).toEqual(0); + expect(fileList.findFileEl('ZZZ Last file in ascending order').index()).toEqual(1); + expect(fileList.findFileEl('somedir').index()).toEqual(2); + expect(fileList.findFileEl('One.txt').index()).toEqual(3); + expect(fileList.findFileEl('Three.pdf').index()).toEqual(4); + expect(fileList.findFileEl('Two.jpg').index()).toEqual(5); + + // Sort by size in ascending order + fileList.$el.find('.column-size .columntitle').click(); + fileList.$el.find('.column-size .columntitle').click(); + + expect(fileList.findFileEl('ZZY Before last file in ascending order').index()).toEqual(0); + expect(fileList.findFileEl('ZZZ Last file in ascending order').index()).toEqual(1); + expect(fileList.findFileEl('One.txt').index()).toEqual(2); + expect(fileList.findFileEl('somedir').index()).toEqual(3); + expect(fileList.findFileEl('Two.jpg').index()).toEqual(4); + expect(fileList.findFileEl('Three.pdf').index()).toEqual(5); + + // Sort by modification time in ascending order + fileList.$el.find('.column-mtime .columntitle').click(); + fileList.$el.find('.column-mtime .columntitle').click(); + + expect(fileList.findFileEl('ZZY Before last file in ascending order').index()).toEqual(0); + expect(fileList.findFileEl('ZZZ Last file in ascending order').index()).toEqual(1); + expect(fileList.findFileEl('One.txt').index()).toEqual(2); + expect(fileList.findFileEl('somedir').index()).toEqual(3); + expect(fileList.findFileEl('Three.pdf').index()).toEqual(4); + expect(fileList.findFileEl('Two.jpg').index()).toEqual(5); + }); + it('shows favorite files on top also when using descending order', function() { + testFiles.push(new FileInfo({ + id: 5, + type: 'file', + name: 'AAB Before last file in descending order', + mimetype: 'text/plain', + mtime: 2, + size: 2, + // Tags would be added by TagsPlugin + tags: [OC.TAG_FAVORITE], + }), new FileInfo({ + id: 6, + type: 'file', + name: 'AAA Last file in descending order', + mimetype: 'text/plain', + mtime: 1, + size: 1, + // Tags would be added by TagsPlugin + tags: [OC.TAG_FAVORITE], + })); + + fileList.setFiles(testFiles); + + // Sort by name in descending order + fileList.$el.find('.column-name .columntitle').click(); + + expect(fileList.findFileEl('AAB Before last file in descending order').index()).toEqual(0); + expect(fileList.findFileEl('AAA Last file in descending order').index()).toEqual(1); + expect(fileList.findFileEl('Two.jpg').index()).toEqual(2); + expect(fileList.findFileEl('Three.pdf').index()).toEqual(3); + expect(fileList.findFileEl('One.txt').index()).toEqual(4); + expect(fileList.findFileEl('somedir').index()).toEqual(5); + + // Sort by size in descending order + fileList.$el.find('.column-size .columntitle').click(); + + expect(fileList.findFileEl('AAB Before last file in descending order').index()).toEqual(0); + expect(fileList.findFileEl('AAA Last file in descending order').index()).toEqual(1); + expect(fileList.findFileEl('Three.pdf').index()).toEqual(2); + expect(fileList.findFileEl('Two.jpg').index()).toEqual(3); + expect(fileList.findFileEl('somedir').index()).toEqual(4); + expect(fileList.findFileEl('One.txt').index()).toEqual(5); + + // Sort by modification time in descending order + fileList.$el.find('.column-mtime .columntitle').click(); + + expect(fileList.findFileEl('AAB Before last file in descending order').index()).toEqual(0); + expect(fileList.findFileEl('AAA Last file in descending order').index()).toEqual(1); + expect(fileList.findFileEl('Two.jpg').index()).toEqual(2); + expect(fileList.findFileEl('Three.pdf').index()).toEqual(3); + expect(fileList.findFileEl('somedir').index()).toEqual(4); + expect(fileList.findFileEl('One.txt').index()).toEqual(5); + }); + }); }); describe('create file', function() { var deferredCreate; diff --git a/core/js/files/client.js b/core/js/files/client.js index da8a1205e4b..d8e615f6d6d 100644 --- a/core/js/files/client.js +++ b/core/js/files/client.js @@ -304,13 +304,6 @@ data.hasPreview = true; } - var isFavorite = props['{' + Client.NS_OWNCLOUD + '}favorite']; - if (!_.isUndefined(isFavorite)) { - data.isFavorite = isFavorite === '1'; - } else { - data.isFavorite = false; - } - var contentType = props[Client.PROPERTY_GETCONTENTTYPE]; if (!_.isUndefined(contentType)) { data.mimetype = contentType; diff --git a/core/js/files/fileinfo.js b/core/js/files/fileinfo.js index 7c8e4586448..1fc239da47a 100644 --- a/core/js/files/fileinfo.js +++ b/core/js/files/fileinfo.js @@ -132,12 +132,7 @@ /** * @type boolean */ - hasPreview: true, - - /** - * @type boolean - */ - isFavorite: false + hasPreview: true }; if (!OC.Files) {