Merge pull request #7311 from opf/fix/30231/allow-edit-work-packages-to-save-order

[30231] Allow users edit_work_packages permission to save query
pull/7324/head
ulferts 6 years ago committed by GitHub
commit 7737dc66be
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 11
      app/contracts/queries/base_contract.rb
  2. 45
      app/contracts/queries/update_contract.rb
  3. 38
      app/contracts/queries/update_form_contract.rb
  4. 25
      app/policies/query_policy.rb
  5. 4
      frontend/src/app/modules/boards/board/board-list/board-list.component.html
  6. 20
      frontend/src/app/modules/boards/board/board-list/board-list.component.ts
  7. 2
      frontend/src/app/modules/boards/board/board-list/board-lists.service.ts
  8. 2
      frontend/src/app/modules/boards/drag-and-drop/reorder-query.service.ts
  9. 10
      lib/api/v3/queries/query_representer.rb
  10. 2
      lib/api/v3/queries/update_form_api.rb
  11. 2
      modules/boards/config/locales/en.yml
  12. 29
      modules/boards/spec/factories/board_factory.rb
  13. 53
      modules/boards/spec/features/board_management_spec.rb
  14. 4
      modules/boards/spec/features/support/board_page.rb
  15. 140
      spec/contracts/queries/update_contract_spec.rb
  16. 2
      spec/features/work_packages/table/hierarchy/hierarchy_spec.rb
  17. 45
      spec/lib/api/v3/queries/query_representer_generation_spec.rb
  18. 69
      spec/policies/query_policy_spec.rb
  19. 2
      spec/services/queries/update_query_service_spec.rb
  20. 2
      spec/services/shared_type_service.rb

@ -68,7 +68,6 @@ module Queries
def validate
validate_project
user_allowed_to_make_public
super
end
@ -80,16 +79,18 @@ module Queries
Project.visible(user).where(id: project_id).exists?
end
def may_not_manage_queries?
!user.allowed_to?(:manage_public_queries, model.project, global: model.project.nil?)
end
def user_allowed_to_make_public
# Add error only when changing public flag
return unless model.is_public_changed?
return if model.project_id.present? && model.project.nil?
if is_public && may_not_manage_queries?
errors.add :public, :error_unauthorized
end
end
def may_not_manage_queries?
!user.allowed_to?(:manage_public_queries, model.project, global: model.project.nil?)
end
end
end

@ -31,5 +31,50 @@ require 'queries/base_contract'
module Queries
class UpdateContract < BaseContract
def validate
user_allowed_to_change
super
end
##
# Check if the current user may save the changes
def user_allowed_to_change
# Allow editions of work package order with edit_work_packages
return if only_order_changed? && user_allowed_to_edit_work_packages?
# Check user self-saving their own queries
# or user saving public queries
if model.is_public?
user_allowed_to_change_public
else
user_allowed_to_change_query
end
end
def user_allowed_to_change_query
unless (model.user == user || model.user.nil?) && user_allowed_to_save_queries?
errors.add :base, :error_unauthorized
end
end
def user_allowed_to_change_public
if may_not_manage_queries?
errors.add :base, :error_unauthorized
end
end
def user_allowed_to_edit_work_packages?
user.allowed_to?(:edit_work_packages, model.project, global: model.project.nil?)
end
def user_allowed_to_save_queries?
user.allowed_to?(:save_queries, model.project, global: model.project.nil?)
end
##
# Determine if only the ordered work packages were set
def only_order_changed?
model.changed == %w[ordered_work_packages]
end
end
end

@ -0,0 +1,38 @@
#-- encoding: UTF-8
#-- copyright
# OpenProject is a project management system.
# Copyright (C) 2012-2018 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-2017 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 docs/COPYRIGHT.rdoc for more details.
#++
require 'queries/base_contract'
module Queries
class UpdateFormContract < BaseContract
# Maintains validations from the base contract
# to ensure users without saving permissions can still
# alter existing queries through the form
end
end

@ -40,7 +40,8 @@ class QueryPolicy < BasePolicy
publicize: publicize_allowed?(cached_query),
depublicize: depublicize_allowed?(cached_query),
star: persisted_and_own_or_public?(cached_query),
unstar: persisted_and_own_or_public?(cached_query)
unstar: persisted_and_own_or_public?(cached_query),
reorder_work_packages: reorder_work_packages?(cached_query)
}
end
@ -49,8 +50,10 @@ class QueryPolicy < BasePolicy
def persisted_and_own_or_public?(query)
query.persisted? &&
(save_queries_allowed?(query) && query.user == user ||
manage_public_queries_allowed?(query) && query.is_public)
(
(save_queries_allowed?(query) && query.user == user) ||
public_manageable_query?(query)
)
end
def viewable?(query)
@ -73,10 +76,18 @@ class QueryPolicy < BasePolicy
end
def depublicize_allowed?(query)
public_manageable_query?(query)
end
def public_manageable_query?(query)
query.is_public &&
manage_public_queries_allowed?(query)
end
def reorder_work_packages?(query)
persisted_and_own_or_public?(query) || edit_work_packages_allowed?(query)
end
def view_work_packages_allowed?(query)
@view_work_packages_cache ||= Hash.new do |hash, project|
hash[project] = user.allowed_to?(:view_work_packages, project, global: project.nil?)
@ -85,6 +96,14 @@ class QueryPolicy < BasePolicy
@view_work_packages_cache[query.project]
end
def edit_work_packages_allowed?(query)
@edit_work_packages_cache ||= Hash.new do |hash, project|
hash[project] = user.allowed_to?(:edit_work_packages, project, global: project.nil?)
end
@edit_work_packages_cache[query.project]
end
def save_queries_allowed?(query)
@save_queries_cache ||= Hash.new do |hash, project|
hash[project] = user.allowed_to?(:save_queries, project, global: project.nil?)

@ -41,9 +41,9 @@
<op-icon icon-classes="icon-small icon-add"></op-icon>
</button>
<wp-card-view [dragAndDropEnabled]="dragAndDropEnabled"
<wp-card-view [dragAndDropEnabled]="canEdit"
[workPackageAddedHandler]="workPackageAddedHandler"
[cardsRemovable]="board.isFree && canManage"
[cardsRemovable]="board.isFree && canEdit"
[highlightingMode]="board.highlightingMode"
[showStatusButton]="showCardStatusButton()">
</wp-card-view>

@ -89,8 +89,8 @@ export class BoardListComponent extends AbstractWidgetComponent implements OnIni
click_to_remove: this.I18n.t('js.boards.click_to_remove_list')
};
/** Are we allowed to drag & drop elements ? */
public dragAndDropEnabled:boolean = false;
/** Are we allowed to remove and drag & drop elements ? */
public canEdit:boolean = false;
/** Initially focus the list */
public initiallyFocused:boolean = false;
@ -128,7 +128,7 @@ export class BoardListComponent extends AbstractWidgetComponent implements OnIni
this.authorisationService
.observeUntil(componentDestroyed(this))
.subscribe(() => {
this.showAddButton = this.canManage && (this.wpInlineCreate.canAdd || this.canReference);
this.showAddButton = this.canEdit && (this.wpInlineCreate.canAdd || this.canReference);
});
this.querySpace.query
@ -136,19 +136,11 @@ export class BoardListComponent extends AbstractWidgetComponent implements OnIni
.pipe(
untilComponentDestroyed(this)
)
.subscribe((query) => this.query = query);
this.boardCache
.state(boardId.toString())
.values$()
.pipe(
untilComponentDestroyed(this)
)
.subscribe((board) => {
this.dragAndDropEnabled = board.editable;
.subscribe((query) => {
this.query = query;
this.canEdit = !!this.query.updateOrderedWorkPackages;
});
this.updateQuery();
}

@ -31,7 +31,7 @@ export class BoardListsService {
filters: filterJson},
undefined,
this.CurrentProject.identifier,
this.buildQueryRequest(params)
this.buildQueryRequest(params),
)
.then(form => {
const query = this.QueryFormDm.buildQueryResource(form);

@ -53,7 +53,7 @@ export class ReorderQueryService {
}
public saveOrderInQuery(query:QueryResource|undefined, orderedIds:string[]):Observable<unknown> {
if (query && !!query.updateImmediately) {
if (query && !!query.updateOrderedWorkPackages) {
const orderedWorkPackages = orderedIds
.map(id => this.pathHelper.api.v3.work_packages.id(id).toString());

@ -124,6 +124,16 @@ module API
}
end
link :updateOrderedWorkPackages do
next unless represented.new_record? && allowed_to?(:create) ||
represented.persisted? && allowed_to?(:reorder_work_packages)
{
href: api_v3_paths.query(represented.id),
method: :patch
}
end
link :delete do
next if represented.new_record? ||
!allowed_to?(:destroy)

@ -45,7 +45,7 @@ module API
# Permissions are enforced nevertheless.
@query.valid_subset!
create_or_update_query_form @query, ::Queries::UpdateContract, UpdateFormRepresenter
create_or_update_query_form @query, ::Queries::UpdateFormContract, UpdateFormRepresenter
end
end
end

@ -1,6 +1,6 @@
# English strings go here
en:
permission_view_boards: "View boards"
permission_show_board_views: "View boards"
permission_manage_board_views: "Manage boards"
project_module_board_view: "Boards"

@ -30,7 +30,34 @@ FactoryBot.define do
end_row: 2,
start_column: 1,
end_column: 1,
options: { query_id: query.id })
options: { 'query_id' => query.id, "filters"=>[{"manualSort"=>{"operator"=>"ow", "values"=>[]}}]})
end
end
factory :board_grid_with_queries, class: Boards::Grid do
project
sequence(:name) { |n| "Board with query #{n}" }
row_count { 1 }
column_count { 4 }
transient do
num_queries { 2 }
end
callback(:after_build) do |board, evaluator| # this is also done after :create
evaluator.num_queries.times do |i|
query = Query.new_default(name: "List #{i + 1}", is_public: true, project: board.project).tap do |q|
q.save!
end
board.widgets << FactoryBot.create(:grid_widget,
start_row: 1,
end_row: 2,
start_column: 1,
end_column: 1,
options: { 'query_id' => query.id, "filters"=>[{"manualSort"=>{"operator"=>"ow", "values"=>[]}}]})
end
end
end
end

@ -43,6 +43,9 @@ describe 'Board management spec', type: :feature, js: true do
let(:board_index) { Pages::BoardIndex.new(project) }
let(:filters) { ::Components::WorkPackages::Filters.new }
let!(:priority) { FactoryBot.create :default_priority }
let!(:status) { FactoryBot.create :default_status }
before do
with_enterprise_token :board_view
project
@ -62,16 +65,14 @@ describe 'Board management spec', type: :feature, js: true do
end
let(:board_view) { FactoryBot.create :board_grid_with_query, project: project }
let!(:priority) { FactoryBot.create :default_priority }
let!(:status) { FactoryBot.create :default_status }
it 'allows management of boards' do
board_view
board_index.visit!
board_page = board_index.open_board board_view
board_page.expect_query 'List 1', editable: true
board_page.expect_editable true
board_page.expect_editable_board true
board_page.expect_editable_list true
board_page.back_to_index
board_index.expect_board board_view.name
@ -175,11 +176,47 @@ describe 'Board management spec', type: :feature, js: true do
board_page = board_index.open_board board_view
board_page.expect_query 'List 1', editable: false
board_page.expect_editable false
board_page.expect_editable_board false
board_page.expect_editable_list false
board_page.back_to_index
list = page.find(board_page.list_selector('List 1'))
list.hover
expect(page).to have_no_selector('.board-list--more-menu')
board_index.expect_board board_view.name
end
end
context 'with view boards + edit work package permission' do
let(:permissions) { %i[show_board_views view_work_packages add_work_packages edit_work_packages] }
let(:board_view) { FactoryBot.create :board_grid_with_queries, project: project }
it 'allows viewing boards index and moving items around' do
board_view
board_index.visit!
board_page = board_index.open_board board_view
board_page.expect_query 'List 1', editable: false
board_page.expect_query 'List 2', editable: false
board_page.expect_editable_board false
board_page.expect_editable_list true
# Add item
board_page.add_card 'List 1', 'Task 1'
# Move item to Second list
board_page.move_card(0, from: 'List 1', to: 'List 2')
board_page.expect_card('List 1', 'Task 1', present: false)
board_page.expect_card('List 2', 'Task 1', present: true)
# Expect added to query
queries = board_page.board(reload: true).contained_queries
first = queries.find_by(name: 'List 1')
second = queries.find_by(name: 'List 2')
expect(first.ordered_work_packages).to be_empty
expect(second.ordered_work_packages.count).to eq(1)
# Expect work package to be saved in query first
subjects = WorkPackage.where(id: second.ordered_work_packages).pluck(:subject)
expect(subjects).to match_array ['Task 1']
board_page.back_to_index

@ -236,7 +236,7 @@ module Pages
find('.board--back-button').click
end
def expect_editable(editable)
def expect_editable_board(editable)
# Editable / draggable check
expect(page).to have_conditional_selector(editable, '.board--container.-editable')
@ -245,7 +245,9 @@ module Pages
# Add new list
expect(page).to have_conditional_selector(editable, '.boards-list--add-item')
end
def expect_editable_list(editable)
# Add new / existing card
expect(page).to have_conditional_selector(editable, '.board-list--card-dropdown-button')
end

@ -0,0 +1,140 @@
#-- encoding: UTF-8
#-- copyright
# OpenProject is a project management system.
# Copyright (C) 2012-2018 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-2017 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 docs/COPYRIGHT.rdoc for more details.
#++
require 'spec_helper'
describe Queries::UpdateContract do
let(:project) { FactoryBot.build_stubbed :project }
let(:query) do
FactoryBot.build_stubbed(:query, project: project, is_public: public, user: user)
end
let(:current_user) do
FactoryBot.build_stubbed(:user) do |user|
allow(user)
.to receive(:allowed_to?) do |permission, permission_project|
permissions.include?(permission) && project == permission_project
end
end
end
subject(:contract) { described_class.new(query, current_user) }
before do
# Assume project is always visible
allow(contract).to receive(:project_visible?).and_return true
end
def expect_valid(valid, symbols = {})
expect(contract.validate).to eq(valid)
symbols.each do |key, arr|
expect(contract.errors.symbols_for(key)).to match_array arr
end
end
describe 'private query' do
let(:public) { false }
context 'when user is author' do
let(:user) { current_user }
context 'user has no permission to save' do
let(:permissions) { %i(edit_work_packages) }
it 'is invalid' do
expect_valid(false, base: %i(error_unauthorized))
end
end
context 'user has permission to save' do
let(:permissions) { %i(save_queries) }
it 'is valid' do
expect_valid(true)
end
end
end
context 'when user is someone else' do
let(:user) { FactoryBot.build_stubbed :user }
let(:permissions) { %i(save_queries) }
it 'is invalid' do
expect_valid(false, base: %i(error_unauthorized))
end
end
end
describe 'public query' do
let(:public) { true }
let(:user) { nil }
context 'user has no permission to save' do
let(:permissions) { %i(invalid_permission) }
it 'is invalid' do
expect_valid(false, base: %i(error_unauthorized))
end
end
context 'user has permission to edit' do
let(:permissions) { %i(edit_work_packages) }
it 'is invalid' do
expect_valid(false, base: %i(error_unauthorized))
end
context 'when assuming only order changed' do
before do
query.ordered_work_packages = [1, 2, 3]
end
it 'is valid' do
expect_valid(true)
end
end
end
context 'user has no permission to manage public' do
let(:permissions) { %i(manage_public_queries) }
it 'is valid' do
expect_valid(true)
end
end
context 'user has permission to save only own' do
let(:permissions) { %i(save_queries) }
it 'is invalid' do
expect_valid(false, base: %i(error_unauthorized))
end
end
end
end

@ -178,7 +178,7 @@ describe 'Work Package table hierarchy', js: true do
member_in_project: project,
member_through_role: role
end
let(:permissions) { %i(view_work_packages add_work_packages) }
let(:permissions) { %i(view_work_packages add_work_packages save_queries) }
let(:role) { FactoryBot.create :role, permissions: permissions }
let(:sort_by) { ::Components::WorkPackages::SortBy.new }

@ -256,6 +256,51 @@ describe ::API::V3::Queries::QueryRepresenter do
end
end
describe 'updateOrderedWorkPackages action link' do
let(:permissions) { %i[update reorder_work_packages] }
it_behaves_like 'has an untitled link' do
let(:link) { 'updateOrderedWorkPackages' }
let(:href) { api_v3_paths.query query.id }
end
context 'when not persisted and lacking permission' do
let(:query) { FactoryBot.build(:query, project: project) }
it_behaves_like 'has no link' do
let(:link) { 'updateOrderedWorkPackages' }
end
end
context 'when not persisted and having permission' do
let(:permissions) { [:create] }
let(:query) { FactoryBot.build(:query, project: project) }
it_behaves_like 'has an untitled link' do
let(:link) { 'updateOrderedWorkPackages' }
let(:href) { api_v3_paths.query query.id }
end
end
context 'when not allowed to update' do
let(:permissions) { [] }
it_behaves_like 'has no link' do
let(:link) { 'updateOrderedWorkPackages' }
end
end
context 'when no user is provided' do
let(:user) { nil }
let(:embed_links) { false }
it_behaves_like 'has no link' do
let(:link) { 'updateOrderedWorkPackages' }
end
end
end
context 'with filter, sort, group by and pageSize' do
let(:representer) do
described_class.new(query,

@ -309,6 +309,75 @@ describe QueryPolicy, type: :controller do
end
end
shared_examples 'update ordered_work_packages' do |global|
context "#{ global ? 'in global context' : 'in project context' }" do
if global
let(:project) { nil }
end
it 'is false if the user has no permission in the project' do
allow(user).to receive(:allowed_to?).and_return false
expect(subject.allowed?(query, :reorder_work_packages)).to be_falsy
end
it 'is true if the user has the edit_work_packages permission in the project AND it public' do
allow(user).to receive(:allowed_to?).with(:edit_work_packages,
project,
global: project.nil?)
.and_return true
query.is_public = true
expect(subject.allowed?(query, :reorder_work_packages)).to be_truthy
end
it 'is false if the user has the edit_work_packages permission in the project AND it is not his' do
allow(user).to receive(:allowed_to?).with(:edit_work_packages,
project,
global: project.nil?)
.and_return true
query.user = FactoryBot.build_stubbed(:user)
query.is_public = false
expect(subject.allowed?(query, :reorder_work_packages)).to be_falsey
end
it 'is true if the user has the save_queries permission in the project AND it is his query' do
allow(user).to receive(:allowed_to?).with(:save_queries,
project,
global: project.nil?)
.and_return true
expect(subject.allowed?(query, :reorder_work_packages)).to be_truthy
end
it 'is true if the user has the manage_public_query permission in the project ' +
'AND it is a public query' do
allow(user).to receive(:allowed_to?).with(:manage_public_queries,
project,
global: project.nil?)
.and_return true
query.is_public = true
expect(subject.allowed?(query, :reorder_work_packages)).to be_truthy
end
it 'is false if the user has the manage_public_query permission in the project ' +
'AND the query is not public ' +
'AND it is not his query' do
allow(user).to receive(:allowed_to?).with(:manage_public_queries,
project,
global: project.nil?)
.and_return true
query.user = FactoryBot.build_stubbed(:user)
query.is_public = false
expect(subject.allowed?(query, :reorder_work_packages)).to be_falsey
end
end
end
it_should_behave_like 'action on persisted', :update, global: true
it_should_behave_like 'action on persisted', :update, global: false
it_should_behave_like 'action on persisted', :destroy, global: true

@ -31,7 +31,7 @@
require 'spec_helper'
describe UpdateQueryService do
let(:query) { FactoryBot.create(:query) }
let(:query) { FactoryBot.create(:query, user: user) }
let(:menu_item) do
FactoryBot.create(:query_menu_item,
query: query)

@ -127,7 +127,7 @@ shared_examples_for 'type service' do
['group1', query_params]
end
let(:params) { { attribute_groups: [query_group_params] } }
let(:query) { FactoryBot.create(:query) }
let(:query) { FactoryBot.create(:query, user_id: 0) }
let(:service_result) { ServiceResult.new(success: true, result: query) }
before do

Loading…
Cancel
Save