Merge pull request #1238 from Retrospring/fix/counter-jank

app/controllers/answer_controller.rb

@@ -51,11 +51,12 @@ class AnswerController < ApplicationController
   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

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 @@ class InboxController < ApplicationController
     @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 @@ class InboxController < ApplicationController
   # 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
 

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 @@ class NotificationsController < ApplicationController
     @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 @@ class NotificationsController < ApplicationController
   # 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
 

app/controllers/settings/export_controller.rb

@@ -21,9 +21,12 @@ class Settings::ExportController < ApplicationController
 
   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

app/helpers/bootstrap_helper.rb

@@ -8,6 +8,7 @@ module BootstrapHelper
       badge_attr:  {},
       icon:        nil,
       class:       "",
+      id:          nil,
       hotkey:      nil,
     }.merge(options)
 
@@ -34,7 +35,7 @@ module BootstrapHelper
       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 = {})

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

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)

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 @@ class Notification < ApplicationRecord
 
       n = notification_type.new(target:,
                                 recipient:,
-                                new:       true)
+                                new:       true,)
       n.save!
       n
     end

app/models/subscription.rb

@@ -28,18 +28,26 @@ class Subscription < ApplicationRecord
     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 @@ class Subscription < ApplicationRecord
       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

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

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

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,

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

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

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 @@ describe Ajax::AnswerController, :ajax_controller, type: :controller do
         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 @@ describe Ajax::AnswerController, :ajax_controller, type: :controller do
           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

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 @@ describe Ajax::CommentController, :ajax_controller, type: :controller do
           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
 

spec/controllers/inbox_controller_spec.rb

@@ -65,12 +65,7 @@ describe InboxController, type: :controller do
           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 @@ describe InboxController, type: :controller do
       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

spec/controllers/settings/export_controller_spec.rb

@@ -17,18 +17,20 @@ describe Settings::ExportController, type: :controller do
       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

spec/helpers/bootstrap_helper_spec.rb

@@ -42,6 +42,13 @@ describe BootstrapHelper, :type => :helper do
         eq('<li class="nav-item "><a class="nav-link" href="/example">Example <span class="badge badge-primary badge-pill">3</span></a></li>')
       )
     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("<li class=\"nav-item \" id=\"testing\"><a class=\"nav-link\" href=\"/example\">Example</a></li>"),
+      )
+    end
   end
 
   describe "#list_group_item" do

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