From 39f80ae6089dbf906922edf19771b982b85d1c50 Mon Sep 17 00:00:00 2001 From: Sam Williams Date: Sat, 6 Jan 2024 10:01:41 -0500 Subject: [PATCH] 5428 Banner Dismissed Forever Bug fix --- app/controllers/application_controller.rb | 8 +++ app/controllers/banners_controller.rb | 16 ++++-- .../controllers/dismiss_controller.js | 13 +++-- app/views/layouts/_banner.html.erb | 20 +++----- config/routes.rb | 6 ++- spec/requests/banners_spec.rb | 50 +++++++++++++++++++ spec/system/banners/dismiss_spec.rb | 19 +++++++ 7 files changed, 111 insertions(+), 21 deletions(-) create mode 100644 spec/requests/banners_spec.rb create mode 100644 spec/system/banners/dismiss_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 90ddbd6cd1..8c1d1b29c6 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -9,6 +9,7 @@ class ApplicationController < ActionController::Base before_action :set_current_user before_action :set_timeout_duration before_action :set_current_organization + before_action :set_active_banner after_action :verify_authorized, except: :index, unless: :devise_controller? # after_action :verify_policy_scoped, only: :index @@ -30,6 +31,13 @@ def after_sign_out_path_for(resource_or_scope) end end + 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 + end + protected def handle_short_url(url_list) diff --git a/app/controllers/banners_controller.rb b/app/controllers/banners_controller.rb index b858a5829b..3db5862190 100644 --- a/app/controllers/banners_controller.rb +++ b/app/controllers/banners_controller.rb @@ -1,5 +1,6 @@ class BannersController < ApplicationController - after_action :verify_authorized + after_action :verify_authorized, except: %i[dismiss] + before_action :set_banner, only: %i[edit update destroy dismiss] def index authorize :application, :admin_or_supervisor? @@ -15,8 +16,11 @@ def new def edit authorize :application, :admin_or_supervisor? + end - @banner = current_organization.banners.find(params[:id]) + def dismiss + session[:dismissed_banner] = @banner.id + render json: {status: :ok} end def create @@ -37,8 +41,6 @@ def create def update authorize :application, :admin_or_supervisor? - @banner = current_organization.banners.find(params[:id]) - Banner.transaction do deactivate_alternate_active_banner @banner.update!(banner_params) @@ -52,12 +54,16 @@ def update def destroy authorize :application, :admin_or_supervisor? - current_organization.banners.find(params[:id]).destroy + @banner.destroy redirect_to banners_path end private + def set_banner + @banner = current_organization.banners.find(params[:id]) + end + def banner_params params.require(:banner).permit(:active, :content, :name).merge(user: current_user) end diff --git a/app/javascript/controllers/dismiss_controller.js b/app/javascript/controllers/dismiss_controller.js index 41a8fb411c..fecbf7353e 100644 --- a/app/javascript/controllers/dismiss_controller.js +++ b/app/javascript/controllers/dismiss_controller.js @@ -2,12 +2,19 @@ import { Controller } from '@hotwired/stimulus' export default class extends Controller { static targets = ['element'] + static values = { + url: String + } dismiss (event) { event.preventDefault() - const { id } = event.params - document.cookie = `dismiss_${id}=true` - this.elementTarget.classList.add('d-none') + fetch(this.urlValue) + .then(response => response.json()) + .then(data => { + if (data.status === 'ok') { + this.elementTarget.classList.add('d-none') + } + }) } } diff --git a/app/views/layouts/_banner.html.erb b/app/views/layouts/_banner.html.erb index 60ed469007..f1814ebae5 100644 --- a/app/views/layouts/_banner.html.erb +++ b/app/views/layouts/_banner.html.erb @@ -1,14 +1,10 @@ -<% banner = current_organization.banners.active.first %> -<% if banner %> - <% cookie_id = "banner_#{banner.id}" %> - <% if !cookies["dismiss_#{cookie_id}"] %> -
-
- <%= banner.content %> -
-
- Dismiss -
+<% if @active_banner %> +
+
+ <%= @active_banner.content %>
- <% end %> +
+ Dismiss +
+
<% end %> diff --git a/config/routes.rb b/config/routes.rb index 6df505f0dd..cbb775a6a5 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -116,7 +116,11 @@ resources :learning_hour_topics, only: %i[new create edit update] resources :followup_reports, only: :index resources :placement_reports, only: :index - resources :banners, only: %i[index new edit create update destroy] + resources :banners, except: %i[show] do + member do + get :dismiss + end + end resources :bulk_court_dates, only: %i[new create] resources :case_groups, only: %i[index new edit create update destroy] resources :learning_hours, only: %i[index show new create edit update destroy] diff --git a/spec/requests/banners_spec.rb b/spec/requests/banners_spec.rb new file mode 100644 index 0000000000..46364a85ed --- /dev/null +++ b/spec/requests/banners_spec.rb @@ -0,0 +1,50 @@ +require "rails_helper" + +RSpec.describe "Banners", type: :request do + let!(:casa_org) { create(:casa_org) } + let!(:active_banner) { create(:banner, casa_org: casa_org) } + let(:volunteer) { create(:volunteer, casa_org: casa_org) } + + context "when user dismisses a banner" do + subject do + get dismiss_banner_path(active_banner) + end + + it "sets session variable" do + sign_in volunteer + subject + expect(session[:dismissed_banner]).to eq active_banner.id + end + + it "does not display banner on page reloads" do + sign_in volunteer + get casa_cases_path + expect(response.body).to include "Please fill out this survey" + + subject + get casa_cases_path + expect(response.body).not_to include "Please fill out this survey" + end + + context "when user logs out and back in" do + it "nils out session variable" do + sign_in volunteer + subject + get destroy_user_session_path + sign_in volunteer + + expect(session[:dismissed_banner]).to be_nil + end + + it "displays banner" do + sign_in volunteer + subject + get destroy_user_session_path + sign_in volunteer + + get casa_cases_path + expect(response.body).to include "Please fill out this survey" + end + end + end +end diff --git a/spec/system/banners/dismiss_spec.rb b/spec/system/banners/dismiss_spec.rb new file mode 100644 index 0000000000..d8efd83b78 --- /dev/null +++ b/spec/system/banners/dismiss_spec.rb @@ -0,0 +1,19 @@ +require "rails_helper" + +RSpec.describe "banners/dismiss", type: :system, js: true do + let!(:casa_org) { create(:casa_org) } + let!(:active_banner) { create(:banner, casa_org: casa_org) } + let(:volunteer) { create(:volunteer, casa_org: casa_org) } + + context "when user dismisses a banner" do + it "hides banner" do + sign_in volunteer + + visit root_path + expect(page).to have_text("Please fill out this survey") + + click_on "Dismiss" + expect(page).not_to have_text("Please fill out this survey") + end + end +end