From 1b839d9328cebb4f82e6e57e55de3befcb4c8366 Mon Sep 17 00:00:00 2001 From: Mac Siri Date: Fri, 5 Jun 2020 08:46:24 -0400 Subject: [PATCH] [deploy] Streamline Tag parsing for both editors (#8175) --- app/models/article.rb | 28 ++++++++++++------ app/models/listing.rb | 4 ++- app/models/tag.rb | 4 +++ app/services/rss_reader/assembler.rb | 4 ++- config/initializers/acts_as_taggable_on.rb | 1 + spec/factories/articles.rb | 2 +- spec/liquid_tags/link_tag_spec.rb | 6 ++-- spec/models/article_spec.rb | 10 +++++++ spec/models/tag_adjustment_spec.rb | 3 +- spec/models/tag_spec.rb | 29 +++++++++++++++++++ .../requests/articles/articles_create_spec.rb | 8 +++++ spec/system/user_uses_the_editor_spec.rb | 1 + 12 files changed, 83 insertions(+), 17 deletions(-) create mode 100644 config/initializers/acts_as_taggable_on.rb diff --git a/app/models/article.rb b/app/models/article.rb index f5fbf5fd1..6ee53c5ae 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -383,12 +383,24 @@ class Article < ApplicationRecord parsed_markdown = MarkdownParser.new(parsed.content) self.reading_time = parsed_markdown.calculate_reading_time self.processed_html = parsed_markdown.finalize - evaluate_front_matter(parsed.front_matter) + + if parsed.front_matter.any? + evaluate_front_matter(parsed.front_matter) + elsif tag_list.any? + set_tag_list(tag_list) + end + self.description = processed_description if description.blank? rescue StandardError => e errors[:base] << ErrorMessageCleaner.new(e.message).clean end + def set_tag_list(tags) + self.tag_list = [] # overwrite any existing tag with those from the front matter + tag_list.add(tags, parse: true) + self.tag_list = tag_list.map { |tag| Tag.find_preferred_alias_for(tag) } + end + def update_main_image_background_hex return if main_image.blank? || main_image_background_hex_color != "#dddddd" @@ -446,12 +458,7 @@ class Article < ApplicationRecord def evaluate_front_matter(front_matter) self.title = front_matter["title"] if front_matter["title"].present? - if front_matter["tags"].present? - self.tag_list = [] # overwrite any existing tag with those from the front matter - tag_list.add(front_matter["tags"], parser: ActsAsTaggableOn::TagParser) - remove_tag_adjustments_from_tag_list - add_tag_adjustments_to_tag_list - end + 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) @@ -497,12 +504,15 @@ class Article < ApplicationRecord def remove_tag_adjustments_from_tag_list tags_to_remove = TagAdjustment.where(article_id: id, adjustment_type: "removal", status: "committed").pluck(:tag_name) - tag_list.remove(tags_to_remove, parser: ActsAsTaggableOn::TagParser) if tags_to_remove.present? + tag_list.remove(tags_to_remove, parse: true) if tags_to_remove.present? end def add_tag_adjustments_to_tag_list tags_to_add = TagAdjustment.where(article_id: id, adjustment_type: "addition", status: "committed").pluck(:tag_name) - tag_list.add(tags_to_add, parser: ActsAsTaggableOn::TagParser) if tags_to_add.present? + return if tags_to_add.blank? + + tag_list.add(tags_to_add, parse: true) + self.tag_list = tag_list.map { |tag| Tag.find_preferred_alias_for(tag) } end def validate_video diff --git a/app/models/listing.rb b/app/models/listing.rb index 6a160ef9e..078ab4ae7 100644 --- a/app/models/listing.rb +++ b/app/models/listing.rb @@ -73,7 +73,9 @@ class Listing < ApplicationRecord end def modify_inputs - ActsAsTaggableOn.default_parser = ActsAsTaggableOn::TagParser + temp_tags = tag_list + self.tag_list = [] # overwrite any existing tag with those from the front matter + tag_list.add(temp_tags, parser: ActsAsTaggableOn::TagParser) self.body_markdown = body_markdown.to_s.gsub(/\r\n/, "\n") end diff --git a/app/models/tag.rb b/app/models/tag.rb index 3980a37ad..fbcdbb2f9 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -76,6 +76,10 @@ class Tag < ActsAsTaggableOn::Tag tag.alias_for.presence || tag.name end + def self.find_preferred_alias_for(word) + find_by(name: word.downcase)&.alias_for.presence || word.downcase + end + def validate_name errors.add(:name, "is too long (maximum is 30 characters)") if name.length > 30 errors.add(:name, "contains non-alphanumeric characters") unless name.match?(/\A[[:alnum:]]+\z/) diff --git a/app/services/rss_reader/assembler.rb b/app/services/rss_reader/assembler.rb index e7385904f..59d82711a 100644 --- a/app/services/rss_reader/assembler.rb +++ b/app/services/rss_reader/assembler.rb @@ -32,7 +32,9 @@ class RssReader private def get_tags - @categories.first(4).map { |tag| tag.lstrip[0..19] }.join(",") + @categories.first(4).map do |tag| + tag.delete(" ").gsub(/[^[:alnum:]]/i, "")[0..19] + end.join(",") end def assemble_body_markdown diff --git a/config/initializers/acts_as_taggable_on.rb b/config/initializers/acts_as_taggable_on.rb new file mode 100644 index 000000000..08d8aa67f --- /dev/null +++ b/config/initializers/acts_as_taggable_on.rb @@ -0,0 +1 @@ +ActsAsTaggableOn.force_lowercase = true diff --git a/spec/factories/articles.rb b/spec/factories/articles.rb index 696fc5c86..5371bc354 100644 --- a/spec/factories/articles.rb +++ b/spec/factories/articles.rb @@ -6,7 +6,7 @@ FactoryBot.define do title { Faker::Food.dish } published { true } date { "01/01/2015" } - tags { Faker::Hipster.words(number: 4).join(", ") } + tags { "javascript, html, css" } canonical_url { Faker::Internet.url } with_canonical_url { false } with_main_image { true } diff --git a/spec/liquid_tags/link_tag_spec.rb b/spec/liquid_tags/link_tag_spec.rb index 164b43513..4dc3bff43 100644 --- a/spec/liquid_tags/link_tag_spec.rb +++ b/spec/liquid_tags/link_tag_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" RSpec.describe LinkTag, type: :liquid_tag do let(:user) { create(:user, username: "username45", name: "Chase Danger", profile_image: nil) } let(:article) do - create(:article, user_id: user.id, title: "test this please", tags: "tag1 tag2 tag3") + create(:article, user_id: user.id, title: "test this please", tags: "html, rss, css") end let(:org) { build_stubbed(:organization) } let(:org_user) do @@ -12,7 +12,7 @@ RSpec.describe LinkTag, type: :liquid_tag do user end let(:org_article) do - create(:article, user_id: org_user.id, title: "test this please", tags: "tag1 tag2 tag3", + create(:article, user_id: org_user.id, title: "test this please", tags: "html, ruby, js", organization_id: org.id) end let(:escaped_article) do @@ -30,7 +30,7 @@ RSpec.describe LinkTag, type: :liquid_tag do end def correct_link_html(article) - tags = article.tag_list.map { |t| "##{t}" }.reverse.join + tags = article.tag_list.map { |t| "##{t}" }.join("\n" + "\s" * 8) <<~HTML
diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index 73e3aaf74..a467d695b 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -242,6 +242,16 @@ RSpec.describe Article, type: :model do expect(build(:article, tags: tags).valid?).to be(false) end + it "rejects tag with non-alphanumerics" do + expect { build(:article, tags: "c++").validate! }.to raise_error(ActiveRecord::RecordInvalid) + end + + it "always downcase tags" do + tags = "UPPERCASE, CAPITALIZE" + article = create(:article, tags: tags) + expect(article.tag_list).to eq(tags.downcase.split(", ")) + end + it "parses tags when description is empty" do body_markdown = "---\ntitle: Title\npublished: false\ndescription:\ntags: one\n---\n\n" expect(build_and_validate_article(body_markdown: body_markdown).tag_list).to eq(["one"]) diff --git a/spec/models/tag_adjustment_spec.rb b/spec/models/tag_adjustment_spec.rb index f2976cab2..978c75abc 100644 --- a/spec/models/tag_adjustment_spec.rb +++ b/spec/models/tag_adjustment_spec.rb @@ -73,8 +73,7 @@ RSpec.describe TagAdjustment, type: :model do describe "validates article tag_list" do it "does not allow addition on articles with 4 tags" do - article_tags_maxed = create(:article) - allow(article_tags_maxed).to receive(:tag_list).and_return([1, 2, 3, 4]) + article_tags_maxed = create(:article, tags: "ruby, javascript, html, css") tag_adjustment = build(:tag_adjustment, user_id: admin_user.id, article_id: article_tags_maxed.id, tag_id: tag.id, tag_name: tag.name) expect(tag_adjustment).to be_invalid end diff --git a/spec/models/tag_spec.rb b/spec/models/tag_spec.rb index 584a80883..58cd0c33c 100644 --- a/spec/models/tag_spec.rb +++ b/spec/models/tag_spec.rb @@ -123,6 +123,35 @@ RSpec.describe Tag, type: :model do end end + describe "::aliased_name" do + it "returns the preferred alias tag" do + preferred_tag = create(:tag, name: "rails") + bad_tag = create(:tag, name: "ror", alias_for: "rails") + expect(described_class.aliased_name(bad_tag.name)).to eq(preferred_tag.name) + end + + it "returns self if there's no preferred alias" do + tag = create(:tag, name: "ror") + expect(described_class.aliased_name(tag.name)).to eq(tag.name) + end + + it "returns nil for non-existing tag" do + expect(described_class.aliased_name("faketag")).to be_nil + end + end + + describe "::find_preferred_alias_for" do + it "returns preferred tag" do + preferred_tag = create(:tag, name: "rails") + tag = create(:tag, name: "ror", alias_for: "rails") + expect(described_class.find_preferred_alias_for(tag.name)).to eq(preferred_tag.name) + end + + it "returns self if there's no preferred tag" do + expect(described_class.find_preferred_alias_for("something")).to eq("something") + end + end + def collect_keywords(record) record.elasticsearch_doc.dig("_source", "tags").flat_map { |t| t["keywords_for_search"] } end diff --git a/spec/requests/articles/articles_create_spec.rb b/spec/requests/articles/articles_create_spec.rb index 547e50d41..aaaa6f4cb 100644 --- a/spec/requests/articles/articles_create_spec.rb +++ b/spec/requests/articles/articles_create_spec.rb @@ -16,6 +16,14 @@ RSpec.describe "ArticlesCreate", type: :request do expect(Article.last.user_id).to eq(user.id) end + it "properly downcase tags" do + new_title = "NEW TITLE #{rand(100)}" + post "/articles", params: { + article: { title: new_title, body_markdown: "Yo ho ho#{rand(100)}", tag_list: "What" } + } + expect(Article.last.tags.map(&:name)).to eq(["what"]) + end + it "creates article with front matter params" do post "/articles", params: { article: { diff --git a/spec/system/user_uses_the_editor_spec.rb b/spec/system/user_uses_the_editor_spec.rb index da8080541..8ce6fba7c 100644 --- a/spec/system/user_uses_the_editor_spec.rb +++ b/spec/system/user_uses_the_editor_spec.rb @@ -95,6 +95,7 @@ RSpec.describe "Using the editor", type: :system do fill_in "article_body_markdown", with: "Hello" find("button", text: /\APublish\z/).click expect(page).to have_text("Hello") + expect(page).to have_link("#what", href: "/t/what") end end end