From c5bf4bf880ae6694b02cfa6b3f8ae0f4cab715ac Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Mon, 10 Jan 2022 15:19:04 -0500 Subject: [PATCH] Extracting duplicate logic (#16035) Prior to this commit, we had two places that need to know the nuances ofquerying for tag flares and what we should include in our queries for serialization. With this commit, we're factoring towards a common source of knowledge and providing a much needed test for the expected output of this serialization. Loosely related to #15916, #15983, #15994, and #16032. --- .../homepage/article_serializer.rb | 17 ++++++++++++++++ app/services/homepage/fetch_articles.rb | 14 +------------ app/services/search/article.rb | 16 +-------------- .../homepage/article_serializer_spec.rb | 20 +++++++++++++++++++ spec/services/homepage/fetch_articles_spec.rb | 8 ++++---- 5 files changed, 43 insertions(+), 32 deletions(-) create mode 100644 spec/serializers/homepage/article_serializer_spec.rb diff --git a/app/serializers/homepage/article_serializer.rb b/app/serializers/homepage/article_serializer.rb index 9f148be91..5c9d27777 100644 --- a/app/serializers/homepage/article_serializer.rb +++ b/app/serializers/homepage/article_serializer.rb @@ -1,5 +1,22 @@ module Homepage class ArticleSerializer < ApplicationSerializer + # @param relation [ActiveRecord::Relation
] + # + # @return [Hash] + def self.serialized_collection_from(relation:) + # Unfortunately the FlareTag class sends one SQL query per each article, + # as we want to optimize by loading them in one query, we're using a different class + tag_flares = Homepage::FetchTagFlares.call(relation) + + # including user and organization as the last step as they are not needed + # by the query that fetches tag flares, they are only needed by the serializer + relation = relation.includes(:user, :organization) + + new(relation, params: { tag_flares: tag_flares }, is_collection: true) + .serializable_hash[:data] + .pluck(:attributes) + end + attributes( :class_name, :cloudinary_video_url, diff --git a/app/services/homepage/fetch_articles.rb b/app/services/homepage/fetch_articles.rb index cd5fe7cf1..f122b3f11 100644 --- a/app/services/homepage/fetch_articles.rb +++ b/app/services/homepage/fetch_articles.rb @@ -31,19 +31,7 @@ module Homepage per_page: per_page, ) - # Unfortunately the FlareTag class sends one SQL query per each article, - # as we want to optimize by loading them in one query, we're using a different class - tag_flares = Homepage::FetchTagFlares.call(articles) - - # including user and organization as the last step as they are not needed - # by the query that fetches tag flares, they are only needed by the serializer - # NOTE: wish there was a way to specify which columns to fetch for `User` and `Organization`... - articles = articles.includes(:user, :organization) - - Homepage::ArticleSerializer - .new(articles, params: { tag_flares: tag_flares }, is_collection: true) - .serializable_hash[:data] - .pluck(:attributes) + Homepage::ArticleSerializer.serialized_collection_from(relation: articles) end end end diff --git a/app/services/search/article.rb b/app/services/search/article.rb index 25e13a7ef..c32a6bbbb 100644 --- a/app/services/search/article.rb +++ b/app/services/search/article.rb @@ -16,13 +16,7 @@ module Search relation = sort(relation, term, sort_by, sort_direction) - tag_flares = Homepage::FetchTagFlares.call(relation) - - # including user and organization as the last step as they are not needed - # by the query that fetches tag flares, they are only needed by the serializer - relation = relation.includes(:user, :organization) - - serialize(relation, tag_flares) + Homepage::ArticleSerializer.serialized_collection_from(relation: relation) end def self.sort(relation, term, sort_by, sort_direction) @@ -34,13 +28,5 @@ module Search relation.reorder(DEFAULT_SORT_BY) end private_class_method :sort - - def self.serialize(articles, tag_flares) - Homepage::ArticleSerializer - .new(articles, params: { tag_flares: tag_flares }, is_collection: true) - .serializable_hash[:data] - .pluck(:attributes) - end - private_class_method :serialize end end diff --git a/spec/serializers/homepage/article_serializer_spec.rb b/spec/serializers/homepage/article_serializer_spec.rb new file mode 100644 index 000000000..e2100e933 --- /dev/null +++ b/spec/serializers/homepage/article_serializer_spec.rb @@ -0,0 +1,20 @@ +require "rails_helper" + +RSpec.describe Homepage::ArticleSerializer, type: :serializer do + describe "#serialized_collection_from" do + let(:user) { create(:user, name: "\"Rowdy\" Roddy Piper \\:/") } + let(:organization) { create(:organization) } + let(:tag) { create(:tag, name: "ama", bg_color_hex: "#f3f3f3", text_color_hex: "#cccccc") } + let(:article) { create(:article, user: user, organization: organization, tags: tag.name) } + + before do + article + stub_const("FlareTag::FLARE_TAG_IDS_HASH", { "ama" => tag.id }) + end + + it "is parseable as JSON (once converted to_json)" do + response = described_class.serialized_collection_from(relation: Article.all) + expect(JSON.parse(response.to_json)[0].dig("user", "name")).to eq(user.name) + end + end +end diff --git a/spec/services/homepage/fetch_articles_spec.rb b/spec/services/homepage/fetch_articles_spec.rb index acaf83fd5..aadff3e68 100644 --- a/spec/services/homepage/fetch_articles_spec.rb +++ b/spec/services/homepage/fetch_articles_spec.rb @@ -4,10 +4,10 @@ RSpec.describe Homepage::FetchArticles, type: :service do describe ".call" do # rubocop:disable RSpec/ExampleLength it "returns results in the correct format", :aggregate_failures do - article = create( - :article, video_thumbnail_url: "https://example.com", tags: Constants::Tags::FLARE_TAG_NAMES.first - ) - create(:comment, commentable: article) + tag = create(:tag, name: "ama", bg_color_hex: "#f3f3f3", text_color_hex: "#cccccc") + article = create(:article, video_thumbnail_url: "https://example.com", tags: tag.name) + + stub_const("FlareTag::FLARE_TAG_IDS_HASH", { "ama" => tag.id }) result = described_class.call.first