diff --git a/app/controllers/events_controller.rb b/app/controllers/events_controller.rb index 91c6d5f5d3..8a86e4bfbc 100644 --- a/app/controllers/events_controller.rb +++ b/app/controllers/events_controller.rb @@ -299,17 +299,26 @@ def team @positions = Kaminari.paginate_array(@all_positions).page(params[:page]).per(params[:per] || (@view == "list" ? 20 : 10)) if @event.parent - ops = @event.ancestor_organizer_positions.includes(:user) - users = ops.map(&:user).uniq - - access_levels = users.filter_map do |user| - access_level = user.access_level_for(@event, ops) - next if access_level[:access_level] == :direct - - [user, access_level[:role]] - end.sort_by { |_, role| role }.to_h - - @indirect_access = access_levels + # `ancestor_organizer_positions` covers this organization as well as its + # ancestors, so both halves come from the one query. The avatars are + # preloaded because every user here is rendered as a `user_mention`. + ops = @event.ancestor_organizer_positions.includes(user: { profile_picture_attachment: :blob }) + direct_ops, ancestor_ops = ops.partition { |op| op.event_id == @event.id } + direct_roles = direct_ops.to_h { |op| [op.user_id, op.role] } + + @indirect_access = ancestor_ops.group_by(&:user).filter_map do |user, user_ops| + # Inheriting from an ancestor is all-or-nothing (see + # OrganizerPosition.role_at_least?): managers inherit their full role, + # everyone else only inherits read access. + inherited_role = user_ops.any?(&:manager?) ? "manager" : "reader" + + # Someone with their own position here is already in the team list + # below, so only mention them when what they inherit outranks it. + direct_role = direct_roles[user.id] + next if direct_role && OrganizerPosition.roles[direct_role] >= OrganizerPosition.roles[inherited_role] + + [user, inherited_role] + end.sort_by { |user, role| [-OrganizerPosition.roles[role], user.name.to_s] }.to_h end @invites = @event.organizer_position_invites.pending.includes(:sender) diff --git a/app/models/user.rb b/app/models/user.rb index b078f72b6e..f04732f4f0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -583,22 +583,6 @@ def disable_backup_codes! BackupCodeMailer.with(user_id: id).backup_codes_disabled.deliver_now end - def access_level_for(event, organizer_positions) - role = nil - access_level = nil - user_ops = organizer_positions.select { |op| op.user == self } - return nil if user_ops.empty? - - user_ops.each do |op| - if role.nil? || OrganizerPosition.roles[op.role] > OrganizerPosition.roles[role] - role = op.role - access_level = op.event == event ? :direct : :indirect - end - end - - { role:, access_level: } - end - def needs_to_enable_2fa? admin_override_pretend? && !use_two_factor_authentication end diff --git a/app/views/events/team.html.erb b/app/views/events/team.html.erb index 9f46d3fe8b..4bec5e8b8c 100644 --- a/app/views/events/team.html.erb +++ b/app/views/events/team.html.erb @@ -33,21 +33,31 @@ <% if @event.parent.present? %> - <%= render "application/callout", title: "The team behind #{@event.parent.name} also has access to #{@event.name}", type: "info", icon: "leader" do %> -

Because <%= @event.name %> is a sub-organization of <%= @event.parent.name %>, all team members of <%= @event.parent.name %> can access and view this organization.

+ <%# The icons are wrapped rather than placed directly in the because + `summary > svg` picks up padding from the dock styles. %> +
+ +
+ <%= inline_icon "leader", size: 24 %> +

The team behind <%= @event.parent.name %> also has access to <%= @event.name %>

+ <%= inline_icon "down-caret", size: 24, class: "flip-when-open" %> +
+
- <% if @indirect_access.present? %> -

These team members can access this organization:

+
+

Because <%= @event.name %> is a sub-organization of <%= @event.parent.name %>, the <%= @event.parent.name %> team has access here too.

-
- <% @indirect_access.each do |user, role| %> - - <%= user_mention(user) %><%= role == "manager" ? "can manage" : "can view" %> - - <% end %> -
- <% end %> - <% end %> + <% if @indirect_access.present? %> +
+ <% @indirect_access.each do |user, role| %> + + <%= user_mention(user) %><%= role == "manager" ? "can manage" : "can view" %> + + <% end %> +
+ <% end %> +
+
<% end %>
diff --git a/spec/controllers/events_controller_spec.rb b/spec/controllers/events_controller_spec.rb index f139d5947d..385443d6fb 100644 --- a/spec/controllers/events_controller_spec.rb +++ b/spec/controllers/events_controller_spec.rb @@ -328,6 +328,99 @@ def sign_in_organizer_of(event) end end + describe "#team" do + render_views + + let(:parent) { create(:event, name: "Parent Organization") } + let(:event) { create(:event, parent:, name: "Sub Organization") } + + before { sign_in_organizer_of(event) } + + # The callout's list, as { user's displayed name => the role it credits them with }. + def indirect_access + get(:team, params: { event_id: event.slug }) + + Nokogiri::HTML5(response.body).css("#parent_organization_access .grid > span").to_h do |row| + [row.at_css(".mention").text.squish, row.text.include?("can manage") ? "manager" : "reader"] + end + end + + it "collapses the parent organization callout by default", :aggregate_failures do + get(:team, params: { event_id: event.slug }) + + callout = Nokogiri::HTML5(response.body).at_css("details#parent_organization_access") + expect(callout.text).to include("The team behind Parent Organization also has access to Sub Organization") + expect(callout.attributes).not_to have_key("open") + end + + it "grants a reader on the parent read access here" do + reader = create(:user) + create(:organizer_position, user: reader, event: parent, role: :reader) + + expect(indirect_access).to eq({ reader.initial_name => "reader" }) + end + + # A member of the parent only inherits read access here, so their own + # member position is the higher of the two and already appears in the + # team list. + it "grants a member on the parent only read access here" do + member = create(:user) + create(:organizer_position, user: member, event: parent, role: :member) + + expect(indirect_access).to eq({ member.initial_name => "reader" }) + end + + it "grants a manager on the parent full management here" do + manager = create(:user) + create(:organizer_position, user: manager, event: parent, role: :manager) + + expect(indirect_access).to eq({ manager.initial_name => "manager" }) + end + + it "takes the highest role when the user holds positions on several ancestors" do + grandparent = create(:event) + parent.update!(parent: grandparent) + user = create(:user) + create(:organizer_position, user:, event: parent, role: :reader) + create(:organizer_position, user:, event: grandparent, role: :manager) + + expect(indirect_access).to eq({ user.initial_name => "manager" }) + end + + it "omits a user whose position here already matches what they inherit" do + user = create(:user) + create(:organizer_position, user:, event: parent, role: :reader) + create(:organizer_position, user:, event:, role: :reader) + + expect(indirect_access).to eq({}) + end + + it "omits a user whose position here outranks what they inherit" do + user = create(:user) + create(:organizer_position, user:, event: parent, role: :member) + create(:organizer_position, user:, event:, role: :member) + + expect(indirect_access).to eq({}) + end + + it "keeps a user whose inherited role outranks their position here" do + user = create(:user) + create(:organizer_position, user:, event: parent, role: :manager) + create(:organizer_position, user:, event:, role: :member) + + expect(indirect_access).to eq({ user.initial_name => "manager" }) + end + + it "lists managers before readers" do + reader = create(:user, full_name: "Aaron Reader") + manager = create(:user, full_name: "Zoe Manager") + create(:organizer_position, user: reader, event: parent, role: :reader) + create(:organizer_position, user: manager, event: parent, role: :manager) + + expect(indirect_access.keys).to eq([manager.initial_name, reader.initial_name]) + end + end + describe "#async_sub_organization_balance" do render_views