From 2365e96b8b1e6de324b84d515fbb0bedd09700f9 Mon Sep 17 00:00:00 2001
From: Jade Philippe <85654561+jp524@users.noreply.github.com>
Date: Tue, 25 Jun 2024 14:52:54 -0500
Subject: [PATCH] Improve banner expiration validations and form (#5828)

* Modify 'Expires At' column

* Do not display seconds in datetime field for Expires At

* Add server-side validation for Expires At

* Add client-side validation for Expires At

* Add server-side validation for Content

* Make expired banners inactive

* Lint code

* Modify banner system spec to use timezone

* Lint code

* Modify test to use 12hr time format

* Improve banner tests by removing travel

* Simplify banner system test of expiration date in the past

* Lint code

* Fix bug in banner Expires At validation

* Modify banner Expires At validation to work with time travelling tests

* Add requests tests for banner creation

* Replace cookies by helper in banner form

* Refactor banner request and model tests
---
 app/controllers/banners_controller.rb   |  2 +-
 app/helpers/banner_helper.rb            |  4 +-
 app/models/banner.rb                    | 14 ++++-
 app/views/banners/_form.html.erb        |  6 ++-
 app/views/banners/index.html.erb        |  2 +-
 spec/helpers/banner_helper_spec.rb      | 14 +++--
 spec/models/banner_spec.rb              | 39 +++++++++++---
 spec/requests/banners_spec.rb           | 72 +++++++++++++++++++++++--
 spec/system/banners/new_spec.rb         | 18 ++++++-
 spec/views/banners/new.html.erb_spec.rb |  9 ++++
 10 files changed, 155 insertions(+), 25 deletions(-)

diff --git a/app/controllers/banners_controller.rb b/app/controllers/banners_controller.rb
index e65c61e588..b9ab11d91e 100644
--- a/app/controllers/banners_controller.rb
+++ b/app/controllers/banners_controller.rb
@@ -65,7 +65,7 @@ def set_banner
   end
 
   def banner_params
-    BannerParameters.new(params, current_user, cookies[:browser_time_zone])
+    BannerParameters.new(params, current_user, browser_time_zone)
   end
 
   def deactivate_alternate_active_banner
diff --git a/app/helpers/banner_helper.rb b/app/helpers/banner_helper.rb
index 9348953d1a..d358a7de3d 100644
--- a/app/helpers/banner_helper.rb
+++ b/app/helpers/banner_helper.rb
@@ -7,11 +7,11 @@ def conditionally_add_hidden_class(current_banner_is_active)
 
   def banner_expiration_time_in_words(banner)
     if banner.expired?
-      "Yes"
+      "Expired"
     elsif banner.expires_at
       "in #{distance_of_time_in_words(Time.now, banner.expires_at)}"
     else
-      "No"
+      "No Expiration"
     end
   end
 end
diff --git a/app/models/banner.rb b/app/models/banner.rb
index 155bc69974..efcdd43488 100644
--- a/app/models/banner.rb
+++ b/app/models/banner.rb
@@ -6,10 +6,14 @@ class Banner < ApplicationRecord
   scope :active, -> { where(active: true) }
 
   validates_presence_of :name
+  validates_presence_of :content
   validate :only_one_banner_is_active_per_organization
+  validate :expires_at_must_be_in_future
 
   def expired?
-    expires_at && Time.current > expires_at
+    expired = expires_at && Time.current > expires_at
+    update(active: false) if active && expired
+    expired
   end
 
   # `expires_at` is stored in the database as UTC, but timezone information will be stripped before displaying on frontend
@@ -27,6 +31,14 @@ def only_one_banner_is_active_per_organization
       errors.add(:base, "Only one banner can be active at a time. Mark the other banners as not active before marking this banner as active.")
     end
   end
+
+  # Validation using line below doesn't work with `travel_to` in specs. Must use detailed method
+  # validates_comparison_of :expires_at, greater_than: Time.current, message: "must take place in the future (after #{Time.current})", allow_blank: true
+  def expires_at_must_be_in_future
+    if expires_at.present? && expires_at < Time.current
+      errors.add(:expires_at, "must take place in the future (after #{Time.current})")
+    end
+  end
 end
 
 # == Schema Information
diff --git a/app/views/banners/_form.html.erb b/app/views/banners/_form.html.erb
index 25ff047cc0..b270d43211 100644
--- a/app/views/banners/_form.html.erb
+++ b/app/views/banners/_form.html.erb
@@ -28,7 +28,11 @@
     </div>
     <span class="input-style-1">
       <%= form.label :expires_at, "Expires at (optional)" %>
-      <%= form.datetime_field :expires_at, value: banner.expires_at_in_time_zone(cookies[:browser_time]), required: false %>
+      <%= form.datetime_field :expires_at,
+                              value: banner.expires_at_in_time_zone(browser_time_zone),
+                              required: false,
+                              include_seconds: false,
+                              min: Time.current.in_time_zone(browser_time_zone) %>
     </span>
     <div class="input-style-1">
       <%= form.label :content %>
diff --git a/app/views/banners/index.html.erb b/app/views/banners/index.html.erb
index fd36a60f82..fdfa86d7f8 100644
--- a/app/views/banners/index.html.erb
+++ b/app/views/banners/index.html.erb
@@ -25,7 +25,7 @@
             <tr>
               <th>Name</th>
               <th>Active?</th>
-              <th>Expires At</th>
+              <th>Expiration</th>
               <th>Last Updated By</th>
               <th>Updated At</th>
               <th>Actions</th>
diff --git a/spec/helpers/banner_helper_spec.rb b/spec/helpers/banner_helper_spec.rb
index 53469ac358..07abb51253 100644
--- a/spec/helpers/banner_helper_spec.rb
+++ b/spec/helpers/banner_helper_spec.rb
@@ -42,8 +42,8 @@
     context "when expires_at isn't set" do
       let(:expires_at) { nil }
 
-      it "returns No" do
-        expect(helper.banner_expiration_time_in_words(banner)).to eq("No")
+      it "returns No Expiration" do
+        expect(helper.banner_expiration_time_in_words(banner)).to eq("No Expiration")
       end
     end
 
@@ -56,10 +56,14 @@
     end
 
     context "when expires_at is in the past" do
-      let(:expires_at) { 7.days.ago }
+      let(:expired_banner) do
+        banner = create(:banner, expires_at: nil)
+        banner.update_columns(expires_at: 2.days.ago)
+        banner
+      end
 
-      it "returns yes" do
-        expect(helper.banner_expiration_time_in_words(banner)).to eq("Yes")
+      it "returns Expired" do
+        expect(helper.banner_expiration_time_in_words(expired_banner)).to eq("Expired")
       end
     end
   end
diff --git a/spec/models/banner_spec.rb b/spec/models/banner_spec.rb
index fda2a03adc..0e700a184e 100644
--- a/spec/models/banner_spec.rb
+++ b/spec/models/banner_spec.rb
@@ -2,9 +2,10 @@
 
 RSpec.describe Banner, type: :model do
   describe "#valid?" do
+    let(:casa_org) { create(:casa_org) }
+    let(:supervisor) { create(:supervisor, casa_org: casa_org) }
+
     it "does not allow multiple active banners for same organization" do
-      casa_org = create(:casa_org)
-      supervisor = create(:supervisor)
       create(:banner, casa_org: casa_org, user: supervisor)
 
       banner = build(:banner, casa_org: casa_org, user: supervisor)
@@ -12,8 +13,6 @@
     end
 
     it "does allow multiple active banners for different organization" do
-      casa_org = create(:casa_org)
-      supervisor = create(:supervisor, casa_org: casa_org)
       create(:banner, casa_org: casa_org, user: supervisor)
 
       another_org = create(:casa_org)
@@ -21,6 +20,21 @@
       banner = build(:banner, casa_org: another_org, user: another_supervisor)
       expect(banner).to be_valid
     end
+
+    it "does not allow an expiry date set in the past" do
+      banner = build(:banner, casa_org: casa_org, user: supervisor, expires_at: 1.hour.ago)
+      expect(banner).to_not be_valid
+    end
+
+    it "allows an expiry date set in the future" do
+      banner = build(:banner, casa_org: casa_org, user: supervisor, expires_at: 1.day.from_now)
+      expect(banner).to be_valid
+    end
+
+    it "does not allow content to be empty" do
+      banner = build(:banner, casa_org: casa_org, user: supervisor, content: nil)
+      expect(banner).to_not be_valid
+    end
   end
 
   describe "#expired?" do
@@ -30,21 +44,30 @@
       expect(banner).not_to be_expired
     end
 
-    it "is false when expires_at is set but is after today" do
+    it "is false when expires_at is set but is in the future" do
       banner = create(:banner, expires_at: 7.days.from_now)
 
       expect(banner).not_to be_expired
     end
 
-    it "is true when expires_at is set but is before today" do
-      banner = create(:banner, expires_at: 7.days.ago)
-
+    it "is true when expires_at is set but is in the past" do
+      banner = create(:banner, expires_at: nil)
+      banner.update_columns(expires_at: 1.hour.ago)
       expect(banner).to be_expired
     end
+
+    it "sets active to false when banner is expired" do
+      banner = create(:banner, expires_at: 1.hour.from_now)
+      expect(banner.active).to be true
+      travel 2.hours
+      banner.expired?
+      expect(banner.active).to be false
+    end
   end
 
   describe "#expires_at_in_time_zone" do
     it "can shift time by timezone for equivalent times" do
+      travel_to Time.new(2024, 6, 1, 11, 0, 0, "UTC")
       banner = create(:banner, expires_at: "2024-06-13 12:00:00 UTC")
 
       expires_at_in_pacific_time = banner.expires_at_in_time_zone("America/Los_Angeles")
diff --git a/spec/requests/banners_spec.rb b/spec/requests/banners_spec.rb
index a5c3399d98..7ed673bdb2 100644
--- a/spec/requests/banners_spec.rb
+++ b/spec/requests/banners_spec.rb
@@ -49,10 +49,8 @@
   end
 
   context "when a banner has expires_at" do
-    let!(:active_banner) { create(:banner, casa_org: casa_org, expires_at: expires_at) }
-
     context "when expires_at is after today" do
-      let(:expires_at) { 7.days.from_now }
+      let!(:active_banner) { create(:banner, casa_org: casa_org, expires_at: 7.days.from_now) }
 
       it "displays the banner" do
         sign_in volunteer
@@ -62,7 +60,10 @@
     end
 
     context "when expires_at is before today" do
-      let(:expires_at) { 7.days.ago }
+      let!(:active_banner) do
+        banner = create(:banner, casa_org: casa_org, expires_at: nil)
+        banner.update_columns(expires_at: 1.day.ago)
+      end
 
       it "does not display the banner" do
         sign_in volunteer
@@ -71,4 +72,67 @@
       end
     end
   end
+
+  context "when creating a banner" do
+    let(:admin) { create(:casa_admin, casa_org: casa_org) }
+    let(:banner_params) do
+      {
+        user: admin,
+        active: false,
+        content: "Test",
+        name: "Test Announcement",
+        expires_at: expires_at
+      }
+    end
+
+    context "when client timezone is ahead of UTC" do
+      before { travel_to Time.new(2024, 6, 1, 11, 0, 0, "+03:00") } # 08:00 UTC
+
+      context "when submitted time is behind client but ahead of UTC" do
+        let(:expires_at) { Time.new(2024, 6, 1, 9, 0, 0, "UTC") } # 12:00 +03:00
+
+        it "succeeds" do
+          sign_in admin
+          post banners_path, params: {banner: banner_params}
+          expect(response).to redirect_to banners_path
+        end
+      end
+
+      context "when submitted time is behind client and behind UTC" do
+        let(:expires_at) { Time.new(2024, 6, 1, 7, 0, 0, "UTC") } # 10:00 +03:00
+
+        it "fails" do
+          sign_in admin
+          post banners_path, params: {banner: banner_params}
+          expect(response).to render_template "banners/new"
+          expect(response.body).to include "Expires at must take place in the future (after 2024-06-01 08:00:00 UTC)"
+        end
+      end
+    end
+
+    context "when client timezone is behind UTC" do
+      before { travel_to Time.new(2024, 6, 1, 11, 0, 0, "-04:00") } # 15:00 UTC
+
+      context "when submitted time is ahead of client and ahead of UTC" do
+        let(:expires_at) { Time.new(2024, 6, 1, 16, 0, 0, "UTC") } # 12:00 -04:00
+
+        it "succeeds" do
+          sign_in admin
+          post banners_path, params: {banner: banner_params}
+          expect(response).to redirect_to banners_path
+        end
+      end
+
+      context "when submitted time is ahead of client but behind UTC" do
+        let(:expires_at) { Time.new(2024, 6, 1, 14, 0, 0, "UTC") } # 10:00 -04:00
+
+        it "fails" do
+          sign_in admin
+          post banners_path, params: {banner: banner_params}
+          expect(response).to render_template "banners/new"
+          expect(response.body).to include "Expires at must take place in the future (after 2024-06-01 15:00:00 UTC)"
+        end
+      end
+    end
+  end
 end
diff --git a/spec/system/banners/new_spec.rb b/spec/system/banners/new_spec.rb
index 2996a01278..4d7cbfbfcd 100644
--- a/spec/system/banners/new_spec.rb
+++ b/spec/system/banners/new_spec.rb
@@ -49,14 +49,28 @@
     within "#banners" do
       click_on "Edit", match: :first
     end
-    fill_in "banner_expires_at", with: 7.days.ago.strftime("%m%d%Y\t%I%M%P")
+    fill_in "banner_expires_at", with: 2.days.from_now.strftime("%m%d%Y\t%I%M%P")
     click_on "Submit"
 
     visit banners_path
     expect(page).to have_text("Expiring Announcement")
 
     visit root_path
-    expect(page).not_to have_text("Please fill out this survey.")
+    expect(page).to have_text("Please fill out this survey.")
+  end
+
+  it "does not allow creation of banner with an expiration time set in the past" do
+    sign_in admin
+
+    visit banners_path
+    click_on "New Banner"
+    fill_in "Name", with: "Announcement not created"
+    fill_in "banner_expires_at", with: 1.week.ago.strftime("%m%d%Y\t%I%M%P")
+    fill_in_rich_text_area "banner_content", with: "Please fill out this survey."
+    click_on "Submit"
+
+    visit banners_path
+    expect(page).to_not have_text("Announcement not created")
   end
 
   describe "when an organization has an active banner" do
diff --git a/spec/views/banners/new.html.erb_spec.rb b/spec/views/banners/new.html.erb_spec.rb
index 332f191cfe..f77738e99b 100644
--- a/spec/views/banners/new.html.erb_spec.rb
+++ b/spec/views/banners/new.html.erb_spec.rb
@@ -9,6 +9,9 @@
 
       allow(view).to receive(:current_user).and_return(user)
       allow(view).to receive(:current_organization).and_return(current_organization)
+      without_partial_double_verification do
+        allow(view).to receive(:browser_time_zone).and_return("America/New_York")
+      end
       allow(current_organization).to receive(:has_alternate_active_banner?).and_return(true)
 
       assign :banners, [current_organization_banner]
@@ -30,6 +33,9 @@
 
         allow(view).to receive(:current_user).and_return(user)
         allow(view).to receive(:current_organization).and_return(current_organization)
+        without_partial_double_verification do
+          allow(view).to receive(:browser_time_zone).and_return("America/New_York")
+        end
         allow(current_organization).to receive(:has_alternate_active_banner?).and_return(true)
 
         assign :banners, [current_organization_banner]
@@ -51,6 +57,9 @@
 
         allow(view).to receive(:current_user).and_return(user)
         allow(view).to receive(:current_organization).and_return(current_organization)
+        without_partial_double_verification do
+          allow(view).to receive(:browser_time_zone).and_return("America/New_York")
+        end
         allow(current_organization).to receive(:has_alternate_active_banner?).and_return(false)
 
         assign :banners, []