From a716a5529c04debc7d6dcfa5f4aacb26fbf6f66e Mon Sep 17 00:00:00 2001 From: Julian Lam Date: Mon, 3 Aug 2020 20:42:45 -0400 Subject: [PATCH] feat: more discrete commit-on-save instead of commit-on-change w/ confirm modals (#8541) * feat: privileges save button, #8537, WIP * fix: disable firefox autocomplete on privilege form fields * feat: closes #8537 privilege changes commit on save - new language strings for confirmation and success modals/toasts - indeterminate privilege handling (/cc @psychobunny) - added new discard button - both discard and save buttons now have confirmation dialogs * fix(tests): remove duplicate template helper test * fix(tests): broken template helper test * feat: confirm dialogs for all privilege copy actions Also, ability to add user to a privilege table without needing to refresh the privilege table. * feat: group row addition w/o table refresh breaking: helpers.getUserPrivileges and helpers.getGroupPrivileges no longer make socket calls to the following hooks: - filter:privileges.list, filter:privileges.admin.list, filter:privileges.global.list, filter:privileges.groups.list, filter:privileges.admin.groups.list, filter:privileges.gloval.groups.list The filters are still called, but done before the helper method is called, and the results are passed in instead. This change should only affect you if you directly call the helper methods, otherwise the change is transparent. * fix: stale ajaxify data on privilege category switch * fix: implicit privileges not showing for user privs * fix: groups, not group, also fix tests * fix(tests): again * fix: wrong tpl rendered when adding group to global priv table --- public/.eslintrc | 1 + .../en-GB/admin/manage/categories.json | 1 - .../en-GB/admin/manage/privileges.json | 14 +- public/less/admin/manage/privileges.less | 29 +++- public/openapi/read.yaml | 11 ++ public/src/admin/manage/privileges.js | 159 +++++++++++++----- public/src/modules/helpers.js | 2 +- src/privileges/admin.js | 12 +- src/privileges/categories.js | 10 +- src/privileges/global.js | 12 +- src/privileges/helpers.js | 6 +- src/views/admin/manage/privileges.tpl | 10 ++ .../admin/partials/privileges/category.tpl | 26 ++- .../admin/partials/privileges/global.tpl | 17 +- test/template-helpers.js | 12 +- 15 files changed, 213 insertions(+), 109 deletions(-) diff --git a/public/.eslintrc b/public/.eslintrc index 322f86a92d..2f938bb71a 100644 --- a/public/.eslintrc +++ b/public/.eslintrc @@ -27,6 +27,7 @@ "object-shorthand": "off", "prefer-arrow-callback": "off", "prefer-spread": "off", + "prefer-object-spread": "off", "prefer-reflect": "off", "prefer-template": "off" }, diff --git a/public/language/en-GB/admin/manage/categories.json b/public/language/en-GB/admin/manage/categories.json index 5d8923c6ac..0c0992fca5 100644 --- a/public/language/en-GB/admin/manage/categories.json +++ b/public/language/en-GB/admin/manage/categories.json @@ -66,7 +66,6 @@ "alert.create-success": "Category successfully created!", "alert.none-active": "You have no active categories.", "alert.create": "Create a Category", - "alert.confirm-moderate": "Are you sure you wish to grant the moderation privilege to this user group? This group is public, and any users can join at will.", "alert.confirm-purge": "

Do you really want to purge this category \"%1\"?

Warning! All topics and posts in this category will be purged!

Purging a category will remove all topics and posts, and delete the category from the database. If you want to remove a category temporarily, you'll want to \"disable\" the category instead.

", "alert.purge-success": "Category purged!", "alert.copy-success": "Settings Copied!", diff --git a/public/language/en-GB/admin/manage/privileges.json b/public/language/en-GB/admin/manage/privileges.json index b97188d214..7295c00103 100644 --- a/public/language/en-GB/admin/manage/privileges.json +++ b/public/language/en-GB/admin/manage/privileges.json @@ -1,6 +1,5 @@ { "global": "Global", - "global.no-users": "No user-specific global privileges.", "admin": "Admin", "group-privileges": "Group Privileges", "user-privileges": "User Privileges", @@ -38,5 +37,16 @@ "admin-categories": "Categories", "admin-privileges": "Privileges", "admin-users": "Users", - "admin-settings": "Settings" + "admin-settings": "Settings", + + "alert.confirm-moderate": "Are you sure you wish to grant the moderation privilege to this user group? This group is public, and any users can join at will.", + "alert.confirm-save": "Please confirm your intention to save these privileges", + "alert.saved": "Privilege changes saved and applied", + "alert.confirm-discard": "Are you sure you wish to discard your privilege changes?", + "alert.discarded": "Privilege changes discarded", + "alert.confirm-copyToAll": "Are you sure you wish to apply this privilege set to all categories?", + "alert.confirm-copyToAllGroup": "Are you sure you wish to apply this group's privilege set to all categories?", + "alert.confirm-copyToChildren": "Are you sure you wish to apply this privilege set to all descendant (child) categories?", + "alert.confirm-copyToChildrenGroup": "Are you sure you wish to apply this group's privilege set to all descendant (child) categories?", + "alert.no-undo": "This action cannot be undone." } \ No newline at end of file diff --git a/public/less/admin/manage/privileges.less b/public/less/admin/manage/privileges.less index 264d8bb632..e7ed543eea 100644 --- a/public/less/admin/manage/privileges.less +++ b/public/less/admin/manage/privileges.less @@ -1,11 +1,3 @@ -.page-manage-privileges { - .ui-autocomplete { - height: 500px; - overflow-y: auto; - overflow-x: hidden; - } -} - .page-admin-privileges { @keyframes fadeOut { 0% {background-color: @brand-primary;} @@ -16,6 +8,25 @@ animation-name: fadeOut; animation-duration: 5s; animation-fill-mode: both; - animation-timing-function: ease-out; + animation-timing-function: ease-out; + } + + .privilege-table { + td[data-delta="true"] > input { + &:after { + border-color: @state-success-text; + background-color: @state-success-text; + } + } + + td[data-delta="false"] > input { + &:after { + border-color: @state-danger-bg; + } + + &:indeterminate:after { + background-color: @state-danger-bg; + } + } } } \ No newline at end of file diff --git a/public/openapi/read.yaml b/public/openapi/read.yaml index 4b538ded4f..e9e801da7c 100644 --- a/public/openapi/read.yaml +++ b/public/openapi/read.yaml @@ -847,6 +847,17 @@ paths: properties: name: type: string + keys: + type: object + properties: + users: + type: array + items: + type: string + groups: + type: array + items: + type: string users: type: array items: diff --git a/public/src/admin/manage/privileges.js b/public/src/admin/manage/privileges.js index 8ce0030285..047701ce75 100644 --- a/public/src/admin/manage/privileges.js +++ b/public/src/admin/manage/privileges.js @@ -28,38 +28,72 @@ define('admin/manage/privileges', [ Privileges.setupPrivilegeTable = function () { $('.privilege-table-container').on('change', 'input[type="checkbox"]', function () { var checkboxEl = $(this); - var privilege = checkboxEl.parent().attr('data-privilege'); + var wrapperEl = checkboxEl.parent(); + var privilege = wrapperEl.attr('data-privilege'); var state = checkboxEl.prop('checked'); var rowEl = checkboxEl.parents('tr'); var member = rowEl.attr('data-group-name') || rowEl.attr('data-uid'); var isPrivate = parseInt(rowEl.attr('data-private') || 0, 10); var isGroup = rowEl.attr('data-group-name') !== undefined; + var delta = checkboxEl.prop('checked') === (wrapperEl.attr('data-value') === 'true') ? null : state; if (member) { if (isGroup && privilege === 'groups:moderate' && !isPrivate && state) { - bootbox.confirm('[[admin/manage/categories:alert.confirm-moderate]]', function (confirm) { + bootbox.confirm('[[admin/manage/privileges:alert.confirm-moderate]]', function (confirm) { if (confirm) { - Privileges.setPrivilege(member, privilege, state, checkboxEl); + wrapperEl.attr('data-delta', delta); + Privileges.exposeAssumedPrivileges(); } else { checkboxEl.prop('checked', !checkboxEl.prop('checked')); } }); } else { - Privileges.setPrivilege(member, privilege, state, checkboxEl); + wrapperEl.attr('data-delta', delta); + Privileges.exposeAssumedPrivileges(); } } else { app.alertError('[[error:invalid-data]]'); } }); + document.getElementById('save').addEventListener('click', function () { + bootbox.confirm('[[admin/manage/privileges:alert.confirm-save]]', function (ok) { + if (ok) { + var tableEl = document.querySelector('.privilege-table-container'); + var requests = tableEl.querySelectorAll('td[data-delta]').forEach(function (el) { + var privilege = el.getAttribute('data-privilege'); + var rowEl = el.parentNode; + var member = rowEl.getAttribute('data-group-name') || rowEl.getAttribute('data-uid'); + var state = el.getAttribute('data-delta') === 'true' ? 1 : 0; + + Privileges.setPrivilege(member, privilege, state); + }); + + $.when(requests).done(function () { + Privileges.refreshPrivilegeTable(); + app.alertSuccess('[[admin/manage/privileges:alert.saved]]'); + }); + } + }); + }); + + document.getElementById('discard').addEventListener('click', function () { + bootbox.confirm('[[admin/manage/privileges:alert.confirm-discard]]', function (ok) { + if (ok) { + Privileges.refreshPrivilegeTable(); + app.alertSuccess('[[admin/manage/privileges:alert.discarded]]'); + } + }); + }); + $('.privilege-table-container').on('click', '[data-action="search.user"]', Privileges.addUserToPrivilegeTable); $('.privilege-table-container').on('click', '[data-action="search.group"]', Privileges.addGroupToPrivilegeTable); $('.privilege-table-container').on('click', '[data-action="copyToChildren"]', function () { - Privileges.copyPrivilegesToChildren(cid, ''); + throwConfirmModal('copyToChildren', Privileges.copyPrivilegesToChildren.bind(null, cid, '')); }); $('.privilege-table-container').on('click', '[data-action="copyToChildrenGroup"]', function () { var groupName = $(this).parents('[data-group-name]').attr('data-group-name'); - Privileges.copyPrivilegesToChildren(cid, groupName); + throwConfirmModal('copyToChildrenGroup', Privileges.copyPrivilegesToChildren.bind(null, cid, groupName)); }); $('.privilege-table-container').on('click', '[data-action="copyPrivilegesFrom"]', function () { @@ -71,13 +105,21 @@ define('admin/manage/privileges', [ }); $('.privilege-table-container').on('click', '[data-action="copyToAll"]', function () { - Privileges.copyPrivilegesToAllCategories(cid, ''); + throwConfirmModal('copyToAll', Privileges.copyPrivilegesToAllCategories.bind(null, cid, '')); }); $('.privilege-table-container').on('click', '[data-action="copyToAllGroup"]', function () { var groupName = $(this).parents('[data-group-name]').attr('data-group-name'); - Privileges.copyPrivilegesToAllCategories(cid, groupName); + throwConfirmModal('copyToAllGroup', Privileges.copyPrivilegesToAllCategories.bind(null, cid, groupName)); }); + function throwConfirmModal(method, onConfirm) { + bootbox.confirm('[[admin/manage/privileges:alert.confirm-' + method + ']]

[[admin/manage/privileges:alert.no-undo]]', function (ok) { + if (ok) { + onConfirm.call(); + } + }); + } + Privileges.exposeAssumedPrivileges(); }; @@ -87,6 +129,7 @@ define('admin/manage/privileges', [ return app.alertError(err.message); } + ajaxify.data.privileges = privileges; var tpl = parseInt(cid, 10) ? 'admin/partials/privileges/category' : 'admin/partials/privileges/global'; Benchpress.parse(tpl, { privileges: privileges, @@ -113,8 +156,18 @@ define('admin/manage/privileges', [ privs.push(el.getAttribute('data-privilege')); } }); + + // Also apply to non-group privileges + privs = privs.concat(privs.map(function (priv) { + if (priv.startsWith('groups:')) { + return priv.slice(7); + } + + return false; + })).filter(Boolean); + for (var x = 0, numPrivs = privs.length; x < numPrivs; x += 1) { - var inputs = $('.privilege-table tr[data-group-name]:not([data-group-name="registered-users"],[data-group-name="guests"],[data-group-name="spiders"]) td[data-privilege="' + privs[x] + '"] input'); + var inputs = $('.privilege-table tr[data-group-name]:not([data-group-name="registered-users"],[data-group-name="guests"],[data-group-name="spiders"]) td[data-privilege="' + privs[x] + '"] input, .privilege-table tr[data-uid] td[data-privilege="' + privs[x] + '"] input'); inputs.each(function (idx, el) { if (!el.checked) { el.indeterminate = true; @@ -123,7 +176,9 @@ define('admin/manage/privileges', [ } }; - Privileges.setPrivilege = function (member, privilege, state, checkboxEl) { + Privileges.setPrivilege = function (member, privilege, state) { + var deferred = $.Deferred(); + socket.emit('admin.categories.setPrivilege', { cid: isNaN(cid) ? 0 : cid, privilege: privilege, @@ -131,12 +186,14 @@ define('admin/manage/privileges', [ member: member, }, function (err) { if (err) { + deferred.reject(err); return app.alertError(err.message); } - checkboxEl.replaceWith(''); - Privileges.refreshPrivilegeTable(); + deferred.resolve(); }); + + return deferred.promise(); }; Privileges.addUserToPrivilegeTable = function () { @@ -151,23 +208,29 @@ define('admin/manage/privileges', [ inputEl.focus(); autocomplete.user(inputEl, function (ev, ui) { - var defaultPrivileges; - if (ajaxify.data.url === '/admin/manage/privileges/admin') { - defaultPrivileges = ['admin:dashboard']; - } else { - defaultPrivileges = cid ? ['find', 'read', 'topics:read'] : ['chat']; - } - socket.emit('admin.categories.setPrivilege', { - cid: isNaN(cid) ? 0 : cid, - privilege: defaultPrivileges, - set: true, - member: ui.item.user.uid, - }, function (err) { - if (err) { - return app.alertError(err.message); - } - - Privileges.refreshPrivilegeTable(); + // Generate data for new row + var privilegeSet = ajaxify.data.privileges.keys.users.reduce(function (memo, cur) { + memo[cur] = false; + return memo; + }, {}); + + app.parseAndTranslate('admin/partials/privileges/' + (isNaN(cid) ? 'global' : 'category'), 'privileges.users', { + privileges: { + users: [ + { + picture: ui.item.user.picture, + username: ui.item.user.username, + uid: ui.item.user.uid, + 'icon:text': ui.item.user['icon:text'], + 'icon:bgColor': ui.item.user['icon:bgColor'], + privileges: privilegeSet, + }, + ], + }, + }, function (html) { + var tableEl = document.querySelectorAll('.privilege-table'); + var rows = tableEl[1].querySelectorAll('tbody tr'); + html.insertBefore(rows[rows.length - 1]); modal.modal('hide'); }); }); @@ -245,24 +308,28 @@ define('admin/manage/privileges', [ } function addGroupToCategory(group, cb) { - var defaultPrivileges; - if (ajaxify.data.url === '/admin/manage/privileges/admin') { - defaultPrivileges = ['groups:admin:dashboard']; - } else { - defaultPrivileges = cid ? ['groups:find', 'groups:read', 'groups:topics:read'] : ['groups:chat']; - } - - socket.emit('admin.categories.setPrivilege', { - cid: isNaN(cid) ? 0 : cid, - privilege: defaultPrivileges, - set: true, - member: group, - }, function (err) { - if (err) { - return app.alertError(err.message); - } + // Generate data for new row + var privilegeSet = ajaxify.data.privileges.keys.groups.reduce(function (memo, cur) { + memo[cur] = false; + return memo; + }, {}); + + app.parseAndTranslate('admin/partials/privileges/' + ((isNaN(cid) || cid === 0) ? 'global' : 'category'), 'privileges.groups', { + privileges: { + groups: [ + { + name: group, + nameEscaped: translator.escape(group), + privileges: privilegeSet, + }, + ], + }, + }, function (html) { + var tableEl = document.querySelector('.privilege-table'); + var rows = tableEl.querySelectorAll('tbody tr'); + html.insertBefore(rows[rows.length - 1]); + Privileges.exposeAssumedPrivileges(); - Privileges.refreshPrivilegeTable(group); if (typeof cb === 'function') { cb(); } diff --git a/public/src/modules/helpers.js b/public/src/modules/helpers.js index e0ed7aa264..f918f5a258 100644 --- a/public/src/modules/helpers.js +++ b/public/src/modules/helpers.js @@ -193,7 +193,7 @@ (member === 'spiders' && !spidersEnabled.includes(priv.name)) || (member === 'Global Moderators' && globalModDisabled.includes(priv.name)); - return ''; + return ''; }).join(''); } diff --git a/src/privileges/admin.js b/src/privileges/admin.js index 5c654b0a39..053a3cdc9b 100644 --- a/src/privileges/admin.js +++ b/src/privileges/admin.js @@ -117,11 +117,19 @@ module.exports = function (privileges) { groups: plugins.fireHook('filter:privileges.admin.groups.list_human', privileges.admin.privilegeLabels.slice()), }); } + + const keys = await utils.promiseParallel({ + users: plugins.fireHook('filter:privileges.admin.list', privileges.admin.userPrivilegeList.slice()), + groups: plugins.fireHook('filter:privileges.admin.groups.list', privileges.admin.groupPrivilegeList.slice()), + }); + const payload = await utils.promiseParallel({ labels: getLabels(), - users: helpers.getUserPrivileges(0, 'filter:privileges.admin.list', privileges.admin.userPrivilegeList), - groups: helpers.getGroupPrivileges(0, 'filter:privileges.admin.groups.list', privileges.admin.groupPrivilegeList), + users: helpers.getUserPrivileges(0, keys.users), + groups: helpers.getGroupPrivileges(0, keys.groups), }); + payload.keys = keys; + // This is a hack because I can't do {labels.users.length} to echo the count in templates.js payload.columnCount = payload.labels.users.length + 2; return payload; diff --git a/src/privileges/categories.js b/src/privileges/categories.js index 868ea065fc..d1c867e27e 100644 --- a/src/privileges/categories.js +++ b/src/privileges/categories.js @@ -22,11 +22,17 @@ module.exports = function (privileges) { }); } + const keys = await utils.promiseParallel({ + users: plugins.fireHook('filter:privileges.list', privileges.userPrivilegeList.slice()), + groups: plugins.fireHook('filter:privileges.groups.list', privileges.groupPrivilegeList.slice()), + }); + const payload = await utils.promiseParallel({ labels: getLabels(), - users: helpers.getUserPrivileges(cid, 'filter:privileges.list', privileges.userPrivilegeList), - groups: helpers.getGroupPrivileges(cid, 'filter:privileges.groups.list', privileges.groupPrivilegeList), + users: helpers.getUserPrivileges(cid, keys.users), + groups: helpers.getGroupPrivileges(cid, keys.groups), }); + payload.keys = keys; // This is a hack because I can't do {labels.users.length} to echo the count in templates.js payload.columnCountUser = payload.labels.users.length + 2; diff --git a/src/privileges/global.js b/src/privileges/global.js index 7b851e1c9a..7c1f3f7dcd 100644 --- a/src/privileges/global.js +++ b/src/privileges/global.js @@ -55,11 +55,19 @@ module.exports = function (privileges) { groups: plugins.fireHook('filter:privileges.global.groups.list_human', privileges.global.privilegeLabels.slice()), }); } + + const keys = await utils.promiseParallel({ + users: plugins.fireHook('filter:privileges.global.list', privileges.global.userPrivilegeList.slice()), + groups: plugins.fireHook('filter:privileges.global.groups.list', privileges.global.groupPrivilegeList.slice()), + }); + const payload = await utils.promiseParallel({ labels: getLabels(), - users: helpers.getUserPrivileges(0, 'filter:privileges.global.list', privileges.global.userPrivilegeList), - groups: helpers.getGroupPrivileges(0, 'filter:privileges.global.groups.list', privileges.global.groupPrivilegeList), + users: helpers.getUserPrivileges(0, keys.users), + groups: helpers.getGroupPrivileges(0, keys.groups), }); + payload.keys = keys; + // This is a hack because I can't do {labels.users.length} to echo the count in templates.js payload.columnCount = payload.labels.users.length + 2; return payload; diff --git a/src/privileges/helpers.js b/src/privileges/helpers.js index 4c2b2289f3..93e189f6a9 100644 --- a/src/privileges/helpers.js +++ b/src/privileges/helpers.js @@ -88,8 +88,7 @@ async function isSystemGroupAllowedToPrivileges(privileges, uid, cid) { return await groups.isMemberOfGroups(uidToSystemGroup[uid], groupKeys); } -helpers.getUserPrivileges = async function (cid, hookName, userPrivilegeList) { - const userPrivileges = await plugins.fireHook(hookName, userPrivilegeList.slice()); +helpers.getUserPrivileges = async function (cid, userPrivileges) { let memberSets = await groups.getMembersOfGroups(userPrivileges.map(privilege => 'cid:' + cid + ':privileges:' + privilege)); memberSets = memberSets.map(function (set) { return set.map(uid => parseInt(uid, 10)); @@ -108,8 +107,7 @@ helpers.getUserPrivileges = async function (cid, hookName, userPrivilegeList) { return memberData; }; -helpers.getGroupPrivileges = async function (cid, hookName, groupPrivilegeList) { - const groupPrivileges = await plugins.fireHook(hookName, groupPrivilegeList.slice()); +helpers.getGroupPrivileges = async function (cid, groupPrivileges) { const [memberSets, allGroupNames] = await Promise.all([ groups.getMembersOfGroups(groupPrivileges.map(privilege => 'cid:' + cid + ':privileges:' + privilege)), groups.getGroups('groups:createtime', 0, -1), diff --git a/src/views/admin/manage/privileges.tpl b/src/views/admin/manage/privileges.tpl index 9930a240cb..a7c2111b63 100644 --- a/src/views/admin/manage/privileges.tpl +++ b/src/views/admin/manage/privileges.tpl @@ -20,3 +20,13 @@ + +
+ + + +
\ No newline at end of file diff --git a/src/views/admin/partials/privileges/category.tpl b/src/views/admin/partials/privileges/category.tpl index 1e798e4e5e..99150e05b9 100644 --- a/src/views/admin/partials/privileges/category.tpl +++ b/src/views/admin/partials/privileges/category.tpl @@ -52,16 +52,20 @@
- - - -
@@ -101,7 +105,6 @@ - @@ -117,20 +120,11 @@ - - - - - - - [[admin/manage/categories:privileges.no-users]] - - diff --git a/src/views/admin/partials/privileges/global.tpl b/src/views/admin/partials/privileges/global.tpl index 314beeebe7..ad36cdca16 100644 --- a/src/views/admin/partials/privileges/global.tpl +++ b/src/views/admin/partials/privileges/global.tpl @@ -24,7 +24,8 @@
-
@@ -50,7 +51,6 @@ - @@ -66,20 +66,11 @@ - - - - - [[admin/manage/privileges:global.no-users]] - - - - diff --git a/test/template-helpers.js b/test/template-helpers.js index ada455ef4f..fb1044bf3b 100644 --- a/test/template-helpers.js +++ b/test/template-helpers.js @@ -148,17 +148,7 @@ describe('helpers', function () { read: true, }; var html = helpers.spawnPrivilegeStates('guests', privs); - assert.equal(html, ''); - done(); - }); - - it('should spawn privilege states', function (done) { - var privs = { - find: true, - read: true, - }; - var html = helpers.spawnPrivilegeStates('guests', privs); - assert.equal(html, ''); + assert.equal(html, ''); done(); });