From f47742abf1872442adfb545c4106635f8aed689f Mon Sep 17 00:00:00 2001 From: Albert Chae Date: Fri, 31 May 2024 10:32:24 -0700 Subject: [PATCH 01/13] Add expires_at to banners --- app/models/banner.rb | 1 + db/migrate/20240531172823_add_expires_at_to_banner.rb | 5 +++++ 2 files changed, 6 insertions(+) create mode 100644 db/migrate/20240531172823_add_expires_at_to_banner.rb diff --git a/app/models/banner.rb b/app/models/banner.rb index dc79e398f2..201633a10c 100644 --- a/app/models/banner.rb +++ b/app/models/banner.rb @@ -25,6 +25,7 @@ def only_one_banner_is_active_per_organization # # id :bigint not null, primary key # active :boolean default(FALSE) +# expires_at :datetime # name :string # created_at :datetime not null # updated_at :datetime not null diff --git a/db/migrate/20240531172823_add_expires_at_to_banner.rb b/db/migrate/20240531172823_add_expires_at_to_banner.rb new file mode 100644 index 0000000000..be5ba47f6c --- /dev/null +++ b/db/migrate/20240531172823_add_expires_at_to_banner.rb @@ -0,0 +1,5 @@ +class AddExpiresAtToBanner < ActiveRecord::Migration[7.1] + def change + add_column :banners, :expires_at, :datetime, null: true + end +end From 0ba34fc04b2716c8aad6c5941e4d2bec70be7d92 Mon Sep 17 00:00:00 2001 From: Albert Chae Date: Fri, 31 May 2024 10:32:24 -0700 Subject: [PATCH 02/13] Add expires_at to banners --- db/schema.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index e909844e6b..1d93c713ff 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2024_05_23_101303) do +ActiveRecord::Schema[7.1].define(version: 2024_05_31_172823) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -95,6 +95,7 @@ t.boolean "active", default: false t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.datetime "expires_at" t.index ["casa_org_id"], name: "index_banners_on_casa_org_id" t.index ["user_id"], name: "index_banners_on_user_id" end From 76adc6161af3e99e9afb914fa69621e282505f2e Mon Sep 17 00:00:00 2001 From: Albert Chae Date: Fri, 31 May 2024 12:17:03 -0700 Subject: [PATCH 03/13] Allow setting expires_at in create/update We are choosing to set the timezone in the user's timezone (defined by `cookies[:browser_time_zone]`) by default --- app/controllers/banners_controller.rb | 7 ++++++- app/views/banners/_form.html.erb | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/app/controllers/banners_controller.rb b/app/controllers/banners_controller.rb index 3db5862190..7d50dd6af1 100644 --- a/app/controllers/banners_controller.rb +++ b/app/controllers/banners_controller.rb @@ -65,7 +65,12 @@ def set_banner end def banner_params - params.require(:banner).permit(:active, :content, :name).merge(user: current_user) + params.require(:banner).permit(:active, :content, :name, :expires_at).merge(user: current_user) + .tap { |banner_params| set_expires_at_in_user_time_zone(banner_params) } + end + + def set_expires_at_in_user_time_zone(banner_params) + banner_params[:expires_at] = banner_params[:expires_at].in_time_zone(cookies[:browser_time_zone]) end def deactivate_alternate_active_banner diff --git a/app/views/banners/_form.html.erb b/app/views/banners/_form.html.erb index a826b27d53..cab31e2f0f 100644 --- a/app/views/banners/_form.html.erb +++ b/app/views/banners/_form.html.erb @@ -26,6 +26,10 @@ <%= form.label :active, "Active?", class: 'form-check-label' %> Warning: This will replace your current active banner + + <%= form.label :expires_at, "Expires at (optional)" %> + <%= form.datetime_field :expires_at, required: false %> +
<%= form.label :content %> <%= form.rich_text_area :content %> From e978a512558a7bf503b3c193cb1af520ca699806 Mon Sep 17 00:00:00 2001 From: Albert Chae Date: Fri, 31 May 2024 12:18:23 -0700 Subject: [PATCH 04/13] Use expires_at to check whether a banner should be displayed or not --- app/controllers/application_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 437b4549e9..a297c3fc9d 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -37,7 +37,9 @@ def set_active_banner return nil unless current_organization @active_banner = current_organization.banners.active.first + @active_banner = nil if session[:dismissed_banner] == @active_banner&.id + @active_banner = nil if @active_banner&.expires_at && Time.now.in_time_zone(cookies[:browser_time_zone]) > @active_banner&.expires_at end protected From 3a4e8bc42ee426e66501fde848a646af67767f97 Mon Sep 17 00:00:00 2001 From: Albert Chae Date: Fri, 31 May 2024 12:40:01 -0700 Subject: [PATCH 05/13] add test for showing/hiding banner --- spec/requests/banners_spec.rb | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/spec/requests/banners_spec.rb b/spec/requests/banners_spec.rb index 46364a85ed..a5c3399d98 100644 --- a/spec/requests/banners_spec.rb +++ b/spec/requests/banners_spec.rb @@ -47,4 +47,28 @@ end end 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 } + + it "displays the banner" do + sign_in volunteer + get casa_cases_path + expect(response.body).to include "Please fill out this survey" + end + end + + context "when expires_at is before today" do + let(:expires_at) { 7.days.ago } + + it "does not display the banner" do + sign_in volunteer + get casa_cases_path + expect(response.body).not_to include "Please fill out this survey" + end + end + end end From 457da2f22ce83b051f38ae75cae10d1dbd3149f3 Mon Sep 17 00:00:00 2001 From: Albert Chae Date: Fri, 31 May 2024 12:49:36 -0700 Subject: [PATCH 06/13] Add system test for expiration time --- spec/system/banners/new_spec.rb | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/spec/system/banners/new_spec.rb b/spec/system/banners/new_spec.rb index 73f7da6f17..2996a01278 100644 --- a/spec/system/banners/new_spec.rb +++ b/spec/system/banners/new_spec.rb @@ -31,6 +31,34 @@ expect(page).to have_text("Please fill out this survey.") end + it "lets you create banner with expiration time and edit it" do + sign_in admin + + visit banners_path + click_on "New Banner" + fill_in "Name", with: "Expiring Announcement" + check "Active?" + fill_in "banner_expires_at", with: 7.days.from_now.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 have_text("Expiring Announcement") + + visit banners_path + 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") + 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.") + end + describe "when an organization has an active banner" do let(:admin) { create(:casa_admin) } let(:organization) { create(:casa_org) } From 18748eb94b894bf17ad1daee6ef234d0ac619bdc Mon Sep 17 00:00:00 2001 From: Albert Chae Date: Fri, 31 May 2024 12:53:55 -0700 Subject: [PATCH 07/13] Display expires at on index page --- app/views/banners/index.html.erb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/views/banners/index.html.erb b/app/views/banners/index.html.erb index b0e2edb0f9..658c029148 100644 --- a/app/views/banners/index.html.erb +++ b/app/views/banners/index.html.erb @@ -25,6 +25,7 @@ Name Active? + Expires At Last Updated By Updated At Actions @@ -43,6 +44,13 @@ No <% end %> + + <% if banner.expires_at %> + <%= distance_of_time_in_words(Time.now, banner.expires_at) %> + <% else %> + No + <% end %> + <%= banner.user.display_name %> From eb9f080f939a6f3a6b0ee0e7de6baa00c7070e7a Mon Sep 17 00:00:00 2001 From: Albert Chae Date: Fri, 31 May 2024 13:48:08 -0700 Subject: [PATCH 08/13] Convert time stored in UTC to local timezone for picker form.datetime_field uses html input `datetime-local` under the hood, which does not factor in timezones, so convert it here explicitly --- app/views/banners/_form.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/banners/_form.html.erb b/app/views/banners/_form.html.erb index cab31e2f0f..48224473ce 100644 --- a/app/views/banners/_form.html.erb +++ b/app/views/banners/_form.html.erb @@ -28,7 +28,7 @@
<%= form.label :expires_at, "Expires at (optional)" %> - <%= form.datetime_field :expires_at, required: false %> + <%= form.datetime_field :expires_at, value: banner.expires_at&.in_time_zone(cookies[:browser_time_zone]), required: false %>
<%= form.label :content %> From d308b1ff953354f301a978eaaf68fca4696cdaef Mon Sep 17 00:00:00 2001 From: Albert Chae Date: Sat, 1 Jun 2024 07:37:37 -0700 Subject: [PATCH 09/13] Factor out expired? method onto Banner model --- app/controllers/application_controller.rb | 2 +- app/models/banner.rb | 4 ++++ spec/models/banner_spec.rb | 20 ++++++++++++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index a297c3fc9d..83e8838ea8 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -39,7 +39,7 @@ def set_active_banner @active_banner = current_organization.banners.active.first @active_banner = nil if session[:dismissed_banner] == @active_banner&.id - @active_banner = nil if @active_banner&.expires_at && Time.now.in_time_zone(cookies[:browser_time_zone]) > @active_banner&.expires_at + @active_banner = nil if @active_banner&.expired? end protected diff --git a/app/models/banner.rb b/app/models/banner.rb index 201633a10c..9754665b86 100644 --- a/app/models/banner.rb +++ b/app/models/banner.rb @@ -8,6 +8,10 @@ class Banner < ApplicationRecord validates_presence_of :name validate :only_one_banner_is_active_per_organization + def expired? + expires_at && Time.current > expires_at + end + private def only_one_banner_is_active_per_organization diff --git a/spec/models/banner_spec.rb b/spec/models/banner_spec.rb index 27e0b6df3e..562b36da5d 100644 --- a/spec/models/banner_spec.rb +++ b/spec/models/banner_spec.rb @@ -22,4 +22,24 @@ expect(banner).to be_valid end end + + describe "#expired?" do + it "is false when expires_at is nil" do + banner = create(:banner, expires_at: nil) + + expect(banner).not_to be_expired + end + + it "is false when expires_at is set but is after today" 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) + + expect(banner).to be_expired + end + end end From b3cfd7ac27592f44508b89136789a993cb7bd4cf Mon Sep 17 00:00:00 2001 From: Albert Chae Date: Sat, 1 Jun 2024 09:25:02 -0700 Subject: [PATCH 10/13] Factor out parameter handling for Banners to BannerParameters --- app/controllers/banners_controller.rb | 7 +---- app/values/banner_parameters.rb | 22 ++++++++++++++ spec/values/banner_parameters_spec.rb | 43 +++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 6 deletions(-) create mode 100644 app/values/banner_parameters.rb create mode 100644 spec/values/banner_parameters_spec.rb diff --git a/app/controllers/banners_controller.rb b/app/controllers/banners_controller.rb index 7d50dd6af1..e65c61e588 100644 --- a/app/controllers/banners_controller.rb +++ b/app/controllers/banners_controller.rb @@ -65,12 +65,7 @@ def set_banner end def banner_params - params.require(:banner).permit(:active, :content, :name, :expires_at).merge(user: current_user) - .tap { |banner_params| set_expires_at_in_user_time_zone(banner_params) } - end - - def set_expires_at_in_user_time_zone(banner_params) - banner_params[:expires_at] = banner_params[:expires_at].in_time_zone(cookies[:browser_time_zone]) + BannerParameters.new(params, current_user, cookies[:browser_time_zone]) end def deactivate_alternate_active_banner diff --git a/app/values/banner_parameters.rb b/app/values/banner_parameters.rb new file mode 100644 index 0000000000..b6ffc5a293 --- /dev/null +++ b/app/values/banner_parameters.rb @@ -0,0 +1,22 @@ +# Calculate values when using banner parameters +class BannerParameters < SimpleDelegator + def initialize(params, user, timezone) + new_params = params.require(:banner).permit(:active, :content, :name, :expires_at).merge(user: user) + + if params.dig(:banner, :expires_at) + new_params[:expires_at] = convert_expires_at_in_user_time_zone(params, timezone) + end + + super(new_params) + end + + private + + def convert_expires_at_in_user_time_zone(params, timezone) + params[:banner][:expires_at].in_time_zone(timezone) + end + + def params + __getobj__ + end +end diff --git a/spec/values/banner_parameters_spec.rb b/spec/values/banner_parameters_spec.rb new file mode 100644 index 0000000000..faffec00c5 --- /dev/null +++ b/spec/values/banner_parameters_spec.rb @@ -0,0 +1,43 @@ +require "rails_helper" + +RSpec.describe BannerParameters do + subject { described_class.new(params, user, timezone) } + + let(:params) { + ActionController::Parameters.new( + banner: ActionController::Parameters.new( + active: "1", + content: "content", + name: "name" + ) + ) + } + + let(:user) { create(:user) } + + let(:timezone) { nil } + + it "returns data" do + expect(subject["active"]).to eq("1") + expect(subject["content"]).to eq("content") + expect(subject["name"]).to eq("name") + expect(subject["expires_at"]).to be_blank + expect(subject["user"]).to eq(user) + end + + context "when expires_at is set" do + let(:params) { + ActionController::Parameters.new( + banner: ActionController::Parameters.new( + expires_at: "2024-06-10T12:12" + ) + ) + } + + let(:timezone) { "America/Los_Angeles" } + + it "attaches timezone information to expires_at" do + expect(subject["expires_at"]).to eq("2024-06-10 12:12:00 -0700") + end + end +end From 4fd69ab8381c58a6b2a2553247da5eead365303f Mon Sep 17 00:00:00 2001 From: Albert Chae Date: Sat, 1 Jun 2024 11:33:22 -0700 Subject: [PATCH 11/13] Move timezone conversion for frontend display to model method --- app/models/banner.rb | 4 ++++ app/views/banners/_form.html.erb | 2 +- spec/models/banner_spec.rb | 14 ++++++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/app/models/banner.rb b/app/models/banner.rb index 9754665b86..999ab2af09 100644 --- a/app/models/banner.rb +++ b/app/models/banner.rb @@ -12,6 +12,10 @@ def expired? expires_at && Time.current > expires_at end + def expires_at_in_time_zone(timezone) + expires_at&.in_time_zone(timezone) + end + private def only_one_banner_is_active_per_organization diff --git a/app/views/banners/_form.html.erb b/app/views/banners/_form.html.erb index 48224473ce..25ff047cc0 100644 --- a/app/views/banners/_form.html.erb +++ b/app/views/banners/_form.html.erb @@ -28,7 +28,7 @@
<%= form.label :expires_at, "Expires at (optional)" %> - <%= form.datetime_field :expires_at, value: banner.expires_at&.in_time_zone(cookies[:browser_time_zone]), required: false %> + <%= form.datetime_field :expires_at, value: banner.expires_at_in_time_zone(cookies[:browser_time]), required: false %>
<%= form.label :content %> diff --git a/spec/models/banner_spec.rb b/spec/models/banner_spec.rb index 562b36da5d..fda2a03adc 100644 --- a/spec/models/banner_spec.rb +++ b/spec/models/banner_spec.rb @@ -42,4 +42,18 @@ expect(banner).to be_expired end end + + describe "#expires_at_in_time_zone" do + it "can shift time by timezone for equivalent times" do + 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") + expect(expires_at_in_pacific_time.to_s).to eq("2024-06-13 05:00:00 -0700") + + expires_at_in_eastern_time = banner.expires_at_in_time_zone("America/New_York") + expect(expires_at_in_eastern_time.to_s).to eq("2024-06-13 08:00:00 -0400") + + expect(expires_at_in_pacific_time).to eq(expires_at_in_eastern_time) + end + end end From 8bb7fbece00cb1ae96c1b77cfb6963b9b74da2ac Mon Sep 17 00:00:00 2001 From: Albert Chae Date: Sat, 1 Jun 2024 11:57:35 -0700 Subject: [PATCH 12/13] Helper method for when a banner expires for index page --- app/helpers/banner_helper.rb | 10 ++++++++++ app/views/banners/index.html.erb | 6 +----- spec/helpers/banner_helper_spec.rb | 28 ++++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/app/helpers/banner_helper.rb b/app/helpers/banner_helper.rb index 910088a489..9348953d1a 100644 --- a/app/helpers/banner_helper.rb +++ b/app/helpers/banner_helper.rb @@ -4,4 +4,14 @@ def conditionally_add_hidden_class(current_banner_is_active) "d-none" end end + + def banner_expiration_time_in_words(banner) + if banner.expired? + "Yes" + elsif banner.expires_at + "in #{distance_of_time_in_words(Time.now, banner.expires_at)}" + else + "No" + end + end end diff --git a/app/views/banners/index.html.erb b/app/views/banners/index.html.erb index 658c029148..fd36a60f82 100644 --- a/app/views/banners/index.html.erb +++ b/app/views/banners/index.html.erb @@ -45,11 +45,7 @@ <% end %> - <% if banner.expires_at %> - <%= distance_of_time_in_words(Time.now, banner.expires_at) %> - <% else %> - No - <% end %> + <%= banner_expiration_time_in_words(banner) %> <%= banner.user.display_name %> diff --git a/spec/helpers/banner_helper_spec.rb b/spec/helpers/banner_helper_spec.rb index 48568403db..53469ac358 100644 --- a/spec/helpers/banner_helper_spec.rb +++ b/spec/helpers/banner_helper_spec.rb @@ -35,4 +35,32 @@ expect(helper.conditionally_add_hidden_class(true)).to eq(nil) end end + + describe "#banner_expiration_time_in_words" do + let(:banner) { create(:banner, expires_at: expires_at) } + + 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") + end + end + + context "when expires_at is in the future" do + let(:expires_at) { 7.days.from_now } + + it "returns a word description of how far in the future" do + expect(helper.banner_expiration_time_in_words(banner)).to eq("in 7 days") + end + end + + context "when expires_at is in the past" do + let(:expires_at) { 7.days.ago } + + it "returns yes" do + expect(helper.banner_expiration_time_in_words(banner)).to eq("Yes") + end + end + end end From 599f2e836cf0827c127324fbbf7c03a8ecc891b0 Mon Sep 17 00:00:00 2001 From: Albert Chae Date: Sat, 1 Jun 2024 14:14:43 -0600 Subject: [PATCH 13/13] Add clarifying comments on "with" or "in" time_zone Frontend->Backend: We don't have timezone so we have to attach timezone from browser Backend->Frontend: We have timezone UTC and need to convert it to the browser timezone before displaying --- app/models/banner.rb | 2 ++ app/values/banner_parameters.rb | 7 +++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/models/banner.rb b/app/models/banner.rb index 999ab2af09..155bc69974 100644 --- a/app/models/banner.rb +++ b/app/models/banner.rb @@ -12,6 +12,8 @@ def expired? expires_at && Time.current > expires_at end + # `expires_at` is stored in the database as UTC, but timezone information will be stripped before displaying on frontend + # so this method converts the time to the user's timezone before displaying it def expires_at_in_time_zone(timezone) expires_at&.in_time_zone(timezone) end diff --git a/app/values/banner_parameters.rb b/app/values/banner_parameters.rb index b6ffc5a293..9d8e631a83 100644 --- a/app/values/banner_parameters.rb +++ b/app/values/banner_parameters.rb @@ -4,7 +4,7 @@ def initialize(params, user, timezone) new_params = params.require(:banner).permit(:active, :content, :name, :expires_at).merge(user: user) if params.dig(:banner, :expires_at) - new_params[:expires_at] = convert_expires_at_in_user_time_zone(params, timezone) + new_params[:expires_at] = convert_expires_at_with_user_time_zone(params, timezone) end super(new_params) @@ -12,7 +12,10 @@ def initialize(params, user, timezone) private - def convert_expires_at_in_user_time_zone(params, timezone) + # `expires_at` comes from the frontend without any timezone information, so we use `in_time_zone` to attach + # timezone information to it before saving to the database. If we don't do this, the time will be stored at UTC + # by default. + def convert_expires_at_with_user_time_zone(params, timezone) params[:banner][:expires_at].in_time_zone(timezone) end