From abc3e17891e4172dd33caa88c9d650f063835fcc Mon Sep 17 00:00:00 2001 From: Henriette Darge Date: Wed, 16 Sep 2026 15:17:05 +0200 Subject: [PATCH 1/6] Use async TreeView within the initial loading state of the project selector. The initial state should only show the root level projects. Children are only loaded when the parent is expanded. --- .../header/projects/node_component.html.erb | 24 ++- .../header/projects/node_component.rb | 2 + app/controllers/header/projects_controller.rb | 59 +++++- app/models/projects/hierarchy.rb | 85 +++++++++ .../projects/children.html_fragment.erb | 40 ++++ config/routes.rb | 1 + .../header/projects_controller_spec.rb | 180 ++++++++++++++---- .../projects/project_autocomplete_spec.rb | 7 +- 8 files changed, 345 insertions(+), 53 deletions(-) create mode 100644 app/views/header/projects/children.html_fragment.erb diff --git a/app/components/header/projects/node_component.html.erb b/app/components/header/projects/node_component.html.erb index cb3bccb97521..88dbd6536558 100644 --- a/app/components/header/projects/node_component.html.erb +++ b/app/components/header/projects/node_component.html.erb @@ -40,7 +40,7 @@ See COPYRIGHT and LICENSE files for more details. end end %> -<% if children.any? %> +<% if children.any? || deferred? %> <% @component.with_sub_tree( label:, select_variant: :none, @@ -51,15 +51,19 @@ See COPYRIGHT and LICENSE files for more details. data: { node_id: project.id, test_selector: "op-header-project-select--item" } ) do |sub| %> <% add_icons.call(sub) %> - <% children.each do |child_node| %> - <%= render Header::Projects::NodeComponent.new( - component: sub, - node: child_node, - current_project_id: @current_project_id, - favorited_ids: @favorited_ids, - jump: @jump, - query_terms: @query_terms - ) %> + <% if deferred? %> + <% sub.with_loading_spinner(src: deferred_children_path) %> + <% else %> + <% children.each do |child_node| %> + <%= render Header::Projects::NodeComponent.new( + component: sub, + node: child_node, + current_project_id: @current_project_id, + favorited_ids: @favorited_ids, + jump: @jump, + query_terms: @query_terms + ) %> + <% end %> <% end %> <% end %> <% else %> diff --git a/app/components/header/projects/node_component.rb b/app/components/header/projects/node_component.rb index ecfa4fa7064a..36140b96bcda 100644 --- a/app/components/header/projects/node_component.rb +++ b/app/components/header/projects/node_component.rb @@ -49,6 +49,8 @@ def current? = project.id == @current_project_id def favorited? = @favorited_ids.include?(project.id) def expanded? = @node[:expanded] def matches_query? = @node[:matches_query] + def deferred_children_path = @node[:deferred_children_path] + def deferred? = deferred_children_path.present? def href @jump.present? ? helpers.project_path(project.identifier, jump: @jump) : helpers.project_path(project.identifier) diff --git a/app/controllers/header/projects_controller.rb b/app/controllers/header/projects_controller.rb index 8a68d634e447..2942c23c77a6 100644 --- a/app/controllers/header/projects_controller.rb +++ b/app/controllers/header/projects_controller.rb @@ -29,17 +29,16 @@ #++ class Header::ProjectsController < ApplicationController - no_authorization_required! :index, :frame + no_authorization_required! :index, :frame, :children MAX_NUMBER_OF_PROJECTS = 300 VALID_FILTER_MODES = %w[all favorited].freeze def index - @current_project_id = params[:current_project_id].presence&.to_i - @jump = params[:jump].presence + set_request_context @query_terms = query.split @projects = load_projects - @favorited_ids = load_favorited_ids + @favorited_ids = load_favorited_ids(@projects) @tree = build_tree(@projects) render layout: false @@ -53,8 +52,26 @@ def frame ), layout: false end + # Renders one node's immediate children, fetched when it's expanded in the tree. + def children + set_request_context + parent = Project.visible.active.find(params.expect(:parent_id)) + @path = JSON.parse(params[:path]) + + child_projects = Project.nearest_visible_descendants(parent, limit: MAX_NUMBER_OF_PROJECTS).to_a + @favorited_ids = load_favorited_ids(child_projects) + @children_nodes = build_tree(child_projects) + + render layout: false, formats: [:html_fragment] + end + private + def set_request_context + @current_project_id = params[:current_project_id].presence&.to_i + @jump = params[:jump].presence + end + def query @query ||= params[:query].to_s.strip end @@ -65,7 +82,7 @@ def filter_mode end def load_projects - projects = base_scope.to_a + projects = root_query_scope.to_a projects = ensure_current_project_present(projects) if (query.present? || filter_mode == "favorited") && projects.any? @@ -77,6 +94,15 @@ def load_projects projects end + # Search & favorited need matches from any depth within the hierarchy. + # The initial tree, however, only loads the first level of hierarchy. + # The rest gets loaded from #children when nodes are expanded. + def root_query_scope + return base_scope if query.present? || filter_mode == "favorited" + + Project.nearest_visible_descendants(limit: MAX_NUMBER_OF_PROJECTS) + end + def base_scope scope = Project.visible.active.order(:lft).limit(MAX_NUMBER_OF_PROJECTS) query.split.each do |term| @@ -130,11 +156,11 @@ def favorite_project_ids user_project_favorites.select(:favorited_id) end - def load_favorited_ids + def load_favorited_ids(projects) return Set.new unless current_user.logged? user_project_favorites - .where(favorited_id: @projects.map(&:id)) + .where(favorited_id: projects.map(&:id)) .pluck(:favorited_id) .to_set end @@ -146,6 +172,10 @@ def user_project_favorites # Builds the nested tree from a flat, lft-ordered list of projects and # decorates each node with its query-match and expansion state. def build_tree(projects) + if lazy_loading? + # Needed to know which projects have visible descendants so we can display a chevron next to them + @projects_with_visible_descendants = Project.with_visible_descendants(projects).pluck(:id).to_set + end decorate_nodes(Project.build_projects_hierarchy(projects)) end @@ -154,9 +184,24 @@ def decorate_nodes(nodes) decorate_nodes(node[:children]) node[:matches_query] = @matching_ids.nil? || @matching_ids.include?(node[:project].id) node[:expanded] = expanded_node?(node) + node[:deferred_children_path] = deferred_children_path_for(node) end end + def deferred_children_path_for(node) + return unless lazy_loading? && node[:children].empty? && @projects_with_visible_descendants.include?(node[:project].id) + + children_header_projects_path( + parent_id: node[:project].id, + current_project_id: @current_project_id, + jump: @jump + ) + end + + def lazy_loading? + query.blank? && filter_mode != "favorited" + end + # A node is expanded so its children are revealed when: # - favorited mode is active (always expand to surface favorites), or # - a child is grafted onto this node because its real parent is hidden diff --git a/app/models/projects/hierarchy.rb b/app/models/projects/hierarchy.rb index aba7deb4e5d1..d20bd7314959 100644 --- a/app/models/projects/hierarchy.rb +++ b/app/models/projects/hierarchy.rb @@ -93,11 +93,96 @@ def project_tree(projects, &) project_tree_from_hierarchy(projects_hierarchy, 0, &) end + # With a boundary, the subtree is already narrow, so the lft/rgt range check stays cheap. + # Without one, every visible project is a candidate, so this walks parent_id through a + # recursive CTE instead, since Postgres can hash that join rather than scanning ranges. + def nearest_visible_descendants(boundary = nil, limit: nil) + return nearest_visible_roots(limit:) if boundary.nil? + + scope = nearest_visible_descendants_within(boundary) + limit ? scope.limit(limit) : scope + end + + # Returns the projects in `candidates` that have at least one visible, active descendant. + # Used to decide whether a node gets an expand arrow or renders as a leaf + # has_subprojects?/leaf? is not enough, as it only looks at lft/rgt and would count an archived or invisible + # descendant as "has children" too. + def with_visible_descendants(candidates) + candidates = Array(candidates) + return none if candidates.empty? + + visible_ids = visible_active_ids_within(candidates) + return none if visible_ids.empty? + + Project + .where(id: candidates.map(&:id)) + .where( + "EXISTS ( + SELECT 1 FROM projects descendant + WHERE descendant.id IN (?) + AND descendant.lft > projects.lft AND descendant.rgt < projects.rgt + )", visible_ids + ) + end + private def sort_by_name(project_hashes) project_hashes.sort_by { |h| h[:project].name&.downcase } end + + def visible_active_ids_within(candidates) + min_lft = candidates.map(&:lft).min + max_rgt = candidates.map(&:rgt).max + Project.visible.active.where("lft > ? AND rgt < ?", min_lft, max_rgt).pluck(:id) + end + + def nearest_visible_descendants_within(boundary) + visible_ids = Project.visible.active.pluck(:id) + return none if visible_ids.empty? + + where(id: visible_ids) + .where("projects.lft > ? AND projects.rgt < ?", boundary.lft, boundary.rgt) + .where(no_visible_ancestor_between_sql, visible_ids, boundary.lft, boundary.rgt) + .order(:lft) + end + + def no_visible_ancestor_between_sql + <<~SQL.squish + NOT EXISTS ( + SELECT 1 FROM projects ancestors + WHERE ancestors.id IN (?) + AND ancestors.lft < projects.lft AND ancestors.rgt > projects.rgt + AND ancestors.lft > ? AND ancestors.rgt < ? + ) + SQL + end + + # Visible/active projects whose ancestors are not visible to the current user + # checked via parent_id rather than the lft/rgt range check, which + # doesn't scale once every visible project is a candidate. + def nearest_visible_roots(limit: nil) + visible_ids = Project.visible.active.pluck(:id) + return [] if visible_ids.empty? + + sql = <<~SQL.squish + WITH RECURSIVE ancestor_walk(descendant_id, current_id) AS ( + SELECT id, parent_id FROM projects WHERE parent_id IS NOT NULL + UNION ALL + SELECT ancestor_walk.descendant_id, projects.parent_id + FROM ancestor_walk + JOIN projects ON projects.id = ancestor_walk.current_id + WHERE projects.parent_id IS NOT NULL + ) + SELECT projects.* FROM projects + WHERE projects.id IN (?) + AND projects.id NOT IN (SELECT DISTINCT descendant_id FROM ancestor_walk WHERE current_id IN (?)) + ORDER BY projects.lft + #{"LIMIT #{limit.to_i}" if limit} + SQL + + find_by_sql([sql, visible_ids, visible_ids]) + end end included do diff --git a/app/views/header/projects/children.html_fragment.erb b/app/views/header/projects/children.html_fragment.erb new file mode 100644 index 000000000000..7217fc19556e --- /dev/null +++ b/app/views/header/projects/children.html_fragment.erb @@ -0,0 +1,40 @@ +<%#-- 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, write to the Free Software +Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + +See COPYRIGHT and LICENSE files for more details. + +++#%> + +<%= render(Primer::Alpha::TreeView::SubTree.new(path: @path, node_variant: :anchor)) do |sub| %> + <% @children_nodes.each do |node| %> + <%= render Header::Projects::NodeComponent.new( + component: sub, + node:, + current_project_id: @current_project_id, + favorited_ids: @favorited_ids, + jump: @jump + ) %> + <% end %> +<% end %> diff --git a/config/routes.rb b/config/routes.rb index 276b0cb7c553..6cb5285739e8 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -514,6 +514,7 @@ resources :projects, only: :index do collection do get :frame + get :children end end end diff --git a/spec/controllers/header/projects_controller_spec.rb b/spec/controllers/header/projects_controller_spec.rb index d867ed4856fb..6a4390f712d1 100644 --- a/spec/controllers/header/projects_controller_spec.rb +++ b/spec/controllers/header/projects_controller_spec.rb @@ -59,9 +59,10 @@ expect(response).to have_http_status(:ok) end - it "includes visible active projects" do + it "includes only root-level visible projects, not deeper descendants" do make_request - expect(assigns(:projects)).to include(parent_project, child_project, other_project) + expect(assigns(:projects)).to include(parent_project, other_project) + expect(assigns(:projects)).not_to include(child_project) end it "renders without layout" do @@ -69,58 +70,74 @@ expect(response).to render_template(layout: false) end - it "keeps ordinary parent projects collapsed by default" do + it "keeps ordinary parent projects collapsed, with a deferred children path instead of loaded children" do make_request parent_node = assigns(:tree).find { |node| node[:project] == parent_project } expect(parent_node[:expanded]).to be(false) + expect(parent_node[:children]).to be_empty + expect(parent_node[:deferred_children_path]).to be_present end - context "when an invisible project is between two visible projects" do - shared_let(:invisible_project) { create(:private_project, name: "Invisible", parent: parent_project) } - shared_let(:visible_grandchild) { create(:project, name: "Visible Grandchild", parent: invisible_project) } + it "does not add a deferred children path to leaf root projects" do + make_request + + other_node = assigns(:tree).find { |node| node[:project] == other_project } + expect(other_node[:deferred_children_path]).to be_nil + end + + context "when a root project's only subprojects are invisible to the current user" do + shared_let(:root_with_hidden_child) { create(:project, name: "Root With Hidden Child") } + shared_let(:hidden_child) { create(:private_project, name: "Hidden Child", parent: root_with_hidden_child) } before do - create(:member, principal: current_user, project: visible_grandchild, roles: [role]) + create(:member, principal: current_user, project: root_with_hidden_child, roles: [role]) end - it "nests the grandchild below its nearest visible ancestor", :aggregate_failures do + it "does not add a deferred children path (no expand arrow with nothing behind it)" do make_request - tree = assigns(:tree) - parent_node = tree.find { |node| node[:project] == parent_project } - - expect(assigns(:projects)).to include(visible_grandchild) - expect(assigns(:projects)).not_to include(invisible_project) - expect(response.body).not_to include("Invisible") - expect(tree.pluck(:project)).not_to include(visible_grandchild) - expect(parent_node[:children].pluck(:project)).to include(visible_grandchild) - expect(parent_node[:expanded]).to be(true) + node = assigns(:tree).find { |n| n[:project] == root_with_hidden_child } + expect(node[:deferred_children_path]).to be_nil end end - context "when a hidden project sits mid-way in a visible chain" do - shared_let(:visible_root) { create(:project, name: "Root Visible") } - shared_let(:visible_mid) { create(:project, name: "Mid Visible", parent: visible_root) } - shared_let(:hidden_middle) { create(:private_project, name: "Hidden Middle", parent: visible_mid) } - shared_let(:visible_leaf) { create(:project, name: "Leaf Visible", parent: hidden_middle) } + context "when a root project's only subprojects are archived" do + shared_let(:root_with_archived_child) { create(:project, name: "Root With Archived Child") } + shared_let(:archived_child) do + create(:project, name: "Archived Child", parent: root_with_archived_child, active: false) + end before do - create(:member, principal: current_user, project: visible_root, roles: [role]) - create(:member, principal: current_user, project: visible_mid, roles: [role]) - create(:member, principal: current_user, project: visible_leaf, roles: [role]) + create(:member, principal: current_user, project: root_with_archived_child, roles: [role]) end - it "expands every visible ancestor down to the grafted leaf", :aggregate_failures do + it "does not add a deferred children path (no expand arrow with nothing behind it)" do make_request - tree = assigns(:tree) - root_node = tree.find { |node| node[:project] == visible_root } - mid_node = root_node[:children].find { |node| node[:project] == visible_mid } + node = assigns(:tree).find { |n| n[:project] == root_with_archived_child } + expect(node[:deferred_children_path]).to be_nil + end + end - expect(root_node[:expanded]).to be(true) - expect(mid_node[:expanded]).to be(true) - expect(mid_node[:children].pluck(:project)).to include(visible_leaf) + context "when a visible project's parent is invisible" do + shared_let(:hidden_root) { create(:private_project, name: "Hidden Root") } + shared_let(:promoted_project) { create(:project, name: "Promoted", parent: hidden_root) } + + before do + create(:member, principal: current_user, project: promoted_project, roles: [role]) + end + + it "promotes the visible project to the top level instead of hiding it", :aggregate_failures do + make_request + + expect(promoted_project.parent_id).to be_present + expect(assigns(:projects)).to include(promoted_project) + expect(assigns(:projects)).not_to include(hidden_root) + expect(response.body).not_to include("Hidden Root") + + promoted_node = assigns(:tree).find { |node| node[:project] == promoted_project } + expect(promoted_node).to be_present end end @@ -242,9 +259,104 @@ end context "with an invalid filter_mode param" do - it "defaults to showing all projects" do + it "defaults to showing all root-level projects" do get :index, params: { filter_mode: "invalid" } - expect(assigns(:projects)).to include(parent_project, child_project, other_project) + expect(assigns(:projects)).to include(parent_project, other_project) + end + end + end + + describe "#children" do + render_views + shared_let(:parent_project) { create(:project, name: "Alpha Parent") } + shared_let(:child_project) { create(:project, name: "Beta Child", parent: parent_project) } + shared_let(:grandchild_project) { create(:project, name: "Gamma Grandchild", parent: child_project) } + shared_let(:role) { create(:project_role) } + + before do + create(:member, principal: current_user, project: parent_project, roles: [role]) + create(:member, principal: current_user, project: child_project, roles: [role]) + create(:member, principal: current_user, project: grandchild_project, roles: [role]) + end + + subject(:make_request) { get :children, params: { parent_id: parent_project.id, path: "[]" } } + + it "returns HTTP 200" do + make_request + expect(response).to have_http_status(:ok) + end + + it "renders without layout" do + make_request + expect(response).to render_template(layout: false) + end + + it "returns only the immediate children of the requested parent" do + make_request + expect(assigns(:children_nodes).pluck(:project)).to contain_exactly(child_project) + end + + it "marks a child with further descendants as deferred rather than loading them eagerly" do + make_request + + child_node = assigns(:children_nodes).find { |node| node[:project] == child_project } + expect(child_node[:children]).to be_empty + expect(child_node[:deferred_children_path]).to be_present + end + + context "when the requested parent has no children" do + subject(:make_request) { get :children, params: { parent_id: grandchild_project.id, path: "[]" } } + + it "returns an empty list" do + make_request + expect(assigns(:children_nodes)).to be_empty + end + end + + context "when the parent is not visible to the current user" do + let(:private_parent) { create(:private_project, name: "Private Parent") } + + subject(:make_request) { get :children, params: { parent_id: private_parent.id, path: "[]" } } + + it "responds with 404" do + make_request + expect(response).to have_http_status(:not_found) + end + end + + context "when a visible grandchild sits behind an invisible child" do + shared_let(:hidden_middle) { create(:private_project, name: "Hidden Middle", parent: parent_project) } + shared_let(:visible_grandchild) { create(:project, name: "Visible Grandchild", parent: hidden_middle) } + + before do + create(:member, principal: current_user, project: visible_grandchild, roles: [role]) + end + + it "skips the invisible child and surfaces the nearest visible descendant" do + make_request + + projects = assigns(:children_nodes).pluck(:project) + expect(projects).to include(visible_grandchild) + expect(projects).not_to include(hidden_middle) + expect(response.body).not_to include("Hidden Middle") + end + end + + context "when a returned child's only subprojects are invisible to the current user" do + shared_let(:sibling_with_hidden_child) { create(:project, name: "Sibling With Hidden Child", parent: parent_project) } + shared_let(:hidden_grandchild) do + create(:private_project, name: "Hidden Grandchild", parent: sibling_with_hidden_child) + end + + before do + create(:member, principal: current_user, project: sibling_with_hidden_child, roles: [role]) + end + + it "does not add a deferred children path to that child (no expand arrow with nothing behind it)" do + make_request + + node = assigns(:children_nodes).find { |n| n[:project] == sibling_with_hidden_child } + expect(node[:deferred_children_path]).to be_nil end end end diff --git a/spec/features/projects/project_autocomplete_spec.rb b/spec/features/projects/project_autocomplete_spec.rb index a561387572e8..7d3277dc13a9 100644 --- a/spec/features/projects/project_autocomplete_spec.rb +++ b/spec/features/projects/project_autocomplete_spec.rb @@ -231,9 +231,12 @@ top_menu.expect_result visible_grandparent.name top_menu.expect_no_result invisible_parent.name - top_menu.expect_item_with_hierarchy_level hierarchy_level: 2, - item_name: visible_grandchild.name end + + top_menu.expand_node_for visible_grandparent.name + top_menu.expect_no_result invisible_parent.name + top_menu.expect_item_with_hierarchy_level hierarchy_level: 2, + item_name: visible_grandchild.name end it "displays workspace type badges for portfolios and programs" do From 2486fb9376f7fdbe37d89cfb6e6c9d9a4157104f Mon Sep 17 00:00:00 2001 From: Henriette Darge Date: Thu, 17 Sep 2026 11:31:23 +0200 Subject: [PATCH 2/6] Replace nearest_visible_descendants with a single query to search for elements within the lft/rgt ranges --- app/models/projects/hierarchy.rb | 64 +++++++++----------------------- 1 file changed, 17 insertions(+), 47 deletions(-) diff --git a/app/models/projects/hierarchy.rb b/app/models/projects/hierarchy.rb index d20bd7314959..da62071f7d1d 100644 --- a/app/models/projects/hierarchy.rb +++ b/app/models/projects/hierarchy.rb @@ -93,14 +93,10 @@ def project_tree(projects, &) project_tree_from_hierarchy(projects_hierarchy, 0, &) end - # With a boundary, the subtree is already narrow, so the lft/rgt range check stays cheap. - # Without one, every visible project is a candidate, so this walks parent_id through a - # recursive CTE instead, since Postgres can hash that join rather than scanning ranges. + # Returns the visible/active projects within `boundary` (or, without one, all visible/active + # root-level projects) that have no visible/active ancestor also in that set. def nearest_visible_descendants(boundary = nil, limit: nil) - return nearest_visible_roots(limit:) if boundary.nil? - - scope = nearest_visible_descendants_within(boundary) - limit ? scope.limit(limit) : scope + find_by_sql(nearest_visible_descendants_sql(boundary, limit)) end # Returns the projects in `candidates` that have at least one visible, active descendant. @@ -137,51 +133,25 @@ def visible_active_ids_within(candidates) Project.visible.active.where("lft > ? AND rgt < ?", min_lft, max_rgt).pluck(:id) end - def nearest_visible_descendants_within(boundary) - visible_ids = Project.visible.active.pluck(:id) - return none if visible_ids.empty? + # Both the root case and the within-boundary case reduce to the same question: which + # projects in a visible set have no ancestor also in that set? Only the set differs - + # unrestricted for roots, narrowed to the boundary's lft/rgt range otherwise. + def nearest_visible_descendants_sql(boundary, limit) + vp_range = "vp.lft > #{boundary.lft.to_i} AND vp.rgt < #{boundary.rgt.to_i}" if boundary + ancestor_range = "ancestor_vp.lft > #{boundary.lft.to_i} AND ancestor_vp.rgt < #{boundary.rgt.to_i}" if boundary - where(id: visible_ids) - .where("projects.lft > ? AND projects.rgt < ?", boundary.lft, boundary.rgt) - .where(no_visible_ancestor_between_sql, visible_ids, boundary.lft, boundary.rgt) - .order(:lft) - end - - def no_visible_ancestor_between_sql <<~SQL.squish - NOT EXISTS ( - SELECT 1 FROM projects ancestors - WHERE ancestors.id IN (?) - AND ancestors.lft < projects.lft AND ancestors.rgt > projects.rgt - AND ancestors.lft > ? AND ancestors.rgt < ? + WITH visible_projects AS (#{Project.visible.active.to_sql}) + SELECT vp.* FROM visible_projects vp + WHERE NOT EXISTS ( + SELECT 1 FROM visible_projects ancestor_vp + WHERE ancestor_vp.lft < vp.lft AND ancestor_vp.rgt > vp.rgt + #{"AND #{ancestor_range}" if ancestor_range} ) - SQL - end - - # Visible/active projects whose ancestors are not visible to the current user - # checked via parent_id rather than the lft/rgt range check, which - # doesn't scale once every visible project is a candidate. - def nearest_visible_roots(limit: nil) - visible_ids = Project.visible.active.pluck(:id) - return [] if visible_ids.empty? - - sql = <<~SQL.squish - WITH RECURSIVE ancestor_walk(descendant_id, current_id) AS ( - SELECT id, parent_id FROM projects WHERE parent_id IS NOT NULL - UNION ALL - SELECT ancestor_walk.descendant_id, projects.parent_id - FROM ancestor_walk - JOIN projects ON projects.id = ancestor_walk.current_id - WHERE projects.parent_id IS NOT NULL - ) - SELECT projects.* FROM projects - WHERE projects.id IN (?) - AND projects.id NOT IN (SELECT DISTINCT descendant_id FROM ancestor_walk WHERE current_id IN (?)) - ORDER BY projects.lft + #{"AND #{vp_range}" if vp_range} + ORDER BY vp.lft #{"LIMIT #{limit.to_i}" if limit} SQL - - find_by_sql([sql, visible_ids, visible_ids]) end end From 58367a65f9e72c61976d8dda48f4b374a532aa45 Mon Sep 17 00:00:00 2001 From: ulferts Date: Thu, 17 Sep 2026 16:52:56 +0200 Subject: [PATCH 3/6] performant query for visible projects without ancestors Initially, the set of visible projects was in a CTE referenced twice in the query, which led to the CTE being manifested. In this case, this is iefficient as the index on lft/rgt cannot be used. But even when preventing the manifestation, the comparison within an EXIST led to the two instances of the set needing to be looped through (nested loop anti join) effectively resulting in the cross product being considered. The solution here is to rely on the awesome_nested_set's algorithm to effectively calculate whether a project has visible ancestors which can only be those preceeding in a list ordered by lft. --- app/models/projects/hierarchy.rb | 65 +++++++++++++++++++++----------- 1 file changed, 43 insertions(+), 22 deletions(-) diff --git a/app/models/projects/hierarchy.rb b/app/models/projects/hierarchy.rb index da62071f7d1d..a15a865b18d1 100644 --- a/app/models/projects/hierarchy.rb +++ b/app/models/projects/hierarchy.rb @@ -96,7 +96,49 @@ def project_tree(projects, &) # Returns the visible/active projects within `boundary` (or, without one, all visible/active # root-level projects) that have no visible/active ancestor also in that set. def nearest_visible_descendants(boundary = nil, limit: nil) - find_by_sql(nearest_visible_descendants_sql(boundary, limit)) + # Both the root case and the within-boundary case reduce to the same question: which + # projects in a visible set have no ancestor also in that set? Only the set differs - + # unrestricted for roots, narrowed to the boundary's descendants otherwise. + scope = if boundary + boundary.descendants + else + Project.all + end + + # With awesome_nested_set, for a list of projects ordered by "lft ASC" + # one part of the criteria for whether a project A is an ancestor of another + # project B is already given by the position in the list (A.lft < B.lft). + # Only projects above the current project can potentially be ancestors and need + # to be checked. + # Note that here, those that have no ancestors are of interest. + # We know that a project has no ancestor if its rgt is higher than all the rgt values + # of the preceding projects (B.rgt > A.rgt) + # + # To calculate this efficiently, a window function is used: + # max_preceding_rgt is the maximum rgt value of all the preceding projects + # (`ROWS BETWEEN UNBOUNDED PRECEDING AND 1 PRECEDING`) where the preceding projects + # are those with a smaller lft value (ORDER BY lft). + # + # Since the value of max_preceding_rgt is calculated on each row, it can now be compared + # directly within itself. + # The first row is a special case, as it has no preceding projects, its max_preceding_rgt is NULL, which is + # why the `IS NULL` condition is necessary. + visible_scope = scope + .visible + .select(" + *, + max(rgt) OVER (ORDER BY lft ROWS BETWEEN UNBOUNDED PRECEDING AND 1 PRECEDING) AS max_preceding_rgt") + + ancestor_scope = Project + .with(projects: visible_scope) + .where("projects.max_preceding_rgt IS NULL OR projects.rgt > projects.max_preceding_rgt") + .order("projects.lft") + + if limit + ancestor_scope.limit(limit) + else + ancestor_scope + end end # Returns the projects in `candidates` that have at least one visible, active descendant. @@ -132,27 +174,6 @@ def visible_active_ids_within(candidates) max_rgt = candidates.map(&:rgt).max Project.visible.active.where("lft > ? AND rgt < ?", min_lft, max_rgt).pluck(:id) end - - # Both the root case and the within-boundary case reduce to the same question: which - # projects in a visible set have no ancestor also in that set? Only the set differs - - # unrestricted for roots, narrowed to the boundary's lft/rgt range otherwise. - def nearest_visible_descendants_sql(boundary, limit) - vp_range = "vp.lft > #{boundary.lft.to_i} AND vp.rgt < #{boundary.rgt.to_i}" if boundary - ancestor_range = "ancestor_vp.lft > #{boundary.lft.to_i} AND ancestor_vp.rgt < #{boundary.rgt.to_i}" if boundary - - <<~SQL.squish - WITH visible_projects AS (#{Project.visible.active.to_sql}) - SELECT vp.* FROM visible_projects vp - WHERE NOT EXISTS ( - SELECT 1 FROM visible_projects ancestor_vp - WHERE ancestor_vp.lft < vp.lft AND ancestor_vp.rgt > vp.rgt - #{"AND #{ancestor_range}" if ancestor_range} - ) - #{"AND #{vp_range}" if vp_range} - ORDER BY vp.lft - #{"LIMIT #{limit.to_i}" if limit} - SQL - end end included do From 9d5ec0c85cdd9486057e829f6d5df7357e8ac321 Mon Sep 17 00:00:00 2001 From: Henriette Darge Date: Fri, 18 Sep 2026 10:35:43 +0200 Subject: [PATCH 4/6] Load the complete upwards tree not only direct ancestors --- app/controllers/header/projects_controller.rb | 9 +++- .../header/projects_controller_spec.rb | 50 +++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/app/controllers/header/projects_controller.rb b/app/controllers/header/projects_controller.rb index 2942c23c77a6..4b70bf3ec72c 100644 --- a/app/controllers/header/projects_controller.rb +++ b/app/controllers/header/projects_controller.rb @@ -133,8 +133,15 @@ def skip_current_project_inclusion? query.present? || filter_mode == "favorited" || @current_project_id.blank? end + # Loads each ancestor's full child set (not just the chain down to `current`), so every + # ancestor on the path renders exactly as if it had been expanded manually - siblings + # included, and with no dangling expand arrow left behind for it. def merge_with_ancestors(projects, current) - (projects + current.self_and_ancestors.visible.active.to_a).uniq(&:id).sort_by(&:lft) + ancestors = current.self_and_ancestors.visible.active.to_a + ancestor_children = (ancestors - [current]).flat_map do |ancestor| + Project.nearest_visible_descendants(ancestor, limit: MAX_NUMBER_OF_PROJECTS).to_a + end + (projects + ancestors + ancestor_children).uniq(&:id).sort_by(&:lft) end # Returns a scope for all visible, active ancestors of the given projects diff --git a/spec/controllers/header/projects_controller_spec.rb b/spec/controllers/header/projects_controller_spec.rb index 6a4390f712d1..a675a5af0308 100644 --- a/spec/controllers/header/projects_controller_spec.rb +++ b/spec/controllers/header/projects_controller_spec.rb @@ -258,6 +258,56 @@ end end + context "when a visible project's sibling isn't loaded yet" do + shared_let(:sibling_project) { create(:project, name: "Alpha Sibling", parent: parent_project) } + + subject(:make_request) { get :index, params: { current_project_id: child_project.id } } + + before do + create(:member, principal: current_user, project: sibling_project, roles: [role]) + end + + it "includes the sibling alongside the current project, with the parent fully loaded", :aggregate_failures do + make_request + + expect(assigns(:projects)).to include(child_project, sibling_project, parent_project) + + parent_node = assigns(:tree).find { |node| node[:project] == parent_project } + expect(parent_node[:children].pluck(:project)).to contain_exactly(child_project, sibling_project) + expect(parent_node[:deferred_children_path]).to be_nil + end + end + + context "when siblings exist at multiple levels of the ancestor chain" do + shared_let(:top_root) { create(:project, name: "Root Multi") } + shared_let(:root_sibling) { create(:project, name: "Root Multi Sibling", parent: top_root) } + shared_let(:mid_parent) { create(:project, name: "Mid Parent", parent: top_root) } + shared_let(:mid_sibling) { create(:project, name: "Mid Sibling", parent: mid_parent) } + shared_let(:leaf_project) { create(:project, name: "Leaf Project", parent: mid_parent) } + + subject(:make_request) { get :index, params: { current_project_id: leaf_project.id } } + + before do + [top_root, root_sibling, mid_parent, mid_sibling, leaf_project].each do |project| + create(:member, principal: current_user, project:, roles: [role]) + end + end + + it "includes siblings at every level, not just the immediate parent", :aggregate_failures do + make_request + + expect(assigns(:projects)).to include(top_root, root_sibling, mid_parent, mid_sibling, leaf_project) + + root_node = assigns(:tree).find { |node| node[:project] == top_root } + expect(root_node[:children].pluck(:project)).to contain_exactly(mid_parent, root_sibling) + expect(root_node[:deferred_children_path]).to be_nil + + mid_node = root_node[:children].find { |node| node[:project] == mid_parent } + expect(mid_node[:children].pluck(:project)).to contain_exactly(leaf_project, mid_sibling) + expect(mid_node[:deferred_children_path]).to be_nil + end + end + context "with an invalid filter_mode param" do it "defaults to showing all root-level projects" do get :index, params: { filter_mode: "invalid" } From 57fcde2f7f96396990f9a258004d50b35f22fedb Mon Sep 17 00:00:00 2001 From: Henriette Darge Date: Tue, 29 Sep 2026 15:07:46 +0200 Subject: [PATCH 5/6] Add tests for the hierarchy helper --- app/models/projects/hierarchy.rb | 6 +- .../models/projects/project_hierarchy_spec.rb | 181 ++++++++++++++++++ 2 files changed, 185 insertions(+), 2 deletions(-) create mode 100644 spec/models/projects/project_hierarchy_spec.rb diff --git a/app/models/projects/hierarchy.rb b/app/models/projects/hierarchy.rb index a15a865b18d1..b216fccc9b15 100644 --- a/app/models/projects/hierarchy.rb +++ b/app/models/projects/hierarchy.rb @@ -94,7 +94,9 @@ def project_tree(projects, &) end # Returns the visible/active projects within `boundary` (or, without one, all visible/active - # root-level projects) that have no visible/active ancestor also in that set. + # projects) that have no visible/active ancestor also in that set. Without a boundary, this + # includes true root-level projects, but also deeper projects whose real ancestors are all + # invisible or archived - they get promoted to the top level instead of hidden. def nearest_visible_descendants(boundary = nil, limit: nil) # Both the root case and the within-boundary case reduce to the same question: which # projects in a visible set have no ancestor also in that set? Only the set differs - @@ -108,7 +110,7 @@ def nearest_visible_descendants(boundary = nil, limit: nil) # With awesome_nested_set, for a list of projects ordered by "lft ASC" # one part of the criteria for whether a project A is an ancestor of another # project B is already given by the position in the list (A.lft < B.lft). - # Only projects above the current project can potentially be ancestors and need + # Only projects before the current project can potentially be ancestors and need # to be checked. # Note that here, those that have no ancestors are of interest. # We know that a project has no ancestor if its rgt is higher than all the rgt values diff --git a/spec/models/projects/project_hierarchy_spec.rb b/spec/models/projects/project_hierarchy_spec.rb new file mode 100644 index 000000000000..ce7166a45487 --- /dev/null +++ b/spec/models/projects/project_hierarchy_spec.rb @@ -0,0 +1,181 @@ +# 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, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe Project, "hierarchy" do + shared_let(:current_user) { create(:user) } + shared_let(:role) { create(:project_role) } + + before do + login_as current_user + end + + describe ".nearest_visible_descendants" do + context "without a boundary" do + shared_let(:root) { create(:project, name: "Root") } + shared_let(:child) { create(:project, name: "Child", parent: root) } + shared_let(:other_root) { create(:project, name: "Other Root") } + shared_let(:archived_root) { create(:project, name: "Archived Root", active: false) } + + before do + [root, child, other_root, archived_root].each do |project| + create(:member, principal: current_user, project:, roles: [role]) + end + end + + it "returns visible, active root-level projects" do + expect(described_class.nearest_visible_descendants).to contain_exactly(root, other_root) + end + + it "does not return a project nested under a visible parent" do + expect(described_class.nearest_visible_descendants).not_to include(child) + end + + it "does not return an archived root project" do + expect(described_class.nearest_visible_descendants).not_to include(archived_root) + end + + context "when a visible project's real parent is invisible" do + shared_let(:hidden_root) { create(:private_project, name: "Hidden Root") } + shared_let(:promoted) { create(:project, name: "Promoted", parent: hidden_root) } + + before do + create(:member, principal: current_user, project: promoted, roles: [role]) + end + + it "promotes the project to the top level instead of hiding it" do + expect(described_class.nearest_visible_descendants).to include(promoted) + expect(described_class.nearest_visible_descendants).not_to include(hidden_root) + end + end + + it "respects the limit and orders by lft" do + lft_first = [root, other_root].min_by(&:lft) + + expect(described_class.nearest_visible_descendants(limit: 1)).to contain_exactly(lft_first) + end + end + + context "with a boundary" do + shared_let(:parent) { create(:project, name: "Parent") } + shared_let(:child) { create(:project, name: "Child", parent:) } + shared_let(:grandchild) { create(:project, name: "Grandchild", parent: child) } + shared_let(:archived_child) { create(:project, name: "Archived Child", parent:, active: false) } + + before do + [parent, child, grandchild, archived_child].each do |project| + create(:member, principal: current_user, project:, roles: [role]) + end + end + + it "returns only the boundary's direct visible, active children" do + expect(described_class.nearest_visible_descendants(parent)).to contain_exactly(child) + end + + it "does not return grandchildren behind a visible child" do + expect(described_class.nearest_visible_descendants(parent)).not_to include(grandchild) + end + + it "does not return an archived child" do + expect(described_class.nearest_visible_descendants(parent)).not_to include(archived_child) + end + + context "when the direct child is invisible but the grandchild is visible" do + shared_let(:hidden_child) { create(:private_project, name: "Hidden Child", parent:) } + shared_let(:visible_grandchild) { create(:project, name: "Visible Grandchild", parent: hidden_child) } + + before do + create(:member, principal: current_user, project: visible_grandchild, roles: [role]) + end + + it "skips the invisible child and surfaces the nearest visible descendant" do + result = described_class.nearest_visible_descendants(parent) + expect(result).to include(visible_grandchild) + expect(result).not_to include(hidden_child) + end + end + end + end + + describe ".with_visible_descendants" do + shared_let(:parent) { create(:project, name: "Parent") } + shared_let(:child) { create(:project, name: "Child", parent:) } + shared_let(:leaf) { create(:project, name: "Leaf") } + + before do + [parent, child, leaf].each do |project| + create(:member, principal: current_user, project:, roles: [role]) + end + end + + it "includes a project that has a visible, active descendant" do + expect(described_class.with_visible_descendants([parent, leaf])).to contain_exactly(parent) + end + + it "excludes a project without descendants" do + expect(described_class.with_visible_descendants([parent, leaf])).not_to include(leaf) + end + + context "when the only descendant is invisible" do + shared_let(:lonely_parent) { create(:project, name: "Lonely Parent") } + shared_let(:hidden_child) { create(:private_project, name: "Hidden Child", parent: lonely_parent) } + + before do + create(:member, principal: current_user, project: lonely_parent, roles: [role]) + end + + it "excludes that project" do + expect(described_class.with_visible_descendants([lonely_parent])).not_to include(lonely_parent) + end + end + + context "when the only descendant is archived" do + shared_let(:archived_parent) { create(:project, name: "Archived Parent") } + shared_let(:archived_child) { create(:project, name: "Archived Child", parent: archived_parent, active: false) } + + before do + create(:member, principal: current_user, project: archived_parent, roles: [role]) + end + + it "excludes that project" do + expect(described_class.with_visible_descendants([archived_parent])).not_to include(archived_parent) + end + end + + it "accepts a single project instead of an array" do + expect(described_class.with_visible_descendants(parent)).to contain_exactly(parent) + end + + it "returns none for an empty array" do + expect(described_class.with_visible_descendants([])).to be_empty + end + end +end From cb5ff44df1cd42ad57a676e4f115efa9fc8724a3 Mon Sep 17 00:00:00 2001 From: Henriette Darge Date: Thu, 1 Oct 2026 12:20:35 +0200 Subject: [PATCH 6/6] Clenaup test, controller and helper in structure and naming --- app/controllers/header/projects_controller.rb | 28 +++-- app/models/projects/hierarchy.rb | 14 ++- .../models/projects/project_hierarchy_spec.rb | 101 +++++++++--------- 3 files changed, 77 insertions(+), 66 deletions(-) diff --git a/app/controllers/header/projects_controller.rb b/app/controllers/header/projects_controller.rb index 4b70bf3ec72c..e146f071ae8d 100644 --- a/app/controllers/header/projects_controller.rb +++ b/app/controllers/header/projects_controller.rb @@ -179,24 +179,25 @@ def user_project_favorites # Builds the nested tree from a flat, lft-ordered list of projects and # decorates each node with its query-match and expansion state. def build_tree(projects) - if lazy_loading? - # Needed to know which projects have visible descendants so we can display a chevron next to them - @projects_with_visible_descendants = Project.with_visible_descendants(projects).pluck(:id).to_set - end - decorate_nodes(Project.build_projects_hierarchy(projects)) + # Empty outside browse mode rather than skipped, so defer_children? doesn't need to know why - + # it just checks membership. Search/favorited already load everything eagerly. Computed once + # here and passed down, rather than recomputed per node. + project_ids_having_visible_descendants = + lazy_loading? ? Project.having_visible_descendants(projects).pluck(:id).to_set : Set.new + decorate_nodes(Project.build_projects_hierarchy(projects), project_ids_having_visible_descendants) end - def decorate_nodes(nodes) + def decorate_nodes(nodes, project_ids_having_visible_descendants) nodes.each do |node| - decorate_nodes(node[:children]) + decorate_nodes(node[:children], project_ids_having_visible_descendants) node[:matches_query] = @matching_ids.nil? || @matching_ids.include?(node[:project].id) node[:expanded] = expanded_node?(node) - node[:deferred_children_path] = deferred_children_path_for(node) + node[:deferred_children_path] = deferred_children_path_for(node, project_ids_having_visible_descendants) end end - def deferred_children_path_for(node) - return unless lazy_loading? && node[:children].empty? && @projects_with_visible_descendants.include?(node[:project].id) + def deferred_children_path_for(node, project_ids_having_visible_descendants) + return unless defer_children?(node, project_ids_having_visible_descendants) children_header_projects_path( parent_id: node[:project].id, @@ -205,6 +206,13 @@ def deferred_children_path_for(node) ) end + # A node's children are deferred (shown behind a lazy-load link instead of loaded eagerly) + # when we're browsing (search/favorited load everything eagerly), its children aren't loaded + # yet, and it actually has visible descendants to show. + def defer_children?(node, project_ids_having_visible_descendants) + lazy_loading? && node[:children].empty? && project_ids_having_visible_descendants.include?(node[:project].id) + end + def lazy_loading? query.blank? && filter_mode != "favorited" end diff --git a/app/models/projects/hierarchy.rb b/app/models/projects/hierarchy.rb index b216fccc9b15..bb98a45ce0a1 100644 --- a/app/models/projects/hierarchy.rb +++ b/app/models/projects/hierarchy.rb @@ -93,10 +93,14 @@ def project_tree(projects, &) project_tree_from_hierarchy(projects_hierarchy, 0, &) end - # Returns the visible/active projects within `boundary` (or, without one, all visible/active - # projects) that have no visible/active ancestor also in that set. Without a boundary, this - # includes true root-level projects, but also deeper projects whose real ancestors are all - # invisible or archived - they get promoted to the top level instead of hidden. + # Returns `boundary`'s nearest visible/active descendants - normally its direct children, + # but if a child is invisible or archived, its own visible/active children are surfaced instead + # of hiding that whole branch (repeating as needed, so a chain of several invisible + # ancestors is skipped entirely). + # + # Without a boundary, this does the same thing for the whole instance: the result is the + # visible/active root-level projects, plus any deeper project promoted to the top level + # because its real ancestors are all invisible or archived. def nearest_visible_descendants(boundary = nil, limit: nil) # Both the root case and the within-boundary case reduce to the same question: which # projects in a visible set have no ancestor also in that set? Only the set differs - @@ -147,7 +151,7 @@ def nearest_visible_descendants(boundary = nil, limit: nil) # Used to decide whether a node gets an expand arrow or renders as a leaf # has_subprojects?/leaf? is not enough, as it only looks at lft/rgt and would count an archived or invisible # descendant as "has children" too. - def with_visible_descendants(candidates) + def having_visible_descendants(candidates) candidates = Array(candidates) return none if candidates.empty? diff --git a/spec/models/projects/project_hierarchy_spec.rb b/spec/models/projects/project_hierarchy_spec.rb index ce7166a45487..4a9f4a5500ba 100644 --- a/spec/models/projects/project_hierarchy_spec.rb +++ b/spec/models/projects/project_hierarchy_spec.rb @@ -39,18 +39,20 @@ end describe ".nearest_visible_descendants" do - context "without a boundary" do - shared_let(:root) { create(:project, name: "Root") } - shared_let(:child) { create(:project, name: "Child", parent: root) } - shared_let(:other_root) { create(:project, name: "Other Root") } - shared_let(:archived_root) { create(:project, name: "Archived Root", active: false) } + shared_let(:root) { create(:project, name: "Root") } + shared_let(:child) { create(:project, name: "Child", parent: root) } + shared_let(:grandchild) { create(:project, name: "Grandchild", parent: child) } + shared_let(:archived_child) { create(:project, name: "Archived child", parent: root, active: false) } + shared_let(:other_root) { create(:project, name: "Other root") } + shared_let(:archived_root) { create(:project, name: "Archived root", active: false) } - before do - [root, child, other_root, archived_root].each do |project| - create(:member, principal: current_user, project:, roles: [role]) - end + before do + [root, child, grandchild, archived_child, other_root, archived_root].each do |project| + create(:member, principal: current_user, project:, roles: [role]) end + end + context "without a boundary" do it "returns visible, active root-level projects" do expect(described_class.nearest_visible_descendants).to contain_exactly(root, other_root) end @@ -63,8 +65,14 @@ expect(described_class.nearest_visible_descendants).not_to include(archived_root) end + it "respects the limit and orders by lft" do + lft_first = [root, other_root].min_by(&:lft) + + expect(described_class.nearest_visible_descendants(limit: 1)).to contain_exactly(lft_first) + end + context "when a visible project's real parent is invisible" do - shared_let(:hidden_root) { create(:private_project, name: "Hidden Root") } + shared_let(:hidden_root) { create(:private_project, name: "Hidden root") } shared_let(:promoted) { create(:project, name: "Promoted", parent: hidden_root) } before do @@ -76,48 +84,35 @@ expect(described_class.nearest_visible_descendants).not_to include(hidden_root) end end - - it "respects the limit and orders by lft" do - lft_first = [root, other_root].min_by(&:lft) - - expect(described_class.nearest_visible_descendants(limit: 1)).to contain_exactly(lft_first) - end end context "with a boundary" do - shared_let(:parent) { create(:project, name: "Parent") } - shared_let(:child) { create(:project, name: "Child", parent:) } - shared_let(:grandchild) { create(:project, name: "Grandchild", parent: child) } - shared_let(:archived_child) { create(:project, name: "Archived Child", parent:, active: false) } - - before do - [parent, child, grandchild, archived_child].each do |project| - create(:member, principal: current_user, project:, roles: [role]) - end - end - it "returns only the boundary's direct visible, active children" do - expect(described_class.nearest_visible_descendants(parent)).to contain_exactly(child) + expect(described_class.nearest_visible_descendants(root)).to contain_exactly(child) end it "does not return grandchildren behind a visible child" do - expect(described_class.nearest_visible_descendants(parent)).not_to include(grandchild) + expect(described_class.nearest_visible_descendants(root)).not_to include(grandchild) end it "does not return an archived child" do - expect(described_class.nearest_visible_descendants(parent)).not_to include(archived_child) + expect(described_class.nearest_visible_descendants(root)).not_to include(archived_child) + end + + it "does not return a project from a different root" do + expect(described_class.nearest_visible_descendants(root)).not_to include(other_root) end context "when the direct child is invisible but the grandchild is visible" do - shared_let(:hidden_child) { create(:private_project, name: "Hidden Child", parent:) } - shared_let(:visible_grandchild) { create(:project, name: "Visible Grandchild", parent: hidden_child) } + shared_let(:hidden_child) { create(:private_project, name: "Hidden child", parent: root) } + shared_let(:visible_grandchild) { create(:project, name: "Visible grandchild", parent: hidden_child) } before do create(:member, principal: current_user, project: visible_grandchild, roles: [role]) end it "skips the invisible child and surfaces the nearest visible descendant" do - result = described_class.nearest_visible_descendants(parent) + result = described_class.nearest_visible_descendants(root) expect(result).to include(visible_grandchild) expect(result).not_to include(hidden_child) end @@ -125,7 +120,7 @@ end end - describe ".with_visible_descendants" do + describe ".having_visible_descendants" do shared_let(:parent) { create(:project, name: "Parent") } shared_let(:child) { create(:project, name: "Child", parent:) } shared_let(:leaf) { create(:project, name: "Leaf") } @@ -137,45 +132,49 @@ end it "includes a project that has a visible, active descendant" do - expect(described_class.with_visible_descendants([parent, leaf])).to contain_exactly(parent) + expect(described_class.having_visible_descendants([parent, leaf])).to contain_exactly(parent) end it "excludes a project without descendants" do - expect(described_class.with_visible_descendants([parent, leaf])).not_to include(leaf) + expect(described_class.having_visible_descendants([parent, leaf])).not_to include(leaf) + end + + it "excludes a candidate that is itself a descendant with no descendants of its own" do + expect(described_class.having_visible_descendants([parent, child, leaf])).to contain_exactly(parent) + end + + it "accepts a single project instead of an array" do + expect(described_class.having_visible_descendants(parent)).to contain_exactly(parent) + end + + it "returns none for an empty array" do + expect(described_class.having_visible_descendants([])).to be_empty end context "when the only descendant is invisible" do - shared_let(:lonely_parent) { create(:project, name: "Lonely Parent") } - shared_let(:hidden_child) { create(:private_project, name: "Hidden Child", parent: lonely_parent) } + shared_let(:lonely_parent) { create(:project, name: "Lonely parent") } + shared_let(:hidden_child) { create(:private_project, name: "Hidden child", parent: lonely_parent) } before do create(:member, principal: current_user, project: lonely_parent, roles: [role]) end it "excludes that project" do - expect(described_class.with_visible_descendants([lonely_parent])).not_to include(lonely_parent) + expect(described_class.having_visible_descendants([lonely_parent])).not_to include(lonely_parent) end end context "when the only descendant is archived" do - shared_let(:archived_parent) { create(:project, name: "Archived Parent") } - shared_let(:archived_child) { create(:project, name: "Archived Child", parent: archived_parent, active: false) } + shared_let(:parent_of_archived) { create(:project, name: "Parent of archived") } + shared_let(:archived_child) { create(:project, name: "Archived child", parent: parent_of_archived, active: false) } before do - create(:member, principal: current_user, project: archived_parent, roles: [role]) + create(:member, principal: current_user, project: parent_of_archived, roles: [role]) end it "excludes that project" do - expect(described_class.with_visible_descendants([archived_parent])).not_to include(archived_parent) + expect(described_class.having_visible_descendants([parent_of_archived])).not_to include(parent_of_archived) end end - - it "accepts a single project instead of an array" do - expect(described_class.with_visible_descendants(parent)).to contain_exactly(parent) - end - - it "returns none for an empty array" do - expect(described_class.with_visible_descendants([])).to be_empty - end end end