Sort organization members and tag moderators by badge count (#19582)

* show tag moderators in descending badge count order

* show organization users in descending badge count order

* specs & docs for find_each_respecting_scope

app/controllers/stories/tagged_articles_controller.rb

@@ -16,7 +16,9 @@ module Stories
 
       @page = (params[:page] || 1).to_i
 
-      @moderators = User.with_role(:tag_moderator, @tag).select(:username, :profile_image, :id)
+      @moderators = User.with_role(:tag_moderator, @tag)
+        .order(badge_achievements_count: :desc)
+        .select(:username, :profile_image, :id)
 
       set_number_of_articles(tag: @tag)
 

app/controllers/stories_controller.rb

@@ -165,6 +165,7 @@ class StoriesController < ApplicationController
       .limited_column_select
       .order(published_at: :desc).page(@page).per(8))
     @organization_article_index = true
+    @organization_users = @organization.users.order(badge_achievements_count: :desc)
     set_organization_json_ld
     set_surrogate_key_header "articles-org-#{@organization.id}"
     render template: "organizations/show"

app/models/application_record.rb

@@ -62,6 +62,48 @@ class ApplicationRecord < ActiveRecord::Base
     connection.execute "SET statement_timeout = #{milliseconds}"
   end
 
+  # ActiveRecord's `find_each` method allows you to work with a large collection of records
+  # in batches, but strictly only orders those batches by IDs in ascending order.
+  # Any other specified order is either ignored or raises an error (depending on configuration).
+  # This method allows performing batch queries of arbitrary order.
+  #
+  # @param batch_size [Integer] Batch size limit
+  # @yieldparam [self]
+  # @return [Enumerator<self>] if no block is given
+  #
+  # @see https://api.rubyonrails.org/v7.0.4.2/classes/ActiveRecord/Batches.html#method-i-find_each
+  def self.find_each_respecting_scope(batch_size: 1000, &block)
+    load_in_batches = Enumerator.new do |e|
+      in_batches_respecting_scope(batch_size: batch_size) do |batch|
+        batch.each { |record| e.yield record }
+      end
+    end
+
+    return load_in_batches unless block
+
+    load_in_batches.each(&block)
+  end
+
+  def self.in_batches_respecting_scope(batch_size: 1000)
+    relation = self
+
+    # Without a specified order, the sorting of PostgreSQL's query results is undefined behaviour
+    relation = relation.order(id: :asc) if all.arel.orders.blank?
+    all_ids = relation.ids.to_a
+
+    # `where` and `order` are unnecessary as we already know the exact records we need (and in what order)
+    # `limit` and `offset` would conflict with the manual batching
+    batch_relation = relation.unscope(:where, :order, :limit, :offset)
+    # We're loading in batches to reduce memory usage; if the results get cached anyway, that defeats the purpose
+    batch_relation.skip_query_cache!
+
+    all_ids.in_groups_of(batch_size, false) do |ids|
+      records = batch_relation.where(id: ids).index_by(&:id)
+      # Avoid yielding nil if e.g. record has been deleted since loading IDs
+      yield ids.filter_map { |id| records[id] }
+    end
+  end
+
   # Decorate object with appropriate decorator
   def decorate
     self.class.decorator_class.new(self)

app/views/organizations/_sidebar.html.erb

@@ -2,7 +2,7 @@
   <%# given the probability that organizations can have lots of users,
       here we're using the any?/find_each pattern to avoid loading possibly
       too many objects in memory %>
-  <% if @organization.users.any? %>
+  <% if @organization_users.any? %>
     <div class="crayons-card crayons-card--secondary org-sidebar-widget">
       <header class="crayons-card__header">
         <h3 class="crayons-subtitle-3">
@@ -10,7 +10,7 @@
         </h3>
       </header>
       <div class="org-sidebar-widget-body">
-        <% @organization.users.find_each do |user| %>
+        <% @organization_users.find_each_respecting_scope do |user| %>
           <div class="org-sidebar-widget-user-pic">
             <a href="/<%= user.username %>">
               <img src="<%= user.profile_image_url_for(length: 90) %>" alt="<%= user.username %> profile image" loading="lazy" />
@@ -42,7 +42,7 @@
       </div>
     </div>
   <% end %>
-  <% if @organization.users.any? %>
+  <% if @organization_users.any? %>
     <div class="crayons-card crayons-card--secondary p-4">
       <div class="flex items-center mb-4">
         <%= crayons_icon_tag(:post, class: "mr-3 color-base", title: t("views.organizations.side.post.icon")) %>
@@ -50,7 +50,7 @@
       </div>
       <div class="flex items-center">
         <%= crayons_icon_tag(:team, class: "mr-3 color-base", title: t("views.organizations.side.member.icon")) %>
-        <%= t "views.organizations.side.member.text", count: @organization.users.size %>
+        <%= t "views.organizations.side.member.text", count: @organization_users.size %>
       </div>
     </div>
   <% end %>

spec/models/application_record_spec.rb

@@ -63,4 +63,66 @@ RSpec.describe ApplicationRecord do
       expect(described_class.statement_timeout).to eq original_timeout
     end
   end
+
+  describe "batch querying respecting scope" do
+    let!(:alice) { create(:user, username: "alice", confirmed_at: 12.hours.ago) }
+    let!(:bob) { create(:user, username: "bob", confirmed_at: 3.days.ago) }
+    let!(:charles) { create(:user, username: "charles", confirmed_at: 3.hours.ago) }
+    let!(:doreen) { create(:user, username: "doreen", confirmed_at: 1.day.ago) }
+    let!(:esther) { create(:user, username: "esther", confirmed_at: 2.weeks.ago) }
+
+    describe ".in_batches_respecting_scope" do
+      it "fetches records in batches of specified size" do
+        expect { |block| User.in_batches_respecting_scope(batch_size: 2, &block) }
+          .to yield_successive_args(
+            [alice, bob],
+            [charles, doreen],
+            [esther],
+          )
+      end
+
+      it "respects scopes such as WHERE and ORDER" do
+        scope = User.where("confirmed_at > ?", 2.days.ago).order(username: :desc)
+        expect { |block| scope.in_batches_respecting_scope(batch_size: 2, &block) }
+          .to yield_successive_args(
+            [doreen, charles],
+            [alice],
+          )
+      end
+
+      it "handles limit and offset appropriately regardless of batch size" do
+        scope = User.order(username: :desc).limit(2).offset(2)
+        expect { |block| scope.in_batches_respecting_scope(batch_size: 10, &block) }
+          .to yield_successive_args([charles, bob])
+      end
+    end
+
+    describe ".find_each_respecting_scope" do
+      it "fetches records in batches" do
+        allow(User).to receive(:in_batches_respecting_scope).with(batch_size: 2).and_call_original
+
+        expect { |block| User.find_each_respecting_scope(batch_size: 2, &block) }
+          .to yield_successive_args(alice, bob, charles, doreen, esther)
+
+        expect(User).to have_received(:in_batches_respecting_scope).once
+      end
+
+      it "respects scopes such as WHERE and ORDER" do
+        scope = User.where("confirmed_at < ?", 18.hours.ago).order(confirmed_at: :asc)
+        expect { |block| scope.find_each_respecting_scope(batch_size: 2, &block) }
+          .to yield_successive_args(esther, bob, doreen)
+      end
+
+      it "returns an enumerator and defers batch querying if called without a block" do
+        allow(User).to receive(:in_batches_respecting_scope).and_call_original
+
+        query = User.order(username: :desc).find_each_respecting_scope
+        expect(query).to be_an(Enumerator)
+        expect(User).not_to have_received(:in_batches_respecting_scope)
+
+        expect(query.map(&:username)).to contain_exactly("esther", "doreen", "charles", "bob", "alice")
+        expect(User).to have_received(:in_batches_respecting_scope)
+      end
+    end
+  end
 end

spec/requests/stories/tagged_articles_spec.rb

@@ -127,6 +127,36 @@ RSpec.describe "Stories::TaggedArticlesIndex" do
           )
         end
 
+        context "when the tag has moderators" do
+          let(:six_badge_mod) { create(:user, badge_achievements_count: 6) }
+          let(:three_badge_mod) { create(:user, badge_achievements_count: 3) }
+          let(:ten_badge_mod) { create(:user, badge_achievements_count: 10) }
+          let(:two_badge_mod) { create(:user, badge_achievements_count: 2) }
+          let(:eight_badge_mod) { create(:user, badge_achievements_count: 8) }
+          let(:mods) { [six_badge_mod, three_badge_mod, ten_badge_mod, two_badge_mod, eight_badge_mod] }
+
+          before do
+            mods.each { |mod| mod.add_role(:tag_moderator, tag) }
+          end
+
+          def nth_avatar(user_position)
+            ".widget-user-pic:nth-child(#{user_position})"
+          end
+
+          it "shows them in the sidebar in descending order of badge achievement count" do
+            get "/t/#{tag.name}"
+
+            page = Capybara.string(response.body)
+            sidebar = page.find("#sidebar-wrapper-left aside.side-bar")
+
+            expect(sidebar.find(nth_avatar(1))).to have_link(nil, href: ten_badge_mod.path)
+            expect(sidebar.find(nth_avatar(2))).to have_link(nil, href: eight_badge_mod.path)
+            expect(sidebar.find(nth_avatar(3))).to have_link(nil, href: six_badge_mod.path)
+            expect(sidebar.find(nth_avatar(4))).to have_link(nil, href: three_badge_mod.path)
+            expect(sidebar.find(nth_avatar(5))).to have_link(nil, href: two_badge_mod.path)
+          end
+        end
+
         context "with user signed in" do
           before do
             sign_in user

spec/system/organization/user_views_an_organization_spec.rb

@@ -73,4 +73,33 @@ RSpec.describe "Organization index" do
       end
     end
   end
+
+  context "when there are multiple members in the organization" do
+    let(:many_members_org) { create(:organization) }
+
+    let(:some_badges_member) { create(:user, badge_achievements_count: 15) }
+    let(:many_badges_member) { create(:user, badge_achievements_count: 50) }
+    let(:no_badges_member) { create(:user, badge_achievements_count: 0) }
+    let(:few_badges_member) { create(:user, badge_achievements_count: 5) }
+    let(:org_members) { [some_badges_member, many_badges_member, no_badges_member, few_badges_member] }
+
+    before do
+      org_members.each { |user| create(:organization_membership, user: user, organization: many_members_org) }
+      visit "/#{many_members_org.slug}"
+    end
+
+    def nth_avatar(user_position)
+      ".org-sidebar-widget-user-pic:nth-child(#{user_position})"
+    end
+
+    it "shows the sidebar with users listed in descending badge count order" do
+      within("#sidebar-left") do
+        expect(page).to have_content("Meet the team")
+        expect(page.find(nth_avatar(1))).to have_link(nil, href: many_badges_member.path)
+        expect(page.find(nth_avatar(2))).to have_link(nil, href: some_badges_member.path)
+        expect(page.find(nth_avatar(3))).to have_link(nil, href: few_badges_member.path)
+        expect(page.find(nth_avatar(4))).to have_link(nil, href: no_badges_member.path)
+      end
+    end
+  end
 end