diff --git a/app/controllers/answer_controller.rb b/app/controllers/answer_controller.rb index ac52ab40c..2a6e5625c 100644 --- a/app/controllers/answer_controller.rb +++ b/app/controllers/answer_controller.rb @@ -51,11 +51,12 @@ def unpin private def mark_notifications_as_read - Notification.where(recipient_id: current_user.id, new: true) + updated = Notification.where(recipient_id: current_user.id, new: true) .and(Notification.where(type: "Notification::QuestionAnswered", target_id: @answer.id) .or(Notification.where(type: "Notification::Commented", target_id: @answer.comments.pluck(:id))) .or(Notification.where(type: "Notification::Smiled", target_id: @answer.smiles.pluck(:id))) .or(Notification.where(type: "Notification::CommentSmiled", target_id: @answer.comment_smiles.pluck(:id)))) .update_all(new: false) # rubocop:disable Rails/SkipsModelValidations + current_user.touch(:notifications_updated_at) if updated.positive? end end diff --git a/app/controllers/inbox_controller.rb b/app/controllers/inbox_controller.rb index 661d49f4a..7f3db342a 100644 --- a/app/controllers/inbox_controller.rb +++ b/app/controllers/inbox_controller.rb @@ -3,8 +3,6 @@ class InboxController < ApplicationController before_action :authenticate_user! - after_action :mark_inbox_entries_as_read, only: %i[show] - def show # rubocop:disable Metrics/MethodLength find_author find_inbox_entries @@ -19,14 +17,11 @@ def show # rubocop:disable Metrics/MethodLength @delete_id = find_delete_id @disabled = true if @inbox.empty? - respond_to do |format| - format.html { render "show" } - format.turbo_stream do - render "show", layout: false, status: :see_other + mark_inbox_entries_as_read - # rubocop disabled as just flipping a flag doesn't need to have validations to be run - @inbox.update_all(new: false) # rubocop:disable Rails/SkipsModelValidations - end + respond_to do |format| + format.html + format.turbo_stream end end @@ -85,8 +80,8 @@ def filter_author_chain(query) # rubocop:disable Rails/SkipsModelValidations def mark_inbox_entries_as_read # using .dup to not modify @inbox -- useful in tests - @inbox&.dup&.update_all(new: false) - current_user.touch(:inbox_updated_at) + updated = @inbox&.dup&.update_all(new: false) + current_user.touch(:inbox_updated_at) if updated.positive? end # rubocop:enable Rails/SkipsModelValidations diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index b0088074d..a240d3f76 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -3,8 +3,6 @@ class NotificationsController < ApplicationController before_action :authenticate_user! - after_action :mark_notifications_as_read, only: %i[index] - TYPE_MAPPINGS = { "answer" => Notification::QuestionAnswered.name, "comment" => Notification::Commented.name, @@ -18,6 +16,7 @@ def index @notifications = cursored_notifications_for(type: @type, last_id: params[:last_id]) paginate_notifications @counters = count_unread_by_type + mark_notifications_as_read respond_to do |format| format.html @@ -52,8 +51,8 @@ def count_unread_by_type # rubocop:disable Rails/SkipsModelValidations def mark_notifications_as_read # using .dup to not modify @notifications -- useful in tests - @notifications&.dup&.update_all(new: false) - current_user.touch(:notifications_updated_at) + updated = @notifications&.dup&.update_all(new: false) + current_user.touch(:notifications_updated_at) if updated.positive? end # rubocop:enable Rails/SkipsModelValidations diff --git a/app/controllers/settings/export_controller.rb b/app/controllers/settings/export_controller.rb index 1a8bc0a26..3957e427c 100644 --- a/app/controllers/settings/export_controller.rb +++ b/app/controllers/settings/export_controller.rb @@ -21,9 +21,12 @@ def create private + # rubocop:disable Rails/SkipsModelValidations def mark_notifications_as_read - Notification::DataExported - .where(recipient: current_user, new: true) - .update_all(new: false) # rubocop:disable Rails/SkipsModelValidations + updated = Notification::DataExported + .where(recipient: current_user, new: true) + .update_all(new: false) + current_user.touch(:notifications_updated_at) if updated.positive? end + # rubocop:enable Rails/SkipsModelValidations end diff --git a/app/helpers/bootstrap_helper.rb b/app/helpers/bootstrap_helper.rb index 4d14e3dce..555442651 100644 --- a/app/helpers/bootstrap_helper.rb +++ b/app/helpers/bootstrap_helper.rb @@ -8,6 +8,7 @@ def nav_entry(body, path, options = {}) badge_attr: {}, icon: nil, class: "", + id: nil, hotkey: nil, }.merge(options) @@ -34,7 +35,7 @@ def nav_entry(body, path, options = {}) body += " #{content_tag(:span, options[:badge], class: badge_class, **options[:badge_attr])}".html_safe end - content_tag(:li, link_to(body.html_safe, path, class: "nav-link", data: { hotkey: options[:hotkey] }), class: classes) + content_tag(:li, link_to(body.html_safe, path, class: "nav-link", data: { hotkey: options[:hotkey] }), class: classes, id: options[:id]) end def list_group_item(body, path, options = {}) diff --git a/app/models/answer.rb b/app/models/answer.rb index 5e43464b0..71417252e 100644 --- a/app/models/answer.rb +++ b/app/models/answer.rb @@ -20,6 +20,7 @@ class Answer < ApplicationRecord after_create do Inbox.where(user: self.user, question: self.question).destroy_all + user.touch :inbox_updated_at # rubocop:disable Rails/SkipsModelValidations Notification.notify self.question.user, self unless self.question.user == self.user or self.question.user.nil? Subscription.subscribe self.user, self diff --git a/app/models/inbox.rb b/app/models/inbox.rb index 6a74687f4..1a1a00660 100644 --- a/app/models/inbox.rb +++ b/app/models/inbox.rb @@ -13,6 +13,18 @@ class Inbox < ApplicationRecord !user.privacy_allow_anonymous_questions? end + after_create do + user.touch(:inbox_updated_at) + end + + after_update do + user.touch(:inbox_updated_at) + end + + after_destroy do + user.touch(:inbox_updated_at) + end + def answer(answer_content, user) raise Errors::AnsweringOtherBlockedSelf if question.user&.blocking?(user) raise Errors::AnsweringSelfBlockedOther if user.blocking?(question.user) diff --git a/app/models/notification.rb b/app/models/notification.rb index 3e15eae4e..e36cad0aa 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -1,9 +1,21 @@ # frozen_string_literal: true class Notification < ApplicationRecord - belongs_to :recipient, class_name: "User", touch: :notifications_updated_at + belongs_to :recipient, class_name: "User" belongs_to :target, polymorphic: true + after_create do + recipient.touch(:notifications_updated_at) + end + + after_update do + recipient.touch(:notifications_updated_at) + end + + after_destroy do + recipient.touch(:notifications_updated_at) + end + class << self include CursorPaginatable @@ -45,7 +57,7 @@ def make_notification(recipient, target, notification_type) n = notification_type.new(target:, recipient:, - new: true) + new: true,) n.save! n end diff --git a/app/models/subscription.rb b/app/models/subscription.rb index 0b05a1b9e..2274fcace 100644 --- a/app/models/subscription.rb +++ b/app/models/subscription.rb @@ -28,18 +28,26 @@ def destruct(target) def notify(source, target) return nil if source.nil? || target.nil? - muted_by = Relationships::Mute.where(target: source.user).pluck(&:source_id) - # As we will need to notify for each person subscribed, # it's much faster to bulk insert than to use +Notification.notify+ - notifications = Subscription.where(answer: target) - .where.not(user: source.user) - .where.not(user_id: muted_by) - .map do |s| - { target_id: source.id, target_type: Comment, recipient_id: s.user_id, new: true, type: Notification::Commented, created_at: source.created_at, updated_at: source.created_at } + notifications = Subscription.for(source, target).pluck(:user_id).map do |recipient_id| + { + target_id: source.id, + target_type: Comment, + recipient_id:, + new: true, + type: Notification::Commented, + created_at: source.created_at, + updated_at: source.created_at, + } end - Notification.insert_all!(notifications) unless notifications.empty? # rubocop:disable Rails/SkipsModelValidations + return if notifications.empty? + + # rubocop:disable Rails/SkipsModelValidations + Notification.insert_all!(notifications) + User.where(id: notifications.pluck(:recipient_id)).touch_all(:notifications_updated_at) + # rubocop:enable Rails/SkipsModelValidations end def denotify(source, target) @@ -48,5 +56,13 @@ def denotify(source, target) subs = Subscription.where(answer: target) Notification.where(target:, recipient: subs.map(&:user)).delete_all end + + def for(source, target) + muted_by = Relationships::Mute.where(target: source.user).pluck(&:source_id) + + Subscription.where(answer: target) + .where.not(user: source.user) + .where.not(user_id: muted_by) + end end end diff --git a/app/views/inbox/show.turbo_stream.haml b/app/views/inbox/show.turbo_stream.haml index bbd233bd0..e88cfc8d8 100644 --- a/app/views/inbox/show.turbo_stream.haml +++ b/app/views/inbox/show.turbo_stream.haml @@ -1,3 +1,5 @@ +- inbox_count = current_user.unread_inbox_count + = turbo_stream.append "entries" do - @inbox.each do |i| = render "inbox/entry", i: @@ -10,3 +12,13 @@ params: { last_id: @inbox_last_id, author: @author }.compact, data: { controller: :hotkey, hotkey: "." }, form: { data: { turbo_stream: true } } + += turbo_stream.update "nav-inbox-desktop" do + = nav_entry t("navigation.inbox"), "/inbox", + badge: inbox_count, badge_attr: { data: { controller: "pwa-badge" } }, + icon: "inbox", hotkey: "g i" + += turbo_stream.update "nav-inbox-mobile" do + = nav_entry t("navigation.inbox"), "/inbox", + badge: inbox_count, badge_color: "primary", badge_pill: true, + icon: "inbox", icon_only: true diff --git a/app/views/navigation/_desktop.html.haml b/app/views/navigation/_desktop.html.haml index 05e9a6a3c..fc0cce386 100644 --- a/app/views/navigation/_desktop.html.haml +++ b/app/views/navigation/_desktop.html.haml @@ -10,7 +10,7 @@ DEV %ul.nav.navbar-nav.me-auto = nav_entry t("navigation.timeline"), root_path, icon: "home", hotkey: "g t" - = nav_entry t("navigation.inbox"), "/inbox", icon: "inbox", badge: inbox_count, badge_attr: { data: { controller: "pwa-badge" } }, hotkey: "g i" + = nav_entry t("navigation.inbox"), "/inbox", icon: "inbox", badge: inbox_count, badge_attr: { data: { controller: "pwa-badge" } }, hotkey: "g i", id: "nav-inbox-desktop" - if APP_CONFIG.dig(:features, :discover, :enabled) || current_user.mod? = nav_entry t("navigation.discover"), discover_path, icon: "compass", hotkey: "g d" %ul.nav.navbar-nav @@ -23,12 +23,7 @@ %li.nav-item.dropdown.d-none.d-sm-block %a.nav-link.dropdown-toggle{ href: '#', data: { bs_toggle: :dropdown } } %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 + = render "navigation/icons/notifications", notification_count: .dropdown-menu.dropdown-menu-end.notification-dropdown %turbo-frame#notifications-dropdown-list - cache current_user.notification_dropdown_cache_key do diff --git a/app/views/navigation/_mobile.html.haml b/app/views/navigation/_mobile.html.haml index 97e37ccc6..2b68f0811 100644 --- a/app/views/navigation/_mobile.html.haml +++ b/app/views/navigation/_mobile.html.haml @@ -5,7 +5,7 @@ = nav_entry t("navigation.timeline"), root_path, icon: 'home', icon_only: true = nav_entry t("navigation.inbox"), '/inbox', badge: inbox_count, badge_color: 'primary', badge_pill: true, - icon: 'inbox', icon_only: true + icon: 'inbox', icon_only: true, id: "nav-inbox-mobile" - 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, diff --git a/app/views/navigation/icons/_notifications.html.haml b/app/views/navigation/icons/_notifications.html.haml new file mode 100644 index 000000000..767ee9801 --- /dev/null +++ b/app/views/navigation/icons/_notifications.html.haml @@ -0,0 +1,6 @@ +- if notification_count.nil? + %i.fa.fa-bell-o +- else + %i.fa.fa-bell +%span.visually-hidden= t("navigation.notifications") +%span.badge= notification_count diff --git a/app/views/notifications/index.turbo_stream.haml b/app/views/notifications/index.turbo_stream.haml index 4339faae3..378a9a445 100644 --- a/app/views/notifications/index.turbo_stream.haml +++ b/app/views/notifications/index.turbo_stream.haml @@ -13,3 +13,6 @@ params: { last_id: @notifications_last_id }, data: { controller: :hotkey, hotkey: "." }, form: { data: { turbo_stream: true } } + += turbo_stream.update "notification-desktop-icon" do + = render "navigation/icons/notifications", notification_count: current_user.unread_notification_count diff --git a/spec/controllers/ajax/answer_controller_spec.rb b/spec/controllers/ajax/answer_controller_spec.rb index 18c65354a..2a8906caa 100644 --- a/spec/controllers/ajax/answer_controller_spec.rb +++ b/spec/controllers/ajax/answer_controller_spec.rb @@ -4,6 +4,8 @@ require "rails_helper" describe Ajax::AnswerController, :ajax_controller, type: :controller do + include ActiveSupport::Testing::TimeHelpers + let(:question) { FactoryBot.create(:question, user: FactoryBot.build(:user, privacy_allow_stranger_answers: asker_allows_strangers)) } let(:asker_allows_strangers) { true } @@ -26,6 +28,8 @@ end include_examples "returns the expected response" + + include_examples "touches user timestamp", :inbox_updated_at end shared_examples "does not create the answer" do @@ -325,6 +329,15 @@ user.save expect { subject }.to(change { Inbox.where(question_id: answer.question.id, user_id: user.id).count }.by(1)) end + + it "updates the inbox caching timestamp for the user who answered" do + initial_timestamp = 1.day.ago + answer.user.update(inbox_updated_at: initial_timestamp) + travel_to 1.day.from_now do + # using string representation to avoid precision issues + expect { subject }.to(change { answer.user.reload.inbox_updated_at.to_s }.from(initial_timestamp.to_s).to(DateTime.now)) + end + end end context "when the answer exists and was not made by the current user" do diff --git a/spec/controllers/ajax/comment_controller_spec.rb b/spec/controllers/ajax/comment_controller_spec.rb index 0f4189b85..ace88ce69 100644 --- a/spec/controllers/ajax/comment_controller_spec.rb +++ b/spec/controllers/ajax/comment_controller_spec.rb @@ -4,6 +4,8 @@ require "rails_helper" describe Ajax::CommentController, :ajax_controller, type: :controller do + include ActiveSupport::Testing::TimeHelpers + let(:answer) { FactoryBot.create(:answer, user: FactoryBot.create(:user)) } describe "#create" do @@ -23,6 +25,18 @@ expect(answer.reload.comments.ids).to include(Comment.last.id) end + context "a user is subscribed to the answer" do + let(:subscribed_user) { FactoryBot.create(:user) } + + it "updates the notification caching timestamp for a subscribed user" do + Subscription.subscribe(subscribed_user, answer) + + travel_to(1.day.from_now) do + expect { subject }.to change { subscribed_user.reload.notifications_updated_at }.to(DateTime.now) + end + end + end + include_examples "returns the expected response" end diff --git a/spec/controllers/inbox_controller_spec.rb b/spec/controllers/inbox_controller_spec.rb index 94a8accff..a6e5917cc 100644 --- a/spec/controllers/inbox_controller_spec.rb +++ b/spec/controllers/inbox_controller_spec.rb @@ -65,12 +65,7 @@ expect { subject }.to change { inbox_entry.reload.new? }.from(true).to(false) end - it "updates the the timestamp used for caching" do - user.update(inbox_updated_at: original_inbox_updated_at) - travel 1.second do - expect { subject }.to change { user.reload.inbox_updated_at.floor }.from(original_inbox_updated_at.floor).to(Time.now.utc.floor) - end - end + include_examples "touches user timestamp", :inbox_updated_at context "when requested the turbo stream format" do subject { get :show, format: :turbo_stream } @@ -280,6 +275,8 @@ it "creates an inbox entry" do expect { subject }.to(change { user.inboxes.count }.by(1)) end + + include_examples "touches user timestamp", :inbox_updated_at end end end diff --git a/spec/controllers/settings/export_controller_spec.rb b/spec/controllers/settings/export_controller_spec.rb index 0750ed6e2..0bc87796d 100644 --- a/spec/controllers/settings/export_controller_spec.rb +++ b/spec/controllers/settings/export_controller_spec.rb @@ -17,18 +17,20 @@ end context "when user has a new DataExported notification" do - let(:notification) do + let!(:notification) do Notification::DataExported.create( target_id: user.id, target_type: "User::DataExport", recipient: user, - new: true + new: true, ) end it "marks the notification as read" do expect { subject }.to change { notification.reload.new }.from(true).to(false) end + + include_examples "touches user timestamp", :notifications_updated_at end end end diff --git a/spec/helpers/bootstrap_helper_spec.rb b/spec/helpers/bootstrap_helper_spec.rb index b996f1627..0f31f85ea 100644 --- a/spec/helpers/bootstrap_helper_spec.rb +++ b/spec/helpers/bootstrap_helper_spec.rb @@ -42,6 +42,13 @@ eq('