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 @@
<%= 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) %>
<%= 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 @@
Name |
Active? |
- Expires At |
+ Expiration |
Last Updated By |
Updated At |
Actions |
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, []