Redmine

#note-3

2. roles_with_active_members can be loaded in advance to avoid running two queries, one for any? and another when iterating over the roles.

Before adding load, I confirmed that the following two queries were executed:

Role Exists?
↳ app/views/projects/_members_box.html.erb:2
Role Load
↳ app/views/projects/_members_box.html.erb:5

After adding load, only one query is executed:

Role Load
↳ app/controllers/projects_controller.rb:178

I confirmed that the patch mentioned in #note-7 fixes the issues mentioned in #note-3.

However, I have two minor suggestions. First, I think _members_box_role.html.erb is a bit hard to read because all line breaks in the code have been removed. Second, it would be safer to use safe_join instead of join(", ").html_safe in _members_box_principals.html.erb.

Here is a proposal patch to address these issues. What do you think?

diff --git a/app/views/projects/_members_box_principals.html.erb b/app/views/projects/_members_box_principals.html.erb
index 5221251b0..165eefdab 100644
--- a/app/views/projects/_members_box_principals.html.erb
+++ b/app/views/projects/_members_box_principals.html.erb
@@ -1 +1,9 @@
-<%= principals.collect {|p| link_to_principal(p, :class => p.is_a?(Group) ? 'icon icon-group' : nil)}.join(", ").html_safe %>
\ No newline at end of file
+<%= safe_join(principals.map {|p| link_to_principal(p, :class => p.is_a?(Group) ? 'icon icon-group' : nil)}, ", ") -%>
+<% if display_more %>
+<span class="show-all-members-trigger" data-project-members-box-expand-target="trigger">
+  <%= link_to(sprite_icon('angle-right', l(:label_show_all), rtl: true),
+              show_all_members_project_path(@project, :role_id => role.id, :offset => principals.size),
+              :class => 'icon icon-angle-right',
+              :data => {:action => 'click->project-members-box-expand#showAll'}) %>
+</span>
+<% end %>
diff --git a/app/views/projects/_members_box_role.html.erb b/app/views/projects/_members_box_role.html.erb
index 4904ca884..952207375 100644
--- a/app/views/projects/_members_box_role.html.erb
+++ b/app/views/projects/_members_box_role.html.erb
@@ -3,5 +3,5 @@
 %>
 <p class="member-role" data-controller="project-members-box-expand">
   <span class="label"><%= role %>:</span>
-  <%= render :partial => 'members_box_principals', :locals => {:principals => principals} %><% if more %><span class="show-all-members-trigger" data-project-members-box-expand-target="trigger"> <%= link_to(sprite_icon('angle-right', l(:label_show_all), rtl: true), show_all_members_project_path(@project, :role_id => role.id, :offset => principals.size), :class => 'icon icon-angle-right', :data => {:action => 'click->project-members-box-expand#showAll'}) %></span><% end %>
+  <%= render :partial => 'members_box_principals', :locals => {:principals => principals, :role => role, :display_more => more} %>
 </p>
diff --git a/app/views/projects/show_all_members.html.erb b/app/views/projects/show_all_members.html.erb
index 437c38c1e..53dea58b1 100644
--- a/app/views/projects/show_all_members.html.erb
+++ b/app/views/projects/show_all_members.html.erb
@@ -1 +1,3 @@
-<% if @principals.any? %>, <%= render :partial => 'members_box_principals', :locals => {:principals => @principals} %><% end %>
+<% if @principals.any? %>
+, <%= render :partial => 'members_box_principals', :locals => {:principals => @principals, :role => @role, :display_more => false} %>
+<% end %>
diff --git a/test/functional/projects_controller_test.rb b/test/functional/projects_controller_test.rb
index 9c67a4dfd..bd2ace519 100644
--- a/test/functional/projects_controller_test.rb
+++ b/test/functional/projects_controller_test.rb
@@ -967,19 +967,6 @@ class ProjectsControllerTest < Redmine::ControllerTest
     end
   end

-  def test_show_should_keep_the_separating_space_inside_the_show_all_trigger
-    project = Project.find(1)
-    role = Role.find(1)
-    (Project::MEMBERS_BOX_PRINCIPALS_PER_ROLE + 5).times {Member.create!(:principal => User.generate!, :project => project, :role_ids => [role.id])}
-
-    get(:show, :params => {:id => 'ecookbook'})
-    assert_response :success
-    # The separator space must be inside the trigger, not a sibling text node,
-    # or clicking "Show all" leaves it stranded before the comma.
-    assert_no_match(/<\/a>\s+<span class="show-all-members-trigger"/, response.body)
-    assert_match(/<span class="show-all-members-trigger"[^>]*> <a /, response.body)
-  end
-
   def test_show_all_members_should_return_every_member_from_the_given_offset
     project = Project.find(1)
     role = Role.find(1)

I reviewed the changes in the v5 patch and confirmed that it works as expected. Looks good to me.

I noticed that the following test seems to fail on MySQL. Could you check it?

Failure:
ProjectsControllerTest#test_show_should_load_roles_with_active_members_to_avoid_a_duplicate_query [test/functional/projects_controller_test.rb:932]:
Expected: 1
  Actual: 0

bin/rails test test/functional/projects_controller_test.rb:920

Thanks for your work on this!

I confirmed that all tests, including the MySQL tests, passed with the v6 patch. LGTM.

Thank you for your work on this!

Thanks, Marius BĂLTEANU. Your proposal in #note-14 sounds like a better UX improvement than my flat list capped at N.

That said, as you mentioned, it needs work on permissions, avatar rendering, and a filter UI, so it'll take a bit more time to get right.

This performance problem itself has been reported more than once (#2198, #27902), and #2198 has been open for years.

I'd suggest keeping this issue focused on the performance fix that doesn't change the current experience much (the patch in #note-11), and treating your proposal as a separate UX-improvement issue.

A correction to the response times reported in the description and in note-2: they were measured in development mode, where the Bullet gem (enabled by default in development since #42555) adds a large overhead to this page. In production mode, with the same data (~4000 members), I get:

- unpatched: ~360 ms
- patched: ~20 ms

So the overview page is much less slow than I reported. The patch still makes it faster, but it turns out the problem is smaller than the description suggests.

Takenori TAKAKI wrote in #note-17:

Thanks, Marius BĂLTEANU. Your proposal in #note-14 sounds like a better UX improvement than my flat list capped at N.

That said, as you mentioned, it needs work on permissions, avatar rendering, and a filter UI, so it'll take a bit more time to get right.

I have added to #44581 the patch that adds a unified members page to users only with "View members" permission, please take a look.

This performance problem itself has been reported more than once (#2198, #27902), and #2198 has been open for years.

I'd suggest keeping this issue focused on the performance fix that doesn't change the current experience much (the patch in #note-11), and treating your proposal as a separate UX-improvement issue.

I agree about the performance issues, but the new members page with a short list of users displayed in the Members box is better, at least from my point of view. I also have the code to render the members with avatars, I will post it once we integrated #44581.

Thanks for opening #44581, Marius — I'll take a look.

As noted in #note-18, the performance problem reported here turned out to be largely due to Bullet, which is enabled by default in development since #42555 (we're addressing that separately in #44569). In production, with the same ~4000 members, the overview page isn't nearly as slow as originally described.

Given that, I think this issue can be closed and the discussion continued in #44581.