From a6f526b9c4e7e6600852bcc5c7c10f3b11b12084 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Fri, 16 Jun 2023 18:07:53 +0200 Subject: [PATCH 01/12] Update inbox/notification counters when paginating --- app/views/inbox/show.turbo_stream.haml | 12 ++++++++++++ app/views/navigation/_desktop.html.haml | 10 +++------- app/views/navigation/_mobile.html.haml | 7 ++++--- app/views/navigation/icons/_notifications.html.haml | 6 ++++++ app/views/notifications/index.turbo_stream.haml | 3 +++ 5 files changed, 28 insertions(+), 10 deletions(-) create mode 100644 app/views/navigation/icons/_notifications.html.haml 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..167ff8305 100644 --- a/app/views/navigation/_desktop.html.haml +++ b/app/views/navigation/_desktop.html.haml @@ -10,7 +10,8 @@ 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-inbox-desktop + = nav_entry t("navigation.inbox"), "/inbox", icon: "inbox", badge: inbox_count, badge_attr: { data: { controller: "pwa-badge" } }, hotkey: "g i" - 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 +24,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..9c53586d6 100644 --- a/app/views/navigation/_mobile.html.haml +++ b/app/views/navigation/_mobile.html.haml @@ -3,9 +3,10 @@ .container %ul.nav.navbar-nav.navbar-icon-row = 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 + #nav-inbox-mobile + = nav_entry t("navigation.inbox"), '/inbox', + badge: inbox_count, badge_color: 'primary', badge_pill: true, + icon: 'inbox', icon_only: true - 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 From 1b05063f4a623c9ab98f54e1ee9ccd95179d26a4 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Fri, 16 Jun 2023 18:09:38 +0200 Subject: [PATCH 02/12] Update tests to check for touching of caching timestamps --- spec/controllers/ajax/answer_controller_spec.rb | 2 ++ spec/controllers/ajax/comment_controller_spec.rb | 15 +++++++++++++++ spec/controllers/inbox_controller_spec.rb | 9 +++------ .../settings/export_controller_spec.rb | 6 ++++-- spec/shared_examples/touches_timestamps.rb | 11 +++++++++++ 5 files changed, 35 insertions(+), 8 deletions(-) create mode 100644 spec/shared_examples/touches_timestamps.rb diff --git a/spec/controllers/ajax/answer_controller_spec.rb b/spec/controllers/ajax/answer_controller_spec.rb index 18c65354a..dd2a1ca6b 100644 --- a/spec/controllers/ajax/answer_controller_spec.rb +++ b/spec/controllers/ajax/answer_controller_spec.rb @@ -26,6 +26,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 diff --git a/spec/controllers/ajax/comment_controller_spec.rb b/spec/controllers/ajax/comment_controller_spec.rb index 0f4189b85..a93fc1263 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,19 @@ 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/shared_examples/touches_timestamps.rb b/spec/shared_examples/touches_timestamps.rb new file mode 100644 index 000000000..339a1b959 --- /dev/null +++ b/spec/shared_examples/touches_timestamps.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +RSpec.shared_examples_for "touches user timestamp" do |timestamp_column| + include ActiveSupport::Testing::TimeHelpers + + it "touches #{timestamp_column}" do + travel_to(1.day.from_now) do + expect { subject }.to change { user.reload.send(timestamp_column) }.to(DateTime.now) + end + end +end From ece64669a11c6bcca825b80b8fe3fdfcf99c98e3 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Fri, 16 Jun 2023 18:18:40 +0200 Subject: [PATCH 03/12] Update caching timestamps in model events --- app/models/inbox.rb | 12 ++++++++++++ app/models/notification.rb | 16 ++++++++++++++-- app/models/subscription.rb | 7 ++++++- 3 files changed, 32 insertions(+), 3 deletions(-) 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..2e8e7b3c2 100644 --- a/app/models/subscription.rb +++ b/app/models/subscription.rb @@ -39,7 +39,12 @@ def notify(source, target) { 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 } 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) From 5a3f65e39abc1c26c36bd92efb84217c9f5a18a6 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Fri, 16 Jun 2023 18:19:31 +0200 Subject: [PATCH 04/12] Ensure counters are up to date when rendering inbox/notifications views --- app/controllers/inbox_controller.rb | 17 ++++++----------- app/controllers/notifications_controller.rb | 7 +++---- 2 files changed, 9 insertions(+), 15 deletions(-) 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 From bcfb215f8c4b7e21e213f101105f035edd71ea72 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Fri, 16 Jun 2023 18:20:21 +0200 Subject: [PATCH 05/12] Ensure caching timestamp is updated when marking notifications as read --- app/controllers/answer_controller.rb | 3 ++- app/controllers/settings/export_controller.rb | 9 ++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) 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/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 From cb89d42813f391eafdf66ae2c3afc7378e7371b8 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Wed, 16 Aug 2023 21:17:18 +0200 Subject: [PATCH 06/12] Put IDs directly on nav entries --- app/helpers/bootstrap_helper.rb | 3 ++- app/views/navigation/_desktop.html.haml | 5 ++--- app/views/navigation/_mobile.html.haml | 7 +++---- 3 files changed, 7 insertions(+), 8 deletions(-) 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/views/navigation/_desktop.html.haml b/app/views/navigation/_desktop.html.haml index 167ff8305..8efb11515 100644 --- a/app/views/navigation/_desktop.html.haml +++ b/app/views/navigation/_desktop.html.haml @@ -1,4 +1,4 @@ -%nav.navbar.navbar-themed.navbar-expand-lg.bg-primary.fixed-top.d-lg-block.d-none.d-print-none{ class: Rails.env.development? ? "navbar-dev" : "", role: :navigation } +%nav.navbar.navbar-themed.navbar-expand-lg.bg-primary.fixed-top.d-lg-block.d-none.d-print-none{ class: Rails.env.tdevelopment? ? "navbar-dev" : "", role: :navigation } .container %a.navbar-brand{ href: '/', title: APP_CONFIG["site_name"] } - if APP_CONFIG["use_svg_logo"] @@ -10,8 +10,7 @@ DEV %ul.nav.navbar-nav.me-auto = nav_entry t("navigation.timeline"), root_path, icon: "home", hotkey: "g t" - #nav-inbox-desktop - = 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 diff --git a/app/views/navigation/_mobile.html.haml b/app/views/navigation/_mobile.html.haml index 9c53586d6..2b68f0811 100644 --- a/app/views/navigation/_mobile.html.haml +++ b/app/views/navigation/_mobile.html.haml @@ -3,10 +3,9 @@ .container %ul.nav.navbar-nav.navbar-icon-row = nav_entry t("navigation.timeline"), root_path, icon: 'home', icon_only: true - #nav-inbox-mobile - = nav_entry t("navigation.inbox"), '/inbox', - badge: inbox_count, badge_color: 'primary', badge_pill: true, - icon: 'inbox', icon_only: true + = nav_entry t("navigation.inbox"), '/inbox', + badge: inbox_count, badge_color: 'primary', badge_pill: 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, From eed4ed5d4bd5147a5df68d4d092e3006ac0ff900 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Wed, 16 Aug 2023 21:19:05 +0200 Subject: [PATCH 07/12] Add test for putting IDs on nav entries --- spec/helpers/bootstrap_helper_spec.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/spec/helpers/bootstrap_helper_spec.rb b/spec/helpers/bootstrap_helper_spec.rb index b996f1627..4cacecd76 100644 --- a/spec/helpers/bootstrap_helper_spec.rb +++ b/spec/helpers/bootstrap_helper_spec.rb @@ -42,6 +42,13 @@ eq('
  • Example 3
    • ') ) end + + it 'should put an ID on the entry an id if given' do + allow(self).to receive(:current_page?).and_return(false) + expect(nav_entry('Example', '/example', id: "testing")).to( + eq('
  • Example
    • ') + ) + end end describe "#list_group_item" do From 54ac832c9195c90393d3ac13758e4bb8bc720460 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Wed, 16 Aug 2023 21:57:31 +0200 Subject: [PATCH 08/12] Ensure inbox caching timestamp gets updated when answering questions and returning to inbox --- app/models/answer.rb | 1 + spec/controllers/ajax/answer_controller_spec.rb | 10 ++++++++++ 2 files changed, 11 insertions(+) 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/spec/controllers/ajax/answer_controller_spec.rb b/spec/controllers/ajax/answer_controller_spec.rb index dd2a1ca6b..3e42c2e73 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 } @@ -327,6 +329,14 @@ 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) + freeze_time do + expect { subject }.to(change { answer.user.reload.inbox_updated_at }.from(initial_timestamp).to(DateTime.now)) + end + end end context "when the answer exists and was not made by the current user" do From a5c58da48c257d2c57cdb8b8d5506f98ad044724 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Thu, 17 Aug 2023 20:54:37 +0200 Subject: [PATCH 09/12] Set time explicitly --- spec/controllers/ajax/answer_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/ajax/answer_controller_spec.rb b/spec/controllers/ajax/answer_controller_spec.rb index 3e42c2e73..1f1632f2a 100644 --- a/spec/controllers/ajax/answer_controller_spec.rb +++ b/spec/controllers/ajax/answer_controller_spec.rb @@ -333,7 +333,7 @@ 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) - freeze_time do + travel_to 1.day.from_now do expect { subject }.to(change { answer.user.reload.inbox_updated_at }.from(initial_timestamp).to(DateTime.now)) end end From efb9b032951115b8fb2e8e84ce4f006509850d6f Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Fri, 18 Aug 2023 18:22:45 +0200 Subject: [PATCH 10/12] Compare time using string representation --- spec/controllers/ajax/answer_controller_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/controllers/ajax/answer_controller_spec.rb b/spec/controllers/ajax/answer_controller_spec.rb index 1f1632f2a..2a8906caa 100644 --- a/spec/controllers/ajax/answer_controller_spec.rb +++ b/spec/controllers/ajax/answer_controller_spec.rb @@ -334,7 +334,8 @@ initial_timestamp = 1.day.ago answer.user.update(inbox_updated_at: initial_timestamp) travel_to 1.day.from_now do - expect { subject }.to(change { answer.user.reload.inbox_updated_at }.from(initial_timestamp).to(DateTime.now)) + # 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 From 620121341e9aa313693298d1d1167927cf1012fd Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Fri, 18 Aug 2023 19:41:21 +0200 Subject: [PATCH 11/12] Fix typo --- app/views/navigation/_desktop.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/navigation/_desktop.html.haml b/app/views/navigation/_desktop.html.haml index 8efb11515..fc0cce386 100644 --- a/app/views/navigation/_desktop.html.haml +++ b/app/views/navigation/_desktop.html.haml @@ -1,4 +1,4 @@ -%nav.navbar.navbar-themed.navbar-expand-lg.bg-primary.fixed-top.d-lg-block.d-none.d-print-none{ class: Rails.env.tdevelopment? ? "navbar-dev" : "", role: :navigation } +%nav.navbar.navbar-themed.navbar-expand-lg.bg-primary.fixed-top.d-lg-block.d-none.d-print-none{ class: Rails.env.development? ? "navbar-dev" : "", role: :navigation } .container %a.navbar-brand{ href: '/', title: APP_CONFIG["site_name"] } - if APP_CONFIG["use_svg_logo"] From d39f37072d76614e603433694375a8abd0d8402e Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Fri, 18 Aug 2023 19:43:59 +0200 Subject: [PATCH 12/12] Fix lint errors --- app/models/subscription.rb | 25 +++++++++++++------ .../ajax/comment_controller_spec.rb | 1 - spec/helpers/bootstrap_helper_spec.rb | 6 ++--- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/app/models/subscription.rb b/app/models/subscription.rb index 2e8e7b3c2..2274fcace 100644 --- a/app/models/subscription.rb +++ b/app/models/subscription.rb @@ -28,15 +28,18 @@ 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 return if notifications.empty? @@ -53,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/spec/controllers/ajax/comment_controller_spec.rb b/spec/controllers/ajax/comment_controller_spec.rb index a93fc1263..ace88ce69 100644 --- a/spec/controllers/ajax/comment_controller_spec.rb +++ b/spec/controllers/ajax/comment_controller_spec.rb @@ -35,7 +35,6 @@ expect { subject }.to change { subscribed_user.reload.notifications_updated_at }.to(DateTime.now) end end - end include_examples "returns the expected response" diff --git a/spec/helpers/bootstrap_helper_spec.rb b/spec/helpers/bootstrap_helper_spec.rb index 4cacecd76..0f31f85ea 100644 --- a/spec/helpers/bootstrap_helper_spec.rb +++ b/spec/helpers/bootstrap_helper_spec.rb @@ -43,10 +43,10 @@ ) end - it 'should put an ID on the entry an id if given' do + it "should put an ID on the entry an id if given" do allow(self).to receive(:current_page?).and_return(false) - expect(nav_entry('Example', '/example', id: "testing")).to( - eq('
  • Example
    • ') + expect(nav_entry("Example", "/example", id: "testing")).to( + eq("
  • Example
    • "), ) end end