From a86f63284e9434e310736b039bb0ffadfe4442a3 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 11 Jan 2023 12:30:59 +0100 Subject: [PATCH] Improve saving applicable users in ext storage Added a checkbox to prevent saving "All users" by mistake and giving access to everyone when not wanted. Signed-off-by: Vincent Petry --- apps/files_external/css/settings.css | 7 +- apps/files_external/css/settings.css.map | 2 +- apps/files_external/css/settings.scss | 6 +- apps/files_external/js/settings.js | 96 +++++++++++++++----- apps/files_external/templates/settings.php | 5 +- apps/files_external/tests/js/settingsSpec.js | 71 ++++++++++++++- 6 files changed, 155 insertions(+), 32 deletions(-) diff --git a/apps/files_external/css/settings.css b/apps/files_external/css/settings.css index 6b4b0d9bf47c0..b834f741ca8ce 100644 --- a/apps/files_external/css/settings.css +++ b/apps/files_external/css/settings.css @@ -9,7 +9,7 @@ text-align: center; } -#externalStorage td > input, #externalStorage td > select { +#externalStorage td > input:not(.applicableToAllUsers), #externalStorage td > select { width: 100%; } @@ -119,6 +119,11 @@ background-color: rgba(134, 255, 110, 0.9); } +#externalStorage td.applicable label { + display: inline-flex; + align-items: center; +} + #externalStorage td.applicable div.chzn-container { position: relative; top: 3px; diff --git a/apps/files_external/css/settings.css.map b/apps/files_external/css/settings.css.map index 442480cd41ab3..491bed8ec21bb 100644 --- a/apps/files_external/css/settings.css.map +++ b/apps/files_external/css/settings.css.map @@ -1 +1 @@ -{"version":3,"sourceRoot":"","sources":["settings.scss"],"names":[],"mappings":"AAAA;EACC;;;AAGD;EACC;;AAEA;EACC;;;AAKD;EACC;;;AAIF;EACI;;;AAGJ;AACC;EACA;EACA;;;AAGD;EACC;EACA;EACA;EACA;EACA;EACA;;;AAGA;EACC;EACA;;;AAGF;EAA0B;EAAiB;;;AAC3C;EAAgC;EAAiB;EAAgB;;;AACjE;EAAoB;;;AACpB;EAA+B;;;AAC/B;EAA2B;;;AAE3B;EACC;;AACA;EAGC;EACA;EACA;;AACA;AAAA;AAAA;AAAA;EAEC;EACA;EACA;EACA;EACA;EACA;EACA;;AACA;AAAA;AAAA;AAAA;EACC;;;AAMJ;EACC;EACA;;;AAGD;AAAA;EAEC;;;AAED;EACC;;;AAGD;EACC;;;AAGD;EACC;;;AAGD;EACC;EACA;EACA;;;AAGD;EACC;;;AAID;EACC;EACA;;;AAGD;EACC;;;AAGD;EACC;;;AAGD;EACC;EACA;;;AAED;EACC;EACA;;;AAED;EACC;;;AAED;EACC;EACA;EACA;;;AAED;EACC;EACA;EACA;EACA;EACA;EACA;EACA;EACA;;;AAGD;EACC;;AACA;EACC;EACA;EACA;EACA;;;AAIF;EACC;;;AAGD;EACC","file":"settings.css"} \ No newline at end of file +{"version":3,"sourceRoot":"","sources":["settings.scss"],"names":[],"mappings":"AAAA;EACC;;;AAGD;EACC;;AAEA;EACC;;;AAKD;EACC;;;AAIF;EACI;;;AAGJ;AACC;EACA;EACA;;;AAGD;EACC;EACA;EACA;EACA;EACA;EACA;;;AAGA;EACC;EACA;;;AAGF;EAA0B;EAAiB;;;AAC3C;EAAgC;EAAiB;EAAgB;;;AACjE;EAAoB;;;AACpB;EAA+B;;;AAC/B;EAA2B;;;AAE3B;EACC;;AACA;EAGC;EACA;EACA;;AACA;AAAA;AAAA;AAAA;EAEC;EACA;EACA;EACA;EACA;EACA;EACA;;AACA;AAAA;AAAA;AAAA;EACC;;;AAMJ;EACC;EACA;;;AAGD;AAAA;EAEC;;;AAED;EACC;;;AAGD;EACC;;;AAGD;EACC;;;AAGD;EACC;EACA;EACA;;;AAGD;EACC;;;AAGD;EACC;EACA;;;AAGD;EACC;EACA;;;AAGD;EACC;;;AAGD;EACC;;;AAGD;EACC;EACA;;;AAED;EACC;EACA;;;AAED;EACC;;;AAED;EACC;EACA;EACA;;;AAED;EACC;EACA;EACA;EACA;EACA;EACA;EACA;EACA;;;AAGD;EACC;;AACA;EACC;EACA;EACA;EACA;;;AAIF;EACC;;;AAGD;EACC","file":"settings.css"} \ No newline at end of file diff --git a/apps/files_external/css/settings.scss b/apps/files_external/css/settings.scss index 7485faa4c92de..d3df5704d3b9f 100644 --- a/apps/files_external/css/settings.scss +++ b/apps/files_external/css/settings.scss @@ -11,7 +11,7 @@ } #externalStorage td { - & > input, & > select { + & > input:not(.applicableToAllUsers), & > select { width: 100%; } } @@ -101,6 +101,10 @@ background-color: rgba(134, 255, 110, 0.9); } +#externalStorage td.applicable label { + display: inline-flex; + align-items: center; +} #externalStorage td.applicable div.chzn-container { position: relative; diff --git a/apps/files_external/js/settings.js b/apps/files_external/js/settings.js index 5228b958b67c8..d3e9fd44c0d9b 100644 --- a/apps/files_external/js/settings.js +++ b/apps/files_external/js/settings.js @@ -24,6 +24,28 @@ function getSelection($row) { return values; } +function getSelectedApplicable($row) { + var users = []; + var groups = []; + var multiselect = getSelection($row); + $.each(multiselect, function(index, value) { + // FIXME: don't rely on string parts to detect groups... + var pos = (value.indexOf)?value.indexOf('(group)'): -1; + if (pos !== -1) { + groups.push(value.substr(0, pos)); + } else { + users.push(value); + } + }); + + // FIXME: this should be done in the multiselect change event instead + $row.find('.applicable') + .data('applicable-groups', groups) + .data('applicable-users', users); + + return { users, groups }; +} + function highlightBorder($element, highlight) { $element.toggleClass('warning-input', highlight); return highlight; @@ -56,7 +78,7 @@ function highlightInput($input) { * @param {Array} array of jQuery elements * @param {number} userListLimit page size for result list */ -function addSelect2 ($elements, userListLimit) { +function initApplicableUsersMultiselect($elements, userListLimit) { var escapeHTML = function (text) { return text.toString() .split('&').join('&') @@ -68,8 +90,8 @@ function addSelect2 ($elements, userListLimit) { if (!$elements.length) { return; } - $elements.select2({ - placeholder: t('files_external', 'All users. Type to select user or group.'), + return $elements.select2({ + placeholder: t('files_external', 'Type to select user or group.'), allowClear: true, multiple: true, toggleSelect: true, @@ -167,6 +189,8 @@ function addSelect2 ($elements, userListLimit) { $div.avatar($div.data('name'),32); } }); + }).on('change', function(event) { + highlightBorder($(event.target).closest('.applicableUsersContainer').find('.select2-choices'), !event.val.length); }); } @@ -721,6 +745,8 @@ MountConfigListView.prototype = _.extend({ this.$el.on('change', '.selectBackend', _.bind(this._onSelectBackend, this)); this.$el.on('change', '.selectAuthMechanism', _.bind(this._onSelectAuthMechanism, this)); + + this.$el.on('change', '.applicableToAllUsers', _.bind(this._onChangeApplicableToAllUsers, this)); }, _onChange: function(event) { @@ -746,6 +772,7 @@ MountConfigListView.prototype = _.extend({ var onCompletion = jQuery.Deferred(); $tr = this.newStorage(storageConfig, onCompletion); + $tr.find('.applicableToAllUsers').prop('checked', false).trigger('change'); onCompletion.resolve(); $tr.find('td.configuration').children().not('[type=hidden]').first().focus(); @@ -764,6 +791,19 @@ MountConfigListView.prototype = _.extend({ this.saveStorageConfig($tr); }, + _onChangeApplicableToAllUsers: function(event) { + var $target = $(event.target); + var $tr = $target.closest('tr'); + var checked = $target.is(':checked'); + + $tr.find('.applicableUsersContainer').toggleClass('hidden', checked); + if (!checked) { + $tr.find('.applicableUsers').select2('val', '', true); + } + + this.saveStorageConfig($tr); + }, + /** * Configure the storage config with a new authentication mechanism * @@ -818,7 +858,7 @@ MountConfigListView.prototype = _.extend({ $tr.removeAttr('id'); $tr.find('select#selectBackend'); if (!deferAppend) { - addSelect2($tr.find('.applicableUsers'), this._userListLimit); + initApplicableUsersMultiselect($tr.find('.applicableUsers'), this._userListLimit); } if (storageConfig.id) { @@ -890,7 +930,14 @@ MountConfigListView.prototype = _.extend({ }) ); } - $tr.find('.applicableUsers').val(applicable).trigger('change'); + if (applicable.length) { + $tr.find('.applicableUsers').val(applicable).trigger('change') + $tr.find('.applicableUsersContainer').removeClass('hidden'); + } else { + // applicable to all + $tr.find('.applicableUsersContainer').addClass('hidden'); + } + $tr.find('.applicableToAllUsers').prop('checked', !applicable.length); var priorityEl = $(''); $tr.append(priorityEl); @@ -967,7 +1014,7 @@ MountConfigListView.prototype = _.extend({ } $rows = $rows.add($tr); }); - addSelect2(self.$el.find('.applicableUsers'), this._userListLimit); + initApplicableUsersMultiselect(self.$el.find('.applicableUsers'), this._userListLimit); self.$el.find('tr#addMountPoint').before($rows); var mainForm = $('#files_external'); if (result.length === 0 && mainForm.attr('data-can-create') === 'false') { @@ -1007,7 +1054,7 @@ MountConfigListView.prototype = _.extend({ } $rows = $rows.add($tr); }); - addSelect2($rows.find('.applicableUsers'), this._userListLimit); + initApplicableUsersMultiselect($rows.find('.applicableUsers'), this._userListLimit); self.$el.find('tr#addMountPoint').before($rows); onCompletion.resolve(); onLoaded2.resolve(); @@ -1126,24 +1173,25 @@ MountConfigListView.prototype = _.extend({ // gather selected users and groups if (!this._isPersonal) { - var groups = []; - var users = []; - var multiselect = getSelection($tr); - $.each(multiselect, function(index, value) { - var pos = (value.indexOf)?value.indexOf('(group)'): -1; - if (pos !== -1) { - groups.push(value.substr(0, pos)); - } else { - users.push(value); - } - }); - // FIXME: this should be done in the multiselect change event instead - $tr.find('.applicable') - .data('applicable-groups', groups) - .data('applicable-users', users); + var multiselect = getSelectedApplicable($tr); + var users = multiselect.users || []; + var groups = multiselect.groups || []; + var isApplicableToAllUsers = $tr.find('.applicableToAllUsers').is(':checked'); + + if (isApplicableToAllUsers) { + storage.applicableUsers = []; + storage.applicableGroups = []; + } else { + storage.applicableUsers = users; + storage.applicableGroups = groups; - storage.applicableUsers = users; - storage.applicableGroups = groups; + if (!storage.applicableUsers.length && !storage.applicableGroups.length) { + if (!storage.errors) { + storage.errors = {}; + } + storage.errors['requiredApplicable'] = true; + } + } storage.priority = parseInt($tr.find('input.priority').val() || '100', 10); } diff --git a/apps/files_external/templates/settings.php b/apps/files_external/templates/settings.php index d8dd91822c5db..073ebe6766a85 100644 --- a/apps/files_external/templates/settings.php +++ b/apps/files_external/templates/settings.php @@ -169,7 +169,10 @@ function writeParameterInput($parameter, $options, $classes = []) { - + +
+ +
diff --git a/apps/files_external/tests/js/settingsSpec.js b/apps/files_external/tests/js/settingsSpec.js index 189979411112b..4032f6f6a37d4 100644 --- a/apps/files_external/tests/js/settingsSpec.js +++ b/apps/files_external/tests/js/settingsSpec.js @@ -35,12 +35,13 @@ describe('OCA.Files_External.Settings tests', function() { beforeEach(function() { clock = sinon.useFakeTimers(); + select2ApplicableUsers = []; select2Stub = sinon.stub($.fn, 'select2').callsFake(function(args) { if (args === 'val') { return select2ApplicableUsers; } return { - on: function() {} + on: function() { return this; } }; }); @@ -63,6 +64,7 @@ describe('OCA.Files_External.Settings tests', function() { '' + '' + '' + + '' + '' + '' + ''+ @@ -172,6 +174,7 @@ describe('OCA.Files_External.Settings tests', function() { function selectBackend(backendName) { view.$el.find('.selectBackend:first').val(backendName).trigger('change'); + view.$el.find('.applicableToAllUsers').prop('checked', true).trigger('change'); } beforeEach(function() { @@ -255,6 +258,59 @@ describe('OCA.Files_External.Settings tests', function() { // TODO: respond and check data-id }); + it('saves storage with applicable users', function() { + var $field1 = $tr.find('input[data-parameter=field1]'); + expect($field1.length).toEqual(1); + $field1.val('test'); + $field1.trigger(new $.Event('keyup', {keyCode: 97})); + + $tr.find('.applicableToAllUsers').prop('checked', false).trigger('change'); + select2ApplicableUsers = ['user1', 'user2', 'group1(group)', 'group2(group)']; + + var $saveButton = $tr.find('td.save .icon-checkmark'); + $saveButton.click(); + + expect(fakeServer.requests.length).toEqual(1); + var request = fakeServer.requests[0]; + expect(request.url).toEqual(OC.getRootPath() + '/index.php/apps/files_external/globalstorages'); + expect(JSON.parse(request.requestBody)).toEqual({ + backend: '\\OC\\TestBackend', + authMechanism: 'mechanism1', + backendOptions: { + 'field1': 'test', + 'field2': '' + }, + mountPoint: 'TestBackend', + priority: 11, + applicableUsers: ['user1', 'user2'], + applicableGroups: ['group1', 'group2'], + mountOptions: { + encrypt: true, + previews: true, + enable_sharing: false, + filesystem_check_changes: 1, + encoding_compatibility: false, + readonly: false, + }, + testOnly: true + }); + + // TODO: respond and check data-id + }); + it('does not saves storage without applicable users and unchecked all users checkbox', function() { + var $field1 = $tr.find('input[data-parameter=field1]'); + expect($field1.length).toEqual(1); + $field1.val('test'); + $field1.trigger(new $.Event('keyup', {keyCode: 97})); + + $tr.find('.applicableToAllUsers').prop('checked', false).trigger('change'); + + var $saveButton = $tr.find('td.save .icon-checkmark'); + $saveButton.click(); + + expect(fakeServer.requests.length).toEqual(0); + }); + it('saves storage after closing mount options popovermenu', function() { $tr.find('.mountOptionsToggle .icon-more').click(); $tr.find('[name=previews]').trigger(new $.Event('keyup', {keyCode: 97})); @@ -279,6 +335,16 @@ describe('OCA.Files_External.Settings tests', function() { }); it('lists missing fields in storage errors', function() { + $tr.find('.applicableToAllUsers').prop('checked', false).trigger('change'); + var storage = view.getStorageConfig($tr); + + expect(storage.errors).toEqual({ + backendOptions: ['field_text', 'field_password'], + requiredApplicable: true, + }); + }); + + it('does not list applicable when all users checkbox is ticked', function() { var storage = view.getStorageConfig($tr); expect(storage.errors).toEqual({ @@ -399,9 +465,6 @@ describe('OCA.Files_External.Settings tests', function() { }); }); }); - describe('applicable user list', function() { - // TODO: test select2 retrieval logic - }); describe('allow user mounts section', function() { // TODO: test allowUserMounting section });