Merge pull request #5690 from opf/merge/release7.1

Merge/release7.1

app/models/queries/columns/base.rb

@@ -33,7 +33,6 @@ class Queries::Columns::Base
                 :sortable,
                 :groupable,
                 :summable,
-                :join,
                 :default_order
   alias_method :summable?, :summable
 
@@ -46,8 +45,6 @@ class Queries::Columns::Base
        default_order).each do |attribute|
       send("#{attribute}=", options[attribute])
     end
-
-    self.join = options.delete(:join)
   end
 
   def caption

app/models/queries/work_packages/columns/property_column.rb

@@ -41,61 +41,59 @@ class Queries::WorkPackages::Columns::PropertyColumn < Queries::WorkPackages::Co
       groupable: false
     },
     project: {
-      sortable: "#{Project.table_name}.name",
+      sortable: "name",
       groupable: true
     },
     subject: {
       sortable: "#{WorkPackage.table_name}.subject"
     },
     type: {
-      sortable: "#{::Type.table_name}.position",
+      sortable: "position",
       groupable: true
     },
     parent: {
-      sortable: ["#{WorkPackage.table_name}.root_id",
-                 "#{WorkPackage.table_name}.lft"],
+      sortable: ["root_id",
+                 "lft"],
       default_order: 'asc'
     },
     status: {
-      sortable: "#{Status.table_name}.position",
+      sortable: "position",
       groupable: true
     },
     priority: {
-      sortable: "#{IssuePriority.table_name}.position",
+      sortable: "position",
       default_order: 'desc',
       groupable: true
     },
     author: {
-      sortable: ["#{User.table_name}.lastname",
-                 "#{User.table_name}.firstname",
-                 "#{WorkPackage.table_name}.author_id"],
+      sortable: ["lastname",
+                 "firstname",
+                 "id"],
       groupable: true
     },
     assigned_to: {
-      sortable: ["#{User.table_name}.lastname",
-                 "#{User.table_name}.firstname",
-                 "#{WorkPackage.table_name}.assigned_to_id"],
+      sortable: ["lastname",
+                 "firstname",
+                 "id"],
       groupable: true
     },
     responsible: {
-      sortable: ["#{User.table_name}.lastname",
-                 "#{User.table_name}.firstname",
-                 "#{WorkPackage.table_name}.responsible_id"],
-      groupable: true,
-      join: 'LEFT OUTER JOIN users as responsible ON ' +
-            "(#{WorkPackage.table_name}.responsible_id = responsible.id)"
+      sortable: ["lastname",
+                 "firstname",
+                 "id"],
+      groupable: true
     },
     updated_at: {
       sortable: "#{WorkPackage.table_name}.updated_at",
       default_order: 'desc'
     },
     category: {
-      sortable: "#{Category.table_name}.name",
+      sortable: "name",
       groupable: true
     },
     fixed_version: {
-      sortable: ["#{Version.table_name}.effective_date",
-                 "#{Version.table_name}.name"],
+      sortable: ["effective_date",
+                 "name"],
       default_order: 'desc',
       groupable: true
     },

app/models/query.rb

@@ -290,18 +290,6 @@ class Query < ActiveRecord::Base
     column_names.empty?
   end
 
-  ##
-  # Returns the columns involved in this query, including those only needed for sorting or grouping
-  # puposes, not only the ones displayed (see :columns).
-  def involved_columns
-    columns = self.columns.map(&:name)
-
-    columns << group_by.to_sym if group_by
-    columns += sort_criteria.map { |x| x.first.to_sym }
-
-    columns.uniq
-  end
-
   def sort_criteria=(arg)
     if arg.is_a?(Hash)
       arg = arg.keys.sort.map { |k| arg[k] }
@@ -314,13 +302,6 @@ class Query < ActiveRecord::Base
     read_attribute(:sort_criteria) || []
   end
 
-  def sort_criteria_sql
-    criteria = SortHelper::SortCriteria.new
-    criteria.available_criteria = sortable_key_by_column_name
-    criteria.criteria = sort_criteria
-    criteria.to_sql
-  end
-
   def sort_criteria_key(arg)
     sort_criteria && sort_criteria[arg] && sort_criteria[arg].first
   end
@@ -345,13 +326,6 @@ class Query < ActiveRecord::Base
     sort_criteria.any?
   end
 
-  # Returns the SQL sort order that should be prepended for grouping
-  def group_by_sort_order
-    if grouped? && (column = group_by_column)
-      Array(column.sortable).map { |s| "#{s} #{column.default_order}" }.join(',')
-    end
-  end
-
   # Returns true if the query is a grouped query
   def grouped?
     !group_by_column.nil?

app/models/query/results.rb

@@ -70,15 +70,11 @@ class ::Query::Results
   end
 
   def work_packages
-    includes = ([:status, :project] +
-      includes_for_columns(query.involved_columns) + (options[:include] || [])).uniq
-
     WorkPackage
       .visible
       .where(query.statement)
       .where(options[:conditions])
-      .includes(includes)
-      .joins((query.group_by_column ? query.group_by_column.join : nil))
+      .includes(all_includes)
       .order(order_option)
       .references(:projects)
   end
@@ -88,7 +84,7 @@ class ::Query::Results
   # If there is a reason: This is a somewhat DRY way of using the sort criteria.
   # If there is no reason: The :work_package method can die over time and be replaced by this one.
   def sorted_work_packages
-    work_packages.order(query.sort_criteria_sql)
+    work_packages.order(sort_criteria_sql)
   end
 
   def versions
@@ -133,7 +129,7 @@ class ::Query::Results
   end
 
   def order_option
-    order_option = [query.group_by_sort_order, options[:order]].reject(&:blank?).join(', ')
+    order_option = [group_by_sort_order].reject(&:blank?).join(', ')
     order_option = nil if order_option.blank?
 
     order_option
@@ -141,15 +137,15 @@ class ::Query::Results
 
   private
 
+  def all_includes
+    (%i(status project) +
+      includes_for_columns(include_columns) +
+      (options[:include] || [])).uniq
+  end
+
   def includes_for_columns(column_names)
     column_names = Array(column_names)
-    includes = (WorkPackage.reflections.keys.map(&:to_sym) & column_names.map(&:to_sym))
-
-    if column_names.any? { |column| custom_field_column?(column) }
-      includes << { custom_values: :custom_field }
-    end
-
-    includes
+    (WorkPackage.reflections.keys.map(&:to_sym) & column_names.map(&:to_sym))
   end
 
   def custom_field_column?(name)
@@ -203,4 +199,95 @@ class ::Query::Results
   def transform_custom_field_keys(custom_field, groups)
     groups.transform_keys { |key| custom_field.cast_value(key) }
   end
+
+  ##
+  # Returns the columns that need to be included to allow:
+  # * sorting
+  # * grouping
+  def include_columns
+    columns = query.sort_criteria.map { |x| x.first.to_sym }
+
+    columns << query.group_by.to_sym if query.group_by
+
+    columns.uniq
+  end
+
+  def sort_criteria_sql
+    criteria = SortHelper::SortCriteria.new
+    criteria.available_criteria = aliased_sorting_by_column_name
+    criteria.criteria = query.sort_criteria
+    criteria.to_sql
+  end
+
+  def aliased_sorting_by_column_name
+    sorting_by_column_name = query.sortable_key_by_column_name
+
+    aliases = include_aliases
+
+    reflection_includes.each do |inc|
+      sorting_by_column_name[inc.to_s] = Array(sorting_by_column_name[inc.to_s]).map { |column| "#{aliases[inc]}.#{column}" }
+    end
+
+    sorting_by_column_name
+  end
+
+  # Returns the SQL sort order that should be prepended for grouping
+  def group_by_sort_order
+    if query.grouped? && (column = query.group_by_column)
+      aliases = include_aliases
+
+      Array(column.sortable).map do |s|
+        aliased_group_by_sort_order(s, column, aliases[column.name])
+      end.join(',')
+    end
+  end
+
+  def aliased_group_by_sort_order(sortable, column, alias_name)
+    if alias_name
+      "#{alias_name}.#{sortable} #{column.default_order}"
+    else
+      "#{sortable} #{column.default_order}"
+    end
+  end
+
+  # To avoid naming conflicts, joined tables are aliased if they are joined
+  # more than once. Here, joining tables that are referenced by multiple
+  # columns are of particular interest.
+  #
+  # Mirroring the way AR creates aliases for included/joined tables: Normally,
+  # included/joined associations are not aliased and as such, they simply use
+  # the table name. But if an association is joined/included that relies on a
+  # table which an already joined/included association also relies upon, that
+  # name is already taken in the DB query. Therefore, the #alias_candidate
+  # method is used which will concatenate the pluralized association name with
+  # the table name the association is defined for.
+  #
+  # There is no handling for cases when the same association is joined/included
+  # multiple times as the rest of the code should prevent that.
+  def include_aliases
+    counts = Hash.new do |h, key|
+      h[key] = 0
+    end
+
+    reflection_includes.each_with_object({}) do |inc, hash|
+      reflection = WorkPackage.reflections[inc.to_s]
+      table_name = reflection.klass.table_name
+
+      hash[inc] = reflection_alias(reflection, counts[table_name])
+
+      counts[table_name] += 1
+    end
+  end
+
+  def reflection_includes
+    WorkPackage.reflections.keys.map(&:to_sym) & all_includes.map(&:to_sym)
+  end
+
+  def reflection_alias(reflection, count)
+    if count.zero?
+      reflection.klass.table_name
+    else
+      reflection.alias_candidate(WorkPackage.table_name)
+    end
+  end
 end

frontend/app/components/wp-query/url-params-helper.test.ts

@@ -126,7 +126,6 @@ describe('UrlParamsHelper', function() {
       let decodedQueryParams = UrlParamsHelper.buildV3GetQueryFromJsonParams(params);
 
       let expected = {
-        'columns[]': ['type', 'status', 'soße'],
         showSums: true,
         timelineVisible: true,
         showHierarchies: true,

frontend/app/components/wp-query/url-params-helper.ts

@@ -111,7 +111,7 @@ export class UrlParamsHelperService {
   public buildV3GetQueryFromJsonParams(updateJson:any) {
     var queryData:any = {
       pageSize: this.PaginationService.getPerPage()
-    }
+    };
 
     if (!updateJson) {
       return queryData;
@@ -119,9 +119,6 @@ export class UrlParamsHelperService {
 
     var properties = JSON.parse(updateJson);
 
-    if (properties.c) {
-      queryData["columns[]"] = properties.c.map((column:any) => column);
-    }
     if (!!properties.s) {
       queryData.showSums = properties.s;
     }

lib/api/v3/utilities/custom_field_injector/link_value_getter.rb

@@ -1,4 +1,5 @@
 #-- encoding: UTF-8
+
 #-- copyright
 # OpenProject is a project management system.
 # Copyright (C) 2012-2017 the OpenProject Foundation (OPF)
@@ -60,9 +61,13 @@ module API
                 if custom_value && custom_value.value.present?
                   title = link_value_title(custom_value)
 
+                  # only use ids for url
+                  href = if custom_value.value.to_i.to_s == custom_value.value
+                           api_v3_paths.send(path_method, custom_value.value)
+                         end
                   [{
                     title: title,
-                    href: api_v3_paths.send(path_method, custom_value.value)
+                    href: href
                   }]
                 else
                   []

spec/lib/api/v3/utilities/custom_field_injector_spec.rb

@@ -345,6 +345,24 @@ describe ::API::V3::Utilities::CustomFieldInjector do
           let(:link) { cf_path }
         end
       end
+
+      context 'value is some invalid string' do
+        let(:value) { 'some invalid string' }
+        let(:raw_value) { 'some invalid string' }
+        let(:typed_value) { 'some invalid string not found' }
+
+        it 'has an empty href' do
+          expect(subject)
+            .to be_json_eql(nil.to_json)
+            .at_path("_links/#{cf_path}/href")
+        end
+
+        it 'has the invalid value as title' do
+          expect(subject)
+            .to be_json_eql(typed_value.to_json)
+            .at_path("_links/#{cf_path}/title")
+        end
+      end
     end
 
     context 'string custom field' do

spec/models/query/results_spec.rb

@@ -34,22 +34,25 @@ describe ::Query::Results, type: :model do
                       show_hierarchies: false
   end
   let(:query_results) do
-    ::Query::Results.new query, include: [:assigned_to,
-                                          :type,
-                                          :priority,
-                                          :category,
-                                          :fixed_version],
-                                order: 'work_packages.root_id DESC, work_packages.lft ASC'
+    ::Query::Results.new query,
+                         include: %i(
+                           assigned_to
+                           type
+                           priority
+                           category
+                           fixed_version
+                         ),
+                         order: 'work_packages.root_id DESC, work_packages.lft ASC'
   end
   let(:project_1) { FactoryGirl.create :project }
   let(:role_pm) do
     FactoryGirl.create(:role,
-                       permissions: [
-                         :view_work_packages,
-                         :edit_work_packages,
-                         :create_work_packages,
-                         :delete_work_packages
-                       ])
+                       permissions: %i(
+                         view_work_packages
+                         edit_work_packages
+                         create_work_packages
+                         delete_work_packages
+                       ))
   end
   let(:role_dev) do
     FactoryGirl.create(:role,
@@ -172,8 +175,7 @@ describe ::Query::Results, type: :model do
 
         it 'returns all work packages of project 2' do
           work_packages = query
-                          .results(include: [:assigned_to, { custom_values: :custom_field }],
-                                   order: 'work_packages.root_id, work_packages.lft')
+                          .results
                           .work_packages
                           .page(1)
                           .per_page(10)
@@ -191,8 +193,7 @@ describe ::Query::Results, type: :model do
 
         it 'returns all work packages of project 2' do
           work_packages = query
-                          .results(include: [:responsible, { custom_values: :custom_field }],
-                                   order: 'work_packages.root_id, work_packages.lft')
+                          .results
                           .work_packages
                           .page(1)
                           .per_page(10)
@@ -225,6 +226,166 @@ describe ::Query::Results, type: :model do
     end
   end
 
+  describe '#sorted_work_packages' do
+    let(:work_package1) { FactoryGirl.create(:work_package, project: project_1) }
+    let(:work_package2) { FactoryGirl.create(:work_package, project: project_1) }
+    let(:work_package3) { FactoryGirl.create(:work_package, project: project_1) }
+    let(:sort_by) { [['parent', 'asc']] }
+    let(:columns) { %i(id subject) }
+
+    let(:query) do
+      FactoryGirl.build_stubbed :query,
+                                show_hierarchies: false,
+                                group_by: group_by,
+                                sort_criteria: sort_by,
+                                project: project_1,
+                                column_names: columns
+    end
+
+    let(:query_results) do
+      ::Query::Results.new query
+    end
+
+    let(:user_a) { FactoryGirl.create(:user, firstname: 'AAA', lastname: 'AAA') }
+    let(:user_m) { FactoryGirl.create(:user, firstname: 'MMM', lastname: 'MMM') }
+    let(:user_z) { FactoryGirl.create(:user, firstname: 'ZZZ', lastname: 'ZZZ') }
+
+    context 'grouping by assigned_to, having the author column selected' do
+      let(:group_by) { 'assigned_to' }
+      let(:columns) { %i(id subject author) }
+
+      before do
+        allow(User).to receive(:current).and_return(user_1)
+
+        work_package1.assigned_to = user_m
+        work_package1.author = user_m
+
+        work_package1.save(validate: false)
+
+        work_package2.assigned_to = user_z
+        work_package2.author = user_a
+
+        work_package2.save(validate: false)
+
+        work_package3.assigned_to = user_m
+        work_package3.author = user_a
+
+        work_package3.save(validate: false)
+      end
+
+      it 'sorts first by assigned_to (group by), then by sort criteria' do
+        # Would look like this in the table
+        #
+        # user_m
+        #   work_package 1
+        #   work_package 3
+        # user_z
+        #   work_package 2
+        expect(query_results.sorted_work_packages)
+          .to match [work_package1, work_package3, work_package2]
+      end
+    end
+
+    context 'sorting by author, grouping by assigned_to' do
+      let(:group_by) { 'assigned_to' }
+      let(:sort_by) { [['author', 'asc']] }
+
+      before do
+        allow(User).to receive(:current).and_return(user_1)
+
+        work_package1.assigned_to = user_m
+        work_package1.author = user_m
+
+        work_package1.save(validate: false)
+
+        work_package2.assigned_to = user_z
+        work_package2.author = user_a
+
+        work_package2.save(validate: false)
+
+        work_package3.assigned_to = user_m
+        work_package3.author = user_a
+
+        work_package3.save(validate: false)
+      end
+
+      it 'sorts first by group by, then by assigned_to' do
+        # Would look like this in the table
+        #
+        # user_m
+        #   work_package 3
+        #   work_package 1
+        # user_z
+        #   work_package 2
+        expect(query_results.sorted_work_packages)
+          .to match [work_package3, work_package1, work_package2]
+
+        query.sort_criteria = [['author', 'desc']]
+
+        # Would look like this in the table
+        #
+        # user_m
+        #   work_package 1
+        #   work_package 3
+        # user_z
+        #   work_package 2
+        expect(query_results.sorted_work_packages)
+          .to match [work_package1, work_package3, work_package2]
+      end
+    end
+
+    context 'sorting by author and responsible, grouping by assigned_to' do
+      let(:group_by) { 'assigned_to' }
+      let(:sort_by) { [['author', 'asc'], ['responsible', 'desc']] }
+
+      before do
+        allow(User).to receive(:current).and_return(user_1)
+
+        work_package1.assigned_to = user_m
+        work_package1.author = user_m
+        work_package1.responsible = user_a
+
+        work_package1.save(validate: false)
+
+        work_package2.assigned_to = user_z
+        work_package2.author = user_m
+        work_package3.responsible = user_m
+
+        work_package2.save(validate: false)
+
+        work_package3.assigned_to = user_m
+        work_package3.author = user_m
+        work_package3.responsible = user_z
+
+        work_package3.save(validate: false)
+      end
+
+      it 'sorts first by group by, then by assigned_to (neutral as equal), then by responsible' do
+        # Would look like this in the table
+        #
+        # user_m
+        #   work_package 3
+        #   work_package 1
+        # user_z
+        #   work_package 2
+        expect(query_results.sorted_work_packages)
+          .to match [work_package3, work_package1, work_package2]
+
+        query.sort_criteria = [['author', 'desc'], ['responsible', 'asc']]
+
+        # Would look like this in the table
+        #
+        # user_m
+        #   work_package 1
+        #   work_package 3
+        # user_z
+        #   work_package 2
+        expect(query_results.sorted_work_packages)
+          .to match [work_package1, work_package3, work_package2]
+      end
+    end
+  end
+
   # Introduced to ensure being able to group by custom fields
   # when running on a MySQL server.
   # When upgrading to rails 5, the sql_mode passed on with the connection