From a76e6b249d4597f6b3fbf8c33772c09e27211af4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Mon, 2 May 2016 15:30:37 +0200 Subject: [PATCH 1/2] Fix invalid links to work package actions --- .../wp-context-menu.service.html | 2 +- frontend/app/work_packages/helpers/index.js | 2 +- .../work-package-context-menu-helper.js | 23 +++-- .../work_packages/table/context_menu_spec.rb | 86 +++++++++++++++++++ .../components/work_packages_context_menu.rb | 64 ++++++++++++++ 5 files changed, 169 insertions(+), 8 deletions(-) create mode 100644 spec/features/work_packages/table/context_menu_spec.rb create mode 100644 spec/support/components/work_packages_context_menu.rb diff --git a/frontend/app/components/context-menus/wp-context-menu/wp-context-menu.service.html b/frontend/app/components/context-menus/wp-context-menu/wp-context-menu.service.html index b091b19cc2..9fc991f10a 100644 --- a/frontend/app/components/context-menus/wp-context-menu/wp-context-menu.service.html +++ b/frontend/app/components/context-menus/wp-context-menu/wp-context-menu.service.html @@ -22,7 +22,7 @@ class="{{action.icon}}"> - + diff --git a/frontend/app/work_packages/helpers/index.js b/frontend/app/work_packages/helpers/index.js index 8b9b743717..2dfe6acf9c 100644 --- a/frontend/app/work_packages/helpers/index.js +++ b/frontend/app/work_packages/helpers/index.js @@ -50,7 +50,7 @@ angular.module('openproject.workPackages.helpers') } ]) .service('WorkPackageContextMenuHelper', ['PERMITTED_BULK_ACTIONS', - 'WorkPackagesTableService', 'UrlParamsHelper', require( + 'WorkPackagesTableService', 'HookService', 'UrlParamsHelper', require( './work-package-context-menu-helper') ]) .factory('WorkPackagesHelper', ['TimezoneService', 'currencyFilter', diff --git a/frontend/app/work_packages/helpers/work-package-context-menu-helper.js b/frontend/app/work_packages/helpers/work-package-context-menu-helper.js index c6a8d24873..d2d93fb477 100644 --- a/frontend/app/work_packages/helpers/work-package-context-menu-helper.js +++ b/frontend/app/work_packages/helpers/work-package-context-menu-helper.js @@ -26,17 +26,19 @@ // See doc/COPYRIGHT.rdoc for more details. //++ -module.exports = function(PERMITTED_BULK_ACTIONS, WorkPackagesTableService, UrlParamsHelper) { +module.exports = function(PERMITTED_BULK_ACTIONS, WorkPackagesTableService, HookService, UrlParamsHelper) { function getPermittedActionLinks(workPackage, permittedActionConstants) { var singularPermittedActions = []; - var allowedActions = getAllowedActions(workPackage.$links, permittedActionConstants); + var allowedActions = getAllowedActions(workPackage, permittedActionConstants); angular.forEach(allowedActions, function(allowedAction) { singularPermittedActions.push({ icon: allowedAction.icon, + text: allowedAction.text, link: workPackage - .$links[allowedAction.link] + .$source + ._links[allowedAction.link] .href }); }); @@ -49,12 +51,13 @@ module.exports = function(PERMITTED_BULK_ACTIONS, WorkPackagesTableService, UrlP var permittedActions = _.filter(PERMITTED_BULK_ACTIONS, function(action) { return _.every(workPackages, function(workPackage) { - return getAllowedActions(workPackage.$links, [action]).length === 1; + return getAllowedActions(workPackage, [action]).length >= 1; }); }); angular.forEach(permittedActions, function(permittedAction) { bulkPermittedActions.push({ icon: permittedAction.icon, + text: permittedAction.text, link: getBulkActionLink(permittedAction, workPackages) }); @@ -80,15 +83,23 @@ module.exports = function(PERMITTED_BULK_ACTIONS, WorkPackagesTableService, UrlP return link + '?' + queryParts.join('&'); } - function getAllowedActions(links, actions) { + function getAllowedActions(workPackage, actions) { var allowedActions = []; angular.forEach(actions, function(action) { - if (links.hasOwnProperty(action.link)) { + if (workPackage.$links.hasOwnProperty(action.link)) { + action.text = I18n.t('js.button_' + action.icon); allowedActions.push(action); } }); + angular.forEach(HookService.call('workPackageTableContextMenu'), function(action) { + if (workPackage.$links.hasOwnProperty(action.link)) { + var index = action.indexBy ? action.indexBy(allowedActions) : allowedActions.length; + allowedActions.splice(index, 0, action) + } + }); + return allowedActions; } diff --git a/spec/features/work_packages/table/context_menu_spec.rb b/spec/features/work_packages/table/context_menu_spec.rb new file mode 100644 index 0000000000..3328938e29 --- /dev/null +++ b/spec/features/work_packages/table/context_menu_spec.rb @@ -0,0 +1,86 @@ +require 'spec_helper' + +describe 'Work package table context menu', js: true do + let(:user) { FactoryGirl.create(:admin) } + let(:work_package) { FactoryGirl.create(:work_package) } + + let(:wp_table) { Pages::WorkPackagesTable.new } + let(:menu) { Components::WorkPackagesContextMenu.new } + + def goto_context_menu + # Go to table + wp_table.visit! + wp_table.expect_work_package_listed(work_package) + + # Open context menu + menu.expect_closed + menu.open_for(work_package) + end + + before do + login_as(user) + work_package + end + + it 'provides a context menu for a single work package' do + # Open detail pane + goto_context_menu + menu.choose('Open details view') + expect(page).to have_selector('#work-package-subject', text: work_package.subject) + + # Open full view + goto_context_menu + menu.choose('Open fullscreen view') + expect(page).to have_selector('.work-packages--show-view #work-package-subject', + text: work_package.subject) + + # Open edit link + goto_context_menu + menu.choose('Edit') + expect(page).to have_selector('#inplace-edit--write-value--subject') + find('#work-packages--edit-actions-cancel').click + expect(page).to have_no_selector('#inplace-edit--write-value--subject') + + # Open log time + goto_context_menu + menu.choose('Log time') + expect(page).to have_selector('h2', text: I18n.t(:label_spent_time)) + + # Open Move + goto_context_menu + menu.choose('Move') + expect(page).to have_selector('h2', text: I18n.t(:button_move)) + expect(page).to have_selector('a.issue', text: "##{work_package.id}") + + # Open Copy + goto_context_menu + menu.choose('Copy') + expect(page).to have_selector('h2', text: I18n.t(:button_copy)) + expect(page).to have_selector('a.issue', text: "##{work_package.id}") + + # Open Delete + goto_context_menu + menu.choose('Delete') + wp_table.dismiss_alert_dialog! + end + + context 'multiple selected' do + let!(:work_package2) { FactoryGirl.create(:work_package) } + + before do + # Go to table + wp_table.visit! + wp_table.expect_work_package_listed(work_package) + wp_table.expect_work_package_listed(work_package2) + + # Select both + all('td.checkbox input').each { |el| el.set(true) } + end + + it 'shows a subset of the available menu items' do + menu.open_for(work_package) + menu.expect_options ['Open details view', 'Open fullscreen view', + 'Edit', 'Copy', 'Move', 'Delete'] + end + end +end diff --git a/spec/support/components/work_packages_context_menu.rb b/spec/support/components/work_packages_context_menu.rb new file mode 100644 index 0000000000..d1d72a7652 --- /dev/null +++ b/spec/support/components/work_packages_context_menu.rb @@ -0,0 +1,64 @@ +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2015 the OpenProject Foundation (OPF) +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# 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 General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See doc/COPYRIGHT.rdoc for more details. +#++ + +module Components + class WorkPackagesContextMenu + include Capybara::DSL + include RSpec::Matchers + + def open_for(work_package) + find("#work-package-#{work_package.id}").right_click + expect_open + end + + def expect_open + expect(page).to have_selector(selector) + end + + def expect_closed + expect(page).to have_no_selector(selector) + end + + def choose(target) + find("#{selector} a", text: target).click + end + + def expect_options(options) + expect_open + options.each do |text| + expect(page).to have_selector("#{selector} a", text: text) + end + end + + private + + def selector + '#work-package-context-menu' + end + end +end From 5f942ee1db632805da3a1172f2bbb216ef1007f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Tue, 3 May 2016 08:17:54 +0200 Subject: [PATCH 2/2] Move wp-context-menu-helper --- .../wp-context-menu.service.test.ts | 13 +++---- .../wp-context-menu-helper.service.js} | 35 +++++++++++-------- frontend/app/work_packages/helpers/index.js | 4 --- .../work-package-context-menu-helper-test.js | 22 ++++++------ 4 files changed, 39 insertions(+), 35 deletions(-) rename frontend/app/{work_packages/helpers/work-package-context-menu-helper.js => components/wp-table/context-menu-helper/wp-context-menu-helper.service.js} (79%) diff --git a/frontend/app/components/context-menus/wp-context-menu/wp-context-menu.service.test.ts b/frontend/app/components/context-menus/wp-context-menu/wp-context-menu.service.test.ts index e1980528c3..897ab7f48a 100644 --- a/frontend/app/components/context-menus/wp-context-menu/wp-context-menu.service.test.ts +++ b/frontend/app/components/context-menus/wp-context-menu/wp-context-menu.service.test.ts @@ -83,9 +83,10 @@ describe('workPackageContextMenu', () => { update: '/work_packages/123/edit', move: '/work_packages/move/new?ids%5B%5D=123', }; - var workPackage = Factory.build('PlanningElement', { - $links: actionLinks - }); + var workPackage = Factory.build('PlanningElement'); + workPackage.$source = { _links: actionLinks }; + workPackage.$links = actionLinks; + var directListElements; beforeEach(angular.mock.inject((_I18n_) => { @@ -125,9 +126,9 @@ describe('workPackageContextMenu', () => { var actionLinks = { 'delete': '/work_packages/bulk', }; - var workPackage = Factory.build('PlanningElement', { - $links: actionLinks - }); + var workPackage = Factory.build('PlanningElement'); + workPackage.$source = { _links: actionLinks }; + workPackage.$links = actionLinks; beforeEach(() => { $rootScope.rows = []; diff --git a/frontend/app/work_packages/helpers/work-package-context-menu-helper.js b/frontend/app/components/wp-table/context-menu-helper/wp-context-menu-helper.service.js similarity index 79% rename from frontend/app/work_packages/helpers/work-package-context-menu-helper.js rename to frontend/app/components/wp-table/context-menu-helper/wp-context-menu-helper.service.js index d2d93fb477..bb0844e02c 100644 --- a/frontend/app/work_packages/helpers/work-package-context-menu-helper.js +++ b/frontend/app/components/wp-table/context-menu-helper/wp-context-menu-helper.service.js @@ -26,7 +26,12 @@ // See doc/COPYRIGHT.rdoc for more details. //++ -module.exports = function(PERMITTED_BULK_ACTIONS, WorkPackagesTableService, HookService, UrlParamsHelper) { +angular + .module('openproject.workPackages.helpers') + .factory('WorkPackageContextMenuHelper', WorkPackageContextMenuHelper); + +function WorkPackageContextMenuHelper(PERMITTED_BULK_ACTIONS, WorkPackagesTableService, HookService, UrlParamsHelper) { + function getPermittedActionLinks(workPackage, permittedActionConstants) { var singularPermittedActions = []; @@ -34,13 +39,13 @@ module.exports = function(PERMITTED_BULK_ACTIONS, WorkPackagesTableService, Hook angular.forEach(allowedActions, function(allowedAction) { singularPermittedActions.push({ - icon: allowedAction.icon, - text: allowedAction.text, - link: workPackage - .$source - ._links[allowedAction.link] - .href - }); + icon: allowedAction.icon, + text: allowedAction.text, + link: workPackage + .$source + ._links[allowedAction.link] + .href + }); }); return singularPermittedActions; @@ -56,11 +61,11 @@ module.exports = function(PERMITTED_BULK_ACTIONS, WorkPackagesTableService, Hook }); angular.forEach(permittedActions, function(permittedAction) { bulkPermittedActions.push({ - icon: permittedAction.icon, - text: permittedAction.text, - link: getBulkActionLink(permittedAction, - workPackages) - }); + icon: permittedAction.icon, + text: permittedAction.text, + link: getBulkActionLink(permittedAction, + workPackages) + }); }); return bulkPermittedActions; @@ -104,7 +109,7 @@ module.exports = function(PERMITTED_BULK_ACTIONS, WorkPackagesTableService, Hook } var WorkPackageContextMenuHelper = { - getPermittedActions: function(workPackages, permittedActionConstants) { + getPermittedActions: function (workPackages, permittedActionConstants) { if (workPackages.length === 1) { return getPermittedActionLinks(workPackages[0], permittedActionConstants); } else if (workPackages.length > 1) { @@ -114,4 +119,4 @@ module.exports = function(PERMITTED_BULK_ACTIONS, WorkPackagesTableService, Hook }; return WorkPackageContextMenuHelper; -}; +} diff --git a/frontend/app/work_packages/helpers/index.js b/frontend/app/work_packages/helpers/index.js index 2dfe6acf9c..2420401e9f 100644 --- a/frontend/app/work_packages/helpers/index.js +++ b/frontend/app/work_packages/helpers/index.js @@ -49,10 +49,6 @@ angular.module('openproject.workPackages.helpers') link: 'delete' } ]) - .service('WorkPackageContextMenuHelper', ['PERMITTED_BULK_ACTIONS', - 'WorkPackagesTableService', 'HookService', 'UrlParamsHelper', require( - './work-package-context-menu-helper') - ]) .factory('WorkPackagesHelper', ['TimezoneService', 'currencyFilter', 'CustomFieldHelper', require('./work-packages-helper') ]) diff --git a/frontend/tests/unit/tests/work_packages/helpers/work-package-context-menu-helper-test.js b/frontend/tests/unit/tests/work_packages/helpers/work-package-context-menu-helper-test.js index 2f15292d25..cafe2de9a2 100644 --- a/frontend/tests/unit/tests/work_packages/helpers/work-package-context-menu-helper-test.js +++ b/frontend/tests/unit/tests/work_packages/helpers/work-package-context-menu-helper-test.js @@ -102,9 +102,9 @@ describe('WorkPackageContextMenuHelper', function() { } }; - var workPackage = Factory.build('PlanningElement', { - $links: actionLinks - }); + var workPackage = Factory.build('PlanningElement'); + workPackage.$source = { _links : actionLinks }; + workPackage.$links = actionLinks; describe('when an array with a single work package is passed as an argument', function() { var workPackages = new Array(workPackage); @@ -126,13 +126,15 @@ describe('WorkPackageContextMenuHelper', function() { }); describe('when more than one work package is passed as an argument', function() { - var anotherWorkPackage = Factory.build('PlanningElement', { - $links: { - update: { - href: '/work_packages/234/edit' - } - } - }); + var anotherWorkPackage = Factory.build('PlanningElement'); + anotherWorkPackage.$source = { + _links: { + update: { + href: '/work_packages/234/edit' + } + } + }; + anotherWorkPackage.$links = { update: '/work_packages/234/edit' }; var workPackages = [anotherWorkPackage, workPackage]; beforeEach(inject(function(_WorkPackagesTableService_) {