From 1b0cb3b54d1a1b08922527aefc8135d56d3a1a8d Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Wed, 17 Jan 2024 04:18:13 -0500 Subject: [PATCH] Announcement reactions query spec improvement and refactor (#28768) --- app/models/announcement.rb | 41 +++++++++++++++++++++++--------- spec/models/announcement_spec.rb | 23 ++++++++++++------ 2 files changed, 46 insertions(+), 18 deletions(-) diff --git a/app/models/announcement.rb b/app/models/announcement.rb index 86f7037a5..e63057002 100644 --- a/app/models/announcement.rb +++ b/app/models/announcement.rb @@ -75,22 +75,41 @@ class Announcement < ApplicationRecord end def reactions(account = nil) - records = begin - scope = announcement_reactions.group(:announcement_id, :name, :custom_emoji_id).order(Arel.sql('MIN(created_at) ASC')) - - if account.nil? - scope.select('name, custom_emoji_id, count(*) as count, false as me') - else - scope.select("name, custom_emoji_id, count(*) as count, exists(select 1 from announcement_reactions r where r.account_id = #{account.id} and r.announcement_id = announcement_reactions.announcement_id and r.name = announcement_reactions.name) as me") + grouped_ordered_announcement_reactions.select( + [:name, :custom_emoji_id, 'COUNT(*) as count'].tap do |values| + values << value_for_reaction_me_column(account) end - end.to_a - - ActiveRecord::Associations::Preloader.new(records: records, associations: :custom_emoji).call - records + ).to_a.tap do |records| + ActiveRecord::Associations::Preloader.new(records: records, associations: :custom_emoji).call + end end private + def grouped_ordered_announcement_reactions + announcement_reactions + .group(:announcement_id, :name, :custom_emoji_id) + .order( + Arel.sql('MIN(created_at)').asc + ) + end + + def value_for_reaction_me_column(account) + if account.nil? + 'FALSE AS me' + else + <<~SQL.squish + EXISTS( + SELECT 1 + FROM announcement_reactions inner_reactions + WHERE inner_reactions.account_id = #{account.id} + AND inner_reactions.announcement_id = announcement_reactions.announcement_id + AND inner_reactions.name = announcement_reactions.name + ) AS me + SQL + end + end + def set_published return unless scheduled_at.blank? || scheduled_at.past? diff --git a/spec/models/announcement_spec.rb b/spec/models/announcement_spec.rb index 612ba8bb0..1e7283ca7 100644 --- a/spec/models/announcement_spec.rb +++ b/spec/models/announcement_spec.rb @@ -115,26 +115,35 @@ describe Announcement do describe '#reactions' do context 'with announcement_reactions present' do + let(:account_reaction_emoji) { Fabricate :custom_emoji } + let(:other_reaction_emoji) { Fabricate :custom_emoji } let!(:account) { Fabricate(:account) } let!(:announcement) { Fabricate(:announcement) } - let!(:announcement_reaction) { Fabricate(:announcement_reaction, announcement: announcement, created_at: 10.days.ago) } - let!(:announcement_reaction_account) { Fabricate(:announcement_reaction, announcement: announcement, created_at: 5.days.ago, account: account) } before do - Fabricate(:announcement_reaction) + Fabricate(:announcement_reaction, announcement: announcement, created_at: 10.days.ago, name: other_reaction_emoji.shortcode) + Fabricate(:announcement_reaction, announcement: announcement, created_at: 5.days.ago, account: account, name: account_reaction_emoji.shortcode) + Fabricate(:announcement_reaction) # For some other announcement end it 'returns the announcement reactions for the announcement' do results = announcement.reactions - expect(results.first.name).to eq(announcement_reaction.name) - expect(results.last.name).to eq(announcement_reaction_account.name) + expect(results).to have_attributes( + size: eq(2), + first: have_attributes(name: other_reaction_emoji.shortcode, me: false), + last: have_attributes(name: account_reaction_emoji.shortcode, me: false) + ) end - it 'returns the announcement reactions for the announcement limited to account' do + it 'returns the announcement reactions for the announcement with `me` set correctly' do results = announcement.reactions(account) - expect(results.first.name).to eq(announcement_reaction.name) + expect(results).to have_attributes( + size: eq(2), + first: have_attributes(name: other_reaction_emoji.shortcode, me: false), + last: have_attributes(name: account_reaction_emoji.shortcode, me: true) + ) end end end