From c9c458252bd297f6d38a49ddfbd7e7f376b87b41 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Tue, 7 Mar 2023 19:53:57 +0100 Subject: [PATCH 1/6] Add functionality for marking all notifications as read --- app/controllers/notifications_controller.rb | 10 ++++++++++ app/views/navigation/_desktop.html.haml | 19 +++++++++++-------- app/views/navigation/_mobile.html.haml | 2 +- .../dropdown/_notifications.html.haml | 3 +++ .../notifications.turbo_stream.haml | 13 +++++++++++++ config/locales/views.en.yml | 1 + config/routes.rb | 1 + 7 files changed, 40 insertions(+), 9 deletions(-) create mode 100644 app/views/navigation/notifications.turbo_stream.haml diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index 860dd3ec..f1c885f4 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -25,6 +25,16 @@ class NotificationsController < ApplicationController end end + def read + current_user.notifications.where(new: true).update_all(new: false) + + respond_to do |format| + format.turbo_stream do + render "navigation/notifications", locals: { notifications: [], notification_count: nil } + end + end + end + private def paginate_notifications diff --git a/app/views/navigation/_desktop.html.haml b/app/views/navigation/_desktop.html.haml index cde52c1d..d709cf4f 100644 --- a/app/views/navigation/_desktop.html.haml +++ b/app/views/navigation/_desktop.html.haml @@ -19,16 +19,19 @@ %a.nav-link{ href: '#', data: { bs_target: '#modal-list-memberships', bs_toggle: :modal } } %i.fa.fa-list.hidden-xs %span.d-none.d-sm-inline.d-md-none= t(".list") - = nav_entry t("navigation.notifications"), notifications_path, badge: notification_count, class: "d-block d-sm-none", hotkey: "g n" + = nav_entry t("navigation.notifications"), notifications_path, class: "d-block d-sm-none", hotkey: "g n" %li.nav-item.dropdown.d-none.d-sm-block %a.nav-link.dropdown-toggle{ href: '#', data: { bs_toggle: :dropdown } } - - if notification_count.nil? - %i.fa.fa-bell-o - - else - %i.fa.fa-bell - %span.visually-hidden= t("navigation.notifications") - %span.badge= notification_count - = render 'navigation/dropdown/notifications', notifications: notifications, size: "desktop" + %turbo-frame#notification-desktop-icon + - if notification_count.nil? + %i.fa.fa-bell-o + - else + %i.fa.fa-bell + %span.visually-hidden= t("navigation.notifications") + %span.badge= notification_count + .dropdown-menu.dropdown-menu-end.notification-dropdown + %turbo-frame#notifications-dropdown-list + = render 'navigation/dropdown/notifications', notifications:, notification_count: nil %li.nav-item.d-none.d-sm-block{ data: { bs_toggle: 'tooltip', bs_placement: 'bottom' }, title: t('.ask_question') } %a.nav-link{ href: "#", name: "toggle-all-ask", data: { bs_target: "#modal-ask-followers", bs_toggle: :modal, hotkey: "n" } } %i.fa.fa-pencil-square-o diff --git a/app/views/navigation/_mobile.html.haml b/app/views/navigation/_mobile.html.haml index 37023212..97e37ccc 100644 --- a/app/views/navigation/_mobile.html.haml +++ b/app/views/navigation/_mobile.html.haml @@ -9,7 +9,7 @@ - if APP_CONFIG.dig(:features, :discover, :enabled) || current_user.mod? = nav_entry t("navigation.discover"), discover_path, icon: 'compass', icon_only: true = nav_entry t("navigation.notifications"), notifications_path("all"), icon: notifications_icon, - badge: notification_count, badge_color: "primary", icon_only: true + badge: notification_count, badge_color: "primary", badge_attr: { id: "notification-mobile-count" }, icon_only: true %li.nav-item.profile--image-dropdown %a.nav-link{ href: '#', data: { bs_toggle: 'dropdown', bs_target: '#rs-mobile-nav-profile' }, aria: { controls: 'rs-mobile-nav-profile', expanded: 'false' } } %img.avatar-md.d-inline{ src: current_user.profile_picture.url(:small) } diff --git a/app/views/navigation/dropdown/_notifications.html.haml b/app/views/navigation/dropdown/_notifications.html.haml index 25dd3e79..613cafea 100644 --- a/app/views/navigation/dropdown/_notifications.html.haml +++ b/app/views/navigation/dropdown/_notifications.html.haml @@ -10,6 +10,9 @@ %a.dropdown-item.text-center{ href: notifications_path } %i.fa.fa-fw.fa-chevron-right = t(".new") + = link_to notifications_read_path, class: "dropdown-item text-center", data: { turbo_stream: true, turbo_method: :post } do + %i.fa.fa-fw.fa-check-double + = t(".mark_as_read") - notifications.each do |notification| .dropdown-item = render "notifications/type/#{notification.target.class.name.downcase.split('::').last}", notification: notification diff --git a/app/views/navigation/notifications.turbo_stream.haml b/app/views/navigation/notifications.turbo_stream.haml new file mode 100644 index 00000000..1ff5be9f --- /dev/null +++ b/app/views/navigation/notifications.turbo_stream.haml @@ -0,0 +1,13 @@ += turbo_stream.update "notifications-dropdown-list" do + = render "navigation/dropdown/notifications", notifications: + += turbo_stream.update "notification-desktop-icon" do + - if notification_count.nil? + %i.fa.fa-bell-o + - else + %i.fa.fa-bell + %span.visually-hidden= t("navigation.notifications") + %span.badge= notification_count + += turbo_stream.update "notification-mobile-count" do + %span.badge.badge-primary= notification_count diff --git a/config/locales/views.en.yml b/config/locales/views.en.yml index 74b7f51a..20994bce 100644 --- a/config/locales/views.en.yml +++ b/config/locales/views.en.yml @@ -318,6 +318,7 @@ en: none: :notifications.index.none all: "Show all notifications" new: "Show all new notifications" + mark_as_read: "Mark all as read" profile: profile: "Show profile" settings: "Settings" diff --git a/config/routes.rb b/config/routes.rb index 8f485cb7..3af4b3a8 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -143,6 +143,7 @@ Rails.application.routes.draw do get "/list/:list_name", to: "timeline#list", as: :list_timeline get "/notifications(/:type)", to: "notifications#index", as: :notifications, defaults: { type: "new" } + post "/notifications", to: "notifications#read", as: :notifications_read post "/inbox/create", to: "inbox#create", as: :inbox_create get "/inbox", to: "inbox#show", as: :inbox From 80d8bebe5735b37c23810004da917f7d4f6c9e5e Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Fri, 10 Mar 2023 21:08:28 +0100 Subject: [PATCH 2/6] Appease the dog overlords --- app/controllers/notifications_controller.rb | 4 +-- app/views/navigation/_desktop.html.haml | 2 +- .../dropdown/_notifications.html.haml | 35 +++++++++---------- 3 files changed, 20 insertions(+), 21 deletions(-) diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index f1c885f4..8338047f 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -26,12 +26,12 @@ class NotificationsController < ApplicationController end def read - current_user.notifications.where(new: true).update_all(new: false) + current_user.notifications.where(new: true).update_all(new: false) # rubocop:disable Rails/SkipsModelValidations respond_to do |format| format.turbo_stream do render "navigation/notifications", locals: { notifications: [], notification_count: nil } - end + end end end diff --git a/app/views/navigation/_desktop.html.haml b/app/views/navigation/_desktop.html.haml index d709cf4f..93a6a027 100644 --- a/app/views/navigation/_desktop.html.haml +++ b/app/views/navigation/_desktop.html.haml @@ -31,7 +31,7 @@ %span.badge= notification_count .dropdown-menu.dropdown-menu-end.notification-dropdown %turbo-frame#notifications-dropdown-list - = render 'navigation/dropdown/notifications', notifications:, notification_count: nil + = render "navigation/dropdown/notifications", notifications:, notification_count: nil %li.nav-item.d-none.d-sm-block{ data: { bs_toggle: 'tooltip', bs_placement: 'bottom' }, title: t('.ask_question') } %a.nav-link{ href: "#", name: "toggle-all-ask", data: { bs_target: "#modal-ask-followers", bs_toggle: :modal, hotkey: "n" } } %i.fa.fa-pencil-square-o diff --git a/app/views/navigation/dropdown/_notifications.html.haml b/app/views/navigation/dropdown/_notifications.html.haml index 613cafea..b95fbb63 100644 --- a/app/views/navigation/dropdown/_notifications.html.haml +++ b/app/views/navigation/dropdown/_notifications.html.haml @@ -1,19 +1,18 @@ -.dropdown-menu.dropdown-menu-end.notification-dropdown{ id: "rs-#{size}-nav-notifications" } - - if notifications.count.zero? - %a.dropdown-item.text-center{ href: notifications_path('all') } - %i.fa.fa-fw.fa-chevron-right - = t(".all") - .dropdown-item.text-center.p-2 - %i.fa.fa-bell-o.notification__bell-icon - %p= t(".none") - - else - %a.dropdown-item.text-center{ href: notifications_path } - %i.fa.fa-fw.fa-chevron-right - = t(".new") - = link_to notifications_read_path, class: "dropdown-item text-center", data: { turbo_stream: true, turbo_method: :post } do - %i.fa.fa-fw.fa-check-double - = t(".mark_as_read") - - notifications.each do |notification| - .dropdown-item - = render "notifications/type/#{notification.target.class.name.downcase.split('::').last}", notification: notification +- if notifications.count.zero? + %a.dropdown-item.text-center{ href: notifications_path('all') } + %i.fa.fa-fw.fa-chevron-right + = t(".all") + .dropdown-item.text-center.p-2 + %i.fa.fa-bell-o.notification__bell-icon + %p= t(".none") +- else + %a.dropdown-item.text-center{ href: notifications_path } + %i.fa.fa-fw.fa-chevron-right + = t(".new") + = link_to notifications_read_path, class: "dropdown-item text-center", data: { turbo_stream: true, turbo_method: :post } do + %i.fa.fa-fw.fa-check-double + = t(".mark_as_read") + - notifications.each do |notification| + .dropdown-item + = render "notifications/type/#{notification.target.class.name.downcase.split('::').last}", notification: From 0db44949e6c1ed0e89284f8218319e594b4af750 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Fri, 10 Mar 2023 21:16:52 +0100 Subject: [PATCH 3/6] Move `NotificationsController#index` test into their own `describe` block --- .../notifications_controller_spec.rb | 60 ++++++++++--------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/spec/controllers/notifications_controller_spec.rb b/spec/controllers/notifications_controller_spec.rb index bcae9a8d..97ac8a8e 100644 --- a/spec/controllers/notifications_controller_spec.rb +++ b/spec/controllers/notifications_controller_spec.rb @@ -3,39 +3,41 @@ require "rails_helper" describe NotificationsController do - subject { get :index, params: { type: :new } } + describe "#index" do + subject { get :index, params: { type: :new } } - let(:user) { FactoryBot.create(:user) } + let(:user) { FactoryBot.create(:user) } - before do - sign_in(user) - end - - context "user has no notifications" do - it "should show an empty list" do - subject - expect(response).to render_template(:index) - - expect(controller.instance_variable_get(:@notifications)).to be_empty - end - end - - context "user has notifications" do - let(:other_user) { FactoryBot.create(:user) } - let(:another_user) { FactoryBot.create(:user) } - let(:question) { FactoryBot.create(:question, user: user) } - let!(:answer) { FactoryBot.create(:answer, question: question, user: other_user) } - let!(:subscription) { Subscription.create(user: user, answer: answer) } - let!(:comment) { FactoryBot.create(:comment, answer: answer, user: other_user) } - - it "should show a list of notifications" do - subject - expect(response).to render_template(:index) - expect(controller.instance_variable_get(:@notifications)).to have_attributes(size: 2) + before do + sign_in(user) end - it "marks notifications as read" do - expect { subject }.to change { Notification.for(user).where(new: true).count }.from(2).to(0) + context "user has no notifications" do + it "should show an empty list" do + subject + expect(response).to render_template(:index) + + expect(controller.instance_variable_get(:@notifications)).to be_empty + end + end + + context "user has notifications" do + let(:other_user) { FactoryBot.create(:user) } + let(:another_user) { FactoryBot.create(:user) } + let(:question) { FactoryBot.create(:question, user: user) } + let!(:answer) { FactoryBot.create(:answer, question: question, user: other_user) } + let!(:subscription) { Subscription.create(user: user, answer: answer) } + let!(:comment) { FactoryBot.create(:comment, answer: answer, user: other_user) } + + it "should show a list of notifications" do + subject + expect(response).to render_template(:index) + expect(controller.instance_variable_get(:@notifications)).to have_attributes(size: 2) + end + + it "marks notifications as read" do + expect { subject }.to change { Notification.for(user).where(new: true).count }.from(2).to(0) + end end end end From b5a72be2882542b0933382ec0ba003d17fec9d40 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Fri, 10 Mar 2023 21:36:43 +0100 Subject: [PATCH 4/6] Add test for marking all notifications as read --- .../notifications_controller_spec.rb | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/spec/controllers/notifications_controller_spec.rb b/spec/controllers/notifications_controller_spec.rb index 97ac8a8e..448c2997 100644 --- a/spec/controllers/notifications_controller_spec.rb +++ b/spec/controllers/notifications_controller_spec.rb @@ -40,4 +40,25 @@ describe NotificationsController do end end end + + describe "#read" do + subject { post :read, format: :turbo_stream } + + let(:recipient) { FactoryBot.create(:user) } + let(:notification_count) { rand(8..20) } + + context "user has some unread notifications" do + before do + notification_count.times do + FactoryBot.create(:user).follow(recipient) + end + + sign_in(recipient) + end + + it "marks all notifications as read" do + expect { subject }.to change { Notification.where(recipient:, new: true).count }.from(notification_count).to(0) + end + end + end end From a3dbad5265aab0b10ccdb180bef7fcfc2ed1544d Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Fri, 10 Mar 2023 21:38:44 +0100 Subject: [PATCH 5/6] Fix lint errors in `NotificationController#index` tests --- spec/controllers/notifications_controller_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/controllers/notifications_controller_spec.rb b/spec/controllers/notifications_controller_spec.rb index 448c2997..0ba10a12 100644 --- a/spec/controllers/notifications_controller_spec.rb +++ b/spec/controllers/notifications_controller_spec.rb @@ -24,10 +24,10 @@ describe NotificationsController do context "user has notifications" do let(:other_user) { FactoryBot.create(:user) } let(:another_user) { FactoryBot.create(:user) } - let(:question) { FactoryBot.create(:question, user: user) } - let!(:answer) { FactoryBot.create(:answer, question: question, user: other_user) } - let!(:subscription) { Subscription.create(user: user, answer: answer) } - let!(:comment) { FactoryBot.create(:comment, answer: answer, user: other_user) } + let(:question) { FactoryBot.create(:question, user:) } + let!(:answer) { FactoryBot.create(:answer, question:, user: other_user) } + let!(:subscription) { Subscription.create(user:, answer:) } + let!(:comment) { FactoryBot.create(:comment, answer:, user: other_user) } it "should show a list of notifications" do subject From 57ed3008d3c171440ceb575be4f2de204ac4406c Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 19 Mar 2023 14:54:55 +0100 Subject: [PATCH 6/6] Fix "Content missing" on "Show all notifications" --- app/views/navigation/dropdown/_notifications.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/navigation/dropdown/_notifications.html.haml b/app/views/navigation/dropdown/_notifications.html.haml index b95fbb63..94736452 100644 --- a/app/views/navigation/dropdown/_notifications.html.haml +++ b/app/views/navigation/dropdown/_notifications.html.haml @@ -1,12 +1,12 @@ - if notifications.count.zero? - %a.dropdown-item.text-center{ href: notifications_path('all') } + %a.dropdown-item.text-center{ href: notifications_path('all'), data: { turbo_frame: "_top" } } %i.fa.fa-fw.fa-chevron-right = t(".all") .dropdown-item.text-center.p-2 %i.fa.fa-bell-o.notification__bell-icon %p= t(".none") - else - %a.dropdown-item.text-center{ href: notifications_path } + %a.dropdown-item.text-center{ href: notifications_path, data: { turbo_frame: "_top" } } %i.fa.fa-fw.fa-chevron-right = t(".new") = link_to notifications_read_path, class: "dropdown-item text-center", data: { turbo_stream: true, turbo_method: :post } do