diff --git a/app/components/groups/row_component.rb b/app/components/groups/row_component.rb index 51d7bebcc93..eea139a63ff 100644 --- a/app/components/groups/row_component.rb +++ b/app/components/groups/row_component.rb @@ -31,7 +31,14 @@ module Groups class RowComponent < OpPrimer::BorderBoxRowComponent def name - render(Primer::Beta::Link.new(href: edit_group_path(model), font_weight: :bold)) { model.name } + depth = model.hierarchy_depth || 0 + link = render(Primer::Beta::Link.new(href: edit_group_path(model), font_weight: :bold)) { model.name } + + if depth > 0 + tag.span(style: "margin-left: #{depth * 20}px") { link } + else + link + end end def user_count diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 8ba62ac7f71..676366dcb8d 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -40,7 +40,7 @@ class GroupsController < ApplicationController edit_membership add_users] def index - @groups = Group.order(Arel.sql("lastname ASC")) + @groups = Group.in_tree_order end def show diff --git a/app/forms/groups/form.rb b/app/forms/groups/form.rb index ae1b2a672b1..b18a50f36b3 100644 --- a/app/forms/groups/form.rb +++ b/app/forms/groups/form.rb @@ -44,10 +44,13 @@ module Groups name: :parent_id, label: Group.human_attribute_name(:parent), include_blank: I18n.t(:label_no_parent_group), + caption: I18n.t(:label_parent_group_caption), input_width: :medium ) do |list| - Group.where.not(id: model.self_and_descendants.select(:id)).each do |group| - list.option(label: group.name, value: group.id, selected: model.parent_id == group.id) + excluded_ids = model.self_and_descendants.pluck(:id).to_set + Group.in_tree_order.reject { |g| excluded_ids.include?(g.id) }.each do |group| + prefix = "\u00A0\u00A0" * (group.hierarchy_depth || 0) + list.option(label: "#{prefix}#{group.name}", value: group.id, selected: model.parent_id == group.id) end end diff --git a/app/models/group.rb b/app/models/group.rb index 89c36de8cb3..07ac814278b 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -32,6 +32,8 @@ class Group < Principal include ::Scopes::Scoped include Groups::Hierarchy + attr_accessor :hierarchy_depth + has_principal_details do belongs_to :parent, class_name: "Group", optional: true diff --git a/app/models/groups/hierarchy.rb b/app/models/groups/hierarchy.rb index 2de7aaca63b..17dc857efa6 100644 --- a/app/models/groups/hierarchy.rb +++ b/app/models/groups/hierarchy.rb @@ -69,6 +69,25 @@ module Groups::Hierarchy parent_id.nil? end + class_methods do + # Returns all groups in depth-first tree order, alphabetical within each level. + # Each group has its `hierarchy_depth` set to its nesting level (0 for roots). + def in_tree_order + all_groups = with_detail.order(Arel.sql("lastname ASC")).to_a + children_by_parent = all_groups.group_by(&:parent_id) + walk_tree(children_by_parent, nil, 0) + end + + private + + def walk_tree(children_by_parent, parent_id, depth) + (children_by_parent[parent_id] || []).flat_map do |group| + group.hierarchy_depth = depth + [group, *walk_tree(children_by_parent, group.id, depth + 1)] + end + end + end + private def descendant_ids diff --git a/config/locales/en.yml b/config/locales/en.yml index f229efdf0e1..ef755505ede 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3874,6 +3874,9 @@ en: label_overall_activity: "Overall activity" label_overview: "Overview" label_page_title: "Page title" + label_parent_group_caption: > + Setting a parent group will make this group a subgroup of the selected parent group. + This will also inherit all memberships, including permissions of the parent group. label_part_of: "part of" label_password_lost: "Forgot your password?" label_password_rule_lowercase: "Lowercase" diff --git a/spec/components/groups/table_component_spec.rb b/spec/components/groups/table_component_spec.rb index 834186bde3f..5376e197e70 100644 --- a/spec/components/groups/table_component_spec.rb +++ b/spec/components/groups/table_component_spec.rb @@ -61,4 +61,24 @@ RSpec.describe Groups::TableComponent, type: :component do it_behaves_like "rendering Border Box Grid headings" it_behaves_like "rendering Border Box Grid rows", row_count: 2, col_count: 3 end + + context "with nested groups" do + let(:parent_group) do + create(:group, lastname: "Parent").tap { |g| g.hierarchy_depth = 0 } + end + let(:child_group) do + create(:group, lastname: "Child", parent: parent_group).tap { |g| g.hierarchy_depth = 1 } + end + let(:groups) { [parent_group, child_group] } + + it "renders child groups with indentation and arrow icon" do + expect(rendered_component).to have_css("span[style='margin-left: 20px']") + expect(rendered_component).to have_css("span[style='margin-left: 20px'] a", text: "Child") + end + + it "renders parent groups without indentation" do + expect(rendered_component).to have_link("Parent") + expect(rendered_component).to have_no_css("span[style='margin-left: 0px']") + end + end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 7b9e6c1dc13..5991ba21846 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -78,7 +78,7 @@ RSpec.describe Group do context "if it does not exist" do it "does not create a group user" do count = group.group_users.count - gu = group.group_users.create user_id: User.maximum(:id).to_i + 1 + gu = group.group_users.create! user_id: User.maximum(:id).to_i + 1 expect(gu).not_to be_valid expect(group.group_users.count).to eq count @@ -87,7 +87,7 @@ RSpec.describe Group do it "updates the timestamp" do updated_at = group.updated_at - group.group_users.create(user:) + group.group_users.create!(user:) expect(updated_at < group.reload.updated_at) .to be_truthy @@ -96,7 +96,7 @@ RSpec.describe Group do context "when removing a user" do it "updates the timestamp" do - group.group_users.create(user:) + group.group_users.create!(user:) updated_at = group.reload.updated_at group.group_users.destroy_all @@ -149,12 +149,12 @@ RSpec.describe Group do before do # Add user1 to group1 and group2 - group1.group_users.create(user: user1) - group2.group_users.create(user: user1) + group1.group_users.create!(user: user1) + group2.group_users.create!(user: user1) # Add user2 to group2 and group3 - group2.group_users.create(user: user2) - group3.group_users.create(user: user2) + group2.group_users.create!(user: user2) + group3.group_users.create!(user: user2) end it "returns groups that contain the given user" do @@ -185,11 +185,11 @@ RSpec.describe Group do before do # target_user is in both groups - shared_group.group_users.create(user: target_user) - other_group.group_users.create(user: target_user) + shared_group.group_users.create!(user: target_user) + other_group.group_users.create!(user: target_user) # viewer is only in shared_group - shared_group.group_users.create(user: viewer) + shared_group.group_users.create!(user: viewer) end it "returns only the groups the viewer can see from the user's groups" do @@ -283,6 +283,48 @@ RSpec.describe Group do end end + describe ".in_tree_order" do + it "returns groups in depth-first order, alphabetical within each level" do + result = described_class.in_tree_order + + grandparent_idx = result.index(grandparent) + parent_idx = result.index(parent_group) + child_idx = result.index(child) + grandchild_idx = result.index(grandchild) + + expect(grandparent_idx).to be < parent_idx + expect(parent_idx).to be < child_idx + expect(child_idx).to be < grandchild_idx + end + + it "sets hierarchy_depth on each group" do + result = described_class.in_tree_order + depths = result.to_h { |g| [g.id, g.hierarchy_depth] } + + expect(depths[grandparent.id]).to eq(0) + expect(depths[parent_group.id]).to eq(1) + expect(depths[child.id]).to eq(2) + expect(depths[grandchild.id]).to eq(3) + expect(depths[unrelated.id]).to eq(0) + end + + it "includes all groups" do + result = described_class.in_tree_order + expect(result).to contain_exactly(grandparent, parent_group, child, grandchild, unrelated) + end + + it "sorts siblings alphabetically" do + sibling_a = create(:group, lastname: "AAA Sibling", parent_id: grandparent.id) + sibling_z = create(:group, lastname: "ZZZ Sibling", parent_id: grandparent.id) + + result = described_class.in_tree_order + sibling_a_idx = result.index(sibling_a) + sibling_z_idx = result.index(sibling_z) + + expect(sibling_a_idx).to be < sibling_z_idx + end + end + describe "circular dependency prevention" do it "is invalid when assigning self as parent" do grandparent.parent_id = grandparent.id