From 2c217e826ad5ba5a4fd762f682ef5cae0e342ecb Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 24 Oct 2026 19:39:39 +0100 Subject: [PATCH 1/6] [DREAM-941] Compute canonical hierarchy positions Adds WorkPackage::HierarchyRowPositions, which numbers every result of a hierarchy query and every visible contextual ancestor in depth-first order. Sibling subtrees follow the earliest query rank of any matching work package they contain, so a child ranked before its parent pulls the parent's subtree forward. Each row's parent is its nearest visible ancestor, matching the ancestor chain the frontend renders. A result the user can no longer see, as a historic query may return, is numbered as a root, so the numbering discloses no current parent of a work package the collection itself redacts. Asking for a row the numbering does not know returns nothing, so a work package that stopped matching between paging and numbering cannot fail the request. https://community.openproject.org/wp/DREAM-941 --- .../work_package/hierarchy_row_positions.rb | 114 +++++++++++++ .../hierarchy_row_positions_spec.rb | 157 ++++++++++++++++++ 2 files changed, 271 insertions(+) create mode 100644 app/models/work_package/hierarchy_row_positions.rb create mode 100644 spec/models/work_package/hierarchy_row_positions_spec.rb diff --git a/app/models/work_package/hierarchy_row_positions.rb b/app/models/work_package/hierarchy_row_positions.rb new file mode 100644 index 000000000000..dba322251661 --- /dev/null +++ b/app/models/work_package/hierarchy_row_positions.rb @@ -0,0 +1,114 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# 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-2013 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, see . +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +class WorkPackage::HierarchyRowPositions + MAX_RESULTS = 10_000 + + def initialize(work_packages, user) + @work_packages = work_packages + @user = user + end + + def count + positions.size + end + + def entries_for(ids) + nodes = ids.map(&:to_i) + return unless nodes.all? { |node| positions.key?(node) } + + nodes.each_with_object({}) { |node, entries| add_with_ancestors(node, entries) } + end + + private + + attr_reader :work_packages, :user + + def add_with_ancestors(node, entries) + while node && !entries.key?(node.to_s) + entries[node.to_s] = { position: positions.fetch(node), parent: parents[node]&.to_s } + node = parents[node] + end + end + + def positions + tree.first + end + + def parents + tree.last + end + + def tree + @tree ||= begin + ranks = work_packages.pluck(:id).each_with_index.to_h + parents, keys = link_ancestors(ranks) + [number_depth_first(parents, keys), parents] + end + end + + def link_ancestors(ranks) + parents = {} + keys = ranks.dup + visible_ancestor_chains(ranks).each do |id, chain| + [nil, *chain, id].each_cons(2) { |parent, child| parents[child] = parent } + chain.each { |ancestor| keys[ancestor] = [keys[ancestor], ranks[id]].compact.min } + end + [parents, keys] + end + + def number_depth_first(parents, keys) + children = keys.keys.sort_by { |id| keys[id] }.group_by { |id| parents[id] } + numbered = {} + stack = children.fetch(nil, []).reverse + while (id = stack.pop) + numbered[id] = numbered.size + 1 + stack.concat(children.fetch(id, []).reverse) + end + numbered + end + + def visible_ancestor_chains(ranks) + return {} if ranks.empty? + + WorkPackageHierarchy + .joins("INNER JOIN work_packages ancestors ON ancestors.id = work_package_hierarchies.ancestor_id") + .where(descendant_id: visible_results) + .where("work_package_hierarchies.generations > 0") + .where(ancestors: { project_id: Project.allowed_to(user, :view_work_packages) }) + .order(generations: :desc) + .pluck(:descendant_id, :ancestor_id) + .group_by(&:first) + .transform_values { |edges| edges.map(&:last) } + end + + def visible_results + WorkPackage.visible(user).where(id: work_packages.unscope(:order).reselect(:id)) + end +end diff --git a/spec/models/work_package/hierarchy_row_positions_spec.rb b/spec/models/work_package/hierarchy_row_positions_spec.rb new file mode 100644 index 000000000000..abfc678354c2 --- /dev/null +++ b/spec/models/work_package/hierarchy_row_positions_spec.rb @@ -0,0 +1,157 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# 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-2013 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, see . +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe WorkPackage::HierarchyRowPositions, with_settings: { cross_project_work_package_relations: true } do + shared_let(:project) { create(:project) } + shared_let(:hidden_project) { create(:project) } + shared_let(:user) { create(:user, member_with_permissions: { project => [:view_work_packages] }) } + + subject(:positions) { described_class.new(ranked(*results), user) } + + def ranked(*work_packages) + ids = work_packages.map(&:id) + WorkPackage + .where(id: ids) + .order(Arel.sql("array_position(ARRAY[#{ids.map(&:to_i).join(',')}]::integer[], work_packages.id)")) + end + + def entry(position, parent = nil) + { position:, parent: parent&.id&.to_s } + end + + def all_entries + positions.entries_for(results.map(&:id)) + end + + context "with flat roots" do + let(:first) { create(:work_package, project:) } + let(:second) { create(:work_package, project:) } + let(:results) { [first, second] } + + it "numbers them in query order" do + expect(positions.count).to eq(2) + expect(all_entries).to eq(first.id.to_s => entry(1), second.id.to_s => entry(2)) + end + end + + context "with a child ranked before its parent" do + let(:parent) { create(:work_package, project:) } + let(:child) { create(:work_package, project:, parent:) } + let(:other) { create(:work_package, project:) } + let(:results) { [child, other, parent] } + + it "places the parent subtree at the child's rank" do + expect(positions.count).to eq(3) + expect(all_entries).to eq(parent.id.to_s => entry(1), + child.id.to_s => entry(2, parent), + other.id.to_s => entry(3)) + end + end + + context "with a parent that does not match" do + let(:parent) { create(:work_package, project:) } + let(:child) { create(:work_package, project:, parent:) } + let(:results) { [child] } + + it "counts the contextual ancestor" do + expect(positions.count).to eq(2) + expect(all_entries).to eq(parent.id.to_s => entry(1), child.id.to_s => entry(2, parent)) + end + end + + context "with a nested hierarchy" do + let(:root) { create(:work_package, project:) } + let(:middle) { create(:work_package, project:, parent: root) } + let(:grandchild) { create(:work_package, project:, parent: middle) } + let(:second_root) { create(:work_package, project:) } + let(:results) { [grandchild, second_root] } + + it "inserts every contextual ancestor depth first" do + expect(positions.count).to eq(4) + expect(all_entries).to eq(root.id.to_s => entry(1), + middle.id.to_s => entry(2, root), + grandchild.id.to_s => entry(3, middle), + second_root.id.to_s => entry(4)) + end + end + + context "with results sharing a contextual ancestor" do + let(:parent) { create(:work_package, project:) } + let(:first_child) { create(:work_package, project:, parent:) } + let(:second_child) { create(:work_package, project:, parent:) } + let(:other) { create(:work_package, project:) } + let(:results) { [first_child, other, second_child] } + + it "counts the ancestor once and keeps siblings in query order" do + expect(positions.count).to eq(4) + expect(all_entries).to eq(parent.id.to_s => entry(1), + first_child.id.to_s => entry(2, parent), + second_child.id.to_s => entry(3, parent), + other.id.to_s => entry(4)) + end + end + + context "with an invisible middle ancestor" do + let(:grandparent) { create(:work_package, project:) } + let(:hidden_parent) { create(:work_package, project: hidden_project, parent: grandparent) } + let(:leaf) { create(:work_package, project:, parent: hidden_parent) } + let(:results) { [leaf] } + + it "parents the row to its nearest visible ancestor" do + expect(positions.count).to eq(2) + expect(all_entries).to eq(grandparent.id.to_s => entry(1), leaf.id.to_s => entry(2, grandparent)) + end + end + + context "without results" do + let(:results) { [] } + + it "has no rows" do + expect(positions.count).to eq(0) + expect(positions.entries_for([])).to eq({}) + end + end + + describe "#entries_for" do + let(:parent) { create(:work_package, project:) } + let(:child) { create(:work_package, project:, parent:) } + let(:other) { create(:work_package, project:) } + let(:results) { [child, other] } + + it "returns the given rows and their visible ancestors only" do + expect(positions.entries_for([child.id])).to eq(parent.id.to_s => entry(1), child.id.to_s => entry(2, parent)) + end + + it "returns nothing when a row is not among the numbered results" do + expect(positions.entries_for([child.id, parent.id + 1000])).to be_nil + end + end +end From a0db14a7143b3a7adc742e2179350506db224020 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 24 Oct 2026 19:45:13 +0100 Subject: [PATCH 2/6] [DREAM-941] Expose hierarchy row positions in API Adds rowCount and rowPositions to work package collections in hierarchy mode. rowCount covers every result and every visible contextual ancestor of the whole query; rowPositions gives each row on the page, and each ancestor it renders under, its global position and nearest visible parent. Both are computed from the scoped result set and omitted above 10,000 results, where numbering the whole query costs too much per request, or when the page holds a row the numbering does not know. https://community.openproject.org/wp/DREAM-941 --- ...k_package_collection_from_query_service.rb | 13 +- .../schemas/work_packages_model.yml | 32 ++++ .../work_package_collection_representer.rb | 19 ++ ...ork_package_collection_representer_spec.rb | 32 ++++ .../index_row_positions_resource_spec.rb | 170 ++++++++++++++++++ ...kage_collection_from_query_service_spec.rb | 33 ++++ 6 files changed, 297 insertions(+), 2 deletions(-) create mode 100644 spec/requests/api/v3/work_packages/index_row_positions_resource_spec.rb diff --git a/app/services/api/v3/work_package_collection_from_query_service.rb b/app/services/api/v3/work_package_collection_from_query_service.rb index 8df74d734624..bf69d904f75e 100644 --- a/app/services/api/v3/work_package_collection_from_query_service.rb +++ b/app/services/api/v3/work_package_collection_from_query_service.rb @@ -66,7 +66,8 @@ def results_to_representer(params) params:, project: query.project, groups: generate_groups, - sums: generate_total_sums) + sums: generate_total_sums, + row_positions: generate_row_positions(results_scope)) end attr_accessor :query, @@ -104,6 +105,13 @@ def generate_groups end end + def generate_row_positions(results_scope) + return unless query.show_hierarchies + return if results_scope.count > ::WorkPackage::HierarchyRowPositions::MAX_RESULTS + + ::WorkPackage::HierarchyRowPositions.new(results_scope, current_user) + end + def generate_total_sums return unless query.display_sums? @@ -138,7 +146,7 @@ def column_name(column) end end - def collection_representer(work_packages, params:, project:, groups:, sums:) + def collection_representer(work_packages, params:, project:, groups:, sums:, row_positions:) resulting_params = calculate_resulting_params(params) if resulting_params[:select] @@ -158,6 +166,7 @@ def collection_representer(work_packages, params:, project:, groups:, sums:) per_page: resulting_params[:pageSize], groups:, total_sums: sums, + row_positions:, embed_schemas: true, current_user:, timestamps: query.timestamps, diff --git a/docs/api/apiv3/components/schemas/work_packages_model.yml b/docs/api/apiv3/components/schemas/work_packages_model.yml index 75690cc77019..4793ddba0e07 100644 --- a/docs/api/apiv3/components/schemas/work_packages_model.yml +++ b/docs/api/apiv3/components/schemas/work_packages_model.yml @@ -7,6 +7,38 @@ allOf: - _links - _embedded properties: + rowCount: + type: integer + minimum: 0 + readOnly: true + description: |- + Number of table rows the whole collection renders in hierarchy mode: every element across all pages plus + every visible ancestor shown for context, each counted once. + + Only present when `showHierarchies` is set and the collection has at most 10,000 elements. + rowPositions: + type: object + readOnly: true + description: |- + Row of each element on this page, and of each visible ancestor those elements are shown under, in the + collection's hierarchy order. Rows are ordered depth first; sibling subtrees follow the earliest position in + the collection's sort order of any element they contain. Keys are work package ids. `position` counts from + 1 across all pages; `parent` is the id of the nearest visible ancestor, or `null` for a root. + + Present whenever `rowCount` is. + additionalProperties: + type: object + required: + - position + - parent + properties: + position: + type: integer + minimum: 1 + parent: + type: + - "string" + - "null" _links: type: object required: diff --git a/lib/api/v3/work_packages/work_package_collection_representer.rb b/lib/api/v3/work_packages/work_package_collection_representer.rb index d79e3077c5ed..1a620eb8fea5 100644 --- a/lib/api/v3/work_packages/work_package_collection_representer.rb +++ b/lib/api/v3/work_packages/work_package_collection_representer.rb @@ -39,6 +39,7 @@ def initialize(models, groups:, total_sums:, current_user:, + row_positions: nil, query_params: {}, project: nil, page: nil, @@ -48,6 +49,7 @@ def initialize(models, query: nil) @project = project @total_sums = total_sums + @row_positions = row_positions @embed_schemas = embed_schemas @timestamps = timestamps @query = query @@ -154,6 +156,16 @@ def initialize(models, }, render_nil: false + property :row_count, + exec_context: :decorator, + getter: ->(*) { row_positions.count if row_position_entries }, + render_nil: false + + property :row_positions, + exec_context: :decorator, + getter: ->(*) { row_position_entries }, + render_nil: false + def current_user_allowed_to_add_work_packages? if project current_user.allowed_in_project?(:add_work_packages, project) @@ -304,12 +316,19 @@ def representation_format_atom mime_type: "application/atom+xml" end + def row_position_entries + return @row_position_entries if defined?(@row_position_entries) + + @row_position_entries = row_positions&.entries_for(represented.map(&:id)) + end + def timestamps_active? timestamps.present? && timestamps.any?(&:historic?) end attr_reader :project, :total_sums, + :row_positions, :embed_schemas private diff --git a/spec/lib/api/v3/work_packages/work_package_collection_representer_spec.rb b/spec/lib/api/v3/work_packages/work_package_collection_representer_spec.rb index ed5cb8735bdd..15e0417d4e3f 100644 --- a/spec/lib/api/v3/work_packages/work_package_collection_representer_spec.rb +++ b/spec/lib/api/v3/work_packages/work_package_collection_representer_spec.rb @@ -45,6 +45,7 @@ let(:query_params) { {} } let(:groups) { nil } let(:total_sums) { nil } + let(:row_positions) { nil } let(:project) { nil } let(:page_parameter) { nil } @@ -65,6 +66,7 @@ project:, groups:, total_sums:, + row_positions:, page: page_parameter, per_page: page_size_parameter, current_user: user, @@ -263,6 +265,11 @@ expect(collection).not_to have_json_path("totalSums") end + it "does not render row positions" do + expect(collection).not_to have_json_path("rowCount") + expect(collection).not_to have_json_path("rowPositions") + end + it "has a schemas link" do # All work packages in the collection are from the same project and have the same type path = api_v3_paths.work_package_schemas [first_wp.project_id, first_wp.type_id] @@ -485,6 +492,31 @@ end end + context "when passing row positions" do + let(:row_positions) { instance_double(WorkPackage::HierarchyRowPositions, count: 7) } + let(:entries) do + work_packages.each_with_index.to_h { |wp, index| [wp.id.to_s, { position: index + 2, parent: nil }] } + end + + before do + allow(row_positions).to receive(:entries_for).with(work_packages.map(&:id)).and_return(entries) + end + + it "renders the row count and the entries for the represented work packages" do + expect(collection).to be_json_eql(7.to_json).at_path("rowCount") + expect(collection).to be_json_eql(entries.to_json).at_path("rowPositions") + end + + context "without entries for the represented work packages" do + let(:entries) { nil } + + it "renders neither property" do + expect(collection).not_to have_json_path("rowCount") + expect(collection).not_to have_json_path("rowPositions") + end + end + end + context "when passing schemas" do let(:embed_schemas) { true } diff --git a/spec/requests/api/v3/work_packages/index_row_positions_resource_spec.rb b/spec/requests/api/v3/work_packages/index_row_positions_resource_spec.rb new file mode 100644 index 000000000000..6947209418dc --- /dev/null +++ b/spec/requests/api/v3/work_packages/index_row_positions_resource_spec.rb @@ -0,0 +1,170 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# 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-2013 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, see . +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" +require "rack/test" + +RSpec.describe "GET api/v3/workspaces/:id/work_packages row positions", content_type: :json do + include Rack::Test::Methods + include API::V3::Utilities::PathHelper + + shared_let(:project) { create(:project, :with_types) } + shared_let(:open_status) { create(:status, is_closed: false) } + shared_let(:closed_status) { create(:closed_status) } + + shared_let(:parent) { create(:work_package, project:, subject: "C parent", status: open_status) } + shared_let(:child) { create(:work_package, project:, subject: "A child", parent:, status: open_status) } + shared_let(:other) { create(:work_package, project:, subject: "B other", status: open_status) } + + current_user { create(:user, member_with_permissions: { project => [:view_work_packages] }) } + + let(:params) { { showHierarchies: true, filters: "[]", sortBy: '[["subject", "asc"]]', pageSize: 20, offset: 1 } } + let(:body) { JSON.parse(last_response.body) } + + def entry(position, parent = nil) + { "position" => position, "parent" => parent&.id&.to_s } + end + + before do + get "#{api_v3_paths.work_packages_by_workspace(project.id)}?#{params.to_query}" + end + + it "numbers the results in canonical hierarchy order" do + expect(body["rowCount"]).to eq(3) + expect(body["rowPositions"]).to eq(parent.id.to_s => entry(1), + child.id.to_s => entry(2, parent), + other.id.to_s => entry(3)) + end + + context "when paginated" do + let(:params) { super().merge(pageSize: 2) } + + it "describes the page's rows and the ancestors they render" do + expect(body["rowCount"]).to eq(3) + expect(body["rowPositions"]).to eq(parent.id.to_s => entry(1), + child.id.to_s => entry(2, parent), + other.id.to_s => entry(3)) + end + + context "on the second page" do + let(:params) { super().merge(offset: 2) } + + it "keeps the parent's position" do + expect(body["rowCount"]).to eq(3) + expect(body["rowPositions"]).to eq(parent.id.to_s => entry(1)) + end + end + end + + context "when the parent does not match the filters" do + let(:params) do + super().merge(filters: [{ subject: { operator: "~", values: ["child"] } }].to_json) + end + + it "counts the parent as a contextual ancestor" do + expect(body["rowCount"]).to eq(2) + expect(body["rowPositions"]).to eq(parent.id.to_s => entry(1), child.id.to_s => entry(2, parent)) + end + end + + context "without results" do + let(:params) do + super().merge(filters: [{ subject: { operator: "~", values: ["nothing matches"] } }].to_json) + end + + it "has no rows" do + expect(body["rowCount"]).to eq(0) + expect(body["rowPositions"]).to eq({}) + end + end + + context "without hierarchies" do + let(:params) { super().merge(showHierarchies: false) } + + it "omits row positions" do + expect(body).not_to include("rowCount", "rowPositions") + end + end + + context "when grouped" do + let(:params) { super().merge(groupBy: "status", showHierarchies: false) } + + it "omits row positions" do + expect(body).not_to include("rowCount", "rowPositions") + end + end + + context "with baseline timestamps", with_ee: %i[baseline_comparison] do + let(:params) { super().merge(timestamps: "2015-01-01T00:00:00Z,PT0S") } + + it "numbers the results in canonical hierarchy order" do + expect(body["rowCount"]).to eq(3) + expect(body["rowPositions"]).to eq(parent.id.to_s => entry(1), + child.id.to_s => entry(2, parent), + other.id.to_s => entry(3)) + end + + context "when a result is no longer visible", with_settings: { cross_project_work_package_relations: true } do + shared_let(:hidden_project) { create(:project) } + let(:baseline) { Time.zone.parse("2015-01-01") } + let!(:hidden) do + create(:work_package, project:, subject: "D hidden", status: open_status, created_at: baseline - 1.day, + journals: { baseline - 1.day => {}, 1.day.ago => {} }).tap do |work_package| + work_package.update_column(:project_id, hidden_project.id) + work_package.journals.last.data.update_column(:project_id, hidden_project.id) + work_package.update!(parent:) + end + end + let(:params) do + super().merge(filters: [{ subject: { operator: "~", values: ["hidden"] } }].to_json) + end + + before do + get "#{api_v3_paths.work_packages_by_workspace(project.id)}?#{params.to_query}" + end + + it "numbers it as a root without disclosing its current parent" do + expect(body.dig("_embedded", "elements").pluck("id")).to eq([hidden.id]) + expect(body["rowCount"]).to eq(1) + expect(body["rowPositions"]).to eq(hidden.id.to_s => entry(1)) + end + end + end + + context "with more results than the metadata covers" do + before do + stub_const("WorkPackage::HierarchyRowPositions::MAX_RESULTS", 2) + get "#{api_v3_paths.work_packages_by_workspace(project.id)}?#{params.to_query}" + end + + it "omits row positions" do + expect(body).not_to include("rowCount", "rowPositions") + end + end +end diff --git a/spec/services/api/v3/work_package_collection_from_query_service_spec.rb b/spec/services/api/v3/work_package_collection_from_query_service_spec.rb index 6bd5d5f3baa4..acd9fe70cafe 100644 --- a/spec/services/api/v3/work_package_collection_from_query_service_spec.rb +++ b/spec/services/api/v3/work_package_collection_from_query_service_spec.rb @@ -82,6 +82,7 @@ :project, :groups, :total_sums, + :row_positions, :page, :per_page, :embed_schemas, @@ -94,6 +95,7 @@ def initialize(work_packages, project:, groups:, total_sums:, + row_positions:, page:, per_page:, embed_schemas:, @@ -106,6 +108,7 @@ def initialize(work_packages, project, groups, total_sums, + row_positions, page, per_page, embed_schemas, @@ -282,6 +285,36 @@ def initialize(group, end end + describe "row_positions" do + context "with query.show_hierarchies being false" do + it "is nil" do + query.show_hierarchies = false + + expect(subject.row_positions) + .to be_nil + end + end + + context "with query.show_hierarchies being true" do + it "numbers the results for the user" do + query.show_hierarchies = true + + expect(subject.row_positions) + .to be_a(WorkPackage::HierarchyRowPositions) + end + end + + context "with more results than the cap" do + it "is nil" do + query.show_hierarchies = true + stub_const("WorkPackage::HierarchyRowPositions::MAX_RESULTS", 0) + + expect(subject.row_positions) + .to be_nil + end + end + end + context "groups" do context "with query.grouped? being false" do it "is nil" do From d7f53505e0c234b8f9af9ad440f7c7ed05a5723f Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 24 Oct 2026 19:48:35 +0100 Subject: [PATCH 3/6] [DREAM-941] Render hierarchy pages in canonical order Renders each hierarchy page depth first, with sibling subtrees ordered by the earliest rank of any result they contain, so a child ranked before its parent no longer defers until the parent appears. When the collection carries row positions whose parents agree with the rendered ancestors, the page follows those positions instead, which keeps it in the query-wide order when a subtree spans pages. The same order applies to every hierarchy table, Gantt and embedded tables included. https://community.openproject.org/wp/DREAM-941 --- .../hal/resources/wp-collection-resource.ts | 9 + .../modes/hierarchy/hierarchy-order.spec.ts | 37 +++- .../modes/hierarchy/hierarchy-order.ts | 72 +++++++ .../modes/hierarchy/hierarchy-render-pass.ts | 199 ++---------------- .../hierarchy/single-hierarchy-row-builder.ts | 4 +- .../table/keyboard_navigation_spec.rb | 4 +- 6 files changed, 136 insertions(+), 189 deletions(-) create mode 100644 frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-order.ts diff --git a/frontend/src/app/features/hal/resources/wp-collection-resource.ts b/frontend/src/app/features/hal/resources/wp-collection-resource.ts index 7da57c399419..099126d74d4d 100644 --- a/frontend/src/app/features/hal/resources/wp-collection-resource.ts +++ b/frontend/src/app/features/hal/resources/wp-collection-resource.ts @@ -50,11 +50,20 @@ export class WorkPackageCollectionResource extends CollectionResource; + public rowCount?:number; + + public rowPositions?:Record; + public sumsSchema?:SchemaResource; public representations:HalResource[]; } +export interface RowPositionEntry { + position:number; + parent:string|null; +} + /** * A reference to a group object as returned from the API. * Augmented with state information such as collapsed state. diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-order.spec.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-order.spec.ts index 45e64fb5cd99..b38f735f67a2 100644 --- a/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-order.spec.ts +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-order.spec.ts @@ -26,6 +26,7 @@ //++ import { buildTable, TableHarness } from '../../../testing/table-harness'; +import { hierarchyOrder } from './hierarchy-order'; describe('Hierarchy ordering across query windows', () => { let harness:TableHarness; @@ -41,10 +42,10 @@ describe('Hierarchy ordering across query windows', () => { expect(harness.rowIds()).toEqual(['1', '2', '3']); }); - it('currently defers the child when its parent is in the same window', async () => { + it('renders a parent before a child that precedes it in the results', async () => { harness = buildTable({ workPackages: [child, other, parent], showHierarchies: true }); await harness.render(); - expect(harness.rowIds()).toEqual(['3', '1', '2']); + expect(harness.rowIds()).toEqual(['1', '2', '3']); }); it('keeps a child under an earlier parent', async () => { @@ -53,3 +54,35 @@ describe('Hierarchy ordering across query windows', () => { expect(harness.rowIds()).toEqual(['1', '2', '3']); }); }); + +describe('hierarchyOrder', () => { + const node = (id:string, ...ancestorIds:string[]) => ({ id, ancestorIds }); + + it.each([ + ['flat results', [node('a'), node('b')], ['a', 'b']], + ['a child before its parent', [node('c', 'p'), node('o'), node('p')], ['p', 'c', 'o']], + ['a contextual parent', [node('c', 'p'), node('o')], ['p', 'c', 'o']], + ['nested contextual ancestors', [node('g', 'r', 'm'), node('r2')], ['r', 'm', 'g', 'r2']], + ['siblings under a shared contextual parent', [node('c1', 'p'), node('o'), node('c2', 'p')], ['p', 'c1', 'c2', 'o']], + ['a parent ranked before its children', [node('p'), node('o'), node('c', 'p')], ['p', 'c', 'o']], + ])('orders %s depth first by earliest rank', (_name, results, expected) => { + expect(hierarchyOrder(results)).toEqual(expected); + }); + + it('follows server positions over the page rank', () => { + const results = [node('y'), node('x2', 'x')]; + const positions = { + x: { position: 1, parent: null }, + x2: { position: 3, parent: 'x' }, + y: { position: 5, parent: null }, + }; + expect(hierarchyOrder(results, positions)).toEqual(['x', 'x2', 'y']); + }); + + it.each([ + ['an entry is missing', { x: { position: 1, parent: null }, x2: { position: 3, parent: 'x' } }], + ['a parent disagrees', { x: { position: 1, parent: null }, x2: { position: 3, parent: null }, y: { position: 5, parent: null } }], + ])('falls back to the page rank when %s', (_name, positions) => { + expect(hierarchyOrder([node('y'), node('x2', 'x')], positions)).toEqual(['y', 'x', 'x2']); + }); +}); diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-order.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-order.ts new file mode 100644 index 000000000000..b44412194e70 --- /dev/null +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-order.ts @@ -0,0 +1,72 @@ +//-- copyright +// OpenProject is an open source project management software. +// Copyright (C) the OpenProject GmbH +// +// 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-2013 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, see . +// +// See COPYRIGHT and LICENSE files for more details. +//++ + +import type { RowPositionEntry } from 'core-app/features/hal/resources/wp-collection-resource'; + +export interface HierarchyNode { + readonly id:string; + /** Visible ancestors, root first. */ + readonly ancestorIds:readonly string[]; +} + +export function hierarchyOrder(results:readonly HierarchyNode[], positions?:Record):string[] { + const parents = new Map(); + const keys = new Map(); + results.forEach(({ id, ancestorIds }, rank) => { + [null, ...ancestorIds, id].forEach((node, index, chain) => { + if (node === null) return; + parents.set(node, chain[index - 1] ?? null); + keys.set(node, Math.min(keys.get(node) ?? rank, rank)); + }); + }); + + if (positions && hierarchyAgrees(results, positions)) { + return [...parents.keys()].sort((a, b) => positions[a].position - positions[b].position); + } + + const children = new Map(); + [...keys.keys()] + .sort((a, b) => keys.get(a)! - keys.get(b)!) + .forEach((id) => { + const parent = parents.get(id)!; + children.set(parent, [...(children.get(parent) ?? []), id]); + }); + + const ordered:string[] = []; + const stack = [...(children.get(null) ?? [])].reverse(); + for (let id = stack.pop(); id !== undefined; id = stack.pop()) { + ordered.push(id); + stack.push(...[...(children.get(id) ?? [])].reverse()); + } + return ordered; +} + +/** Whether the collection's parent edges match every edge of the results' visible ancestor chains. */ +export function hierarchyAgrees(results:readonly HierarchyNode[], positions:Readonly>):boolean { + return results.every(({ id, ancestorIds }) => [null, ...ancestorIds, id] + .every((node, index, chain) => node === null || positions[node]?.parent === (chain[index - 1] ?? null))); +} diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-render-pass.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-render-pass.ts index c80544472981..88ee539fe895 100644 --- a/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-render-pass.ts +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-render-pass.ts @@ -30,16 +30,11 @@ import { WorkPackageResource } from 'core-app/features/hal/resources/work-packag import { PrimaryRenderPass } from 'core-app/features/work-packages/components/wp-fast-table/builders/primary-render-pass'; import { States } from 'core-app/core/states/states.service'; import { WorkPackageTable } from 'core-app/features/work-packages/components/wp-fast-table/wp-fast-table'; -import type { WorkPackageTableRow } from 'core-app/features/work-packages/components/wp-fast-table/wp-table.interfaces'; -import { - ancestorClassIdentifier, - hierarchyGroupClass, -} from 'core-app/features/work-packages/components/wp-fast-table/helpers/wp-table-hierarchy-helpers'; +import { ancestorClassIdentifier } from 'core-app/features/work-packages/components/wp-fast-table/helpers/wp-table-hierarchy-helpers'; import { WorkPackageViewHierarchies } from 'core-app/features/work-packages/routing/wp-view-base/view-services/wp-table-hierarchies'; import { IsolatedQuerySpace } from 'core-app/features/work-packages/directives/query-space/isolated-query-space'; import { WorkPackageViewHierarchiesService } from 'core-app/features/work-packages/routing/wp-view-base/view-services/wp-view-hierarchy.service'; import { LazyInject } from 'core-app/shared/helpers/angular/lazy-inject.decorator'; -import { ApiV3Service } from 'core-app/core/apiv3/api-v3.service'; import { ancestorOccurrenceKey, type DraftOccurrence, @@ -47,25 +42,18 @@ import { wpOccurrenceKey, } from 'core-app/features/work-packages/components/wp-fast-table/rendered-occurrence-ledger'; import { additionalHierarchyRowClassName, SingleHierarchyRowBuilder } from './single-hierarchy-row-builder'; +import { hierarchyOrder } from './hierarchy-order'; export class HierarchyRenderPass extends PrimaryRenderPass { @LazyInject() querySpace:IsolatedQuerySpace; @LazyInject() states:States; - @LazyInject() apiV3Service:ApiV3Service; - @LazyInject() wpTableHierarchies:WorkPackageViewHierarchiesService; // Remember which rows were already rendered readonly rendered:Record = {}; - // Remember additional parents inserted that are not part of the results table - private additionalParents:Record = {}; - - // Defer children to be rendered when their parent occurs later in the table - private deferred:Record = {}; - // Collapsed state private hierarchies:WorkPackageViewHierarchies; @@ -97,158 +85,24 @@ export class HierarchyRenderPass extends PrimaryRenderPass { * Render the hierarchy table into the document fragment */ protected doRender() { - this.workPackageTable.originalRows.forEach((wpId:string) => { - const row:WorkPackageTableRow = this.workPackageTable.originalRowIndex[wpId]; - const workPackage:WorkPackageResource = row.object; - - // If we need to defer this row, skip it for now - if (this.deferInsertion(workPackage)) { - return; - } - - if (workPackage.getAncestors().length) { - // If we have ancestors, render it - this.buildWithHierarchy(row); + const results = this.workPackageTable.originalRows.map((wpId) => this.workPackageTable.originalRowIndex[wpId].object); + const ancestors = new Map(results.flatMap((wp) => wp.getAncestors()).map((ancestor) => [ancestor.id!, ancestor])); + const order = hierarchyOrder( + results.map((wp) => ({ id: wp.id!, ancestorIds: wp.getAncestors().map((ancestor) => ancestor.id!) })), + this.querySpace.results.value?.rowPositions, + ); + + order.forEach((id) => { + const row = this.workPackageTable.originalRowIndex[id]; + if (row) { + const [tr, hidden] = this.rowBuilder.buildEmpty(row.object); + this.markRendered(tr, row.object, hidden); } else { - // Render a work package root with no parents - const [tr, hidden] = this.rowBuilder.buildEmpty(workPackage); - this.markRendered(tr, workPackage, hidden); - } - - // Render all potentially deferred rows - this.renderAllDeferredChildren(workPackage); - }); - } - - /** - * If the given work package has a visible ancestor in the table, return true - * and remember the work package until the ancestor is rendered. - * @param workPackage - * @returns {boolean} - */ - public deferInsertion(workPackage:WorkPackageResource):boolean { - const ancestors = workPackage.getAncestors(); - - // Will only defer if at least one ancestor exists - if (ancestors.length === 0) { - return false; - } - - // Cases for wp - // 1. No wp.ancestors in table -> Render them immediately (defer=false) - // 2. Parent in table -> deferred[parent] = wp - // 3. Parent not in table BUT a ancestor in table - // -> deferred[a ancestor] = parent - // -> deferred[parent] = wp - // 4. Any ancestor already rendered -> Render normally (don't defer) - const ancestorChain = ancestors.concat([workPackage]); - for (let i = ancestorChain.length - 2; i >= 0; --i) { - const parent = ancestorChain[i]; - - const inTable = this.workPackageTable.originalRowIndex[parent.id!]; - const alreadyRendered = this.rendered[parent.id!]; - - if (alreadyRendered) { - // parent is already rendered. - // Don't defer, but render all intermediate parents below it - return false; - } - - if (inTable) { - // Get the current elements - let elements = this.deferred[parent.id!] || []; - // Append to them the child and all children below - let newElements:WorkPackageResource[] = ancestorChain.slice(i + 1, ancestorChain.length); - newElements = newElements.map((child) => this.apiV3Service.work_packages.cache.state(child.id!).value!); - // Append all new elements - elements = elements.concat(newElements); - // Remove duplicates (Regression #29652) - const seen = new Set(); - this.deferred[parent.id!] = elements.filter((el) => { - if (seen.has(el.id!)) { return false; } - seen.add(el.id!); - return true; - }); - return true; - } - // Otherwise, continue the chain upwards - } - - return false; - } - - /** - * Render any deferred children of the given work package. If recursive children were - * deferred, each of them will be passed through renderCallback. - * @param workPackage - */ - private renderAllDeferredChildren(workPackage:WorkPackageResource) { - const wpId = workPackage.id!; - const deferredChildren = this.deferred[wpId] || []; - - // If the work package has deferred children to render, - // run them through the callback - deferredChildren.forEach((child:WorkPackageResource) => { - this.insertUnderParent(this.indexedOrGiven(child), child.parent ?? workPackage); - - // Descend into any children the child WP might have and callback - this.renderAllDeferredChildren(child); - }); - } - - private indexedOrGiven(workPackage:WorkPackageResource):WorkPackageResource { - return this.workPackageTable.originalRowIndex[workPackage.id!]?.object ?? workPackage; - } - - private buildWithHierarchy(row:WorkPackageTableRow) { - // Ancestor data [root, med, thisrow] - const ancestors = row.object.getAncestors(); - const ancestorGroups:string[] = []; - - // Iterate ancestors - ancestors.forEach((el:WorkPackageResource, index:number) => { - const ancestor = this.states.workPackages.get(el.id!).getValueOr(el); - - // If we see the parent the first time, - // build it as an additional row and insert it into the ancestry - if (!this.rendered[ancestor.id!]) { - const [ancestorRow, hidden] = this.rowBuilder.buildAncestorRow(ancestor, ancestorGroups, index); - // Insert the ancestor row, either right here if it's a root node - // Or below the appropriate parent - - if (index === 0) { - // Special case, first ancestor => root without parent - this.markRendered(ancestorRow, ancestor, hidden, true); - } else { - // This ancestor must be inserted in the last position of its root - const parent = ancestors[index - 1]; - this.insertAtExistingHierarchy(ancestor, ancestorRow, parent, hidden, true); - } - - // Remember we just added this extra ancestor row - this.additionalParents[ancestor.id!] = ancestor; + const ancestor = this.states.workPackages.get(id).getValueOr(ancestors.get(id)!); + const [tr, hidden] = this.rowBuilder.buildAncestorRow(ancestor); + this.markRendered(tr, ancestor, hidden, true); } - - // Push the correct ancestor groups for identifying a hierarchy group - ancestorGroups.push(hierarchyGroupClass(ancestor.id!)); - ancestors.slice(0, index).forEach((previousAncestor) => { - ancestorGroups.push(hierarchyGroupClass(previousAncestor.id!)); - }); }); - - // Insert this row to parent - const parent = ancestors.at(-1); - this.insertUnderParent(row.object, parent!); - } - - /** - * Insert the given node as a child of the parent - * @param workPackage - * @param parent - */ - private insertUnderParent(workPackage:WorkPackageResource, parent:WorkPackageResource) { - const [tr, hidden] = this.rowBuilder.buildEmpty(workPackage); - this.insertAtExistingHierarchy(workPackage, tr, parent, hidden, false); } /** @@ -262,25 +116,6 @@ export class HierarchyRenderPass extends PrimaryRenderPass { this.registerAppended(row, this.buildOccurrence(workPackage, hidden, isAncestor)); } - /** - * Append a row to the given parent hierarchy group. - */ - private insertAtExistingHierarchy(workPackage:WorkPackageResource, - el:HTMLTableRowElement, - parent:WorkPackageResource, - hidden:boolean, - isAncestor:boolean) { - // Either append to the hierarchy group root (= the parentID row itself) - const hierarchyRoot = `.__hierarchy-root-${parent.id}`; - // Or, if it has descendants, append to the LATEST of that set - const hierarchyGroup = `.__hierarchy-group-${parent.id}`; - - // Insert into table - this.spliceRow(el, `${hierarchyRoot},${hierarchyGroup}`, this.buildOccurrence(workPackage, hidden, isAncestor)); - - this.rendered[workPackage.id!] = true; - } - private occurrenceKey(workPackage:WorkPackageResource, isAncestor:boolean):OccurrenceKey { return isAncestor ? ancestorOccurrenceKey(workPackage.id!) : wpOccurrenceKey(workPackage.id!); } diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/single-hierarchy-row-builder.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/single-hierarchy-row-builder.ts index c19fd1cf2240..0c82fddd91ba 100644 --- a/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/single-hierarchy-row-builder.ts +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/single-hierarchy-row-builder.ts @@ -131,9 +131,7 @@ export class SingleHierarchyRowBuilder extends SingleRowBuilder { /** * Append an additional ancestor row that is not yet loaded */ - public buildAncestorRow(ancestor:WorkPackageResource, - ancestorGroups:string[], - index:number):[HTMLTableRowElement, boolean] { + public buildAncestorRow(ancestor:WorkPackageResource):[HTMLTableRowElement, boolean] { const workPackage = this.states.workPackages.get(ancestor.id!).value!; const [tr, hidden] = this.buildEmpty(workPackage); const identifier = ancestorClassIdentifier(ancestor.id!); diff --git a/spec/features/work_packages/table/keyboard_navigation_spec.rb b/spec/features/work_packages/table/keyboard_navigation_spec.rb index 30febe59b2c3..cedd8b1c97c5 100644 --- a/spec/features/work_packages/table/keyboard_navigation_spec.rb +++ b/spec/features/work_packages/table/keyboard_navigation_spec.rb @@ -77,14 +77,14 @@ click_on "Sort ascending" end expect(page).to have_css(".generic-table--sort-header #subject.asc") - wp_table.expect_work_package_order(sibling, parent, child) + wp_table.expect_work_package_order(parent, child, sibling) # The column menu drops focus to body after an action until DREAM-894. wp_table.tab_into_table wp_table.expect_focus_on_column_header("Subject") wp_table.press_keys(:down) - wp_table.expect_focus_on_cell(sibling, :subject) + wp_table.expect_focus_on_cell(parent, :subject) wp_table.press_keys(%i[shift tab]) wp_table.expect_focus_outside_table From 242c36fc77d58f50f31accabc1db6efb358cc1e7 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 24 Oct 2026 19:52:57 +0100 Subject: [PATCH 4/6] [DREAM-941] Publish hierarchy row positions Lets TableRowPositions publish aria-rowindex and aria-rowcount in hierarchy mode from the collection's rowCount and rowPositions. The collection must still list the rendered results in order, every rendered row must have an entry, every edge of a result's visible ancestor chain must match the collection's parents, and positions must rise in rendered order; anything else withdraws the whole family. The renderer only orders a page by the positions under the same condition, so a reorder rendered before its collection arrives keeps its new order. Rows read their ancestry from the work package cache they re-render from, so a cache that is newer than the collection never mislabels rows, whether a row refreshes or the positions are recaptured later. Expanded relation columns still withdraw positions. https://community.openproject.org/wp/DREAM-941 --- .../modes/hierarchy/hierarchy-render-pass.ts | 4 +- .../semantics/body-row-positions.spec.ts | 31 ++++++- .../semantics/body-row-positions.ts | 32 +++++++ .../semantics/table-row-positions.spec.ts | 86 +++++++++++++++++++ .../semantics/table-row-positions.ts | 54 +++++++++--- .../wp-fast-table/testing/table-harness.ts | 6 +- .../components/wp-fast-table/wp-fast-table.ts | 1 + 7 files changed, 201 insertions(+), 13 deletions(-) diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-render-pass.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-render-pass.ts index 88ee539fe895..40ea7b0b6a76 100644 --- a/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-render-pass.ts +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/builders/modes/hierarchy/hierarchy-render-pass.ts @@ -43,6 +43,7 @@ import { } from 'core-app/features/work-packages/components/wp-fast-table/rendered-occurrence-ledger'; import { additionalHierarchyRowClassName, SingleHierarchyRowBuilder } from './single-hierarchy-row-builder'; import { hierarchyOrder } from './hierarchy-order'; +import { describesRows } from '../../../semantics/table-row-positions'; export class HierarchyRenderPass extends PrimaryRenderPass { @LazyInject() querySpace:IsolatedQuerySpace; @@ -87,9 +88,10 @@ export class HierarchyRenderPass extends PrimaryRenderPass { protected doRender() { const results = this.workPackageTable.originalRows.map((wpId) => this.workPackageTable.originalRowIndex[wpId].object); const ancestors = new Map(results.flatMap((wp) => wp.getAncestors()).map((ancestor) => [ancestor.id!, ancestor])); + const source = this.querySpace.results.value; const order = hierarchyOrder( results.map((wp) => ({ id: wp.id!, ancestorIds: wp.getAncestors().map((ancestor) => ancestor.id!) })), - this.querySpace.results.value?.rowPositions, + source && describesRows(source, this.workPackageTable.originalRows) ? source.rowPositions : undefined, ); order.forEach((id) => { diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/body-row-positions.spec.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/body-row-positions.spec.ts index 12e0e1d7582b..8399a6999d55 100644 --- a/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/body-row-positions.spec.ts +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/body-row-positions.spec.ts @@ -25,7 +25,7 @@ // See COPYRIGHT and LICENSE files for more details. //++ -import { bodyRowPositions, draftAgrees, PositionWindow } from './body-row-positions'; +import { bodyRowPositions, draftAgrees, PositionWindow, hierarchyRowPositions } from './body-row-positions'; const groups = [ { headerKey: 'G', sumsKey: 'G-sum', count: 4, hasSums: true }, @@ -132,3 +132,32 @@ describe('draftAgrees', () => { expect(draftAgrees(grouped(), [structural('G'), member('X')], groups)).toBe(false); }); }); + +describe('hierarchyRowPositions', () => { + const rows = [ + { key: 'ancestor:1', workPackageId: '1', ancestorIds: null }, + { key: 'wp:2', workPackageId: '2', ancestorIds: ['1'] }, + ]; + const entries = { 1: { position: 3, parent: null }, 2: { position: 5, parent: '1' } }; + + it('keys the collection positions by occurrence', () => { + expect(hierarchyRowPositions(9, entries, rows)).toEqual({ count: 9, ordinals: new Map([['ancestor:1', 3], ['wp:2', 5]]) }); + }); + + it.each([ + ['a negative count', -1, entries], + ['a fractional count', 9.5, entries], + ['a missing entry', 9, { 1: entries[1] }], + ['a disagreeing parent', 9, { ...entries, 2: { position: 5, parent: null } }], + ['a position beyond the count', 4, entries], + ['positions out of rendered order', 9, { 1: { position: 5, parent: null }, 2: { position: 3, parent: '1' } }], + ['a zero position', 9, { 1: { position: 0, parent: null }, 2: entries[2] }], + ])('rejects %s', (_name, count, given) => { + expect(hierarchyRowPositions(count, given, rows)).toBeNull(); + }); + + it('rejects a result whose ancestry disagrees above its parent', () => { + const nested = [rows[0], { key: 'wp:2', workPackageId: '2', ancestorIds: ['4', '1'] }]; + expect(hierarchyRowPositions(9, entries, nested)).toBeNull(); + }); +}); diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/body-row-positions.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/body-row-positions.ts index d692d7a735fb..c54858a77b66 100644 --- a/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/body-row-positions.ts +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/body-row-positions.ts @@ -25,6 +25,9 @@ // See COPYRIGHT and LICENSE files for more details. //++ +import type { RowPositionEntry } from 'core-app/features/hal/resources/wp-collection-resource'; +import { hierarchyAgrees } from '../builders/modes/hierarchy/hierarchy-order'; + export interface BodyRowPositions { readonly count:number; readonly ordinals:ReadonlyMap; @@ -168,3 +171,32 @@ export function draftAgrees(body:BodyRowPositions, rows:readonly DraftRow[], gro } return true; } + +export interface HierarchyDraftRow { + readonly key:string; + readonly workPackageId:string; + /** Visible ancestors of a result row, root first; `null` for a contextual ancestor, whose edges its descendants vouch for. */ + readonly ancestorIds:readonly string[]|null; +} + +export function hierarchyRowPositions( + count:number, + entries:Readonly>, + rows:readonly HierarchyDraftRow[], +):BodyRowPositions|null { + if (!Number.isSafeInteger(count) || count < 0) return null; + + const results = rows.flatMap(({ workPackageId, ancestorIds }) => (ancestorIds ? [{ id: workPackageId, ancestorIds }] : [])); + if (!hierarchyAgrees(results, entries)) return null; + + const ordinals = new Map(); + let previous = 0; + for (const { key, workPackageId } of rows) { + if (!Object.hasOwn(entries, workPackageId)) return null; + const { position } = entries[workPackageId]; + if (!Number.isSafeInteger(position) || position <= previous || position > count) return null; + ordinals.set(key, position); + previous = position; + } + return { count, ordinals }; +} diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.spec.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.spec.ts index 4c5d5caed86e..c14b48f60947 100644 --- a/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.spec.ts +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.spec.ts @@ -27,6 +27,7 @@ import { buildTable, TableHarness, TableHarnessOptions } from '../testing/table-harness'; import { WorkPackageViewCollapsedGroupsService } from 'core-app/features/work-packages/routing/wp-view-base/view-services/wp-view-collapsed-groups.service'; +import { WorkPackageViewHierarchiesService } from 'core-app/features/work-packages/routing/wp-view-base/view-services/wp-view-hierarchy.service'; import { waitFor } from '@testing-library/dom'; import { nextTask } from 'core-common/testing/timing'; import { States } from 'core-app/core/states/states.service'; @@ -133,6 +134,91 @@ describe('TableRowPositions', () => { expect(harness.row('21')).not.toHaveAttribute('aria-rowindex'); }); + describe('in hierarchy mode', () => { + const parent = { id: '1' }; + const child = { id: '2', ancestors: [parent] }; + const other = { id: '3' }; + const entries = { + 1: { position: 1, parent: null }, + 2: { position: 2, parent: '1' }, + 3: { position: 3, parent: null }, + }; + const mountHierarchy = (options:Partial = {}) => mount({ + workPackages: [child, other, parent], + showHierarchies: true, + pagination: { offset: 1, pageSize: 20, total: 3 }, + rowPositions: { count: 3, entries }, + ...options, + }); + + it('publishes the positions the collection carries', async () => { + await mountHierarchy(); + harness.expectRowPositions(['1', '2', '3', '4', '5', '6'], '6'); + }); + + it('keeps query-wide positions of a later page, gaps included', async () => { + await mountHierarchy({ + workPackages: [{ id: '5', ancestors: [parent] }], + pagination: { offset: 2, pageSize: 1, total: 8 }, + rowPositions: { count: 9, entries: { 1: { position: 1, parent: null }, 5: { position: 7, parent: '1' } } }, + }); + harness.expectRowPositions(['1', '2', '8', '11', '12'], '12'); + }); + + it('withdraws positions without row metadata', async () => { + await mountHierarchy({ rowPositions: undefined }); + harness.expectNoRowPositions(); + }); + + it('withdraws positions when a rendered parent disagrees with the collection', async () => { + await mountHierarchy({ rowPositions: { count: 3, entries: { ...entries, 2: { position: 2, parent: null } } } }); + harness.expectNoRowPositions(); + }); + + it('withdraws positions when a rendered row has no entry', async () => { + await mountHierarchy({ rowPositions: { count: 3, entries: { 1: entries[1], 2: entries[2] } } }); + harness.expectNoRowPositions(); + }); + + it('renders a reorder without a new collection in its new order and withdraws positions', async () => { + await mountHierarchy(); + harness.table.initialSetup(['3', '1', '2'].map((id) => harness.table.originalRowIndex[id].object)); + await waitFor(() => expect(harness.rowIds()).toEqual(['3', '1', '2'])); + harness.expectNoRowPositions(); + }); + + it('withdraws positions when a refreshed row moves to another parent', async () => { + await mountHierarchy(); + harness.injector.get(States).workPackages.get('2').putValue(buildWorkPackage({ id: '2' })); + await waitFor(() => harness.expectNoRowPositions()); + }); + + it('withdraws positions when a refreshed row moves above its parent', async () => { + await mountHierarchy(); + harness.injector.get(States).workPackages.get('2').putValue(buildWorkPackage({ id: '2', ancestors: [{ id: '4' }, parent] })); + await nextTask(); + expect(harness.row('2').querySelector('.wp-table--hierarchy-span')).toHaveAttribute('data-indentation', '65px'); + harness.expectNoRowPositions(); + }); + + it('keeps positions when a refreshed row keeps its parent', async () => { + await mountHierarchy(); + harness.injector.get(States).workPackages.get('2').putValue(buildWorkPackage({ id: '2', ancestors: [parent] })); + await nextTask(); + harness.expectRowPositions(['1', '2', '3', '4', '5', '6'], '6'); + }); + + it('keeps positions and the count when a parent collapses', async () => { + await mountHierarchy(); + harness.injector.get(WorkPackageViewHierarchiesService).collapse('1'); + await waitFor(() => expect(harness.row('2')).not.toBeVisible()); + expect(harness.row('2')).toHaveAttribute('aria-rowindex', '3'); + expect(harness.row('1')).toHaveAttribute('aria-rowindex', '2'); + expect(harness.row('3')).toHaveAttribute('aria-rowindex', '4'); + expect(table()).toHaveAttribute('aria-rowcount', '6'); + }); + }); + it('does not activate capability-off consumers', async () => { await mount({ configuration: { keyboardNavigationEnabled: false } }); expect(harness.table.rowPositions).toBeNull(); diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.ts index b3ce9469d874..3c925003bbcf 100644 --- a/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.ts +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.ts @@ -31,11 +31,13 @@ import { WorkPackageViewRelationColumnsService } from 'core-app/features/work-pa import { WorkPackageViewColumnsService } from 'core-app/features/work-packages/routing/wp-view-base/view-services/wp-view-columns.service'; import { WorkPackageViewBaselineService } from 'core-app/features/work-packages/routing/wp-view-base/view-services/wp-view-baseline.service'; import type { GroupObject, WorkPackageCollectionResource } from 'core-app/features/hal/resources/wp-collection-resource'; +import type { WorkPackageResource } from 'core-app/features/hal/resources/work-package-resource'; import { LazyInject } from 'core-app/shared/helpers/angular/lazy-inject.decorator'; +import { States } from 'core-app/core/states/states.service'; import { groupOccurrenceKey, placeholderOccurrenceKey, wpOccurrenceKey, type RenderDraft, type RenderedOccurrence } from '../rendered-occurrence-ledger'; import type { WorkPackageTable } from '../wp-fast-table'; import { isEmptyResultsRow, isExposedRow, isInlineCreateRow, isTotalsRow } from '../helpers/structural-rows'; -import { bodyRowPositions, draftAgrees, type BodyRowPositions, type PositionGroup } from './body-row-positions'; +import { bodyRowPositions, draftAgrees, hierarchyRowPositions, type BodyRowPositions, type PositionGroup } from './body-row-positions'; export interface CapturedRowPositions { readonly source:WorkPackageCollectionResource; @@ -47,6 +49,12 @@ const UNSCOPED_COLLECTION_HREF = /\/api\/v3\/(?:(?:projects|workspaces)\/[^/]+\/ type ProjectedRowKind = 'ledger'|'skipped'|'empty'|'totals'|'create'|'foreign'; +export function describesRows(source:WorkPackageCollectionResource, rows:readonly string[]):boolean { + return source.count === rows.length + && source.elements.length === rows.length + && source.elements.every((wp, index) => wp.id === rows[index]); +} + function setIfChanged(element:Element, name:string, value:string|null):void { if (value === null) { if (element.hasAttribute(name)) element.removeAttribute(name); @@ -64,6 +72,8 @@ export class TableRowPositions { @LazyInject(WorkPackageViewBaselineService) private readonly baseline:WorkPackageViewBaselineService; + @LazyInject(States) private readonly states:States; + public readonly injector:Injector = this.table.injector; private snapshot:CapturedRowPositions|null = null; @@ -72,7 +82,9 @@ export class TableRowPositions { public capture(draft:RenderDraft):CapturedRowPositions|null { const source = this.table.querySpace.results.value; - if (!source || this.withdrawnByMode() || !this.matchesRenderedRows(source)) return null; + if (!source || this.withdrawnByMode()) return null; + if (!describesRows(source, this.table.originalRows)) return null; + if (this.hierarchies.isEnabled) return this.captureHierarchy(source, draft); const groups = this.table.querySpace.groups.value ?? []; if (groups.length > 0 && !this.trustsGroupedAggregates(source, groups)) return null; @@ -98,6 +110,11 @@ export class TableRowPositions { return draftAgrees(body, rows, positionGroups) ? { source, body } : null; } + /** Re-derives the committed positions after a row re-rendered from a newer resource, withdrawing them if its ancestry moved. */ + public refreshed():void { + if (this.snapshot) this.revalidate(); + } + public commit(snapshot:CapturedRowPositions|null):void { this.snapshot = snapshot; } @@ -116,19 +133,36 @@ export class TableRowPositions { }); } + private revalidate():void { + const draft = this.table.lastRenderPass?.draft; + this.commit(draft ? this.capture(draft) : null); + } + + private captureHierarchy(source:WorkPackageCollectionResource, draft:RenderDraft):CapturedRowPositions|null { + const { rowCount, rowPositions } = source; + if (rowCount === undefined || !rowPositions) return null; + + const occurrences = draft.occurrences.filter((occurrence) => occurrence.key !== placeholderOccurrenceKey()); + if (occurrences.some((occurrence) => occurrence.workPackageId === null || occurrence.workPackage === null)) return null; + + const body = hierarchyRowPositions(rowCount, rowPositions, occurrences.map((occurrence) => ({ + key: occurrence.key, + workPackageId: occurrence.workPackageId!, + ancestorIds: occurrence.key === wpOccurrenceKey(occurrence.workPackageId!) ? this.renderedAncestorIds(occurrence.workPackage!) : null, + }))); + return body ? { source, body } : null; + } + + /** Rows re-render from the work package cache, so the cached resource describes the row on screen, not the draft's. */ + private renderedAncestorIds(drafted:WorkPackageResource):string[] { + return this.states.workPackages.get(drafted.id!).getValueOr(drafted).getAncestors().map((ancestor) => ancestor.id!); + } + private withdrawnByMode():boolean { - if (this.hierarchies.isEnabled) return true; return (this.columns.hasRelationColumns() || this.columns.hasChildRelationsColumn()) && Object.keys(this.relationColumns.current).length > 0; } - private matchesRenderedRows(source:WorkPackageCollectionResource):boolean { - const { originalRows } = this.table; - return source.count === originalRows.length - && source.elements.length === originalRows.length - && source.elements.every((wp, index) => wp.id === originalRows[index]); - } - private trustsGroupedAggregates(source:WorkPackageCollectionResource, groups:GroupObject[]):boolean { return source.groups === groups && !this.baseline.isActive() diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/testing/table-harness.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/testing/table-harness.ts index 27fa2a4b482f..3d7f1fbb7f6f 100644 --- a/frontend/src/app/features/work-packages/components/wp-fast-table/testing/table-harness.ts +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/testing/table-harness.ts @@ -46,7 +46,7 @@ import { States } from 'core-app/core/states/states.service'; import { CausedUpdatesService } from 'core-app/features/boards/board/caused-updates/caused-updates.service'; import { HAL_NEW_RESOURCE_ID } from 'core-app/features/hal/helpers/is-new-resource'; import { QueryResource } from 'core-app/features/hal/resources/query-resource'; -import { WorkPackageCollectionResource } from 'core-app/features/hal/resources/wp-collection-resource'; +import { RowPositionEntry, WorkPackageCollectionResource } from 'core-app/features/hal/resources/wp-collection-resource'; import { HalResourceNotificationService } from 'core-app/features/hal/services/hal-resource-notification.service'; import { HalResourceService } from 'core-app/features/hal/services/hal-resource.service'; import { InlineCreateRowBuilder } from 'core-app/features/work-packages/components/wp-inline-create/inline-create-row-builder'; @@ -118,6 +118,8 @@ export interface NonSortableColumnSpec { id:string, sortable:false } export interface TableHarnessOptions { pagination?:{ offset:number; pageSize:number; total:number }; + /** Hierarchy row metadata of the results collection, as the API returns it. */ + rowPositions?:{ count:number; entries:Record }; /** Self link of the results collection; defaults to the global work packages collection. */ collectionHref?:string; workPackages:WorkPackageFixture[]; @@ -316,6 +318,8 @@ export function buildTable(options:TableHarnessOptions):TableHarness { total: options.pagination?.total ?? resources.length, pageSize: options.pagination?.pageSize ?? Math.max(resources.length, 1), offset: options.pagination?.offset ?? 1, + rowCount: options.rowPositions?.count, + rowPositions: options.rowPositions?.entries, sumsSchema: { href: sumsSchemaHref }, } as WorkPackageCollectionResource); querySpace.initialized.putValue(null); diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/wp-fast-table.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/wp-fast-table.ts index 7b81a121f7c1..7b11ba272fb9 100644 --- a/frontend/src/app/features/work-packages/components/wp-fast-table/wp-fast-table.ts +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/wp-fast-table.ts @@ -250,6 +250,7 @@ export class WorkPackageTable { }); if (replaced > 0) { + this.rowPositions?.refreshed(); this.semantics?.paintWorkPackage(workPackage.id!); this.notifyRowsRefreshed(workPackage.id!); } From 4389d9deec767962f6f3faa0777db678e97efd45 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 24 Oct 2026 19:57:33 +0100 Subject: [PATCH 5/6] [DREAM-941] Restore row positions after a drop Refetches the current page of a saved, ungrouped query once a drop has been persisted. When the refetched collection lists the rows in the order already on screen, it replaces the results and the positions are recaptured in place, so they come back without a reload and without a second render that would replace rows, focus or selection. Recapture reads each row's ancestry from the work package cache, so a row that moved while the refetch was in flight keeps the positions withdrawn. A render that was still pending when the results changed recaptures at commit. A failed refetch, a different order, a grouped query or an unsaved query leaves the positions withdrawn as before. https://community.openproject.org/wp/DREAM-941 --- .../state/drag-and-drop-lifetime.spec.ts | 52 ++++++++++++++++--- .../state/drag-and-drop-transformer.ts | 20 ++++++- .../semantics/table-row-positions.spec.ts | 41 +++++++++++++++ .../semantics/table-row-positions.ts | 13 +++++ .../components/wp-fast-table/wp-fast-table.ts | 2 +- 5 files changed, 120 insertions(+), 8 deletions(-) diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/drag-and-drop-lifetime.spec.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/drag-and-drop-lifetime.spec.ts index a0de2371befb..94ca165d0b88 100644 --- a/frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/drag-and-drop-lifetime.spec.ts +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/drag-and-drop-lifetime.spec.ts @@ -29,7 +29,7 @@ import { createEnvironmentInjector, EnvironmentInjector, Injector, Provider } fr import { TestBed } from '@angular/core/testing'; import { HttpClient } from '@angular/common/http'; import { waitFor } from '@testing-library/dom'; -import { of, Subject } from 'rxjs'; +import { of, Subject, throwError } from 'rxjs'; import * as Turbo from '@hotwired/turbo'; import { nextTask } from 'core-common/testing/timing'; import { ApiV3Service } from 'core-app/core/apiv3/api-v3.service'; @@ -463,10 +463,14 @@ describe('accepted table drag persistence lifetime', () => { describe('with row positions', () => { const positions = ['1', '2', '3', '4', '5', '6']; - it('stays withdrawn through a switch to manual sorting until a matching collection arrives', async () => { + it('restores positions from the refetched collection after a switch to manual sorting', async () => { await mount('default', true, true); table.expectRowPositions(positions, '6'); - const rebuilt = table.nextRender(); + const current = table.querySpace.results.value!; + const reordered = ['2', '1', '3'].map((id) => current.elements.find((wp) => wp.id === id)!); + const reloaded = { ...current, elements: reordered } as WorkPackageCollectionResource; + const load = vi.spyOn(table.injector.get(WorkPackagesListService), 'loadQueryFromExisting') + .mockReturnValue(of({ results: reloaded } as QueryResource)); const dropping = table.drop('1', '2', 'bottom'); await expectCalls(['order']); expect(table.rowIds()).toEqual(['2', '1', '3']); @@ -477,12 +481,48 @@ describe('accepted table drag persistence lifetime', () => { querySaved.next(original); expect(await dropping).toBe(true); expect(original.sortBy).toEqual([manualSort]); - await rebuilt; + await waitFor(() => expect(load).toHaveBeenCalled()); + + const settled = table.rows(); + await waitFor(() => table.expectRowPositions(positions, '6')); + expect(table.rows()).toEqual(settled); + expect(table.querySpace.results.value).toBe(reloaded); + expect(load).toHaveBeenCalledExactlyOnceWith(original, { pageSize: 50, offset: 1 }, '9'); + }); + + it('stays withdrawn when the refetched collection is in another order', async () => { + await mount('default', true, true); + const current = table.querySpace.results.value!; + const load = vi.spyOn(table.injector.get(WorkPackagesListService), 'loadQueryFromExisting') + .mockReturnValue(of({ results: { ...current } } as QueryResource)); + const dropping = table.drop('1', '2', 'bottom'); + await expectCalls(['order']); + orderSaved.resolve('now'); + await expectCalls(['order', 'query']); + querySaved.next(original); + expect(await dropping).toBe(true); + await waitFor(() => expect(load).toHaveBeenCalled()); + await nextTask(); expect(table.rowIds()).toEqual(['2', '1', '3']); + expect(table.querySpace.results.value).toBe(current); table.expectNoRowPositions(); + }); - await table.render(['2', '1', '3'].map((id) => ({ id, attributes: { status: id === '1' ? status1 : status2 } }))); - table.expectRowPositions(positions, '6'); + it('stays withdrawn when the collection cannot be refetched', async () => { + await mount('default', true, true); + vi.spyOn(table.injector.get(WorkPackagesListService), 'loadQueryFromExisting') + .mockReturnValue(throwError(() => new Error('offline'))); + const rebuilt = table.nextRender(); + const dropping = table.drop('1', '2', 'bottom'); + await expectCalls(['order']); + orderSaved.resolve('now'); + await expectCalls(['order', 'query']); + querySaved.next(original); + expect(await dropping).toBe(true); + await rebuilt; + await nextTask(); + expect(table.rowIds()).toEqual(['2', '1', '3']); + table.expectNoRowPositions(); }); it('stays withdrawn when the saved order remains visible after query saving fails', async () => { diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/drag-and-drop-transformer.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/drag-and-drop-transformer.ts index 69748a9c9a93..a7a97ff44c40 100644 --- a/frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/drag-and-drop-transformer.ts +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/handlers/state/drag-and-drop-transformer.ts @@ -46,6 +46,7 @@ import { collapsedGroupClass } from 'core-app/features/work-packages/components/ import { reorderById, type Edge } from 'core-common/drag-and-drop/reorder'; import { WorkPackageTable } from '../../wp-fast-table'; import { firstValueFrom } from 'rxjs'; +import isPersistedResource from 'core-app/features/hal/helpers/is-persisted-resource'; export class DragAndDropTransformer { @LazyInject() private readonly states:States; @@ -259,6 +260,17 @@ export class DragAndDropTransformer { actionService.onNewOrder([...ids]); }; const reportUiFailure = (error:unknown) => notification.handleRawError(error, workPackage); + const reloadResults = async () => { + const query = querySpace.query.value; + if (!isCurrent() || !query || !isPersistedResource(query) || querySpace.groups.value?.length) return; + const projectIdentifier = query.project?.href?.split('/').pop(); + const { results } = await firstValueFrom(listService.loadQueryFromExisting(query, listService.getPaginationInfo(), projectIdentifier)); + const rendered = table.originalRows; + if (!isCurrent() || results.elements.length !== rendered.length + || results.elements.some((element, index) => element.id !== rendered[index])) return; + querySpace.results.putValue(results); + table.rowPositions?.recapture(); + }; // Acceptance begins at the first write: navigation cannot cancel later phases. await preparedOrder.persist(); @@ -271,7 +283,13 @@ export class DragAndDropTransformer { await preparedQuery.persist(); } complete(true); - recoverIfCurrent(); + if (isCurrent()) { + void refreshLiveView(preparedOrder.order) + .then( + () => reloadResults().catch(() => undefined), + reportUiFailure, + ); + } } catch (error:unknown) { recoverIfCurrent(); notification.handleRawError(error, workPackage); diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.spec.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.spec.ts index c14b48f60947..a7dfeea67a96 100644 --- a/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.spec.ts +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.spec.ts @@ -33,6 +33,10 @@ import { nextTask } from 'core-common/testing/timing'; import { States } from 'core-app/core/states/states.service'; import { groupOccurrenceKey } from '../rendered-occurrence-ledger'; import { buildWorkPackage } from '../testing/work-package-fixture'; +import { Subject } from 'rxjs'; +import { WorkPackagesListService } from 'core-app/features/work-packages/components/wp-list/wp-list.service'; +import { WorkPackageViewOrderService } from 'core-app/features/work-packages/routing/wp-view-base/view-services/wp-view-order.service'; +import type { QueryResource } from 'core-app/features/hal/resources/query-resource'; function deferred() { let resolve!:(value:T) => void; @@ -201,6 +205,43 @@ describe('TableRowPositions', () => { harness.expectNoRowPositions(); }); + it('keeps positions withdrawn when the collection is recaptured after a row moved', async () => { + await mountHierarchy(); + harness.injector.get(States).workPackages.get('2').putValue(buildWorkPackage({ id: '2' })); + await waitFor(() => harness.expectNoRowPositions()); + harness.table.rowPositions!.recapture(); + harness.expectNoRowPositions(); + }); + + it('does not label a row that moved while the post-drop collection was loading', async () => { + await mountHierarchy(); + harness.querySpace.query.value!.id = '10'; + const current = harness.querySpace.results.value!; + const loaded = new Subject(); + const load = vi.fn(() => loaded); + Object.assign(harness.injector.get(WorkPackagesListService), { + loadQueryFromExisting: load, + getPaginationInfo: () => ({ pageSize: 20, offset: 1 }), + }); + vi.spyOn(harness.injector.get(WorkPackageViewOrderService), 'prepareMove') + .mockResolvedValue({ order: ['3', '1', '2'], persist: () => Promise.resolve() }); + const rendered = harness.nextRender(); + expect(await harness.drop('3', '1', 'top')).toBe(true); + await rendered; + await waitFor(() => expect(load).toHaveBeenCalled()); + harness.injector.get(States).workPackages.get('2').putValue(buildWorkPackage({ id: '2' })); + await nextTask(); + harness.expectNoRowPositions(); + loaded.next({ results: { + ...current, + elements: ['3', '1', '2'].map((id) => current.elements.find((wp) => wp.id === id)!), + rowPositions: { 3: { position: 1, parent: null }, 1: { position: 2, parent: null }, 2: { position: 3, parent: '1' } }, + } } as unknown as QueryResource); + await nextTask(); + expect(harness.row('2').querySelector('.wp-table--hierarchy-span')).toHaveAttribute('data-indentation', '25px'); + harness.expectNoRowPositions(); + }); + it('keeps positions when a refreshed row keeps its parent', async () => { await mountHierarchy(); harness.injector.get(States).workPackages.get('2').putValue(buildWorkPackage({ id: '2', ancestors: [parent] })); diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.ts index 3c925003bbcf..2bcdc2cd2ab2 100644 --- a/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.ts +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/semantics/table-row-positions.ts @@ -110,11 +110,24 @@ export class TableRowPositions { return draftAgrees(body, rows, positionGroups) ? { source, body } : null; } + /** Re-derives positions for the rows on screen after the results collection changed without a render. */ + public recapture():void { + if (this.table.hasPendingRender) return; + this.revalidate(); + this.paint(); + } + /** Re-derives the committed positions after a row re-rendered from a newer resource, withdrawing them if its ancestry moved. */ public refreshed():void { if (this.snapshot) this.revalidate(); } + /** Commits a render's positions, recapturing them if the results changed while the render was pending. */ + public commitRendered(captured:CapturedRowPositions|null, draft:RenderDraft):void { + const current = this.table.querySpace.results.value; + this.commit(captured?.source === current ? captured : this.capture(draft)); + } + public commit(snapshot:CapturedRowPositions|null):void { this.snapshot = snapshot; } diff --git a/frontend/src/app/features/work-packages/components/wp-fast-table/wp-fast-table.ts b/frontend/src/app/features/work-packages/components/wp-fast-table/wp-fast-table.ts index 7b11ba272fb9..b4c2790fc36d 100644 --- a/frontend/src/app/features/work-packages/components/wp-fast-table/wp-fast-table.ts +++ b/frontend/src/app/features/work-packages/components/wp-fast-table/wp-fast-table.ts @@ -297,7 +297,7 @@ export class WorkPackageTable { this.timelineBody.replaceChildren(timelinePass.timelineBody); } this.ledger.commit(pass.draft); - this.rowPositions?.commit(pending.positions); + this.rowPositions?.commitRendered(pending.positions, pass.draft); this.lastRenderPass = pass; this.publishRendered(); } From 03dee19d68b1e615d635a31422762f0e7e182192 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 24 Oct 2026 20:24:49 +0100 Subject: [PATCH 6/6] [DREAM-941] Cover hierarchy row positions in features Asserts positions in hierarchy mode instead of their absence, keeps the collapsed descendants' slots, numbers a contextual ancestor once across two pages, and expects positions to return after a drop without revisiting the query. https://community.openproject.org/wp/DREAM-941 --- .../sorting/manual_sorting_spec.rb | 33 ++++++++++++++++--- .../table/treegrid_semantics_spec.rb | 28 +++++++++++++++- 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/spec/features/work_packages/sorting/manual_sorting_spec.rb b/spec/features/work_packages/sorting/manual_sorting_spec.rb index 99c444815f3c..e15f6b260716 100644 --- a/spec/features/work_packages/sorting/manual_sorting_spec.rb +++ b/spec/features/work_packages/sorting/manual_sorting_spec.rb @@ -156,6 +156,34 @@ def expect_query_order(query, expected) wp_page.expect_no_parent end + context "when reordering siblings without changing their parent" do + let(:work_package5) { create(:work_package, subject: "WP5", project:, parent: work_package1) } + let(:work_package6) { create(:work_package, subject: "WP6", project:, parent: work_package1) } + + before do + work_package5 + work_package6 + wp_table.visit! + wp_table.expect_work_package_order(work_package1, work_package2, work_package3, work_package4, + work_package5, work_package6) + end + + it "keeps the new order and its row positions once each drop settles" do + wp_table.drag_and_drop_work_package from: 5, to: 4 + wp_table.expect_and_dismiss_toaster message: "Successful creation." + wp_table.expect_work_package_order(work_package1, work_package2, work_package3, work_package4, + work_package6, work_package5) + + wp_table.drag_and_drop_work_package from: 5, to: 4 + loading_indicator_saveguard + wp_table.expect_work_package_order(work_package1, work_package2, work_package3, work_package4, + work_package5, work_package6) + wp_table.expect_row_positions(rowcount: 8, rows: [1, 2, 3, 4, 5, 6, 7, 8]) + wp_table.expect_work_package_order(work_package1, work_package2, work_package3, work_package4, + work_package5, work_package6) + end + end + context "when dragging an element partly out of the hierarchy" do let(:work_package5) do create(:work_package, subject: "WP5", project:, parent: work_package1) @@ -376,7 +404,7 @@ def expect_query_order(query, expected) wp_table.expect_work_package_order work_package3, work_package1, work_package2, work_package4 end - it "exposes row positions again once a drop has reloaded the query" do + it "exposes row positions again once a drop settles" do wp_table.expect_row_positions(rowcount: 6, rows: [1, 2, 3, 4, 5, 6]) wp_table.drag_and_drop_work_package from: 1, to: 3 @@ -388,9 +416,6 @@ def expect_query_order(query, expected) wp_table.drag_and_drop_work_package from: 0, to: 2 wp_table.expect_work_package_order work_package3, work_package1, work_package2, work_package4 expect_query_order(query, [work_package3.id, work_package1.id, work_package2.id]) - - wp_table.visit_query query - wp_table.expect_work_package_order work_package3, work_package1, work_package2, work_package4 wp_table.expect_row_positions(rowcount: 6, rows: [1, 2, 3, 4, 5, 6]) end diff --git a/spec/features/work_packages/table/treegrid_semantics_spec.rb b/spec/features/work_packages/table/treegrid_semantics_spec.rb index 8c75aeba5551..f4826aa3d26e 100644 --- a/spec/features/work_packages/table/treegrid_semantics_spec.rb +++ b/spec/features/work_packages/table/treegrid_semantics_spec.rb @@ -255,7 +255,7 @@ it "exposes levels and expansion and removes collapsed descendants from the accessibility tree" do wp_table.visit_query(query) wp_table.expect_work_package_listed(parent, child, grandchild, sibling) - wp_table.expect_no_row_positions + wp_table.expect_row_positions(rowcount: 6, rows: [1, 2, 3, 4, 5, 6]) expect(wp_table.treegrid).to have_selector(:row, "Alpha parent", aria: { level: 1, expanded: true, selected: false }) expect(wp_table.treegrid).to have_selector(:row, "Bravo child", aria: { level: 2, expanded: true, selected: false }) @@ -276,6 +276,7 @@ descendants = [child, grandchild].map { |wp| wp_table.treegrid.find(wp_table.row_selector(wp), visible: :hidden) } expect(descendants.last).to match_selector(:element, aria: { hidden: true, selected: true }, visible: :hidden) descendants.each { |row| expect_excluded_from_accessibility_tree(row) } + wp_table.expect_row_positions(rowcount: 6, rows: [1, 2, 3, 6]) expect_axe_clean_table within(wp_table.treegrid.find(:row, "Alpha parent", aria: { expanded: false })) do @@ -307,6 +308,31 @@ end end + context "in hierarchy mode across pages" do + let!(:parent) { create(:work_package, project:, subject: "Zulu parent") } + let!(:child) { create(:work_package, project:, parent:, subject: "Alpha child") } + let!(:others) { %w[Bravo Charlie Delta].map { |name| create(:work_package, project:, subject: "#{name} other") } } + let!(:query) do + create(:query, user:, project:, column_names: %w[id subject], show_hierarchies: true, + sort_criteria: [%w[subject asc]]) + end + + before do + allow(Setting).to receive(:per_page_options).and_return "3" + end + + it "numbers contextual ancestors once and continues the numbering on the next page" do + wp_table.visit_query(query) + wp_table.expect_work_package_order(parent, child, *others.take(2)) + wp_table.expect_row_positions(rowcount: 7, rows: [1, 2, 3, 4, 5, 7]) + + within(".op-pagination--pages") { click_on "2", exact: true } + wp_table.expect_work_package_order(parent, others.last) + wp_table.expect_row_positions(rowcount: 7, rows: [1, 2, 6, 7]) + expect_axe_clean_table + end + end + context "with empty results" do let!(:doomed) { create(:work_package, project:, subject: "Doomed task") } let!(:query) do