[26675] Remove implicit ID sorting on parent sort (#6039)

* [26675] Remove implicit ID sorting on parent sort

https://community.openproject.com/wp/26675

* parent sort -> breadth first sort, top down

[ci skip]

app/models/queries/columns/base.rb

@@ -29,18 +29,22 @@
 #++
 
 class Queries::Columns::Base
+  attr_reader :groupable,
+              :sortable,
+              :association
+
   attr_accessor :name,
-                :sortable,
-                :groupable,
+                :sortable_join,
                 :summable,
-                :default_order,
-                :association
+                :default_order
+
   alias_method :summable?, :summable
 
   def initialize(name, options = {})
     self.name = name
 
     %i(sortable
+       sortable_join
        groupable
        summable
        association

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

@@ -55,10 +55,17 @@ class Queries::WorkPackages::Columns::PropertyColumn < Queries::WorkPackages::Co
     },
     parent: {
       association: 'ancestors_relations',
+      default_order: 'asc',
       sortable: ["COALESCE(#{Relation.table_name}.from_id, #{WorkPackage.table_name}.id)",
-                 "#{Relation.table_name}.hierarchy",
-                 "#{WorkPackage.table_name}.id"],
-      default_order: 'asc'
+                 "COALESCE(#{Relation.table_name}.hierarchy, 0)"],
+      sortable_join: <<-SQL
+        JOIN (
+          SELECT relations.hierarchy, relations.to_id, relations.from_id from relations
+        JOIN (
+          #{Relation.hierarchy_or_reflexive.group(:to_id).select('MAX(relations.hierarchy) hierarchy, relations.to_id').to_sql}) max_depth
+        ON max_depth.hierarchy = relations.hierarchy AND max_depth.to_id = relations.to_id) depth_relations
+        ON depth_relations.to_id = work_packages.id
+      SQL
     },
     status: {
       association: 'status',

app/models/query.rb

@@ -93,7 +93,7 @@ class Query < ActiveRecord::Base
   def set_default_sort
     return if sort_criteria.any?
 
-    self.sort_criteria = [['parent', 'asc']]
+    self.sort_criteria = [['parent', 'asc'], ['id', 'asc']]
   end
 
   def context

app/models/query/results.rb

@@ -75,6 +75,7 @@ class ::Query::Results
       .where(query.statement)
       .where(options[:conditions])
       .includes(all_includes)
+      .joins(all_joins)
       .order(order_option)
       .references(:projects)
   end
@@ -117,11 +118,11 @@ class ::Query::Results
     return nil unless query.grouped?
 
     group_work_packages = work_packages.select { |wp| query.group_by_column.value(wp) == group }
-    query.available_columns.inject({}) { |result, column|
+    query.available_columns.inject({}) do |result, column|
       sum = sum_of(column, group_work_packages)
       result[column] = sum unless sum.nil?
       result
-    }
+    end
   end
 
   def column_group_sums
@@ -143,6 +144,10 @@ class ::Query::Results
       (options[:include] || [])).uniq
   end
 
+  def all_joins
+    query.sort_criteria_columns.map { |column, _direction| column.sortable_join }.compact
+  end
+
   def includes_for_columns(column_names)
     column_names = Array(column_names)
     (WorkPackage.reflections.keys.map(&:to_sym) & column_names.map(&:to_sym))

app/models/relation.rb

@@ -123,6 +123,10 @@ class Relation < ActiveRecord::Base
       .non_reflexive
   end
 
+  def self.hierarchy_or_reflexive
+    with_type_columns_0(WorkPackage._dag_options.type_columns - %i(hierarchy))
+  end
+
   def self.non_hierarchy_of_work_package(work_package)
     of_work_package(work_package)
       .non_hierarchy

frontend/app/components/modals/sorting-modal/sorting-modal.service.html

@@ -9,7 +9,7 @@
       <div id="modal-sorting"
            class="modal-content-container loading-indicator--location"
            data-indicator-name="sorting-modal">
-        <div class="form--row" ng-repeat="sort in sortationObjects">
+        <div class="form--row modal-sorting-row-{{$index}}" ng-repeat="sort in sortationObjects">
           <div class="form--field -full-width">
             <label
                 for="modal-sorting-attribute-{{$index}}"

spec/features/work_packages/table_sorting_spec.rb

@@ -29,51 +29,106 @@
 require 'spec_helper'
 require 'features/work_packages/work_packages_page'
 
-describe 'Select work package row', type: :feature do
+describe 'Select work package row', type: :feature, js: true do
   let(:user) { FactoryGirl.create(:admin) }
   let(:project) { FactoryGirl.create(:project) }
-  let(:work_package_1) do
-    FactoryGirl.create(:work_package, project: project)
-  end
-  let(:work_package_2) do
-    FactoryGirl.create(:work_package, project: project)
-  end
   let(:work_packages_page) { WorkPackagesPage.new(project) }
+  let(:wp_table) { Pages::WorkPackagesTable.new(project) }
 
-  let(:version_1) do
-    FactoryGirl.create(:version, project: project,
-                                 name: 'aaa_version')
-  end
-  let(:version_2) do
-    FactoryGirl.create(:version, project: project,
-                                 name: 'zzz_version')
-  end
-  let(:columns) { ::Components::WorkPackages::Columns.new }
+  describe 'sorting by version' do
+    let(:work_package_1) do
+      FactoryGirl.create(:work_package, project: project)
+    end
+    let(:work_package_2) do
+      FactoryGirl.create(:work_package, project: project)
+    end
 
-  before do
-    login_as(user)
+    let(:version_1) do
+      FactoryGirl.create(:version, project: project,
+                                   name: 'aaa_version')
+    end
+    let(:version_2) do
+      FactoryGirl.create(:version, project: project,
+                                   name: 'zzz_version')
+    end
+    let(:columns) { ::Components::WorkPackages::Columns.new }
+
+    before do
+      login_as(user)
+
+      work_package_1
+      work_package_2
 
-    work_package_1
-    work_package_2
+      work_packages_page.visit_index
+    end
 
-    work_packages_page.visit_index
+    include_context 'ui-select helpers'
+    include_context 'work package table helpers'
+
+    context 'sorting by version' do
+      before do
+        work_package_1.update_attribute(:fixed_version_id, version_2.id)
+        work_package_2.update_attribute(:fixed_version_id, version_1.id)
+      end
+
+      it 'sorts by version although version is not selected as a column' do
+        columns.remove 'Version'
+
+        sort_wp_table_by('Version')
+
+        expect_work_packages_to_be_in_order([work_package_1, work_package_2])
+      end
+    end
   end
 
-  include_context 'ui-select helpers'
-  include_context 'work package table helpers'
+  describe 'parent sorting' do
+    let(:sort_by) { ::Components::WorkPackages::SortBy.new }
+
+    let(:parent) do
+      FactoryGirl.create :work_package,
+                         project: project
+    end
+    let(:child1) do
+      FactoryGirl.create :work_package,
+                         project: project,
+                         parent: parent,
+                         start_date: Date.today,
+                         due_date: (Date.today + 2.days)
+    end
+    let(:child2) do
+      FactoryGirl.create :work_package,
+                         project: project,
+                         parent: parent,
+                         start_date: (Date.today + 1.days),
+                         due_date: (Date.today + 4.days)
+    end
+    let(:child3) do
+      FactoryGirl.create :work_package,
+                         project: project,
+                         parent: parent,
+                         start_date: (Date.today + 2.days),
+                         due_date: (Date.today + 5.days)
+    end
 
-  context 'sorting by version', js: true do
     before do
-      work_package_1.update_attribute(:fixed_version_id, version_2.id)
-      work_package_2.update_attribute(:fixed_version_id, version_1.id)
+      parent
+      child1
+      child2
+      child3
+
+      login_as user
+      wp_table.visit!
     end
 
-    it 'sorts by version although version is not selected as a column' do
-      columns.remove 'Version'
+    it 'provides the default sortation and allows second criterion after parent' do
+      wp_table.expect_work_package_listed parent, child1, child2, child3
+      wp_table.expect_work_package_order parent.id, child1.id, child2.id, child3.id
 
-      sort_wp_table_by('Version')
+      sort_by.expect_criteria(['Parent', 'asc'], ['ID', 'asc'])
+      sort_by.update_criteria(['Parent', 'asc'], ['Start date', 'desc'])
 
-      expect_work_packages_to_be_in_order([work_package_1, work_package_2])
+      wp_table.expect_work_package_listed parent, child1, child2, child3
+      wp_table.expect_work_package_order parent.id, child3.id, child2.id, child1.id
     end
   end
 end

spec/models/query/results_spec.rb

@@ -386,6 +386,63 @@ describe ::Query::Results, type: :model do
       end
     end
 
+    context 'sorting by parent' do
+      let(:work_package1) { FactoryGirl.create(:work_package, project: project_1, subject: '1') }
+      let(:work_package2) { FactoryGirl.create(:work_package, project: project_1, parent: work_package1, subject: '2') }
+      let(:work_package3) { FactoryGirl.create(:work_package, project: project_1, parent: work_package2, subject: '3') }
+      let(:work_package4) { FactoryGirl.create(:work_package, project: project_1, parent: work_package1, subject: '4') }
+      let(:work_package5) { FactoryGirl.create(:work_package, project: project_1, parent: work_package4, subject: '5') }
+      let(:work_package6) { FactoryGirl.create(:work_package, project: project_1, parent: work_package4, subject: '6') }
+      let(:work_package7) { FactoryGirl.create(:work_package, project: project_1, subject: '7') }
+      let(:work_package8) { FactoryGirl.create(:work_package, project: project_1, subject: '8') }
+      let(:work_package9) { FactoryGirl.create(:work_package, project: project_1, parent: work_package8, subject: '9') }
+
+      # have to sort by a second criteria as the order within each level (except for the first)
+      # is undefined without
+      let(:sort_by) { [['parent', 'asc'], ['subject', 'asc']] }
+
+      before do
+        allow(User).to receive(:current).and_return(user_1)
+
+        # intentionally messing with the id
+        work_package8
+        work_package9
+        work_package1
+        work_package4
+        work_package5
+        work_package3
+        work_package6
+        work_package2
+        work_package7
+      end
+
+      it 'sorts breadth first by parent where each level, except for the first, is orderd by the second criteria' do
+        expect(query_results.sorted_work_packages)
+          .to match [work_package8,
+                     work_package9,
+                     work_package1,
+                     work_package2,
+                     work_package4,
+                     work_package3,
+                     work_package5,
+                     work_package6,
+                     work_package7]
+
+        query.sort_criteria = [['parent', 'desc'], ['subject', 'asc']]
+
+        expect(query_results.sorted_work_packages)
+          .to match [work_package7,
+                     work_package3,
+                     work_package5,
+                     work_package6,
+                     work_package2,
+                     work_package4,
+                     work_package1,
+                     work_package9,
+                     work_package8]
+      end
+    end
+
     context 'filtering by bool cf' do
       let(:bool_cf) { FactoryGirl.create(:bool_wp_custom_field, is_filter: true) }
       let(:custom_value) do

spec/models/query_spec.rb

@@ -45,7 +45,7 @@ describe Query, type: :model do
       query = Query.new_default
 
       expect(query.sort_criteria)
-        .to match_array([['parent', 'asc']])
+        .to match_array([['parent', 'asc'], ['id', 'asc']])
     end
 
     it 'does not use the default sortation if an order is provided' do

spec/support/components/work_packages/sort_by.rb

@@ -0,0 +1,117 @@
+#-- 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 Components
+  module WorkPackages
+    class SortBy
+      include Capybara::DSL
+      include RSpec::Matchers
+
+      def sort_via_header(name, descending: false)
+        text = descending ? 'Sort descending' : 'Sort ascending'
+
+        open_table_column_context_menu(name)
+
+        within_column_context_menu do
+          click_link text
+        end
+      end
+
+      def update_criteria(first, second=nil, third=nil)
+        open_modal
+
+        [first, second, third]
+          .compact
+          .each_with_index do |entry, i|
+
+          column, direction = entry
+          update_nth_criteria(i, column, descending: descending?(direction))
+        end
+
+        apply_changes
+      end
+
+      def expect_criteria(first, second=nil, third=nil)
+        open_modal
+
+        [first, second, third]
+          .compact
+          .each_with_index do |entry, i|
+
+          column, direction = entry
+          page.within(".modal-sorting-row-#{i}") do
+            expect(page).to have_selector("#modal-sorting-attribute-#{i} option", text: column)
+            checked_radio = (descending?(direction) ? 'Descending' : 'Ascending')
+            expect(page.find_field(checked_radio)).to be_checked
+          end
+        end
+
+        cancel_changes
+      end
+
+      def update_nth_criteria(i, column, descending: false)
+        page.within(".modal-sorting-row-#{i}") do
+          select column, from: "modal-sorting-attribute-#{i}"
+          choose(descending ? 'Descending' : 'Ascending')
+        end
+      end
+
+      def open_modal
+        SettingsMenu.new.open_and_choose('Sort by ...')
+      end
+
+      def cancel_changes
+        page.within('.ng-modal-inner') do
+          click_on 'Cancel'
+        end
+      end
+
+      def apply_changes
+        page.within('.ng-modal-inner') do
+          click_on 'Apply'
+        end
+      end
+
+      private
+
+      def descending?(direction)
+        ['desc', 'descending'].include?(direction.to_s)
+      end
+
+      def open_table_column_context_menu(name)
+        page.find(".generic-table--sort-header ##{name.downcase}").click
+      end
+
+      def within_column_context_menu
+        page.within('#column-context-menu') do
+          yield
+        end
+      end
+    end
+  end
+end