From 107dfcee15e6389059ddfa8b50b91f620a8fa2af Mon Sep 17 00:00:00 2001 From: CI Date: Thu, 24 Aug 2017 00:47:40 +0000 Subject: [PATCH 1/4] Update reference to OpenProject-Translations --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 55e8adba3d..b4a6c60701 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -60,7 +60,7 @@ GIT GIT remote: https://github.com/opf/openproject-translations.git - revision: 6c5ffada972fa05a43a361444da744511b4e7fad + revision: 6bc98c85f8349f2450cab2ff408c4e45fb3fa44d branch: release/7.2 specs: openproject-translations (7.2.3) From ddcf35f3f727c9dbd5359e2c6b6dcd4093d60a7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Thu, 24 Aug 2017 10:39:08 +0200 Subject: [PATCH 2/4] Fix invalid value_objects being passed to representer There are queries that seem to pass values not available in APIv3 path helper to the instance representer. ``` Original error: # Stacktrace: /opt/openproject/lib/api/v3/queries/filters/query_filter_instance_representer.rb:82:in `block (2 levels) in ' /opt/openproject/vendor/bundle/ruby/2.4.0/gems/activerecord-5.0.4/lib/active_record/relation/delegation.rb:38:in `map' /opt/openproject/vendor/bundle/ruby/2.4.0/gems/activerecord-5.0.4/lib/active_record/relation/delegation.rb:38:in `map' /opt/openproject/lib/api/v3/queries/filters/query_filter_instance_representer.rb:80:in `block in ' ``` --- .../query_filter_instance_representer.rb | 9 ++++++- .../query_filter_instance_representer_spec.rb | 24 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/lib/api/v3/queries/filters/query_filter_instance_representer.rb b/lib/api/v3/queries/filters/query_filter_instance_representer.rb index 6d7ced6a23..247658c214 100644 --- a/lib/api/v3/queries/filters/query_filter_instance_representer.rb +++ b/lib/api/v3/queries/filters/query_filter_instance_representer.rb @@ -78,8 +78,15 @@ module API next unless represented.ar_object_filter? represented.value_objects.map do |value_object| + href = begin + api_v3_paths.send(value_object.class.name.demodulize.underscore, value_object.id) + rescue + Rails.logger.error "Failed to get href for value_object #{value_object}" + nil + end + { - href: api_v3_paths.send(value_object.class.name.demodulize.underscore, value_object.id), + href: href, title: value_object.name } end diff --git a/spec/lib/api/v3/queries/filters/query_filter_instance_representer_spec.rb b/spec/lib/api/v3/queries/filters/query_filter_instance_representer_spec.rb index fbadad6e56..4ac6aa0562 100644 --- a/spec/lib/api/v3/queries/filters/query_filter_instance_representer_spec.rb +++ b/spec/lib/api/v3/queries/filters/query_filter_instance_representer_spec.rb @@ -93,6 +93,30 @@ describe ::API::V3::Queries::Filters::QueryFilterInstanceRepresenter do .at_path('name') end + context 'with an invalid value_objects' do + let(:filter) do + Queries::WorkPackages::Filter::AssignedToFilter.new(operator: operator, values: values) + end + let(:values) { ['1'] } + + before do + allow(filter) + .to receive(:value_objects) + .and_return([User.anonymous]) + end + + it "has a 'values' collection" do + expected = { + href: nil, + title: 'Anonymous' + } + + is_expected + .to be_json_eql([expected].to_json) + .at_path('_links/values') + end + end + context 'with a non ar object filter' do let(:values) { ['lorem ipsum'] } let(:filter) do From 75d2f010ba0b8029b2412cdd50c84822ab411b31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Thu, 24 Aug 2017 10:12:41 +0200 Subject: [PATCH 3/4] [25108][26176] Assigned_to filter returns group values Since 7.0.0, the `assigned_to is ` filter also returns work packages assigned to a group that `` belongs to. Since this is misleading and judged a bug by several customers, instead add a new filter that provides exactly this behavior, while using `assigned_to` only for the exact value specified there. https://community.openproject.com/wp/26176 https://community.openproject.com/wp/25108 --- app/models/queries/work_packages.rb | 1 + .../filter/assigned_to_filter.rb | 24 --- .../filter/assignee_or_group_filter.rb | 92 +++++++++ config/locales/en.yml | 1 + ..._or_group_filter_dependency_representer.rb | 41 ++++ .../filter/assigned_to_filter_spec.rb | 8 +- .../filter/assignee_or_group_filter_spec.rb | 192 ++++++++++++++++++ .../queries/filters/shared_filter_examples.rb | 3 +- spec_legacy/unit/query_spec.rb | 4 +- 9 files changed, 335 insertions(+), 31 deletions(-) create mode 100644 app/models/queries/work_packages/filter/assignee_or_group_filter.rb create mode 100644 lib/api/v3/queries/schemas/assignee_or_group_filter_dependency_representer.rb create mode 100644 spec/models/queries/work_packages/filter/assignee_or_group_filter_spec.rb diff --git a/app/models/queries/work_packages.rb b/app/models/queries/work_packages.rb index d31fba1bf9..ae401f9eb2 100644 --- a/app/models/queries/work_packages.rb +++ b/app/models/queries/work_packages.rb @@ -33,6 +33,7 @@ module Queries::WorkPackages register = Queries::Register register.filter Query, filters_module::AssignedToFilter + register.filter Query, filters_module::AssigneeOrGroupFilter register.filter Query, filters_module::AuthorFilter register.filter Query, filters_module::CategoryFilter register.filter Query, filters_module::CreatedAtFilter diff --git a/app/models/queries/work_packages/filter/assigned_to_filter.rb b/app/models/queries/work_packages/filter/assigned_to_filter.rb index 8eb898cf6d..e9c08f5763 100644 --- a/app/models/queries/work_packages/filter/assigned_to_filter.rb +++ b/app/models/queries/work_packages/filter/assigned_to_filter.rb @@ -57,28 +57,4 @@ class Queries::WorkPackages::Filter::AssignedToFilter < def self.key :assigned_to_id end - - private - - def values_replaced - vals = super - vals += group_members_added(vals) - vals + user_groups_added(vals) - end - - def group_members_added(vals) - User - .joins(:groups) - .where(groups_users: { id: vals }) - .pluck(:id) - .map(&:to_s) - end - - def user_groups_added(vals) - Group - .joins(:users) - .where(users_users: { id: vals }) - .pluck(:id) - .map(&:to_s) - end end diff --git a/app/models/queries/work_packages/filter/assignee_or_group_filter.rb b/app/models/queries/work_packages/filter/assignee_or_group_filter.rb new file mode 100644 index 0000000000..eab367cca3 --- /dev/null +++ b/app/models/queries/work_packages/filter/assignee_or_group_filter.rb @@ -0,0 +1,92 @@ +#-- encoding: UTF-8 + +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2017 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 doc/COPYRIGHT.rdoc for more details. +#++ + +class Queries::WorkPackages::Filter::AssigneeOrGroupFilter < + Queries::WorkPackages::Filter::PrincipalBaseFilter + def allowed_values + @allowed_values ||= begin + values = principal_loader.user_values + + if Setting.work_package_group_assignment? + values += principal_loader.group_values + end + + me_value + values.sort + end + end + + def type + :list_optional + end + + def order + 4 + end + + def human_name + I18n.t('query_fields.assignee_or_group') + end + + def self.key + :assignee_or_group + end + + def where + operator_strategy.sql_for_field( + values_replaced, + self.class.model.table_name, + 'assigned_to_id' + ) + end + + private + + def values_replaced + vals = super + vals += group_members_added(vals) + vals + user_groups_added(vals) + end + + def group_members_added(vals) + User + .joins(:groups) + .where(groups_users: { id: vals }) + .pluck(:id) + .map(&:to_s) + end + + def user_groups_added(vals) + Group + .joins(:users) + .where(users_users: { id: vals }) + .pluck(:id) + .map(&:to_s) + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index 0e7af91b46..f85804e8b2 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1758,6 +1758,7 @@ en: query_fields: assigned_to_role: "Assignee's role" member_of_group: "Assignee's group" + assignee_or_group: "Assignee or belonging group" subproject_id: "Subproject" repositories: diff --git a/lib/api/v3/queries/schemas/assignee_or_group_filter_dependency_representer.rb b/lib/api/v3/queries/schemas/assignee_or_group_filter_dependency_representer.rb new file mode 100644 index 0000000000..8eb05b813d --- /dev/null +++ b/lib/api/v3/queries/schemas/assignee_or_group_filter_dependency_representer.rb @@ -0,0 +1,41 @@ +#-- encoding: UTF-8 + +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2017 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 doc/COPYRIGHT.rdoc for more details. +#++ + +module API + module V3 + module Queries + module Schemas + class AssigneeOrGroupFilterDependencyRepresenter < + AssignedToFilterDependencyRepresenter + end + end + end + end +end diff --git a/spec/models/queries/work_packages/filter/assigned_to_filter_spec.rb b/spec/models/queries/work_packages/filter/assigned_to_filter_spec.rb index 57ce964e4a..63d7ba781c 100644 --- a/spec/models/queries/work_packages/filter/assigned_to_filter_spec.rb +++ b/spec/models/queries/work_packages/filter/assigned_to_filter_spec.rb @@ -102,9 +102,9 @@ describe Queries::WorkPackages::Filter::AssignedToFilter, type: :model do group.users << assignee end - it 'returns the work package' do + it 'does not return the work package' do is_expected - .to match_array [work_package] + .to be_empty end end @@ -126,9 +126,9 @@ describe Queries::WorkPackages::Filter::AssignedToFilter, type: :model do group.users << user end - it 'returns the work package' do + it 'does not return the work package' do is_expected - .to match_array [work_package] + .to be_empty end end diff --git a/spec/models/queries/work_packages/filter/assignee_or_group_filter_spec.rb b/spec/models/queries/work_packages/filter/assignee_or_group_filter_spec.rb new file mode 100644 index 0000000000..8637e218d4 --- /dev/null +++ b/spec/models/queries/work_packages/filter/assignee_or_group_filter_spec.rb @@ -0,0 +1,192 @@ +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2017 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 doc/COPYRIGHT.rdoc for more details. +#++ + +require 'spec_helper' + +describe Queries::WorkPackages::Filter::AssigneeOrGroupFilter, type: :model do + let(:instance) do + filter = described_class.new + filter.values = values + filter.operator = operator + filter + end + + let(:operator) { '=' } + let(:values) { [] } + + describe 'where filter results' do + let(:work_package) { FactoryGirl.create(:work_package, assigned_to: assignee) } + let(:assignee) { FactoryGirl.create(:user) } + let(:group) { FactoryGirl.create(:group) } + + subject { WorkPackage.where(instance.where) } + + context 'for the user value' do + let(:values) { [assignee.id.to_s] } + + it 'returns the work package' do + is_expected + .to match_array [work_package] + end + end + + context 'for the me value with the user being logged in' do + let(:values) { ['me'] } + + before do + allow(User) + .to receive(:current) + .and_return(assignee) + end + + it 'returns the work package' do + is_expected + .to match_array [work_package] + end + end + + context 'for the me value with another user being logged in' do + let(:values) { ['me'] } + + before do + allow(User) + .to receive(:current) + .and_return(FactoryGirl.create(:user)) + end + + it 'does not return the work package' do + is_expected + .to be_empty + end + end + + context 'for a group value with the group being assignee' do + let(:assignee) { group } + let(:values) { [group.id.to_s] } + + it 'returns the work package' do + is_expected + .to match_array [work_package] + end + end + + context 'for a group value with a group member being assignee' do + let(:values) { [group.id.to_s] } + + before do + group.users << assignee + end + + it 'returns the work package' do + is_expected + .to match_array [work_package] + end + end + + context 'for a group value with no group member being assignee' do + let(:values) { [group.id.to_s] } + + it 'does not return the work package' do + is_expected + .to be_empty + end + end + + context "for a user value with the user's group being assignee" do + let(:values) { [user.id.to_s] } + let(:assignee) { group } + let(:user) { FactoryGirl.create(:user) } + + before do + group.users << user + end + + it 'returns the work package' do + is_expected + .to match_array [work_package] + end + end + + context "for a user value with the user not being member of the assigned group" do + let(:values) { [user.id.to_s] } + let(:assignee) { group } + let(:user) { FactoryGirl.create(:user) } + + it 'does not return the work package' do + is_expected + .to be_empty + end + end + + context 'for an unmatched value' do + let(:values) { ['0'] } + + it 'does not return the work package' do + is_expected + .to be_empty + end + end + end + + it_behaves_like 'basic query filter' do + let(:order) { 4 } + let(:type) { :list_optional } + let(:class_key) { :assignee_or_group } + let(:human_name) { I18n.t('query_fields.assignee_or_group') } + + describe '#valid_values!' do + let(:user) { FactoryGirl.build_stubbed(:user) } + let(:loader) do + loader = double('loader') + + allow(loader) + .to receive(:user_values) + .and_return([[user.name, user.id.to_s]]) + allow(loader) + .to receive(:group_values) + .and_return([]) + + loader + end + + before do + allow(::Queries::WorkPackages::Filter::PrincipalLoader) + .to receive(:new) + .and_return(loader) + + instance.values = [user.id.to_s, '99999'] + end + + it 'remove the invalid value' do + instance.valid_values! + + expect(instance.values).to match_array [user.id.to_s] + end + end + end +end diff --git a/spec/support/queries/filters/shared_filter_examples.rb b/spec/support/queries/filters/shared_filter_examples.rb index 18bf1e4b49..5641c75ca1 100644 --- a/spec/support/queries/filters/shared_filter_examples.rb +++ b/spec/support/queries/filters/shared_filter_examples.rb @@ -49,6 +49,7 @@ shared_examples_for 'basic query filter' do let(:instance_key) { nil } let(:class_key) { raise 'needs to be defined' } let(:type) { raise 'needs to be defined' } + let(:human_name) { nil } let(:order) { nil } describe '.key' do @@ -79,7 +80,7 @@ shared_examples_for 'basic query filter' do describe '#human_name' do it 'is the l10 name for the filter' do - expect(instance.human_name).to eql(name) + expect(instance.human_name).to eql(human_name.presence || name) end end end diff --git a/spec_legacy/unit/query_spec.rb b/spec_legacy/unit/query_spec.rb index 88dda4bbd2..1b1bb590a2 100644 --- a/spec_legacy/unit/query_spec.rb +++ b/spec_legacy/unit/query_spec.rb @@ -216,10 +216,10 @@ describe Query, type: :model do query = Query.new(name: '_', filters: [{ assigned_to_id: { operator: '=', values: ['me'] } }]) result = query.results.work_packages - assert_equal WorkPackage.visible.where(assigned_to_id: ([2] + user.reload.group_ids)).sort_by(&:id), result.sort_by(&:id) + assert_equal WorkPackage.visible.where(assigned_to_id: ([2])).sort_by(&:id), result.sort_by(&:id) assert result.include?(i1) - assert result.include?(i2) + assert !result.include?(i2) assert !result.include?(i3) User.current = nil From f7f17c8d4e39c9fb2812afa2113d44be76fea2c7 Mon Sep 17 00:00:00 2001 From: CI Date: Thu, 24 Aug 2017 13:12:17 +0000 Subject: [PATCH 4/4] Update reference to OpenProject-Translations --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index b4a6c60701..4da45905b7 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -60,7 +60,7 @@ GIT GIT remote: https://github.com/opf/openproject-translations.git - revision: 6bc98c85f8349f2450cab2ff408c4e45fb3fa44d + revision: bb2bfb6e5058f3ea4129ae75732bc410c662a08c branch: release/7.2 specs: openproject-translations (7.2.3)