From a7bbdb1bd09bbf1c424dbe891973b388888fcff8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Thu, 14 Jan 2021 16:06:34 +0100
Subject: [PATCH] Simplify rendering a banner

Now the banner component accepts either a banner or a section and loads
the banner if it's a section, so we don't have to add the `@banners`
variable in several controllers.
---
 app/components/shared/banner_component.rb     | 14 +++-
 app/controllers/budgets_controller.rb         |  1 -
 .../concerns/commentable_actions.rb           | 10 ---
 app/controllers/pages_controller.rb           |  1 -
 app/controllers/welcome_controller.rb         |  1 -
 app/views/admin/banners/_form.html.erb        |  2 +-
 app/views/admin/banners/index.html.erb        |  2 +-
 app/views/budgets/index.html.erb              |  2 +-
 app/views/debates/index.html.erb              |  2 +-
 app/views/pages/help/index.html.erb           |  2 +-
 app/views/proposals/index.html.erb            |  2 +-
 app/views/shared/_banner.html.erb             |  3 -
 app/views/welcome/index.html.erb              |  2 +-
 .../shared/banner_component_spec.rb           | 64 +++++++++++++++++++
 14 files changed, 82 insertions(+), 26 deletions(-)
 delete mode 100644 app/views/shared/_banner.html.erb

diff --git a/app/components/shared/banner_component.rb b/app/components/shared/banner_component.rb
index fcbb5a727..de61e1602 100644
--- a/app/components/shared/banner_component.rb
+++ b/app/components/shared/banner_component.rb
@@ -1,8 +1,16 @@
 class Shared::BannerComponent < ApplicationComponent
-  attr_reader :banner
+  attr_reader :banner_or_section
 
-  def initialize(banner)
-    @banner = banner
+  def initialize(banner_or_section)
+    @banner_or_section = banner_or_section
+  end
+
+  def banner
+    @banner ||= if banner_or_section.respond_to?(:sections)
+                  banner_or_section
+                else
+                  Banner.in_section(banner_or_section).with_active.sample
+                end
   end
 
   private
diff --git a/app/controllers/budgets_controller.rb b/app/controllers/budgets_controller.rb
index 9fb0b0bf6..ec7e93f57 100644
--- a/app/controllers/budgets_controller.rb
+++ b/app/controllers/budgets_controller.rb
@@ -17,7 +17,6 @@ class BudgetsController < ApplicationController
   def index
     @finished_budgets = @budgets.finished.order(created_at: :desc)
     @budgets_coordinates = current_budget_map_locations
-    @banners = Banner.in_section("budgets").with_active
   end
 
   private
diff --git a/app/controllers/concerns/commentable_actions.rb b/app/controllers/concerns/commentable_actions.rb
index 8b2a6d3bc..5523285bf 100644
--- a/app/controllers/concerns/commentable_actions.rb
+++ b/app/controllers/concerns/commentable_actions.rb
@@ -16,7 +16,6 @@ module CommentableActions
     index_customization
 
     @tag_cloud = tag_cloud
-    @banners = Banner.in_section(section(resource_model.name)).with_active
 
     set_resource_votes(@resources)
 
@@ -110,15 +109,6 @@ module CommentableActions
       nil
     end
 
-    def section(resource_name)
-      case resource_name
-      when "Proposal"
-        "proposals"
-      when "Debate"
-        "debates"
-      end
-    end
-
     def featured_proposals
       @featured_proposals ||= []
     end
diff --git a/app/controllers/pages_controller.rb b/app/controllers/pages_controller.rb
index a4b01cf09..03bb40828 100644
--- a/app/controllers/pages_controller.rb
+++ b/app/controllers/pages_controller.rb
@@ -6,7 +6,6 @@ class PagesController < ApplicationController
 
   def show
     @custom_page = SiteCustomization::Page.published.find_by(slug: params[:id])
-    @banners = Banner.in_section("help_page").with_active
 
     if @custom_page.present?
       @cards = @custom_page.cards
diff --git a/app/controllers/welcome_controller.rb b/app/controllers/welcome_controller.rb
index 4e3081e73..9fa9bf1f2 100644
--- a/app/controllers/welcome_controller.rb
+++ b/app/controllers/welcome_controller.rb
@@ -11,7 +11,6 @@ class WelcomeController < ApplicationController
     @header = Widget::Card.header.first
     @feeds = Widget::Feed.active
     @cards = Widget::Card.body
-    @banners = Banner.in_section("homepage").with_active
     @remote_translations = detect_remote_translations(@feeds,
                                                       @recommended_debates,
                                                       @recommended_proposals)
diff --git a/app/views/admin/banners/_form.html.erb b/app/views/admin/banners/_form.html.erb
index 70e55079e..8737b760b 100644
--- a/app/views/admin/banners/_form.html.erb
+++ b/app/views/admin/banners/_form.html.erb
@@ -79,6 +79,6 @@
   </div>
 
   <div class="row">
-    <%= render "shared/banner", banner: @banner %>
+    <%= render Shared::BannerComponent.new(@banner) %>
   </div>
 <% end %>
diff --git a/app/views/admin/banners/index.html.erb b/app/views/admin/banners/index.html.erb
index 67b151d86..bd71afb5f 100644
--- a/app/views/admin/banners/index.html.erb
+++ b/app/views/admin/banners/index.html.erb
@@ -30,7 +30,7 @@
       <tr>
         <td colspan="3">
           <span class="help-text"><%= t("admin.banners.index.preview") %></span>
-          <%= render "shared/banner", banner: banner %>
+          <%= render Shared::BannerComponent.new(banner) %>
         </td>
       </tr>
     </tbody>
diff --git a/app/views/budgets/index.html.erb b/app/views/budgets/index.html.erb
index 1beeb1179..4121e817d 100644
--- a/app/views/budgets/index.html.erb
+++ b/app/views/budgets/index.html.erb
@@ -1,4 +1,4 @@
-<%= render "shared/banner" %>
+<%= render Shared::BannerComponent.new("budgets") %>
 
 <% provide :title do %><%= t("budgets.index.title") %><% end %>
 
diff --git a/app/views/debates/index.html.erb b/app/views/debates/index.html.erb
index 0d9651360..24142020b 100644
--- a/app/views/debates/index.html.erb
+++ b/app/views/debates/index.html.erb
@@ -40,7 +40,7 @@
   <div class="row">
     <div id="debates" class="debates-list small-12 medium-9 column">
 
-      <%= render "shared/banner" %>
+      <%= render Shared::BannerComponent.new("debates") %>
 
       <% unless @search_terms || !has_featured? %>
         <%= render "featured_debates" %>
diff --git a/app/views/pages/help/index.html.erb b/app/views/pages/help/index.html.erb
index 505ff2196..34f441e69 100644
--- a/app/views/pages/help/index.html.erb
+++ b/app/views/pages/help/index.html.erb
@@ -3,7 +3,7 @@
   <%= render "shared/canonical", href: help_url %>
 <% end %>
 
-<%= render "shared/banner" %>
+<%= render Shared::BannerComponent.new("help_page") %>
 
 <div class="jumbo light">
   <div class="row">
diff --git a/app/views/proposals/index.html.erb b/app/views/proposals/index.html.erb
index c28790495..8ec111379 100644
--- a/app/views/proposals/index.html.erb
+++ b/app/views/proposals/index.html.erb
@@ -49,7 +49,7 @@
   <div class="row">
     <div id="proposals" class="proposals-list small-12 medium-9 column">
 
-      <%= render "shared/banner" %>
+      <%= render Shared::BannerComponent.new("proposals") %>
 
       <% if show_featured_proposals? %>
         <div id="featured-proposals" class="row featured-proposals">
diff --git a/app/views/shared/_banner.html.erb b/app/views/shared/_banner.html.erb
deleted file mode 100644
index c98b1185d..000000000
--- a/app/views/shared/_banner.html.erb
+++ /dev/null
@@ -1,3 +0,0 @@
-<% banner ||= @banners&.sample %>
-
-<%= render Shared::BannerComponent.new(banner) %>
diff --git a/app/views/welcome/index.html.erb b/app/views/welcome/index.html.erb
index d6d37a4ee..8c2611397 100644
--- a/app/views/welcome/index.html.erb
+++ b/app/views/welcome/index.html.erb
@@ -4,7 +4,7 @@
   <%= render "shared/canonical", href: root_url %>
 <% end %>
 
-<%= render "shared/banner" %>
+<%= render Shared::BannerComponent.new("homepage") %>
 
 <% provide :social_media_meta_tags do %>
   <%= render "shared/social_media_meta_tags",
diff --git a/spec/components/shared/banner_component_spec.rb b/spec/components/shared/banner_component_spec.rb
index 77168e63a..9c0ccda27 100644
--- a/spec/components/shared/banner_component_spec.rb
+++ b/spec/components/shared/banner_component_spec.rb
@@ -7,6 +7,70 @@ describe Shared::BannerComponent, type: :component do
     expect(page.find(".banner")).to have_content "Vote now!"
   end
 
+  it "renders given a section" do
+    create(:banner, web_sections: [WebSection.find_by!(name: "debates")], title: "Debate banner")
+    create(:banner, web_sections: [WebSection.find_by!(name: "proposals")], title: "Proposal banner")
+
+    render_inline Shared::BannerComponent.new("debates")
+
+    expect(page.find(".banner")).to have_content "Debate banner"
+    expect(page).not_to have_content "Proposal banner"
+  end
+
+  context "several banners available in the same section" do
+    before do
+      create(:banner,
+             web_sections: [WebSection.find_by!(name: "debates")],
+             title: "First banner",
+             description: "First description",
+             target_url: "/first_target"
+            )
+
+      create(:banner,
+             web_sections: [WebSection.find_by!(name: "debates")],
+             title: "Second banner",
+             description: "Second description",
+             target_url: "/second_target"
+            )
+    end
+
+    it "only renders one banner" do
+      render_inline Shared::BannerComponent.new("debates")
+
+      expect(page).to have_css ".banner", count: 1
+    end
+
+    it "consistently renders one banner" do
+      render_inline Shared::BannerComponent.new("debates")
+
+      if page.has_content?("First banner")
+        expect(page).to have_content "First description"
+        expect(page).to have_css "[href='/first_target']"
+        expect(page).not_to have_content "Second banner"
+      else
+        expect(page).to have_content "Second description"
+        expect(page).to have_css "[href='/second_target']"
+      end
+    end
+
+    it "only renders active banners" do
+      Banner.first.update!(post_ended_at: 1.day.ago)
+
+      render_inline Shared::BannerComponent.new("debates")
+
+      expect(page).to have_content "Second banner"
+      expect(page).not_to have_content "First banner"
+    end
+
+    it "does not render anything with no active banners" do
+      Banner.all.each { |banner| banner.update!(post_ended_at: 1.day.ago) }
+
+      render_inline Shared::BannerComponent.new("debates")
+
+      expect(page).not_to have_css ".banner"
+    end
+  end
+
   it "does not render anything given nil" do
     render_inline Shared::BannerComponent.new(nil)