From 502bef156eae9d651972477e35d7da4267fe979a Mon Sep 17 00:00:00 2001 From: Vaidehi Joshi Date: Tue, 10 Mar 2020 08:29:59 -0700 Subject: [PATCH] Allow an article's `main_image` to be removed (#5687) [deploy] Currently, we don't allow users to remove the `main_image` of an article once it has been set. It can be changed, but never removed. This change allows the `main_image` to be removed by checking for the `cover_image` key in the frontmatter that is sent along in the article's markdown. This means that a user should be able to remove the image associated with an article by removing the value of `cover_image:` from their frontmatter. --- app/models/article.rb | 15 +++++- spec/factories/articles.rb | 3 +- .../article_published_empty_cover_image.txt | 9 ++++ spec/models/article_spec.rb | 46 +++++++++++++++++++ spec/requests/api/v0/articles_spec.rb | 28 +++++++++++ 5 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 spec/fixtures/files/article_published_empty_cover_image.txt diff --git a/app/models/article.rb b/app/models/article.rb index ec479cd3f..8801d9d13 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -525,7 +525,7 @@ class Article < ApplicationRecord end 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 = front_matter["cover_image"] if front_matter["cover_image"].present? + self.main_image = determine_image(front_matter) self.canonical_url = front_matter["canonical_url"] if front_matter["canonical_url"].present? self.description = front_matter["description"] if front_matter["description"].present? || front_matter["title"].present? # Do this if frontmatte exists at all self.collection_id = nil if front_matter["title"].present? @@ -533,6 +533,19 @@ class Article < ApplicationRecord self.automatically_renew = front_matter["automatically_renew"] if front_matter["automatically_renew"].present? && tag_list.include?("hiring") 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 + end + end + def parse_date(date) # once published_at exist, it can not be adjusted published_at || date || Time.current diff --git a/spec/factories/articles.rb b/spec/factories/articles.rb index bc0696e08..af2040c3e 100644 --- a/spec/factories/articles.rb +++ b/spec/factories/articles.rb @@ -9,6 +9,7 @@ FactoryBot.define do tags { Faker::Hipster.words(number: 4).join(", ") } canonical_url { Faker::Internet.url } with_canonical_url { false } + with_main_image { true } with_date { false } with_tags { true } with_hr_issue { false } @@ -18,7 +19,7 @@ FactoryBot.define do end association :user, factory: :user, strategy: :create description { Faker::Hipster.paragraph(sentence_count: 1)[0..100] } - main_image { Faker::Avatar.image } + main_image { with_main_image ? Faker::Avatar.image : nil } language { "en" } experience_level_rating { rand(4..6) } body_markdown do diff --git a/spec/fixtures/files/article_published_empty_cover_image.txt b/spec/fixtures/files/article_published_empty_cover_image.txt new file mode 100644 index 000000000..362ed7e58 --- /dev/null +++ b/spec/fixtures/files/article_published_empty_cover_image.txt @@ -0,0 +1,9 @@ +--- +title: Sample Article +published: true +description: this is a sample article +tags: test +cover_image: +--- + +Suspendisse ac lobortis velit, a feugiat sapien. Aenean condimentum, nulla at fermentum sagittis, tellus nisi suscipit velit, vel sollicitudin odio ligula a odio. Integer eget efficitur massa, in sodales velit. Nunc fermentum consequat scelerisque. Morbi elementum tristique faucibus. Nulla vel lectus non justo euismod varius. Vivamus id nisl sit amet odio tincidunt fringilla. Pellentesque odio odio, vulputate in risus eu, lacinia porttitor lorem. Nunc posuere tempus est, imperdiet suscipit odio maximus id. Nam eget feugiat magna. diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index f12445f6b..1d0a49f31 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -263,6 +263,52 @@ RSpec.describe Article, type: :model do expect(test_article.body_text).to eq(sanitized_html) end end + + context "when a main_image does not already exist" do + let!(:article_without_main_image) { build(:article, with_main_image: false) } + let(:image) { Faker::Avatar.image } + + before { article_without_main_image.validate } + + it "can parse the main_image" do + expect(article_without_main_image.main_image).to eq(nil) + end + + it "can parse the main_image when added" do + article_without_main_image.main_image = image + article_without_main_image.validate + + expect(article_without_main_image.main_image).to eq(image) + end + end + + context "when a main_image exists" do + # The `with_main_image` flag is the factory default, but we're being explicit here. + let!(:article_with_main_image) { build(:article, with_main_image: true) } + let(:image) { article_with_main_image.main_image } + + before { article_with_main_image.validate } + + it "can parse the main_image" do + expect(article_with_main_image.main_image).to eq(image) + end + + it "can parse the main_image when removed" do + article_with_main_image.main_image = nil + article_with_main_image.validate + + expect(article_with_main_image.main_image).to eq(nil) + end + + it "can parse the main_image when changed" do + expect(article_with_main_image.main_image).to eq(image) + + other_image = Faker::Avatar.image + article_with_main_image.main_image = other_image + article_with_main_image.validate + expect(article_with_main_image.main_image).to eq(other_image) + end + end end describe "#class_name" do diff --git a/spec/requests/api/v0/articles_spec.rb b/spec/requests/api/v0/articles_spec.rb index 24cc161ef..b8a2a21e4 100644 --- a/spec/requests/api/v0/articles_spec.rb +++ b/spec/requests/api/v0/articles_spec.rb @@ -806,6 +806,34 @@ RSpec.describe "Api::V0::Articles", type: :request do expect(article.body_markdown).to eq(body_markdown) end + it "updates the main_image to be empty if given an empty cover_image" do + image = Faker::Avatar.image + article.update(main_image: image) + expect(article.main_image).to eq(image) + + body_markdown = file_fixture("article_published_empty_cover_image.txt").read + put_article( + title: Faker::Book.title, + body_markdown: body_markdown, + ) + expect(response).to have_http_status(:ok) + expect(article.reload.main_image).to eq(nil) + end + + it "updates the main_image to be empty if given a different cover_image" do + image = Faker::Avatar.image + article.update(main_image: image) + expect(article.main_image).to eq(image) + + body_markdown = file_fixture("article_published_cover_image.txt").read + put_article( + title: Faker::Book.title, + body_markdown: body_markdown, + ) + expect(response).to have_http_status(:ok) + expect(article.reload.main_image).to eq("https://dummyimage.com/100x100") + end + it "updates the tags" do expect do put_article(