diff --git a/app/controllers/stories/tagged_articles_controller.rb b/app/controllers/stories/tagged_articles_controller.rb index dff85308f..cc36ef580 100644 --- a/app/controllers/stories/tagged_articles_controller.rb +++ b/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) diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index 83c82b243..1df0d6244 100644 --- a/app/controllers/stories_controller.rb +++ b/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" diff --git a/app/models/application_record.rb b/app/models/application_record.rb index 5d0e1533c..ed05f75ab 100644 --- a/app/models/application_record.rb +++ b/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] 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) diff --git a/app/views/organizations/_sidebar.html.erb b/app/views/organizations/_sidebar.html.erb index 9f6f3458a..9bce1237a 100644 --- a/app/views/organizations/_sidebar.html.erb +++ b/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? %>

@@ -10,7 +10,7 @@

- <% @organization.users.find_each do |user| %> + <% @organization_users.find_each_respecting_scope do |user| %>
<%= user.username %> profile image @@ -42,7 +42,7 @@
<% end %> - <% if @organization.users.any? %> + <% if @organization_users.any? %>
<%= crayons_icon_tag(:post, class: "mr-3 color-base", title: t("views.organizations.side.post.icon")) %> @@ -50,7 +50,7 @@
<%= 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 %>
<% end %> diff --git a/spec/models/application_record_spec.rb b/spec/models/application_record_spec.rb index df4c5d675..6e3879a79 100644 --- a/spec/models/application_record_spec.rb +++ b/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 diff --git a/spec/requests/stories/tagged_articles_spec.rb b/spec/requests/stories/tagged_articles_spec.rb index dcaed37dd..476b55823 100644 --- a/spec/requests/stories/tagged_articles_spec.rb +++ b/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 diff --git a/spec/system/organization/user_views_an_organization_spec.rb b/spec/system/organization/user_views_an_organization_spec.rb index a3bcc9865..fa5302f06 100644 --- a/spec/system/organization/user_views_an_organization_spec.rb +++ b/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