From c4efee10bf697ccf5c37dd20a26001501b0af69f Mon Sep 17 00:00:00 2001 From: Vaidehi Joshi Date: Thu, 8 Oct 2020 07:51:25 -0700 Subject: [PATCH] Do not require meta_keywords to be set (#10721) * Remove extra character from meta_keywords in /listings/index.html.erb * Remove meta_keywords from MANDATORY_CONFIGS * Add and use meta_keywords_tag helper * Use modern tag syntax instead of deprecated syntax * Add and use meta_keywords_default helper * Add and use meta_keywords_article helper * Remove * from meta_keywords_field.label * Update meta_keyword specs to account for no keywords being set --- .../concerns/verify_setup_completed.rb | 2 - app/helpers/application_helper.rb | 24 ++++ app/views/admin/configs/show.html.erb | 118 +++++++++--------- app/views/articles/index.html.erb | 2 +- app/views/articles/search/_meta.html.erb | 2 +- app/views/articles/show.html.erb | 4 +- app/views/articles/tags/_meta.html.erb | 2 +- app/views/collections/_meta.html.erb | 2 +- app/views/comments/index.html.erb | 2 +- app/views/events/index.html.erb | 3 +- app/views/events/show.html.erb | 2 +- app/views/listings/index.html.erb | 3 +- app/views/notifications/index.html.erb | 2 +- app/views/pages/badges.html.erb | 2 +- app/views/pages/code_of_conduct.html.erb | 3 +- app/views/pages/contact.html.erb | 2 +- app/views/pages/editor_guide.html.erb | 2 +- app/views/pages/information.html.erb | 2 +- app/views/pages/markdown_basics.html.erb | 2 +- app/views/pages/privacy.html.erb | 2 +- .../pages/publishing_from_rss_guide.html.erb | 3 +- app/views/pages/report_abuse.html.erb | 3 +- app/views/pages/show.html.erb | 2 +- app/views/pages/sponsors.html.erb | 2 +- app/views/pages/terms.html.erb | 2 +- app/views/podcast_episodes/_meta.html.erb | 4 +- app/views/podcast_episodes/show.html.erb | 2 +- app/views/reading_list_items/index.html.erb | 2 +- app/views/tags/index.html.erb | 2 +- app/views/users/_meta.html.erb | 2 +- app/views/videos/index.html.erb | 2 +- spec/requests/articles/articles_show_spec.rb | 13 +- spec/requests/stories_index_spec.rb | 20 ++- .../admin/admin_manages_configuration_spec.rb | 4 +- 34 files changed, 148 insertions(+), 98 deletions(-) diff --git a/app/controllers/concerns/verify_setup_completed.rb b/app/controllers/concerns/verify_setup_completed.rb index 168e1d682..f41a40227 100644 --- a/app/controllers/concerns/verify_setup_completed.rb +++ b/app/controllers/concerns/verify_setup_completed.rb @@ -14,8 +14,6 @@ module VerifySetupCompleted mascot_user_id mascot_image_url - meta_keywords - suggested_tags suggested_users ].freeze diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index b7bc97945..a590864d4 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -216,6 +216,30 @@ module ApplicationHelper SiteConfig.community_member_label.pluralize end + def meta_keywords_default + return if SiteConfig.meta_keywords[:default].blank? + + tag.meta name: "keywords", content: SiteConfig.meta_keywords[:default] + end + + def meta_keywords_article(article_tags = nil) + return if SiteConfig.meta_keywords[:article].blank? + + content = if article_tags.present? + "#{article_tags}, #{SiteConfig.meta_keywords[:article]}" + else + SiteConfig.meta_keywords[:article] + end + + tag.meta name: "keywords", content: content + end + + def meta_keywords_tag(tag_name) + return if SiteConfig.meta_keywords[:tag].blank? + + tag.meta name: "keywords", content: "#{SiteConfig.meta_keywords[:tag]}, #{tag_name}" + end + def app_url(uri = nil) URL.url(uri) end diff --git a/app/views/admin/configs/show.html.erb b/app/views/admin/configs/show.html.erb index fa90405b5..48b36b2c5 100644 --- a/app/views/admin/configs/show.html.erb +++ b/app/views/admin/configs/show.html.erb @@ -642,7 +642,7 @@ <%= f.fields_for :meta_keywords do |meta_keywords_field| %> <% SiteConfig.meta_keywords.each do |area, keywords| %>
- <%= meta_keywords_field.label "#{area}*" %> + <%= meta_keywords_field.label area.to_s %> <%= meta_keywords_field.text_field area, class: "form-control", value: SiteConfig.meta_keywords[area], @@ -814,12 +814,12 @@
<%= render partial: "card_header", - locals: { - header: "Rate limits and anti-spam", - state: "collapse", - target: "rateLimitsBodyContainer", - expanded: "false" - } %> + locals: { + header: "Rate limits and anti-spam", + state: "collapse", + target: "rateLimitsBodyContainer", + expanded: "false" + } %>
<% configurable_rate_limits.each do |key, field_hash| %>
@@ -835,28 +835,28 @@
<%= admin_config_label :spam_trigger_terms %> <%= f.text_field :spam_trigger_terms, - class: "form-control", - value: SiteConfig.spam_trigger_terms.join(","), - placeholder: Constants::SiteConfig::DETAILS[:spam_trigger_terms][:placeholder] %> + class: "form-control", + value: SiteConfig.spam_trigger_terms.join(","), + placeholder: Constants::SiteConfig::DETAILS[:spam_trigger_terms][:placeholder] %>
<%= Constants::SiteConfig::DETAILS[:spam_trigger_terms][:description] %>
<%= render partial: "card_header", - locals: { - header: "Social Media", - state: "collapse", - target: "socialMediaBodyContainer", - expanded: "false" - } %> + locals: { + header: "Social Media", + state: "collapse", + target: "socialMediaBodyContainer", + expanded: "false" + } %>
<%= admin_config_label :twitter_hashtag %> <%= f.text_field :twitter_hashtag, - class: "form-control", - value: SiteConfig.twitter_hashtag.to_s, - placeholder: Constants::SiteConfig::DETAILS[:twitter_hashtag][:placeholder] %> + class: "form-control", + value: SiteConfig.twitter_hashtag.to_s, + placeholder: Constants::SiteConfig::DETAILS[:twitter_hashtag][:placeholder] %>
<%= Constants::SiteConfig::DETAILS[:twitter_hashtag][:description] %>
@@ -878,38 +878,38 @@
<%= render partial: "card_header", - locals: { - header: "Sponsors", - state: "collapse", - target: "sponsorsContainer", - expanded: "false" - } %> + locals: { + header: "Sponsors", + state: "collapse", + target: "sponsorsContainer", + expanded: "false" + } %>
<%= f.label :sponsor_headline %> <%= f.text_field :sponsor_headline, - class: "form-control", - value: SiteConfig.sponsor_headline, - placeholder: Constants::SiteConfig::DETAILS[:sponsor_headline][:placeholder] %> + class: "form-control", + value: SiteConfig.sponsor_headline, + placeholder: Constants::SiteConfig::DETAILS[:sponsor_headline][:placeholder] %>
<%= Constants::SiteConfig::DETAILS[:sponsor_headline][:description] %>
<%= render partial: "card_header", - locals: { - header: "Tags", - state: "collapse", - target: "tagsBodyContainer", - expanded: "false" - } %> + locals: { + header: "Tags", + state: "collapse", + target: "tagsBodyContainer", + expanded: "false" + } %>
<%= admin_config_label :sidebar_tags %> <%= f.text_field :sidebar_tags, - class: "form-control", - value: SiteConfig.sidebar_tags.join(","), - placeholder: Constants::SiteConfig::DETAILS[:sidebar_tags][:placeholder] %> + class: "form-control", + value: SiteConfig.sidebar_tags.join(","), + placeholder: Constants::SiteConfig::DETAILS[:sidebar_tags][:placeholder] %>
<%= Constants::SiteConfig::DETAILS[:sidebar_tags][:description] %>
@@ -917,47 +917,47 @@
<%= render partial: "card_header", - locals: { - header: "User Experience and Brand", - state: "collapse", - target: "uxBodyContainer", - expanded: "false" - } %> + locals: { + header: "User Experience and Brand", + state: "collapse", + target: "uxBodyContainer", + expanded: "false" + } %>
<%= f.label :feed_style %> <%= f.text_field :feed_style, - class: "form-control", - value: SiteConfig.feed_style, - placeholder: Constants::SiteConfig::DETAILS[:feed_style][:placeholder] %> + class: "form-control", + value: SiteConfig.feed_style, + placeholder: Constants::SiteConfig::DETAILS[:feed_style][:placeholder] %>
<%= Constants::SiteConfig::DETAILS[:feed_style][:description] %>
<%= f.label :feed_strategy %> <%= f.text_field :feed_strategy, - class: "form-control", - value: SiteConfig.feed_strategy, - placeholder: Constants::SiteConfig::DETAILS[:feed_strategy][:placeholder] %> + class: "form-control", + value: SiteConfig.feed_strategy, + placeholder: Constants::SiteConfig::DETAILS[:feed_strategy][:placeholder] %>
<%= Constants::SiteConfig::DETAILS[:feed_strategy][:description] %>
<%= f.label :default_font %> <%= select_tag "site_config[default_font]", - options_for_select( - [%w[sans-serif sans_serif], %w[serif serif], %w[open-dyslexic open_dyslexic]], - SiteConfig.default_font, - ), - multiple: false, - class: "form-control selectpicker" %> + options_for_select( + [%w[sans-serif sans_serif], %w[serif serif], %w[open-dyslexic open_dyslexic]], + SiteConfig.default_font, + ), + multiple: false, + class: "form-control selectpicker" %>
<%= Constants::SiteConfig::DETAILS[:default_font][:description] %>
<%= f.label :primary_brand_color_hex %> <%= f.text_field :primary_brand_color_hex, - class: "form-control", - value: SiteConfig.primary_brand_color_hex, - pattern: "^#+([a-fA-F0-9]{6})$", - placeholder: Constants::SiteConfig::DETAILS[:primary_brand_color_hex][:placeholder] %> + class: "form-control", + value: SiteConfig.primary_brand_color_hex, + pattern: "^#+([a-fA-F0-9]{6})$", + placeholder: Constants::SiteConfig::DETAILS[:primary_brand_color_hex][:placeholder] %>
<%= Constants::SiteConfig::DETAILS[:primary_brand_color_hex][:description] %>
diff --git a/app/views/articles/index.html.erb b/app/views/articles/index.html.erb index fec8c14b0..e81a1fbc5 100644 --- a/app/views/articles/index.html.erb +++ b/app/views/articles/index.html.erb @@ -7,7 +7,7 @@ - + <%= meta_keywords_default %> diff --git a/app/views/articles/search/_meta.html.erb b/app/views/articles/search/_meta.html.erb index 752f3a311..2e77cd623 100644 --- a/app/views/articles/search/_meta.html.erb +++ b/app/views/articles/search/_meta.html.erb @@ -1,7 +1,7 @@ <% title "Search Results" %> - +<%= meta_keywords_default %> diff --git a/app/views/articles/show.html.erb b/app/views/articles/show.html.erb index e7df9fb82..66100ff66 100644 --- a/app/views/articles/show.html.erb +++ b/app/views/articles/show.html.erb @@ -50,12 +50,12 @@ <% if internal_navigation? %> - + <%= meta_keywords_article(@article.cached_tag_list) %> <% else %> <%= content_for :page_meta do %> - + <%= meta_keywords_article(@article.cached_tag_list) %> diff --git a/app/views/articles/tags/_meta.html.erb b/app/views/articles/tags/_meta.html.erb index 2eca17093..bf6387eb8 100644 --- a/app/views/articles/tags/_meta.html.erb +++ b/app/views/articles/tags/_meta.html.erb @@ -6,7 +6,7 @@ - +<%= meta_keywords_tag(@tag) %> diff --git a/app/views/collections/_meta.html.erb b/app/views/collections/_meta.html.erb index 9f2499087..eca61364b 100644 --- a/app/views/collections/_meta.html.erb +++ b/app/views/collections/_meta.html.erb @@ -1,7 +1,7 @@ <% title title_string %> " /> - +<%= meta_keywords_default %> " /> diff --git a/app/views/comments/index.html.erb b/app/views/comments/index.html.erb index 7516e508e..c1761356e 100644 --- a/app/views/comments/index.html.erb +++ b/app/views/comments/index.html.erb @@ -6,7 +6,7 @@ <%= content_for :page_meta do %> "> - + <%= meta_keywords_article %> diff --git a/app/views/events/index.html.erb b/app/views/events/index.html.erb index a3961ed6d..4acfc1484 100644 --- a/app/views/events/index.html.erb +++ b/app/views/events/index.html.erb @@ -2,7 +2,8 @@ <% title("#{community_name} EVENTS") %> " /> - +<%= meta_keywords_default %> + " /> diff --git a/app/views/events/show.html.erb b/app/views/events/show.html.erb index f45e68efd..81de187a2 100644 --- a/app/views/events/show.html.erb +++ b/app/views/events/show.html.erb @@ -2,7 +2,7 @@ <% title("#{@event.title} - #{community_name}") %> " /> - + <%= meta_keywords_default %> " /> diff --git a/app/views/listings/index.html.erb b/app/views/listings/index.html.erb index 11c57988e..b01691161 100644 --- a/app/views/listings/index.html.erb +++ b/app/views/listings/index.html.erb @@ -3,7 +3,8 @@ - + <%= meta_keywords_default %> + diff --git a/app/views/notifications/index.html.erb b/app/views/notifications/index.html.erb index 7487bf198..68abf70bc 100644 --- a/app/views/notifications/index.html.erb +++ b/app/views/notifications/index.html.erb @@ -3,7 +3,7 @@ <%= content_for :page_meta do %> " /> - + <%= meta_keywords_default %> " /> diff --git a/app/views/pages/badges.html.erb b/app/views/pages/badges.html.erb index 9d292bea0..51ae0c43a 100644 --- a/app/views/pages/badges.html.erb +++ b/app/views/pages/badges.html.erb @@ -3,7 +3,7 @@ <%= content_for :page_meta do %> " /> - + <%= meta_keywords_default %> " /> diff --git a/app/views/pages/code_of_conduct.html.erb b/app/views/pages/code_of_conduct.html.erb index 45a5faf42..52fbaf6d1 100644 --- a/app/views/pages/code_of_conduct.html.erb +++ b/app/views/pages/code_of_conduct.html.erb @@ -3,7 +3,8 @@ <%= content_for :page_meta do %> - + <%= meta_keywords_default %> + diff --git a/app/views/pages/contact.html.erb b/app/views/pages/contact.html.erb index 0b62d461b..18dececb5 100644 --- a/app/views/pages/contact.html.erb +++ b/app/views/pages/contact.html.erb @@ -3,7 +3,7 @@ <%= content_for :page_meta do %> " /> - + <%= meta_keywords_default %> " /> diff --git a/app/views/pages/editor_guide.html.erb b/app/views/pages/editor_guide.html.erb index 8916ebb6d..491b1c946 100644 --- a/app/views/pages/editor_guide.html.erb +++ b/app/views/pages/editor_guide.html.erb @@ -3,7 +3,7 @@ <%= content_for :page_meta do %> " /> - + <%= meta_keywords_default %> <% end %>
diff --git a/app/views/pages/information.html.erb b/app/views/pages/information.html.erb index a8fd2b1bd..4d572453a 100644 --- a/app/views/pages/information.html.erb +++ b/app/views/pages/information.html.erb @@ -3,7 +3,7 @@ <%= content_for :page_meta do %> " /> - + <%= meta_keywords_default %> " /> diff --git a/app/views/pages/markdown_basics.html.erb b/app/views/pages/markdown_basics.html.erb index 0fd4a68a4..6b04212b9 100644 --- a/app/views/pages/markdown_basics.html.erb +++ b/app/views/pages/markdown_basics.html.erb @@ -3,7 +3,7 @@ <%= content_for :page_meta do %> " /> - + <%= meta_keywords_default %> <% end %>
diff --git a/app/views/pages/privacy.html.erb b/app/views/pages/privacy.html.erb index 179552ee7..287d6d325 100644 --- a/app/views/pages/privacy.html.erb +++ b/app/views/pages/privacy.html.erb @@ -3,7 +3,7 @@ <%= content_for :page_meta do %> " /> - + <%= meta_keywords_default %> " /> diff --git a/app/views/pages/publishing_from_rss_guide.html.erb b/app/views/pages/publishing_from_rss_guide.html.erb index dff6e1a87..2e105284e 100644 --- a/app/views/pages/publishing_from_rss_guide.html.erb +++ b/app/views/pages/publishing_from_rss_guide.html.erb @@ -3,8 +3,7 @@ <%= content_for :page_meta do %> " /> - - + <%= meta_keywords_default %> <% end %>
diff --git a/app/views/pages/report_abuse.html.erb b/app/views/pages/report_abuse.html.erb index 9cd979494..2c1963deb 100644 --- a/app/views/pages/report_abuse.html.erb +++ b/app/views/pages/report_abuse.html.erb @@ -2,7 +2,8 @@ <%= content_for :page_meta do %> " /> - + <%= meta_keywords_default %> + " /> diff --git a/app/views/pages/show.html.erb b/app/views/pages/show.html.erb index eb67cb26f..88313e3dc 100644 --- a/app/views/pages/show.html.erb +++ b/app/views/pages/show.html.erb @@ -2,7 +2,7 @@ <%= content_for :page_meta do %> - + <%= meta_keywords_default %> diff --git a/app/views/pages/sponsors.html.erb b/app/views/pages/sponsors.html.erb index a2b6fb099..e5d30d87f 100644 --- a/app/views/pages/sponsors.html.erb +++ b/app/views/pages/sponsors.html.erb @@ -3,7 +3,7 @@ <%= content_for :page_meta do %> " /> - + <%= meta_keywords_default %> <% end %>
diff --git a/app/views/pages/terms.html.erb b/app/views/pages/terms.html.erb index 1663c2302..de54a78cb 100644 --- a/app/views/pages/terms.html.erb +++ b/app/views/pages/terms.html.erb @@ -3,7 +3,7 @@ <%= content_for :page_meta do %> " /> - + <%= meta_keywords_default %> " /> diff --git a/app/views/podcast_episodes/_meta.html.erb b/app/views/podcast_episodes/_meta.html.erb index 865f0b10f..5886c45f8 100644 --- a/app/views/podcast_episodes/_meta.html.erb +++ b/app/views/podcast_episodes/_meta.html.erb @@ -4,7 +4,7 @@ <%= content_for :page_meta do %> - + <%= meta_keywords_default %> @@ -26,7 +26,7 @@ <%= content_for :page_meta do %> - + <%= meta_keywords_default %> diff --git a/app/views/podcast_episodes/show.html.erb b/app/views/podcast_episodes/show.html.erb index 8aa7062d5..0308d66f0 100644 --- a/app/views/podcast_episodes/show.html.erb +++ b/app/views/podcast_episodes/show.html.erb @@ -3,7 +3,7 @@ <%= content_for :page_meta do %> " /> - + <%= meta_keywords_default %> " /> diff --git a/app/views/reading_list_items/index.html.erb b/app/views/reading_list_items/index.html.erb index d87d9d113..f2ea6e52e 100644 --- a/app/views/reading_list_items/index.html.erb +++ b/app/views/reading_list_items/index.html.erb @@ -3,7 +3,7 @@ <%= content_for :page_meta do %> " /> - + <%= meta_keywords_default %> " /> diff --git a/app/views/tags/index.html.erb b/app/views/tags/index.html.erb index c08e3f84f..afc3169f7 100644 --- a/app/views/tags/index.html.erb +++ b/app/views/tags/index.html.erb @@ -3,7 +3,7 @@ <%= content_for :page_meta do %> " /> - + <%= meta_keywords_default %> " /> diff --git a/app/views/users/_meta.html.erb b/app/views/users/_meta.html.erb index bf34d06dc..8240df889 100644 --- a/app/views/users/_meta.html.erb +++ b/app/views/users/_meta.html.erb @@ -1,6 +1,6 @@ - +<%= meta_keywords_default %> diff --git a/app/views/videos/index.html.erb b/app/views/videos/index.html.erb index 9a59cb78b..e44429fa5 100644 --- a/app/views/videos/index.html.erb +++ b/app/views/videos/index.html.erb @@ -3,7 +3,7 @@ <%= content_for :page_meta do %> - + <%= meta_keywords_default %> diff --git a/spec/requests/articles/articles_show_spec.rb b/spec/requests/articles/articles_show_spec.rb index 17846350b..b9bc82507 100644 --- a/spec/requests/articles/articles_show_spec.rb +++ b/spec/requests/articles/articles_show_spec.rb @@ -78,7 +78,7 @@ RSpec.describe "ArticlesShow", type: :request do end # rubocop:enable RSpec/ExampleLength - context "when keywords are set up" do + context "when keywords are set" do it "shows keywords" do SiteConfig.meta_keywords = { article: "hello, world" } article.update_column(:cached_tag_list, "super sheep") @@ -87,6 +87,17 @@ RSpec.describe "ArticlesShow", type: :request do end end + context "when keywords are not" do + it "does not show keywords" do + SiteConfig.meta_keywords = { article: "" } + article.update_column(:cached_tag_list, "super sheep") + get article.path + expect(response.body).not_to include( + '', + ) + end + end + context "when user signed in" do before do sign_in user diff --git a/spec/requests/stories_index_spec.rb b/spec/requests/stories_index_spec.rb index 658f91ec2..c3606e855 100644 --- a/spec/requests/stories_index_spec.rb +++ b/spec/requests/stories_index_spec.rb @@ -119,12 +119,20 @@ RSpec.describe "StoriesIndex", type: :request do expect(response.headers["X-Accel-Expires"]).to eq("600") end - it "shows default meta keywords" do + it "shows default meta keywords if set" do SiteConfig.meta_keywords = { default: "cool developers, civil engineers" } get "/" expect(response.body).to include("") end + it "does not show default meta keywords if not set" do + SiteConfig.meta_keywords = { default: "" } + get "/" + expect(response.body).not_to include( + "", + ) + end + it "shows only one cover if basic feed style" do create_list(:article, 3, featured: true, score: 20, main_image: "https://example.com/image.jpg") @@ -348,12 +356,20 @@ RSpec.describe "StoriesIndex", type: :request do expect(response.body).to include(sponsorship.blurb_html) end - it "shows meta keywords" do + it "shows meta keywords if set" do SiteConfig.meta_keywords = { tag: "software engineering, ruby" } get "/t/#{tag.name}" expect(response.body).to include("") end + it "does not show meta keywords if not set" do + SiteConfig.meta_keywords = { tag: "" } + get "/t/#{tag.name}" + expect(response.body).not_to include( + "", + ) + end + context "with user signed in" do before do sign_in user diff --git a/spec/system/admin/admin_manages_configuration_spec.rb b/spec/system/admin/admin_manages_configuration_spec.rb index ab4f96cc3..7e96e3703 100644 --- a/spec/system/admin/admin_manages_configuration_spec.rb +++ b/spec/system/admin/admin_manages_configuration_spec.rb @@ -8,9 +8,7 @@ RSpec.describe "Admin manages configuration", type: :system do visit admin_config_path end - # Note: The :meta_keywords are handled slightly differently in the view, so we - # can't check them the same way as the rest. - (VerifySetupCompleted::MANDATORY_CONFIGS - [:meta_keywords]).each do |option| + VerifySetupCompleted::MANDATORY_CONFIGS.each do |option| it "marks #{option} as required" do selector = "label[for='site_config_#{option}']" expect(first(selector).text).to include("Required")