Add expiration to banners #5800

albertchae · 2024-05-31T19:58:27Z

What github issue is this PR for, if any?

Resolves #5604

What changed, and why?

Add an optional expiration time to banners.
The timezone will be set to the creator's current timezone.
If a banner is active, but the time is past the expires_at, we will hide the banner.
If the banner is not active, we hide the banner regardless (existing behavior).
Also added displaying the banner on the banners index page

How is this tested? (please write tests!) 💖💪

Note: if you see a flake in your test build in github actions, please post in slack #casa "Flaky test: " :) 💪
Note: We love capybara tests! If you are writing both haml/js and ruby, please try to test your work with tests at every level including system tests like https://github.com/rubyforgood/casa/tree/main/spec/system

Added tests for showing/hiding banner based on expires_at. Also added tests for setting expires_at in the banner new/edit form.

Screenshots please :)

Run your local server and take a screenshot of your work! Try to include the URL of the page as well as the contents of the page.

Feelings gif (optional)

What gif best describes your feeling working on this issue? https://giphy.com/
How to embed:

elasticspoon

LGTM. Not sure how you feel about potentially adding a test to validate cookie time zone behavior.

Also a note for future me

it may make more sense to have a checkbox for the expiration that dictates whether than field should be shown at all.

elasticspoon · 2024-06-01T02:46:50Z

spec/requests/banners_spec.rb

Do you think it makes sense to add a test verify the time zone stuff? Something along the lines of:
Set banner => visit page, it shows => change cookies timezone => visit again and confirm that is it gone?

Set banner => visit page, it shows => change cookies timezone => visit again and confirm that is it gone?

Hmm I was thinking of doing something similar but I don't think the scenario you describe would happen since the expiration time is a fixed point in time, so changing timezone would only affect how it gets displayed. So I did try to test this scenario

Timezone in PT, set banner to N => visit banner edit and see N:00 => change cookies timezone to ET => visit banner edit and see N+3

But was not sure how to do this in capybara/selenium. This is what I tried without success

it "displays expiration time in your current time zone" do Capybara.using_session("America/Los_Angeles") do sign_in admin visit banners_path click_on "New Banner" fill_in "Name", with: "name" fill_in "banner_expires_at", with: "06082024\t0710pm" click_on "Submit" visit banners_path expect(page).to have_text("name") visit banners_path within "#banners" do click_on "Edit", match: :first end expect(page).to have_field("banner_expires_at", with: "2024-06-08T19:10") end Capybara.using_session("America/New_York") do sign_in admin visit banners_path within "#banners" do click_on "Edit", match: :first end expect(page).to have_field("banner_expires_at", with: "2024-06-08T22:10") end end

schoork · 2024-06-01T10:33:57Z

app/controllers/application_controller.rb

    @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


Let's extract this to a single method and then test against it.

extracted it to Banner#expired and added a test. Also switched to Time.current per https://thoughtbot.com/blog/its-about-time-zones.

I also realized that we don't need the timezone conversion here because Time.current/Time.now are computed in the backend and > is timezone aware, so I removed that as well.

schoork · 2024-06-01T10:34:11Z

app/controllers/banners_controller.rb

+    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) }


This is doing two things. Can we split the call to conversion out of the banner_params method? You might take a look at the CaseContactParameters class.

Moved this out to a BannerParameters class

schoork · 2024-06-01T10:34:19Z

app/controllers/banners_controller.rb

+  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])


Is this method testable? Can we make it testable and then write some tests for it? Maybe this should be a method on the model.

I believe the tests for BannerParameters cover this now. I debated putting it on the model originally but formatting parameters from a request seems like a controller responsibility, so I think having it in the controller/params object makes sense but let me know if you think otherwise

schoork · 2024-06-01T10:36:09Z

app/views/banners/_form.html.erb

@@ -26,6 +26,10 @@
      <%= form.label :active, "Active?", class: 'form-check-label' %>
      <span data-reveal-target="item" class="text-danger <%= conditionally_add_hidden_class(form.object.active) %>">Warning: This will replace your current active banner</span>
    </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_zone]), required: false %>


This logic should be in a view helper (and tested).

banner.expires_at&.in_time_zone(cookies[:browser_time_zone])

You might take a look at datetime_local_field. It might eliminate the need for the logic.

Unfortunately I don't think datetime_local_field helps us here. According to your link, it wraps ActionView::Helpers::FormHelper#datetime_local_field which in turn states that it's only an alias for datetime_field. According to https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/datetime-local#setting_timezones, timezone aware logic was intentionally left out of this element.

That said, I did try to move this logic to BannerHelper but ultimately put it in the model. It kind of feels like it would fit best in a presenter type object but might be overkill for now? Any thoughts?

Either model or presenter is fine. I agree presenter makes more sense, but if it's just one method does feel like overkill.

schoork · 2024-06-01T10:36:47Z

app/views/banners/index.html.erb

+                  <% if banner.expires_at %>
+                    <%= distance_of_time_in_words(Time.now, banner.expires_at) %>
+                  <% else %>
+                    No
+                  <% end %>


A great candidate for a method in the model or view helper (with tests).

Moved this to BannerHelper which also helped me realize I was missing the case for when the expiration had passed, so I fixed that too

spec/requests/banners_spec.rb

We are choosing to set the timezone in the user's timezone (defined by `cookies[:browser_time_zone]`) by default

form.datetime_field uses html input `datetime-local` under the hood, which does not factor in timezones, so convert it here explicitly

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

compwron

nice

github-actions bot added ruby Pull requests that update Ruby code Tests! 🎉💖👏 erb labels May 31, 2024

albertchae force-pushed the 5604-banner-expiration branch 2 times, most recently from 10835fb to 2138b30 Compare May 31, 2024 21:00

albertchae marked this pull request as ready for review May 31, 2024 21:09

albertchae requested review from compwron, FireLemons, littleforest and elasticspoon as code owners May 31, 2024 21:09

elasticspoon approved these changes Jun 1, 2024

View reviewed changes

schoork requested changes Jun 1, 2024

View reviewed changes

albertchae force-pushed the 5604-banner-expiration branch 2 times, most recently from ef26ccb to 2619a57 Compare June 1, 2024 20:03

albertchae added 13 commits June 1, 2024 14:26

Add expires_at to banners

f47742a

Add expires_at to banners

0ba34fc

Allow setting expires_at in create/update

76adc61

We are choosing to set the timezone in the user's timezone (defined by `cookies[:browser_time_zone]`) by default

Use expires_at to check whether a banner should be displayed or not

e978a51

add test for showing/hiding banner

3a4e8bc

Add system test for expiration time

457da2f

Display expires at on index page

18748eb

Convert time stored in UTC to local timezone for picker

eb9f080

form.datetime_field uses html input `datetime-local` under the hood, which does not factor in timezones, so convert it here explicitly

Factor out expired? method onto Banner model

d308b1f

Factor out parameter handling for Banners to BannerParameters

b3cfd7a

Move timezone conversion for frontend display to model method

4fd69ab

Helper method for when a banner expires for index page

8bb7fbe

Add clarifying comments on "with" or "in" time_zone

599f2e8

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

albertchae force-pushed the 5604-banner-expiration branch from d77fa38 to 599f2e8 Compare June 1, 2024 20:26

compwron approved these changes Jun 1, 2024

View reviewed changes

compwron merged commit 45444b7 into main Jun 1, 2024
18 of 20 checks passed

compwron deleted the 5604-banner-expiration branch June 1, 2024 23:47

elasticspoon mentioned this pull request Jun 5, 2024

Improve banner expiration validations and form #5815

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add expiration to banners #5800

Add expiration to banners #5800

albertchae commented May 31, 2024 •

edited

Loading

elasticspoon left a comment

elasticspoon Jun 1, 2024

albertchae Jun 1, 2024

schoork Jun 1, 2024

albertchae Jun 1, 2024

schoork Jun 1, 2024

albertchae Jun 1, 2024

schoork Jun 1, 2024

albertchae Jun 1, 2024

schoork Jun 1, 2024

albertchae Jun 1, 2024

schoork Jun 1, 2024

schoork Jun 1, 2024

albertchae Jun 1, 2024

compwron left a comment

		@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

		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) }

Add expiration to banners #5800

Add expiration to banners #5800

Conversation

albertchae commented May 31, 2024 • edited Loading

What github issue is this PR for, if any?

What changed, and why?

How is this tested? (please write tests!) 💖💪

Screenshots please :)

Feelings gif (optional)

elasticspoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compwron left a comment

Choose a reason for hiding this comment

albertchae commented May 31, 2024 •

edited

Loading