Improve banner expiration validations and form

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

app/models/banner.rb

@@ -6,10 +6,13 @@ class Banner < ApplicationRecord
   scope :active, -> { where(active: true) }
 
   validates_presence_of :name
+  validates_presence_of :content
   validate :only_one_banner_is_active_per_organization
+  validates_comparison_of :expires_at, greater_than: Time.current, message: "must take place in the future (after %{value} )", allow_blank: true
 
   def expired?
-    expires_at && Time.current > expires_at
+    expired = expires_at && Time.current > expires_at
+    update(active: false) if active && expired
   end
 
   # `expires_at` is stored in the database as UTC, but timezone information will be stripped before displaying on frontend

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(cookies[:browser_time_zone]),
+                              required: false,
+                              include_seconds: false,
+                              min: Time.current.in_time_zone(cookies[:browser_time_zone]) %>
     </span>
     <div class="input-style-1">
       <%= form.label :content %>

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>

spec/helpers/banner_helper_spec.rb

@@ -43,7 +43,7 @@
       let(:expires_at) { nil }
 
       it "returns No" do
-        expect(helper.banner_expiration_time_in_words(banner)).to eq("No")
+        expect(helper.banner_expiration_time_in_words(banner)).to eq("No Expiration")
       end
     end
 
@@ -56,10 +56,12 @@
     end
 
     context "when expires_at is in the past" do
-      let(:expires_at) { 7.days.ago }
+      let(:expires_at) { 1.day.from_now }
 
       it "returns yes" do
-        expect(helper.banner_expiration_time_in_words(banner)).to eq("Yes")
+        banner
+        travel 2.days
+        expect(helper.banner_expiration_time_in_words(banner)).to eq("Expired")
       end
     end
   end

spec/models/banner_spec.rb

@@ -2,25 +2,39 @@
 
 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)
       expect(banner).to_not be_valid
     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)
       another_supervisor = create(:supervisor, casa_org: another_org)
       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,17 +44,25 @@
       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: 1.hour.from_now)
+      travel 2.hours
       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

spec/requests/banners_spec.rb

@@ -62,9 +62,10 @@
     end
 
     context "when expires_at is before today" do
-      let(:expires_at) { 7.days.ago }
+      let(:expires_at) { 2.days.from_now }
 
       it "does not display the banner" do
+        travel 3.days
         sign_in volunteer
         get casa_cases_path
         expect(response.body).not_to include "Please fill out this survey"

spec/system/banners/new_spec.rb

@@ -49,14 +49,29 @@
     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: "Annoucement"
+    fill_in "banner_expires_at", with: 1.hour.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"
+
+    message = page.find("#banner_expires_at").native.attribute("validationMessage")
+    expect(message).to start_with("Value must be")
+    # Can't get an exact match for the date/time since cookie "browser_time_zone" isn't set in the test Chrome session
   end
 
   describe "when an organization has an active banner" do