Merge branch 'release/7.4' into dev

app/models/queries/relations/filters/from_filter.rb

@@ -31,6 +31,8 @@ module Queries
   module Relations
     module Filters
       class FromFilter < ::Queries::Relations::Filters::RelationFilter
+        include ::Queries::Relations::Filters::VisibilityChecking
+
         def type
           :integer
         end
@@ -38,6 +40,12 @@ module Queries
         def self.key
           :from_id
         end
+
+        private
+
+        def visibility_checked_sql(operator, values, visible_sql)
+          ["from_id #{operator} (?) AND to_id IN (#{visible_sql})", values]
+        end
       end
     end
   end

app/models/queries/relations/filters/involved_filter.rb

@@ -36,6 +36,8 @@ module Queries
       #   Given relations [{ from_id: 3, to_id: 7 }, { from_id: 8, to_id: 3}]
       #   filtering by involved=3 would yield both these relations.
       class InvolvedFilter < ::Queries::Relations::Filters::RelationFilter
+        include ::Queries::Relations::Filters::VisibilityChecking
+
         def type
           :integer
         end
@@ -44,15 +46,21 @@ module Queries
           :involved
         end
 
-        def where
+        private
-          integer_values = values.map(&:to_i)
 
-          case operator
+        def visibility_checked_sql(operator_string, values, visible_sql)
-          when "="
+          concatenation = if operator == '='
-            ["from_id IN (?) OR to_id IN (?)", integer_values, integer_values]
+                            "OR"
-          when "!"
+                          else
-            ["from_id NOT IN (?) AND to_id NOT IN (?)", integer_values, integer_values]
+                            "AND"
                           end
+
+          sql = <<-SQL.strip_heredoc
+            (from_id #{operator_string} (?) AND to_id IN (#{visible_sql}))
+             #{concatenation} (to_id #{operator_string} (?) AND from_id IN (#{visible_sql}))
+          SQL
+
+          [sql, values, values]
         end
       end
     end

app/models/queries/relations/filters/relation_filter.rb

@@ -36,6 +36,10 @@ module Queries
         def human_name
           Relation.human_attribute_name(name)
         end
+
+        def visibility_checked?
+          false
+        end
       end
     end
   end

app/models/queries/relations/filters/to_filter.rb

@@ -31,6 +31,8 @@ module Queries
   module Relations
     module Filters
       class ToFilter < ::Queries::Relations::Filters::RelationFilter
+        include ::Queries::Relations::Filters::VisibilityChecking
+
         def type
           :integer
         end
@@ -38,6 +40,16 @@ module Queries
         def self.key
           :to_id
         end
+
+        def visibility_checked?
+          true
+        end
+
+        private
+
+        def visibility_checked_sql(operator, values, visible_sql)
+          ["to_id #{operator} (?) AND from_id IN (#{visible_sql})", values]
+        end
       end
     end
   end

app/models/queries/relations/filters/visibility_checking.rb

@@ -0,0 +1,62 @@
+#-- 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.
+#++
+
+module Queries
+  module Relations
+    module Filters
+      module VisibilityChecking
+        def visibility_checked?
+          true
+        end
+
+        def where
+          integer_values = values.map(&:to_i)
+
+          visible_sql = WorkPackage.visible(User.current).select(:id).to_sql
+
+          operator_string = case operator
+                            when "="
+                              "IN"
+                            when "!"
+                              "NOT IN"
+                            end
+
+          visibility_checked_sql(operator_string, values, visible_sql)
+        end
+
+        private
+
+        def visibility_checked_sql(_operator, _values, _visible_sql)
+          raise NotImplementedError
+        end
+      end
+    end
+  end
+end

app/models/queries/relations/relation_query.rb

@@ -35,9 +35,19 @@ module Queries
 
       def default_scope
         Relation
-          .visible
           .direct
       end
+
+      def results
+        # Filters marked to already check visibility free us from the need
+        # to check it here.
+
+        if filters.any?(&:visibility_checked?)
+          super
+        else
+          super.visible
+        end
+      end
     end
   end
 end

app/models/work_package.rb

@@ -188,6 +188,31 @@ class WorkPackage < ActiveRecord::Base
     Relation.of_work_package(self)
   end
 
+  def visible_relations(user)
+    # This duplicates chaining
+    #  .relations.visible
+    # The duplication is made necessary to achive a performant sql query on MySQL.
+    # Chaining would result in
+    #   WHERE (relations.from_id = [ID] OR relations.to_id = [ID])
+    #   AND relations.from_id IN (SELECT [IDs OF VISIBLE WORK_PACKAGES])
+    #   AND relations.to_id IN (SELECT [IDs OF VISIBLE WORK_PACKAGES])
+    # This performs OK on postgresql but is very slow on MySQL
+    # The SQL generated by this method:
+    #   WHERE (relations.from_id = [ID] AND relations.to_id IN (SELECT [IDs OF VISIBLE WORK_PACKAGES])
+    #   OR (relations.to_id = [ID] AND relations.from_id IN (SELECT [IDs OF VISIBLE WORK_PACKAGES]))
+    # is arguably easier to read and performs equally good on both DBs.
+    relations_from = Relation
+                     .where(from: self)
+                     .where(to: WorkPackage.visible(user))
+
+    relations_to = Relation
+                   .where(to: self)
+                   .where(from: WorkPackage.visible(user))
+
+    relations_from
+      .or(relations_to)
+  end
+
   def relation(id)
     Relation.of_work_package(self).find(id)
   end

app/views/my/access_token.html.erb

@@ -89,7 +89,7 @@ See docs/COPYRIGHT.rdoc for more details.
                 <td><%= I18n.t('my_account.access_tokens.indefinite_expiration') %></td>
                 <td>
                   <%= link_to l(:button_reset),
-                              { action: 'generate_api_key' },
+                              { action: 'generate_rss_key' },
                               method: :post,
                               class: 'icon icon-delete' %>
                 </td>

lib/api/v3/relations/relations_api.rb

@@ -41,8 +41,6 @@ module API
         resources :relations do
           get do
             scope = Relation
-                    .visible(current_user)
-                    .direct
                     .non_hierarchy
                     .includes(::API::V3::Relations::RelationRepresenter.to_eager_load)
 

lib/api/v3/work_packages/work_package_relations_api.rb

@@ -37,11 +37,12 @@ module API
           # @todo Redirect to relations endpoint as soon as `list relations` API endpoint
           #       including filters is complete.
           get do
-            relations = @work_package
+            query = ::Queries::Relations::RelationQuery.new(user: current_user)
-                        .relations
+
-                        .direct
+            relations = query
+                        .where(:involved, '=', @work_package.id)
+                        .results
                         .non_hierarchy
-                        .visible(current_user)
 
             ::API::V3::Relations::RelationCollectionRepresenter.new(
               relations,

lib/api/v3/work_packages/work_package_representer.rb

@@ -563,7 +563,7 @@ module API
 
         def relations
           self_path = api_v3_paths.work_package_relations(represented.id)
-          visible_relations = represented.relations.visible.non_hierarchy
+          visible_relations = represented.visible_relations(current_user).non_hierarchy
 
           ::API::V3::Relations::RelationCollectionRepresenter.new(visible_relations,
                                                                   self_path,

spec/lib/api/v3/work_packages/work_package_representer_spec.rb

@@ -958,7 +958,7 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do
 
         before do
           allow(work_package)
-            .to receive_message_chain(:relations, :visible, :non_hierarchy)
+            .to receive_message_chain(:visible_relations, :non_hierarchy)
             .and_return([relation])
         end
 

spec/models/queries/relations/filters/from_filter_spec.rb

@@ -0,0 +1,84 @@
+#-- 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::Relations::Filters::FromFilter, type: :model do
+  include_context 'filter tests'
+  let(:values) { ['1'] }
+  let(:model) { Relation }
+  let(:current_user) { FactoryGirl.build_stubbed(:user) }
+
+  it_behaves_like 'basic query filter' do
+    let(:class_key) { :from_id }
+    let(:type) { :integer }
+    # The name is not very good but as long as the filter is not displayed in the UI ...
+    let(:human_name) { 'Work package' }
+
+    describe '#allowed_values' do
+      it 'is nil' do
+        expect(instance.allowed_values).to be_nil
+      end
+    end
+  end
+
+  describe '#visibility_checked?' do
+    it 'is true' do
+      expect(instance).to be_visibility_checked
+    end
+  end
+
+  describe '#scope' do
+    before do
+      login_as(current_user)
+    end
+    let(:visible_sql) { WorkPackage.visible(current_user).select(:id).to_sql }
+
+    context 'for "="' do
+      let(:operator) { '=' }
+
+      it 'is the same as handwriting the query' do
+        expected = model.where("from_id IN ('1') AND to_id IN (#{visible_sql})")
+
+        expect(instance.scope.to_sql).to eql expected.to_sql
+      end
+    end
+
+    context 'for "!"' do
+      let(:operator) { '!' }
+
+      it 'is the same as handwriting the query' do
+        expected = model.where("from_id NOT IN ('1') AND to_id IN (#{visible_sql})")
+
+        expect(instance.scope.to_sql).to eql expected.to_sql
+      end
+    end
+  end
+end

spec/models/queries/relations/filters/involved_filter_spec.rb

@@ -0,0 +1,84 @@
+#-- 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::Relations::Filters::InvolvedFilter, type: :model do
+  include_context 'filter tests'
+  let(:values) { ['1'] }
+  let(:model) { Relation }
+  let(:current_user) { FactoryGirl.build_stubbed(:user) }
+
+  it_behaves_like 'basic query filter' do
+    let(:class_key) { :involved }
+    let(:type) { :integer }
+
+    describe '#allowed_values' do
+      it 'is nil' do
+        expect(instance.allowed_values).to be_nil
+      end
+    end
+  end
+
+  describe '#visibility_checked?' do
+    it 'is true' do
+      expect(instance).to be_visibility_checked
+    end
+  end
+
+  describe '#scope' do
+    before do
+      login_as(current_user)
+    end
+    let(:visible_sql) { WorkPackage.visible(current_user).select(:id).to_sql }
+
+    context 'for "="' do
+      let(:operator) { '=' }
+
+      it 'is the same as handwriting the query' do
+        sql = "(from_id IN ('1') AND to_id IN (#{visible_sql}))\n OR (to_id IN ('1') AND from_id IN (#{visible_sql}))\n"
+        expected = model.where(sql)
+
+        expect(instance.scope.to_sql).to eql expected.to_sql
+      end
+    end
+
+    context 'for "!"' do
+      let(:operator) { '!' }
+
+      it 'is the same as handwriting the query' do
+        sql = "(from_id NOT IN ('1') AND to_id IN (#{visible_sql}))\n AND (to_id NOT IN ('1') AND from_id IN (#{visible_sql}))\n"
+        expected = model.where(sql)
+
+        expect(instance.scope.to_sql).to eql expected.to_sql
+      end
+    end
+  end
+end

spec/models/queries/relations/filters/to_filter_spec.rb

@@ -0,0 +1,84 @@
+#-- 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::Relations::Filters::ToFilter, type: :model do
+  include_context 'filter tests'
+  let(:values) { ['1'] }
+  let(:model) { Relation }
+  let(:current_user) { FactoryGirl.build_stubbed(:user) }
+
+  it_behaves_like 'basic query filter' do
+    let(:class_key) { :to_id }
+    let(:type) { :integer }
+    # The name is not very good but as long as the filter is not displayed in the UI ...
+    let(:human_name) { 'Related work package' }
+
+    describe '#allowed_values' do
+      it 'is nil' do
+        expect(instance.allowed_values).to be_nil
+      end
+    end
+  end
+
+  describe '#visibility_checked?' do
+    it 'is true' do
+      expect(instance).to be_visibility_checked
+    end
+  end
+
+  describe '#scope' do
+    before do
+      login_as(current_user)
+    end
+    let(:visible_sql) { WorkPackage.visible(current_user).select(:id).to_sql }
+
+    context 'for "="' do
+      let(:operator) { '=' }
+
+      it 'is the same as handwriting the query' do
+        expected = model.where("to_id IN ('1') AND from_id IN (#{visible_sql})")
+
+        expect(instance.scope.to_sql).to eql expected.to_sql
+      end
+    end
+
+    context 'for "!"' do
+      let(:operator) { '!' }
+
+      it 'is the same as handwriting the query' do
+        expected = model.where("to_id NOT IN ('1') AND from_id IN (#{visible_sql})")
+
+        expect(instance.scope.to_sql).to eql expected.to_sql
+      end
+    end
+  end
+end

spec/models/queries/relations/relation_query_spec.rb

@@ -30,12 +30,12 @@ require 'spec_helper'
 
 describe Queries::Relations::RelationQuery, type: :model do
   let(:instance) { described_class.new }
-  let(:base_scope) { Relation.visible.direct }
+  let(:base_scope) { Relation.direct }
 
   context 'without a filter' do
     describe '#results' do
       it 'is the same as getting all the relations' do
-        expect(instance.results.to_sql).to eql base_scope.to_sql
+        expect(instance.results.to_sql).to eql base_scope.visible.to_sql
       end
     end
 
@@ -56,6 +56,7 @@ describe Queries::Relations::RelationQuery, type: :model do
         expected = base_scope
                    .merge(Relation
                           .where("relations.follows IN ('1') OR relations.blocks IN ('1')"))
+                   .visible
 
         expect(instance.results.to_sql).to eql expected.to_sql
       end
@@ -73,4 +74,24 @@ describe Queries::Relations::RelationQuery, type: :model do
       end
     end
   end
+
+  context 'with a from filter' do
+    let(:current_user) { FactoryGirl.build_stubbed(:user) }
+    before do
+      login_as(current_user)
+      instance.where('from_id', '=', ['1'])
+    end
+
+    describe '#results' do
+      it 'is the same as handwriting the query (with visibility checked within the filter query)' do
+        visible_sql = WorkPackage.visible(current_user).select(:id).to_sql
+
+        expected = base_scope
+                   .merge(Relation
+                          .where("from_id IN ('1') AND to_id IN (#{visible_sql})"))
+
+        expect(instance.results.to_sql).to eql expected.to_sql
+      end
+    end
+  end
 end