From 1d47a5c079e35a2bf88d504327f44bebd9ae241c Mon Sep 17 00:00:00 2001 From: taitus Date: Fri, 25 Jun 2021 13:07:59 +0200 Subject: [PATCH 1/3] Add link_url validation for cards when are not header cards Currently it is not necessary to include the link_url field. When we display these cards without link_url, they create an empty link that redirects to the same page. I understand that this is not a desired behavior, so I think it is better to add a validation in this case and force administrators to add a link_url when creating a card. --- app/models/widget/card.rb | 1 + db/dev_seeds/sdg.rb | 3 ++- spec/models/widget/card_spec.rb | 8 ++++++++ spec/system/admin/widgets/cards_spec.rb | 1 + spec/system/sdg_management/homepage_spec.rb | 2 ++ 5 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/models/widget/card.rb b/app/models/widget/card.rb index fa8b2f8e7..01b00a1c1 100644 --- a/app/models/widget/card.rb +++ b/app/models/widget/card.rb @@ -9,6 +9,7 @@ class Widget::Card < ApplicationRecord include Globalizable validates_translation :title, presence: true + validates :link_url, presence: true, if: -> { !header? } def self.header where(header: true) diff --git a/db/dev_seeds/sdg.rb b/db/dev_seeds/sdg.rb index f7fe40a3a..8381f01fb 100644 --- a/db/dev_seeds/sdg.rb +++ b/db/dev_seeds/sdg.rb @@ -33,6 +33,7 @@ end section "Creating SDG homepage cards" do SDG::Phase.all.each do |phase| - Widget::Card.create!(cardable: phase, title: "#{phase.title} card") + Widget::Card.create!(cardable: phase, title: "#{phase.title} card", + link_text: "Link Text", link_url: "/any_path") end end diff --git a/spec/models/widget/card_spec.rb b/spec/models/widget/card_spec.rb index 2dea5b38e..c031ed615 100644 --- a/spec/models/widget/card_spec.rb +++ b/spec/models/widget/card_spec.rb @@ -17,6 +17,14 @@ describe Widget::Card do it "is not valid without a title" do expect(build(:widget_card, title: "")).not_to be_valid end + + context "regular cards" do + it "is not valid without a link_url" do + card = build(:widget_card, header: false, link_url: nil) + + expect(card).not_to be_valid + end + end end describe "#header" do diff --git a/spec/system/admin/widgets/cards_spec.rb b/spec/system/admin/widgets/cards_spec.rb index 1621d90a6..4f7477142 100644 --- a/spec/system/admin/widgets/cards_spec.rb +++ b/spec/system/admin/widgets/cards_spec.rb @@ -172,6 +172,7 @@ describe "Cards", :admin do href: admin_site_customization_page_widget_cards_path(custom_page)) fill_in "Title", with: "Card for a custom page" + fill_in "Link URL", with: "/any_path" click_button "Create card" expect(page).to have_current_path admin_site_customization_page_widget_cards_path(custom_page) diff --git a/spec/system/sdg_management/homepage_spec.rb b/spec/system/sdg_management/homepage_spec.rb index 6f8428b25..1fb339d5a 100644 --- a/spec/system/sdg_management/homepage_spec.rb +++ b/spec/system/sdg_management/homepage_spec.rb @@ -22,6 +22,7 @@ describe "SDG homepage configuration" do click_link "Create planning card" within(".translatable-fields") { fill_in "Title", with: "My planning card" } + fill_in "Link URL", with: "/any_path" click_button "Create card" within(".planning-cards") do @@ -54,6 +55,7 @@ describe "SDG homepage configuration" do click_link "Create header" within(".translatable-fields") { fill_in "Title", with: "My header" } + fill_in "Link URL", with: "/any_path" click_button "Create card" within(".sdg-header") do From 108de86da574b7e4a93015f682b3bed0b6c67893 Mon Sep 17 00:00:00 2001 From: taitus Date: Fri, 25 Jun 2021 13:12:40 +0200 Subject: [PATCH 2/3] Add link_url validation for cards when are header cards Add link_url presence validation only when link_text is provided only for header cards. In this case it makes sense to allow creating a "header card" without link_url, since we can show the header without link text and without link url and it still does its function. --- app/models/widget/card.rb | 2 +- spec/models/widget/card_spec.rb | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/app/models/widget/card.rb b/app/models/widget/card.rb index 01b00a1c1..835defe6c 100644 --- a/app/models/widget/card.rb +++ b/app/models/widget/card.rb @@ -9,7 +9,7 @@ class Widget::Card < ApplicationRecord include Globalizable validates_translation :title, presence: true - validates :link_url, presence: true, if: -> { !header? } + validates :link_url, presence: true, if: -> { !header? || link_text.present? } def self.header where(header: true) diff --git a/spec/models/widget/card_spec.rb b/spec/models/widget/card_spec.rb index c031ed615..bb530cd50 100644 --- a/spec/models/widget/card_spec.rb +++ b/spec/models/widget/card_spec.rb @@ -25,6 +25,30 @@ describe Widget::Card do expect(card).not_to be_valid end end + + context "header cards" do + it "is valid with no link_url and no link_text" do + header = build(:widget_card, :header, link_text: nil, link_url: nil) + + expect(header).to be_valid + end + + it "is not valid with link_text and no link_url" do + header = build(:widget_card, :header, link_text: "link text", link_url: nil) + + expect(header).not_to be_valid + expect(header.errors.count).to be 1 + expect(header.errors[:link_url].count).to be 1 + expect(header.errors[:link_url].first).to eq "can't be blank" + end + + it "is valid if link_text and link_url are both provided" do + header = build(:widget_card, :header, link_text: "Text link", + link_url: "https://consulproject.org") + + expect(header).to be_valid + end + end end describe "#header" do From db23d536dd66d7cd707453eaa128353e4f7db9a5 Mon Sep 17 00:00:00 2001 From: taitus Date: Fri, 25 Jun 2021 13:15:11 +0200 Subject: [PATCH 3/3] Do not show link content section when links fields are not defined. --- app/views/shared/_header.html.erb | 12 ++++++++---- spec/system/home_spec.rb | 26 ++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/app/views/shared/_header.html.erb b/app/views/shared/_header.html.erb index e3eeff304..06e0fbc54 100644 --- a/app/views/shared/_header.html.erb +++ b/app/views/shared/_header.html.erb @@ -1,13 +1,17 @@ <% if header.present? %> -
+
"> <%= header.label %>

<%= header.title %>

+

<%= header.description %>

-
"> - <%= link_to header.link_text, header.link_url, class: "button expanded large" %> -
+ + <% if header.link_text.present? && header.link_url.present? %> +
"> + <%= link_to header.link_text, header.link_url, class: "button expanded large" %> +
+ <% end %>
<% if header.image.present? %> diff --git a/spec/system/home_spec.rb b/spec/system/home_spec.rb index f1482e2d9..6a2a36456 100644 --- a/spec/system/home_spec.rb +++ b/spec/system/home_spec.rb @@ -174,4 +174,30 @@ describe "Home" do expect(page).not_to have_css(".title", text: "Featured") end + + describe "Header Card" do + scenario "if there is header card with link, the link content is rendered" do + create(:widget_card, :header, link_text: "Link text", link_url: "consul.dev") + + visit root_path + + expect(page).to have_link "Link text", href: "consul.dev" + end + + scenario "if there is header card without link, the link content is not rendered" do + create(:widget_card, :header, link_text: nil, link_url: nil) + + visit root_path + + within(".header-card") { expect(page).not_to have_link } + end + + scenario "if there is header card without link and with text, the link content is not rendered" do + create(:widget_card, :header, link_text: "", link_url: "", link_text_es: "Link ES", title_es: "ES") + + visit root_path(locale: :es) + + within(".header-card") { expect(page).not_to have_link } + end + end end