FIX: Reload only notifications when refreshing notification count (#8221)

FIX: Reload only notifications when refreshing notification count (#8221)

Previously, we used to reload the whole User instance which discarded any changes made (for example setting ‘unstage’ to false).

diff --git a/app/models/notification.rb b/app/models/notification.rb
index 6a4413e7f5..4f300f8138 100644
--- a/app/models/notification.rb
+++ b/app/models/notification.rb
@@ -218,7 +218,8 @@ class Notification < ActiveRecord::Base
 
   def refresh_notification_count
     begin
-      user.reload.publish_notifications_state
+      user.notifications.reset
+      user.publish_notifications_state
     rescue ActiveRecord::RecordNotFound
       # happens when we delete a user
     end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 29dcad458b..8055ad48a0 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1821,7 +1821,7 @@ describe User do
     let!(:staged_user) { Fabricate(:staged, email: 'staged@account.com', active: true, username: 'staged1', name: 'Stage Name') }
     let(:params) { { email: 'staged@account.com', active: true, username: 'unstaged1', name: 'Foo Bar' } }
 
-    it "correctyl unstages a user" do
+    it "correctly unstages a user" do
       user = User.unstage(params)
 
       expect(user.id).to eq(staged_user.id)
@@ -1829,6 +1829,7 @@ describe User do
       expect(user.name).to eq('Foo Bar')
       expect(user.active).to eq(false)
       expect(user.email).to eq('staged@account.com')
+      expect(user.staged).to eq(false)
     end
 
     it "returns nil when the user cannot be unstaged" do
@@ -1838,13 +1839,14 @@ describe User do
     end
 
     it "removes all previous notifications during unstaging" do
-      Fabricate(:notification, user: user)
-      Fabricate(:private_message_notification, user: user)
-      user.reload
+      Fabricate(:notification, user: staged_user)
+      Fabricate(:private_message_notification, user: staged_user)
+      staged_user.reload
 
-      expect(user.total_unread_notifications).to eq(2)
+      expect(staged_user.total_unread_notifications).to eq(2)
       user = User.unstage(params)
       expect(user.total_unread_notifications).to eq(0)
+      expect(user.staged).to eq(false)
     end
 
     it "triggers an event" do

GitHub sha: 3ad07aac

FIX: Load user model when some attributes are missing.

I am sorry, I think we need a followup here.

@jomaxro and myself have been noticing notifications acting weird post this fix. I am not 100% sure this is the issue but something feels astray. Not sure it makes sense for this code to reach in to a leaf object and play with it, user.reset … maybe … but expiring a strategic part of the object when so much is memoized already in the notification pipeline (unread and so on) feels kind of risky.

When I remove the fix the the test cases all continue passing.

3 Likes

This cleans up my main objection here, FIX: notifications are missing under certain conditions but I am still curious why the test was not failing?

Additionally prior to this fix I was able to repro situations on multi browsers where replies were not being notified.

3 Likes

https://review.discourse.org/t/dev-add-test-8960/9084