Merge pull request #5310 from nextcloud/fix-tag-label-removed-when-share-view-is-opened

Fix tag label removed when share view is opened

apps/files/js/detailsview.js

@@ -300,6 +300,16 @@
 		addDetailView: function(detailView) {
 			this._detailFileInfoViews.push(detailView);
 			this._dirty = true;
+		},
+
+		/**
+		 * Returns an array with the added DetailFileInfoViews.
+		 *
+		 * @return Array<OCA.Files.DetailFileInfoView> an array with the added
+		 *         DetailFileInfoViews.
+		 */
+		getDetailViews: function() {
+			return [].concat(this._detailFileInfoViews);
 		}
 	});
 

apps/files/js/filelist.js

@@ -3020,6 +3020,21 @@
 			if (this.breadcrumb) {
 				this.breadcrumb.addDetailView(detailView);
 			}
+		},
+
+		/**
+		 * Returns the registered detail views.
+		 *
+		 * @return null|Array<OCA.Files.DetailFileInfoView> an array with the
+		 *         registered DetailFileInfoViews, or null if the details view
+		 *         is not enabled.
+		 */
+		getRegisteredDetailViews: function() {
+			if (this._detailsView) {
+				return this._detailsView.getDetailViews();
+			}
+
+			return null;
 		}
 	};
 

apps/files/js/mainfileinfodetailview.js

@@ -171,6 +171,8 @@
 		 * Renders this details view
 		 */
 		render: function() {
+			this.trigger('pre-render');
+
 			if (this.model) {
 				var isFavorite = (this.model.get('tags') || []).indexOf(OC.TAG_FAVORITE) >= 0;
 				this.$el.html(this.template({
@@ -209,6 +211,8 @@
 				this.$el.empty();
 			}
 			this.delegateEvents();
+
+			this.trigger('post-render');
 		}
 	});
 

apps/files/tests/js/detailsviewSpec.js

@@ -35,6 +35,27 @@ describe('OCA.Files.DetailsView tests', function() {
 		expect(detailsView.$el.find('.tabsContainer').length).toEqual(1);
 	});
 	describe('file info detail view', function() {
+		it('returns registered view', function() {
+			var testView = new OCA.Files.DetailFileInfoView();
+			var testView2 = new OCA.Files.DetailFileInfoView();
+			detailsView.addDetailView(testView);
+			detailsView.addDetailView(testView2);
+
+			detailViews = detailsView.getDetailViews();
+
+			expect(detailViews).toContain(testView);
+			expect(detailViews).toContain(testView2);
+
+			// Modify array and check that registered detail views are not
+			// modified
+			detailViews.pop();
+			detailViews.pop();
+
+			detailViews = detailsView.getDetailViews();
+
+			expect(detailViews).toContain(testView);
+			expect(detailViews).toContain(testView2);
+		});
 		it('renders registered view', function() {
 			var testView = new OCA.Files.DetailFileInfoView();
 			var testView2 = new OCA.Files.DetailFileInfoView();

apps/files/tests/js/filelistSpec.js

@@ -2116,10 +2116,12 @@ describe('OCA.Files.FileList tests', function() {
 			beforeEach(function() {
 				addTabStub = sinon.stub(OCA.Files.DetailsView.prototype, 'addTabView');
 				addDetailStub = sinon.stub(OCA.Files.DetailsView.prototype, 'addDetailView');
+				getDetailsStub = sinon.stub(OCA.Files.DetailsView.prototype, 'getDetailViews');
 			});
 			afterEach(function() {
 				addTabStub.restore();
 				addDetailStub.restore();
+				getDetailsStub.restore();
 			});
 			it('forward the registered views to the underlying DetailsView', function() {
 				fileList.destroy();
@@ -2133,6 +2135,19 @@ describe('OCA.Files.FileList tests', function() {
 				// twice because the filelist already registers one by default
 				expect(addDetailStub.calledTwice).toEqual(true);
 			});
+			it('forward getting the registered views to the underlying DetailsView', function() {
+				fileList.destroy();
+				fileList = new OCA.Files.FileList($('#app-content-files'), {
+					detailsViewEnabled: true
+				});
+				var expectedRegisteredDetailsView = [];
+				getDetailsStub.returns(expectedRegisteredDetailsView);
+
+				var registeredDetailViews = fileList.getRegisteredDetailViews();
+
+				expect(getDetailsStub.calledOnce).toEqual(true);
+				expect(registeredDetailViews).toEqual(expectedRegisteredDetailsView);
+			});
 			it('does not error when registering panels when not details view configured', function() {
 				fileList.destroy();
 				fileList = new OCA.Files.FileList($('#app-content-files'), {
@@ -2144,6 +2159,17 @@ describe('OCA.Files.FileList tests', function() {
 				expect(addTabStub.notCalled).toEqual(true);
 				expect(addDetailStub.notCalled).toEqual(true);
 			});
+			it('returns null when getting the registered views when not details view configured', function() {
+				fileList.destroy();
+				fileList = new OCA.Files.FileList($('#app-content-files'), {
+					detailsViewEnabled: false
+				});
+
+				var registeredDetailViews = fileList.getRegisteredDetailViews();
+
+				expect(getDetailsStub.notCalled).toEqual(true);
+				expect(registeredDetailViews).toBeNull();
+			});
 		});
 		it('triggers file action when clicking on row if no details view configured', function() {
 			fileList.destroy();

apps/systemtags/js/filesplugin.js

@@ -31,7 +31,30 @@
 				return;
 			}
 
-			fileList.registerDetailView(new OCA.SystemTags.SystemTagsInfoView());
+			var systemTagsInfoView = new OCA.SystemTags.SystemTagsInfoView();
+			fileList.registerDetailView(systemTagsInfoView);
+
+			_.each(fileList.getRegisteredDetailViews(), function(detailView) {
+				if (detailView instanceof OCA.Files.MainFileInfoDetailView) {
+					var systemTagsInfoViewToggleView =
+						new OCA.SystemTags.SystemTagsInfoViewToggleView({
+							systemTagsInfoView: systemTagsInfoView
+						});
+					systemTagsInfoViewToggleView.render();
+
+					// The toggle view element is detached before the
+					// MainFileInfoDetailView is rendered to prevent its event
+					// handlers from being removed.
+					systemTagsInfoViewToggleView.listenTo(detailView, 'pre-render', function() {
+						systemTagsInfoViewToggleView.$el.detach();
+					});
+					systemTagsInfoViewToggleView.listenTo(detailView, 'post-render', function() {
+						detailView.$el.find('.file-details').append(systemTagsInfoViewToggleView.$el);
+					});
+
+					return;
+				}
+			});
 		}
 	};
 

apps/systemtags/js/merged.json

@@ -2,5 +2,6 @@
   "app.js",
   "systemtagsfilelist.js",
   "filesplugin.js",
-  "systemtagsinfoview.js"
+  "systemtagsinfoview.js",
+  "systemtagsinfoviewtoggleview.js"
 ]

apps/systemtags/js/systemtagsinfoview.js

@@ -37,8 +37,6 @@
 		 */
 		_inputView: null,
 
-		_toggleHandle: null,
-
 		initialize: function(options) {
 			var self = this;
 			options = options || {};
@@ -60,9 +58,6 @@
 
 			this._inputView.on('select', this._onSelectTag, this);
 			this._inputView.on('deselect', this._onDeselectTag, this);
-
-			this._toggleHandle = $('<span>').addClass('tag-label').text(t('systemtags', 'Tags'));
-			this._toggleHandle.prepend($('<span>').addClass('icon icon-tag'));
 		},
 
 		/**
@@ -128,15 +123,15 @@
 						self._inputView.setData(appliedTags);
 
 						if (appliedTags.length !== 0) {
-							self.$el.removeClass('hidden');
+							self.show();
 						} else {
-							self.$el.addClass('hidden');
+							self.hide();
 						}
 					}
 				});
 			}
 
-			this.$el.addClass('hidden');
+			this.hide();
 		},
 
 		/**
@@ -147,20 +142,26 @@
 
 			this.$el.append(this._inputView.$el);
 			this._inputView.render();
+		},
 
-			$('#app-sidebar').find('.mainFileInfoView .file-details').append(this._toggleHandle);
-			this._toggleHandle.off('click');
-			this._toggleHandle.on('click', function () {
-				self.$el.toggleClass('hidden');
-				if (!self.$el.hasClass('hidden')) {
-					self.$el.find('.systemTagsInputField').select2('open');
-				}
-			});
+		isVisible: function() {
+			return !this.$el.hasClass('hidden');
+		},
+
+		show: function() {
+			this.$el.removeClass('hidden');
+		},
+
+		hide: function() {
+			this.$el.addClass('hidden');
+		},
+
+		openDropdown: function() {
+			this.$el.find('.systemTagsInputField').select2('open');
 		},
 
 		remove: function() {
 			this._inputView.remove();
-			this._toggleHandle.remove();
 		}
 	});
 

apps/systemtags/js/systemtagsinfoviewtoggleview.js

@@ -0,0 +1,103 @@
+/**
+ *
+ * @copyright Copyright (c) 2017, Daniel Calviño Sánchez (danxuliu@gmail.com)
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+(function(OCA) {
+
+	var TEMPLATE =
+		'<span class="icon icon-tag"/>' + t('systemtags', 'Tags');
+
+	/**
+	 * @class OCA.SystemTags.SystemTagsInfoViewToggleView
+	 * @classdesc
+	 *
+	 * View to toggle the visibility of a SystemTagsInfoView.
+	 *
+	 * This toggle view must be explicitly rendered before it is used.
+	 */
+	var SystemTagsInfoViewToggleView = OC.Backbone.View.extend(
+		/** @lends OC.Backbone.View.prototype */ {
+
+		tagName: 'span',
+
+		className: 'tag-label',
+
+		events: {
+			'click': 'click'
+		},
+
+		/**
+		 * @type OCA.SystemTags.SystemTagsInfoView
+		 */
+		_systemTagsInfoView: null,
+
+		template: function(data) {
+			if (!this._template) {
+				this._template = Handlebars.compile(TEMPLATE);
+			}
+			return this._template(data);
+		},
+
+		/**
+		 * Initialize this toggle view.
+		 *
+		 * The options must provide a systemTagsInfoView parameter that
+		 * references the SystemTagsInfoView to associate to this toggle view.
+		 */
+		initialize: function(options) {
+			var self = this;
+			options = options || {};
+
+			this._systemTagsInfoView = options.systemTagsInfoView;
+			if (!this._systemTagsInfoView) {
+				throw 'Missing required parameter "systemTagsInfoView"';
+			}
+		},
+
+		/**
+		 * Toggles the visibility of the associated SystemTagsInfoView.
+		 *
+		 * When the systemTagsInfoView is shown its dropdown is also opened.
+		 */
+		click: function() {
+			if (this._systemTagsInfoView.isVisible()) {
+				this._systemTagsInfoView.hide();
+			} else {
+				this._systemTagsInfoView.show();
+				this._systemTagsInfoView.openDropdown();
+			}
+		},
+
+		/**
+		 * Renders this toggle view.
+		 *
+		 * @return OCA.SystemTags.SystemTagsInfoViewToggleView this object.
+		 */
+		render: function() {
+			this.$el.html(this.template());
+
+			return this;
+		},
+
+	});
+
+	OCA.SystemTags.SystemTagsInfoViewToggleView = SystemTagsInfoViewToggleView;
+
+})(OCA);

apps/systemtags/tests/js/systemtagsinfoviewSpec.js

@@ -201,4 +201,50 @@ describe('OCA.SystemTags.SystemTagsInfoView tests', function() {
 
 		});
 	});
+	describe('visibility', function() {
+		it('reports visibility based on the "hidden" class name', function() {
+			view.$el.addClass('hidden');
+
+			expect(view.isVisible()).toBeFalsy();
+
+			view.$el.removeClass('hidden');
+
+			expect(view.isVisible()).toBeTruthy();
+		});
+		it('is not visible after rendering', function() {
+			view.render();
+
+			expect(view.isVisible()).toBeFalsy();
+		});
+		it('shows and hides the element', function() {
+			view.show();
+
+			expect(view.isVisible()).toBeTruthy();
+
+			view.hide();
+
+			expect(view.isVisible()).toBeFalsy();
+
+			view.show();
+
+			expect(view.isVisible()).toBeTruthy();
+		});
+	});
+	describe('select2', function() {
+		var select2Stub;
+
+		beforeEach(function() {
+			select2Stub = sinon.stub($.fn, 'select2');
+		});
+		afterEach(function() {
+			select2Stub.restore();
+		});
+		it('opens dropdown', function() {
+			view.openDropdown();
+
+			expect(select2Stub.calledOnce).toBeTruthy();
+			expect(select2Stub.thisValues[0].selector).toEqual('.systemTagsInputField');
+			expect(select2Stub.withArgs('open')).toBeTruthy();
+		});
+	});
 });

apps/systemtags/tests/js/systemtagsinfoviewtoggleviewSpec.js

@@ -0,0 +1,93 @@
+/**
+ *
+ * @copyright Copyright (c) 2017, Daniel Calviño Sánchez (danxuliu@gmail.com)
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+describe('OCA.SystemTags.SystemTagsInfoViewToggleView', function () {
+
+	var systemTagsInfoView;
+	var view;
+
+	beforeEach(function() {
+		systemTagsInfoView = new OCA.SystemTags.SystemTagsInfoView();
+		view = new OCA.SystemTags.SystemTagsInfoViewToggleView({ systemTagsInfoView: systemTagsInfoView });
+	});
+
+	afterEach(function() {
+		view.remove();
+		systemTagsInfoView.remove();
+	});
+
+	describe('initialize', function() {
+		it('fails if a "systemTagsInfoView" parameter is not provided', function() {
+			var constructor = function() {
+				return new OCA.SystemTags.SystemTagsInfoViewToggleView({});
+			}
+
+			expect(constructor).toThrow();
+		});
+	});
+
+	describe('click on element', function() {
+
+		var isVisibleStub;
+		var showStub;
+		var hideStub;
+		var openDropdownStub;
+
+		beforeEach(function() {
+			isVisibleStub = sinon.stub(systemTagsInfoView, 'isVisible');
+			showStub = sinon.stub(systemTagsInfoView, 'show');
+			hideStub = sinon.stub(systemTagsInfoView, 'hide');
+			openDropdownStub = sinon.stub(systemTagsInfoView, 'openDropdown');
+		});
+
+		afterEach(function() {
+			isVisibleStub.restore();
+			showStub.restore();
+			hideStub.restore();
+			openDropdownStub.restore();
+		});
+
+		it('shows a not visible SystemTagsInfoView', function() {
+			isVisibleStub.returns(false);
+
+			view.$el.click();
+
+			expect(isVisibleStub.calledOnce).toBeTruthy();
+			expect(showStub.calledOnce).toBeTruthy();
+			expect(openDropdownStub.calledOnce).toBeTruthy();
+			expect(openDropdownStub.calledAfter(showStub)).toBeTruthy();
+			expect(hideStub.notCalled).toBeTruthy();
+		});
+
+		it('hides a visible SystemTagsInfoView', function() {
+			isVisibleStub.returns(true);
+
+			view.$el.click();
+
+			expect(isVisibleStub.calledOnce).toBeTruthy();
+			expect(hideStub.calledOnce).toBeTruthy();
+			expect(showStub.notCalled).toBeTruthy();
+			expect(openDropdownStub.notCalled).toBeTruthy();
+		});
+
+	});
+
+});

tests/acceptance/features/app-files.feature

@@ -52,3 +52,19 @@ Feature: app-files
     And I authenticate with password "fedcba"
     Then I see that the current page is the Authenticate page for the shared link I wrote down
     And I see that a wrong password for the shared file message is shown
+
+  Scenario: show the input field for tags in the details view
+    Given I am logged in
+    And I open the details view for "welcome.txt"
+    And I see that the details view for "All files" section is open
+    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: show the input field for tags in the details view after the sharing tab has loaded
+    Given I am logged in
+    And I open the details view for "welcome.txt"
+    And I see that the details view for "All files" section is open
+    And I open the "Sharing" tab in the details view
+    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

tests/acceptance/features/bootstrap/FilesAppContext.php

@@ -102,6 +102,69 @@ class FilesAppContext implements Context, ActorAwareInterface {
 				describedAs("Current section details view in Files app");
 	}
 
+	/**
+	 * @return Locator
+	 */
+	public static function fileDetailsInCurrentSectionDetailsViewWithText($fileDetailsText) {
+		return Locator::forThe()->xpath("//span[normalize-space() = '$fileDetailsText']")->
+				descendantOf(self::fileDetailsInCurrentSectionDetailsView())->
+				describedAs("File details with text \"$fileDetailsText\" in current section details view in Files app");
+	}
+
+	/**
+	 * @return Locator
+	 */
+	private static function fileDetailsInCurrentSectionDetailsView() {
+		return Locator::forThe()->css(".file-details")->
+				descendantOf(self::currentSectionDetailsView())->
+				describedAs("File details in current section details view in Files app");
+	}
+
+	/**
+	 * @return Locator
+	 */
+	public static function inputFieldForTagsInCurrentSectionDetails() {
+		return Locator::forThe()->css(".systemTagsInfoView")->
+				descendantOf(self::currentSectionDetailsView())->
+				describedAs("Input field for tags in current section details view in Files app");
+	}
+
+	/**
+	 * @return Locator
+	 */
+	public static function tabHeaderInCurrentSectionDetailsViewNamed($tabHeaderName) {
+		return Locator::forThe()->xpath("//li[normalize-space() = '$tabHeaderName']")->
+				descendantOf(self::tabHeadersInCurrentSectionDetailsView())->
+				describedAs("Tab header named $tabHeaderName in current section details view in Files app");
+	}
+
+	/**
+	 * @return Locator
+	 */
+	private static function tabHeadersInCurrentSectionDetailsView() {
+		return Locator::forThe()->css(".tabHeaders")->
+				descendantOf(self::currentSectionDetailsView())->
+				describedAs("Tab headers in current section details view in Files app");
+	}
+
+	/**
+	 * @return Locator
+	 */
+	public static function tabInCurrentSectionDetailsViewNamed($tabName) {
+		return Locator::forThe()->xpath("//div[@id=//*[contains(concat(' ', normalize-space(@class), ' '), ' tabHeader ') and normalize-space() = '$tabName']/@data-tabid]")->
+				descendantOf(self::currentSectionDetailsView())->
+				describedAs("Tab named $tabName in current section details view in Files app");
+	}
+
+	/**
+	 * @return Locator
+	 */
+	public static function loadingIconForTabInCurrentSectionDetailsViewNamed($tabName) {
+		return Locator::forThe()->css(".loading")->
+				descendantOf(self::tabInCurrentSectionDetailsViewNamed($tabName))->
+				describedAs("Loading icon for tab named $tabName in current section details view in Files app");
+	}
+
 	/**
 	 * @return Locator
 	 */
@@ -246,6 +309,20 @@ class FilesAppContext implements Context, ActorAwareInterface {
 		$this->actor->find(self::detailsMenuItem(), 2)->click();
 	}
 
+	/**
+	 * @Given I open the input field for tags in the details view
+	 */
+	public function iOpenTheInputFieldForTagsInTheDetailsView() {
+		$this->actor->find(self::fileDetailsInCurrentSectionDetailsViewWithText("Tags"), 10)->click();
+	}
+
+	/**
+	 * @Given I open the :tabName tab in the details view
+	 */
+	public function iOpenTheTabInTheDetailsView($tabName) {
+		$this->actor->find(self::tabHeaderInCurrentSectionDetailsViewNamed($tabName), 10)->click();
+	}
+
 	/**
 	 * @Given I mark :fileName as favorite
 	 */
@@ -343,6 +420,23 @@ class FilesAppContext implements Context, ActorAwareInterface {
 		PHPUnit_Framework_Assert::assertNotNull($this->actor->find(self::favoritedStateIconForFile($fileName), 10));
 	}
 
+	/**
+	 * @Then I see that the input field for tags in the details view is shown
+	 */
+	public function iSeeThatTheInputFieldForTagsInTheDetailsViewIsShown() {
+		PHPUnit_Framework_Assert::assertTrue(
+				$this->actor->find(self::inputFieldForTagsInCurrentSectionDetails(), 10)->isVisible());
+	}
+
+	/**
+	 * @When I see that the :tabName tab in the details view is eventually loaded
+	 */
+	public function iSeeThatTheTabInTheDetailsViewIsEventuallyLoaded($tabName) {
+		if (!$this->waitForElementToBeEventuallyNotShown(self::loadingIconForTabInCurrentSectionDetailsViewNamed($tabName), $timeout = 10)) {
+			PHPUnit_Framework_Assert::fail("The $tabName tab in the details view has not been loaded after $timeout seconds");
+		}
+	}
+
 	/**
 	 * @Then I see that the working icon for password protect is shown
 	 */
@@ -354,20 +448,7 @@ class FilesAppContext implements Context, ActorAwareInterface {
 	 * @Then I see that the working icon for password protect is eventually not shown
 	 */
 	public function iSeeThatTheWorkingIconForPasswordProtectIsEventuallyNotShown() {
-		$timeout = 10;
-		$timeoutStep = 1;
-
-		$actor = $this->actor;
-		$passwordProtectWorkingIcon = self::passwordProtectWorkingIcon();
-
-		$workingIconNotFoundCallback = function() use ($actor, $passwordProtectWorkingIcon) {
-			try {
-				return !$actor->find($passwordProtectWorkingIcon)->isVisible();
-			} catch (NoSuchElementException $exception) {
-				return true;
-			}
-		};
-		if (!Utils::waitFor($workingIconNotFoundCallback, $timeout, $timeoutStep)) {
+		if (!$this->waitForElementToBeEventuallyNotShown(self::passwordProtectWorkingIcon(), $timeout = 10)) {
 			PHPUnit_Framework_Assert::fail("The working icon for password protect is still shown after $timeout seconds");
 		}
 	}
@@ -382,4 +463,17 @@ class FilesAppContext implements Context, ActorAwareInterface {
 		$this->iSeeThatTheWorkingIconForPasswordProtectIsEventuallyNotShown();
 	}
 
+	private function waitForElementToBeEventuallyNotShown($elementLocator, $timeout = 10, $timeoutStep = 1) {
+		$actor = $this->actor;
+
+		$elementNotFoundCallback = function() use ($actor, $elementLocator) {
+			try {
+				return !$actor->find($elementLocator)->isVisible();
+			} catch (NoSuchElementException $exception) {
+				return true;
+			}
+		};
+
+		return Utils::waitFor($elementNotFoundCallback, $timeout, $timeoutStep);
+	}
 }

tests/karma.config.js

@@ -102,6 +102,7 @@ module.exports = function(config) {
 					// need to enforce loading order...
 					'apps/systemtags/js/app.js',
 					'apps/systemtags/js/systemtagsinfoview.js',
+					'apps/systemtags/js/systemtagsinfoviewtoggleview.js',
 					'apps/systemtags/js/systemtagsfilelist.js',
 					'apps/systemtags/js/filesplugin.js'
 				],