From 14a945326df8a533890e46606bfb61394b49e18f Mon Sep 17 00:00:00 2001 From: VISHAL DEEPAK Date: Thu, 3 Feb 2022 19:46:48 +0530 Subject: [PATCH] =?UTF-8?q?Add=20boolean=20attribute=20main=5Fimage=5Ffrom?= =?UTF-8?q?=5Ffrontmatter=20to=20indicate=20if=20cove=E2=80=A6=20(#16075)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add boolean attribute main_image_from_frontmatter to indicate if cover image was set via frontmatter * use article model spec for main_image_from_frontmatter test cases * Add data migration script and spec for main_image_from_frontmatter for articles * Update app/models/article.rb Co-authored-by: Jeremy Friesen Co-authored-by: Jeremy Friesen Co-authored-by: Michael Kohl --- app/models/article.rb | 20 +++++----- ...main_image_from_frontmatter_to_articles.rb | 5 +++ db/schema.rb | 1 + ...fill_column_main_image_from_frontmatter.rb | 11 ++++++ spec/factories/articles.rb | 1 + ...column_main_image_from_frontmatter_spec.rb | 26 +++++++++++++ spec/models/article_spec.rb | 37 +++++++++++++++++++ 7 files changed, 90 insertions(+), 11 deletions(-) create mode 100644 db/migrate/20220111140539_add_main_image_from_frontmatter_to_articles.rb create mode 100644 lib/data_update_scripts/20220120052641_backfill_column_main_image_from_frontmatter.rb create mode 100644 spec/lib/data_update_scripts/backfill_column_main_image_from_frontmatter_spec.rb diff --git a/app/models/article.rb b/app/models/article.rb index 34926ab29..973d51b59 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -632,7 +632,7 @@ class Article < ApplicationRecord set_tag_list(front_matter["tags"]) if front_matter["tags"].present? self.published = front_matter["published"] if %w[true false].include?(front_matter["published"].to_s) self.published_at = parse_date(front_matter["date"]) if published - self.main_image = determine_image(front_matter) + set_main_image(front_matter) self.canonical_url = front_matter["canonical_url"] if front_matter["canonical_url"].present? update_description = front_matter["description"].present? || front_matter["title"].present? @@ -642,16 +642,14 @@ class Article < ApplicationRecord self.collection_id = Collection.find_series(front_matter["series"], user).id if front_matter["series"].present? end - def determine_image(front_matter) - # In order to clear out the cover_image, we check for the key in the front_matter. - # If the key exists, we use the value from it (a url or `nil`). - # Otherwise, we fall back to the main_image on the article. - has_cover_image = front_matter.include?("cover_image") - - if has_cover_image && (front_matter["cover_image"].present? || main_image) - front_matter["cover_image"] - else - main_image + def set_main_image(front_matter) + # At one point, we have set the main_image based on the front matter. Forever will that now dictate the behavior. + if main_image_from_frontmatter? + self.main_image = front_matter["cover_image"] + elsif front_matter.key?("cover_image") + # They've chosen the set cover image in the front matter, so we'll proceed with that assumption. + self.main_image = front_matter["cover_image"] + self.main_image_from_frontmatter = true end end diff --git a/db/migrate/20220111140539_add_main_image_from_frontmatter_to_articles.rb b/db/migrate/20220111140539_add_main_image_from_frontmatter_to_articles.rb new file mode 100644 index 000000000..e941a67ac --- /dev/null +++ b/db/migrate/20220111140539_add_main_image_from_frontmatter_to_articles.rb @@ -0,0 +1,5 @@ +class AddMainImageFromFrontmatterToArticles < ActiveRecord::Migration[6.1] + def change + add_column :articles, :main_image_from_frontmatter, :boolean, default: false + end +end diff --git a/db/schema.rb b/db/schema.rb index bcfa1a0be..ccdc3045f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -112,6 +112,7 @@ ActiveRecord::Schema.define(version: 2022_01_26_205052) do t.datetime "last_experience_level_rating_at" t.string "main_image" t.string "main_image_background_hex_color", default: "#dddddd" + t.boolean "main_image_from_frontmatter", default: false t.integer "nth_published_by_author", default: 0 t.integer "organic_page_views_count", default: 0 t.integer "organic_page_views_past_month_count", default: 0 diff --git a/lib/data_update_scripts/20220120052641_backfill_column_main_image_from_frontmatter.rb b/lib/data_update_scripts/20220120052641_backfill_column_main_image_from_frontmatter.rb new file mode 100644 index 000000000..c91103a31 --- /dev/null +++ b/lib/data_update_scripts/20220120052641_backfill_column_main_image_from_frontmatter.rb @@ -0,0 +1,11 @@ +module DataUpdateScripts + class BackfillColumnMainImageFromFrontmatter + def run + Article.where(main_image_from_frontmatter: false).find_each do |article| + fixed_body_markdown = MarkdownProcessor::Fixer::FixAll.call(article.body_markdown || "") + parsed = FrontMatterParser::Parser.new(:md).call(fixed_body_markdown) + article.update!(main_image_from_frontmatter: true) if parsed.front_matter&.key?("cover_image") + end + end + end +end diff --git a/spec/factories/articles.rb b/spec/factories/articles.rb index 921631965..0b76e1d7f 100644 --- a/spec/factories/articles.rb +++ b/spec/factories/articles.rb @@ -34,6 +34,7 @@ FactoryBot.define do date: #{date if with_date} series: #{with_collection&.slug if with_collection} canonical_url: #{canonical_url if with_canonical_url} + #{"cover_image: #{Faker::Avatar.image}" if with_main_image && main_image_from_frontmatter} --- #{Faker::Hipster.paragraph(sentence_count: 2)} diff --git a/spec/lib/data_update_scripts/backfill_column_main_image_from_frontmatter_spec.rb b/spec/lib/data_update_scripts/backfill_column_main_image_from_frontmatter_spec.rb new file mode 100644 index 000000000..374e35412 --- /dev/null +++ b/spec/lib/data_update_scripts/backfill_column_main_image_from_frontmatter_spec.rb @@ -0,0 +1,26 @@ +require "rails_helper" +require Rails.root.join( + "lib/data_update_scripts/20220120052641_backfill_column_main_image_from_frontmatter.rb", +) + +describe DataUpdateScripts::BackfillColumnMainImageFromFrontmatter do + let(:article_without_image) { create(:article, with_main_image: false) } + let(:article_main_image) { create(:article, with_main_image: true) } + let(:article_markdown_image) do + create(:article, + body_markdown: "---\ntitle: hey hey hahuu\npublished: false\ncover_image: \n---\nYo ho ho#{rand(100)}") + end + + before do + article_without_image + article_main_image + article_markdown_image + end + + it "set main_image_from_frontmatter to true only for articles with cover_image in body_markdown" do + described_class.new.run + expect(article_without_image.reload.main_image_from_frontmatter).to be false + expect(article_main_image.reload.main_image_from_frontmatter).to be false + expect(article_markdown_image.reload.main_image_from_frontmatter).to be true + end +end diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index 8e13225e1..dd409d919 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -712,6 +712,43 @@ RSpec.describe Article, type: :model do end end + describe "#main_image_from_frontmatter" do + let(:article) { create(:article, user: user, main_image_from_frontmatter: false) } + + it "set to true if markdown has cover_image" do + article = create( + :article, + user: user, + body_markdown: "---\ntitle: hey\npublished: false\ncover_image: #{Faker::Avatar.image}\n---\nYo", + ) + expect(article.main_image_from_frontmatter).to eq(true) + end + + context "when false" do + it "does not remove main image if cover image not passed in markdown" do + expect(article.main_image).not_to be_nil + article.update! body_markdown: "---\ntitle: hey\npublished: false\n---\nYo ho ho#{rand(100)}" + expect(article.reload.main_image).not_to be_nil + end + + it "does remove main image if cover image is passed empty in markdown" do + expect(article.main_image).not_to be_nil + article.update! body_markdown: "---\ntitle: hey\npublished: false\ncover_image: \n---\nYo ho ho#{rand(100)}" + expect(article.reload.main_image).to be_nil + end + end + + context "when true" do + let(:article) { create(:article, main_image_from_frontmatter: true, user: user) } + + it "removes main image when cover_image not provided" do + expect(article.main_image).not_to be_nil + article.update! body_markdown: "---\ntitle: hey\npublished: false\n---\nYo ho ho#{rand(100)}" + expect(article.reload.main_image).to be_nil + end + end + end + describe ".active_help" do it "returns properly filtered articles under the 'help' tag" do filtered_article = create(:article, user: user, tags: "help",