From eaad244181c555423837058d58e6a39c2eadb111 Mon Sep 17 00:00:00 2001 From: Aleksey Date: Sat, 16 Nov 2019 20:36:27 +0300 Subject: [PATCH] Refactoring & Fix: editing group and challenges from user dashboard (#11418) * Refactoring & Feature: edit/delete group and challenge tasks - Remove showOption from tasks props - Pass all needed data to task for understand in mapGetter function which controls we should show - Improve current solution with edit and delete logic * Fix: this in template * Fix & Test: extend tests, fix can Edit/Delete functions * Fix: allow user edit challenge tasks on dashboard * Fix: test case after code change * fix import path * Fix: - Extend canEdit and canDelete functions with admin role - Clarify canEdit and canDelete conditions - Extend test cases --- .../components/challenges/challengeDetail.vue | 5 +- .../group-plans/taskInformation.vue | 4 - .../client/src/components/tasks/column.vue | 7 +- website/client/src/components/tasks/task.vue | 30 +++++++- website/client/src/store/getters/tasks.js | 75 +++++++++++++++++-- .../store/getters/tasks/canDelete.spec.js | 70 +++++++++++++++-- .../unit/store/getters/tasks/canEdit.spec.js | 75 +++++++++++++++++++ 7 files changed, 236 insertions(+), 30 deletions(-) create mode 100644 website/client/tests/unit/store/getters/tasks/canEdit.spec.js diff --git a/website/client/src/components/challenges/challengeDetail.vue b/website/client/src/components/challenges/challengeDetail.vue index de0962c386..d2cbc7b300 100644 --- a/website/client/src/components/challenges/challengeDetail.vue +++ b/website/client/src/components/challenges/challengeDetail.vue @@ -123,7 +123,7 @@ class="col-12 col-sm-6" :type="column" :task-list-override="tasksByType[column]" - :show-options="showOptions" + :challenge="challenge" @editTask="editTask" @taskDestroyed="taskDestroyed" /> @@ -386,9 +386,6 @@ export default { canJoin () { return !this.isMember; }, - showOptions () { - return this.isLeader; - }, }, mounted () { if (!this.searchId) this.searchId = this.challengeId; diff --git a/website/client/src/components/group-plans/taskInformation.vue b/website/client/src/components/group-plans/taskInformation.vue index d98baeedd6..f874a79507 100644 --- a/website/client/src/components/group-plans/taskInformation.vue +++ b/website/client/src/components/group-plans/taskInformation.vue @@ -75,7 +75,6 @@ class="col-12 col-md-3" :type="column" :task-list-override="tasksByType[column]" - :show-options="showOptions" :group="group" :search-text="searchText" @editTask="editTask" @@ -199,9 +198,6 @@ export default { return (this.group.leader && this.group.leader._id === this.user._id) || (this.group.managers && Boolean(this.group.managers[this.user._id])); }, - showOptions () { - return this.canCreateTasks; - }, }, watch: { // call again the method if the route changes (when this route is already active) diff --git a/website/client/src/components/tasks/column.vue b/website/client/src/components/tasks/column.vue index 11a5d0b993..a381877936 100644 --- a/website/client/src/components/tasks/column.vue +++ b/website/client/src/components/tasks/column.vue @@ -90,8 +90,8 @@ :key="task.id" :task="task" :is-user="isUser" - :show-options="showOptions" :group="group" + :challenge="challenge" @editTask="editTask" @moveTo="moveTo" @taskDestroyed="taskDestroyed" @@ -372,10 +372,7 @@ export default { selectedTags: {}, taskListOverride: {}, group: {}, - showOptions: { - type: Boolean, - default: true, - }, + challenge: {}, }, // @TODO: maybe we should store the group on state? data () { const icons = Object.freeze({ diff --git a/website/client/src/components/tasks/task.vue b/website/client/src/components/tasks/task.vue index 699f57b1e4..acb2df06e2 100644 --- a/website/client/src/components/tasks/task.vue +++ b/website/client/src/components/tasks/task.vue @@ -99,6 +99,7 @@
@@ -137,7 +138,7 @@
@@ -789,6 +790,7 @@ import moment from 'moment'; import axios from 'axios'; import Vue from 'vue'; import uuid from 'uuid'; +import isEmpty from 'lodash/isEmpty'; import { mapState, mapGetters, mapActions } from '@/libs/store'; import scoreTask from '@/../../common/script/ops/scoreTask'; import * as Analytics from '@/libs/analytics'; @@ -825,7 +827,7 @@ export default { markdown: markdownDirective, }, mixins: [notifications], - props: ['task', 'isUser', 'group', 'dueDate', 'showOptions'], // @TODO: maybe we should store the group on state? + props: ['task', 'isUser', 'group', 'challenge', 'dueDate'], // @TODO: maybe we should store the group on state? data () { return { random: uuid.v4(), // used to avoid conflicts between checkboxes ids @@ -859,6 +861,7 @@ export default { getTagsFor: 'tasks:getTagsFor', getTaskClasses: 'tasks:getTaskClasses', canDelete: 'tasks:canDelete', + canEdit: 'tasks:canEdit', }), hasChecklist () { return this.task.checklist && this.task.checklist.length > 0; @@ -937,6 +940,27 @@ export default { return this.task.challenge.shortName ? this.task.challenge.shortName.toString() : ''; }, + isChallangeTask () { + return !isEmpty(this.task.challenge); + }, + isGroupTask () { + return this.task.group.taskId || this.task.group.id; + }, + taskCategory () { + let taskCategory = 'default'; + if (this.isGroupTask) taskCategory = 'group'; + else if (this.isChallangeTask) taskCategory = 'challenge'; + return taskCategory; + }, + showDelete () { + return this.canDelete(this.task, this.taskCategory, this.isUser, this.group, this.challenge); + }, + showEdit () { + return this.canEdit(this.task, this.taskCategory, this.isUser, this.group, this.challenge); + }, + showOptions () { + return this.showEdit || this.showDelete || this.isUser; + }, }, methods: { ...mapActions({ @@ -950,7 +974,7 @@ export default { this.scoreChecklistItem({ taskId: this.task._id, itemId: item.id }); }, edit (e, task) { - if (this.isRunningYesterdailies) return; + if (this.isRunningYesterdailies || !this.showEdit) return; // Prevent clicking on a link from opening the edit modal const target = e.target || e.srcElement; diff --git a/website/client/src/store/getters/tasks.js b/website/client/src/store/getters/tasks.js index 42ccc0cc7a..a8e81ff545 100644 --- a/website/client/src/store/getters/tasks.js +++ b/website/client/src/store/getters/tasks.js @@ -34,12 +34,75 @@ function getTaskColor (task) { return 'best'; } -export function canDelete () { - return task => { - const isUserChallenge = Boolean(task.userId); - const activeChallenge = isUserChallenge - && task.challenge && task.challenge.id && !task.challenge.broken; - return !activeChallenge; +export function canDelete (store) { + return (task, taskCategory, onUserDashboard, group, challenge) => { + const user = store.state.user.data; + const userId = user.id || user._id; + + const isUserAdmin = user.contributor && !!user.contributor.admin; + const isUserGroupLeader = group && (group.leader + && group.leader._id === userId); + const isUserGroupManager = group && (group.managers + && Boolean(group.managers[userId])); + const isUserChallenge = userId === (challenge && challenge.leader.id); + + let isUserCanDeleteTask = onUserDashboard; + + switch (taskCategory) { + case 'challenge': + if (!onUserDashboard) { + isUserCanDeleteTask = isUserChallenge || isUserAdmin; + } else { + isUserCanDeleteTask = isUserAdmin; + } + break; + case 'group': + if (!onUserDashboard) { + isUserCanDeleteTask = isUserGroupLeader || isUserGroupManager || isUserAdmin; + } else { + isUserCanDeleteTask = isUserAdmin; + } + break; + default: + break; + } + return Boolean(isUserCanDeleteTask); + }; +} + +export function canEdit (store) { + return (task, taskCategory, onUserDashboard, group, challenge) => { + let isUserCanEditTask = onUserDashboard; + const user = store.state.user.data; + const userId = user.id || user._id; + + const isUserAdmin = user.contributor && !!user.contributor.admin; + const isUserGroupLeader = group && (group.leader + && group.leader._id === userId); + const isUserGroupManager = group && (group.managers + && Boolean(group.managers[userId])); + const isUserChallenge = userId === (challenge && challenge.leader.id); + + + switch (taskCategory) { + case 'challenge': + if (!onUserDashboard) { + isUserCanEditTask = isUserChallenge || isUserAdmin; + } else { + isUserCanEditTask = true; + } + break; + case 'group': + if (!onUserDashboard) { + isUserCanEditTask = isUserGroupLeader || isUserGroupManager || isUserAdmin; + } else { + isUserCanEditTask = true; + } + break; + default: + break; + } + return Boolean(isUserCanEditTask); }; } diff --git a/website/client/tests/unit/store/getters/tasks/canDelete.spec.js b/website/client/tests/unit/store/getters/tasks/canDelete.spec.js index 1dec648b4b..e9e9ef0add 100644 --- a/website/client/tests/unit/store/getters/tasks/canDelete.spec.js +++ b/website/client/tests/unit/store/getters/tasks/canDelete.spec.js @@ -1,19 +1,73 @@ import generateStore from '@/store'; + describe('canDelete getter', () => { - it('cannot delete active challenge task', () => { - const store = generateStore(); + let store; + let group; + let challenge; + let task; + beforeEach(() => { + store = generateStore(); - const task = { userId: 1, challenge: { id: 2 } }; - expect(store.getters['tasks:canDelete'](task)).to.equal(false); + store.state.user.data = { + id: 10, + contributor: { + admin: false, + }, + }; + + group = { + leader: { + _id: 123, + }, + managers: { + 123984: {}, + }, + }; + + challenge = { + leader: { + id: 123, + }, + }; + + task = { userId: 1, challenge: { id: 2 } }; + }); + it('cannot Delete challenge or group task in own dashboard', () => { + expect(store.getters['tasks:canDelete'](task, 'challenge', true, null, challenge)).to.equal(false); + expect(store.getters['tasks:canDelete'](task, 'group', true, group, null)).to.equal(false); }); - it('can delete broken challenge task', () => { - const store = generateStore(); + it('can Delete any challenge task as admin', () => { + store.state.user.data.contributor.admin = true; + expect(store.getters['tasks:canDelete'](task, 'challenge', true, null, challenge)).to.equal(true); + }); - const task = { userId: 1, challenge: { id: 2, broken: true } }; - expect(store.getters['tasks:canDelete'](task)).to.equal(true); + it('can Delete own challenge task if leader', () => { + store.state.user.data.id = 123; + + expect(store.getters['tasks:canDelete'](task, 'challenge', false, null, challenge)).to.equal(true); + }); + + it('cannot Delete challenge task if non leader on challenge page', () => { + expect(store.getters['tasks:canDelete'](task, 'challenge', false, null, challenge)).to.equal(false); + }); + + it('can Delete group task as leader on group page', () => { + store.state.user.data.id = 123; + + expect(store.getters['tasks:canDelete'](task, 'group', false, group)).to.equal(true); + }); + + it('can Delete group task if manager on group page', () => { + store.state.user.data.id = 123984; + + expect(store.getters['tasks:canDelete'](task, 'group', false, group)).to.equal(true); + }); + + it('cannot Delete group task if not a leader on group page', () => { + expect(store.getters['tasks:canDelete'](task, 'group', false, group)).to.equal(false); }); }); diff --git a/website/client/tests/unit/store/getters/tasks/canEdit.spec.js b/website/client/tests/unit/store/getters/tasks/canEdit.spec.js new file mode 100644 index 0000000000..56a143da56 --- /dev/null +++ b/website/client/tests/unit/store/getters/tasks/canEdit.spec.js @@ -0,0 +1,75 @@ +import generateStore from '@/store'; + + +describe('canEdit getter', () => { + let store; + let group; + let challenge; + let task; + + beforeEach(() => { + store = generateStore(); + + store.state.user.data = { + id: 10, + contributor: { + admin: false, + }, + }; + + group = { + leader: { + _id: 123, + }, + managers: { + 123984: {}, + }, + }; + + challenge = { + leader: { + id: 123, + }, + }; + + task = { userId: 1, challenge: { id: 2 } }; + }); + it('can Edit task in own dashboard', () => { + expect(store.getters['tasks:canEdit'](task, 'challenge', true, null, challenge)).to.equal(true); + expect(store.getters['tasks:canEdit'](task, 'group', true, group, null)).to.equal(true); + }); + + it('can Edit any challenge task if admin', () => { + store.state.user.data.contributor.admin = true; + + expect(store.getters['tasks:canEdit'](task, 'challenge', true, null, challenge)).to.equal(true); + expect(store.getters['tasks:canEdit'](task, 'challenge', false, null, challenge)).to.equal(true); + }); + + it('can Edit own challenge task if leader', () => { + store.state.user.data.id = 123; + + expect(store.getters['tasks:canEdit'](task, 'challenge', true, null, challenge)).to.equal(true); + expect(store.getters['tasks:canEdit'](task, 'challenge', false, null, challenge)).to.equal(true); + }); + + it('cannot Edit challenge task if not leader on challenge page', () => { + expect(store.getters['tasks:canEdit'](task, 'challenge', false, null, challenge)).to.equal(false); + }); + + it('can Edit group task as leader on group page', () => { + store.state.user.data.id = 123; + + expect(store.getters['tasks:canEdit'](task, 'group', false, group)).to.equal(true); + }); + + it('can Edit group task if manager on group page', () => { + store.state.user.data.id = 123984; + + expect(store.getters['tasks:canEdit'](task, 'group', false, group)).to.equal(true); + }); + + it('cannot Edit group task if not leader on group page', () => { + expect(store.getters['tasks:canEdit'](task, 'group', false, group)).to.equal(false); + }); +});