From f10f87cc68618a502edc50c407913ca71d6065b5 Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <gxtan1990@gmail.com>
Date: Thu, 25 Jun 2020 10:14:07 +0800
Subject: [PATCH] FIX: Avoid marking notifications as seen in readonly mode.

---
 app/controllers/notifications_controller.rb   |  5 +++--
 .../requests/notifications_controller_spec.rb | 20 +++++++++++++------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb
index 94779fc0a1f..a928b493bc5 100644
--- a/app/controllers/notifications_controller.rb
+++ b/app/controllers/notifications_controller.rb
@@ -25,11 +25,12 @@ class NotificationsController < ApplicationController
       notifications = Notification.recent_report(current_user, limit)
       changed = false
 
-      if notifications.present?
+      if notifications.present? && !(params.has_key?(:slient) || @readonly_mode)
         # ordering can be off due to PMs
         max_id = notifications.map(&:id).max
-        changed = current_user.saw_notification_id(max_id) unless params.has_key?(:silent)
+        changed = current_user.saw_notification_id(max_id)
       end
+
       user.reload
       user.publish_notifications_state if changed
 
diff --git a/spec/requests/notifications_controller_spec.rb b/spec/requests/notifications_controller_spec.rb
index bb3c9022a34..4270afb728c 100644
--- a/spec/requests/notifications_controller_spec.rb
+++ b/spec/requests/notifications_controller_spec.rb
@@ -33,7 +33,8 @@ end
 describe NotificationsController do
   context 'when logged in' do
     context 'as normal user' do
-      let!(:user) { sign_in(Fabricate(:user)) }
+      fab!(:user) { sign_in(Fabricate(:user)) }
+      fab!(:notification) { Fabricate(:notification, user: user) }
 
       describe '#index' do
         it 'should succeed for recent' do
@@ -47,7 +48,6 @@ describe NotificationsController do
         end
 
         it 'should mark notifications as viewed' do
-          Fabricate(:notification, user: user)
           expect(user.reload.unread_notifications).to eq(1)
           expect(user.reload.total_unread_notifications).to eq(1)
           get "/notifications.json", params: { recent: true }
@@ -56,7 +56,6 @@ describe NotificationsController do
         end
 
         it 'should not mark notifications as viewed if silent param is present' do
-          Fabricate(:notification, user: user)
           expect(user.reload.unread_notifications).to eq(1)
           expect(user.reload.total_unread_notifications).to eq(1)
           get "/notifications", params: { recent: true, silent: true }
@@ -64,6 +63,17 @@ describe NotificationsController do
           expect(user.reload.total_unread_notifications).to eq(1)
         end
 
+        it 'should not mark notifications as viewed in readonly mode' do
+          Discourse.received_redis_readonly!
+          expect(user.reload.unread_notifications).to eq(1)
+          expect(user.reload.total_unread_notifications).to eq(1)
+          get "/notifications", params: { recent: true, silent: true }
+          expect(user.reload.unread_notifications).to eq(1)
+          expect(user.reload.total_unread_notifications).to eq(1)
+        ensure
+          Discourse.clear_redis_readonly!
+        end
+
         context 'when username params is not valid' do
           it 'should raise the right error' do
             get "/notifications.json", params: { username: 'somedude' }
@@ -78,7 +88,6 @@ describe NotificationsController do
       end
 
       it "can update a single notification" do
-        notification = Fabricate(:notification, user: user)
         notification2 = Fabricate(:notification, user: user)
         put "/notifications/mark-read.json", params: { id: notification.id }
         expect(response.status).to eq(200)
@@ -91,7 +100,6 @@ describe NotificationsController do
       end
 
       it "updates the `read` status" do
-        Fabricate(:notification, user: user)
         expect(user.reload.unread_notifications).to eq(1)
         expect(user.reload.total_unread_notifications).to eq(1)
         put "/notifications/mark-read.json"
@@ -120,7 +128,7 @@ describe NotificationsController do
     end
 
     context 'as admin' do
-      let!(:admin) { sign_in(Fabricate(:admin)) }
+      fab!(:admin) { sign_in(Fabricate(:admin)) }
 
       describe '#create' do
         it "can create notification" do