From dd6ebde8247d849c1d429d5807c545f09091128d Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 15 Feb 2016 19:29:35 +1100 Subject: [PATCH] FIX: Always ensure notifications are treated as read once clicked UX: improve messaging so notifications list is far more stable PERF: improve performance of notifcation lookup queries - Add feature "SetTransientHeader" that allows shipping info to server in the next Ajax request - remove local storage hack used for notifications - amend lookupStale to return hydrated objects, move logic into store - stop magically clearing various notifications (likes, invitee accepted, group_summary, granted badge) --- .../discourse/adapters/notification.js.es6 | 3 +- .../discourse/adapters/rest.js.es6 | 24 +++++++++-- .../components/notification-item.js.es6 | 4 ++ .../discourse/components/user-menu.js.es6 | 2 +- .../subscribe-user-notifications.js.es6 | 30 +++++++++---- .../discourse/lib/stale-result.js.es6 | 12 ------ .../javascripts/discourse/mixins/ajax.js | 10 +++++ .../mixins/stale-local-storage.js.es6 | 34 --------------- .../javascripts/discourse/models/store.js.es6 | 38 +++++++++++++--- app/assets/javascripts/main_include.js | 1 - app/controllers/application_controller.rb | 23 ++++++++++ app/models/notification.rb | 30 +++++++++---- app/models/user.rb | 43 ++++++++++++++----- ...28_add_unread_pm_index_to_notifications.rb | 6 +++ .../application_controller_spec.rb | 29 +++++++++++++ 15 files changed, 203 insertions(+), 86 deletions(-) delete mode 100644 app/assets/javascripts/discourse/lib/stale-result.js.es6 delete mode 100644 app/assets/javascripts/discourse/mixins/stale-local-storage.js.es6 create mode 100644 db/migrate/20160215075528_add_unread_pm_index_to_notifications.rb diff --git a/app/assets/javascripts/discourse/adapters/notification.js.es6 b/app/assets/javascripts/discourse/adapters/notification.js.es6 index f8298a4307a..ddee883f629 100644 --- a/app/assets/javascripts/discourse/adapters/notification.js.es6 +++ b/app/assets/javascripts/discourse/adapters/notification.js.es6 @@ -1,4 +1,3 @@ import RestAdapter from 'discourse/adapters/rest'; -import StaleLocalStorage from 'discourse/mixins/stale-local-storage'; -export default RestAdapter.extend(StaleLocalStorage); +export default RestAdapter.extend({cache: true}); diff --git a/app/assets/javascripts/discourse/adapters/rest.js.es6 b/app/assets/javascripts/discourse/adapters/rest.js.es6 index 1404e76d507..5faaf6d0bfa 100644 --- a/app/assets/javascripts/discourse/adapters/rest.js.es6 +++ b/app/assets/javascripts/discourse/adapters/rest.js.es6 @@ -1,6 +1,8 @@ -import StaleResult from 'discourse/lib/stale-result'; +import { hashString } from 'discourse/lib/hash'; + const ADMIN_MODELS = ['plugin', 'site-customization', 'embeddable-host']; + export function Result(payload, responseJson) { this.payload = payload; this.responseJson = responseJson; @@ -19,6 +21,15 @@ function rethrow(error) { export default Ember.Object.extend({ + + storageKey(type, findArgs, options) { + if (options && options.cacheKey) { + return options.cacheKey; + } + const hashedArgs = Math.abs(hashString(JSON.stringify(findArgs))); + return `${type}_${hashedArgs}`; + }, + basePath(store, type) { if (ADMIN_MODELS.indexOf(type.replace('_', '-')) !== -1) { return "/admin/"; } return "/"; @@ -56,8 +67,15 @@ export default Ember.Object.extend({ return ajax(this.pathFor(store, type, findArgs)).catch(rethrow); }, - findStale() { - return new StaleResult(); + findStale(store, type, findArgs, options) { + if (this.cached) { + return this.cached[this.storageKey(type, findArgs, options)]; + } + }, + + cacheFind(store, type, findArgs, opts, hydrated) { + this.cached = this.cached || {}; + this.cached[this.storageKey(type,findArgs,opts)] = hydrated; }, update(store, type, id, attrs) { diff --git a/app/assets/javascripts/discourse/components/notification-item.js.es6 b/app/assets/javascripts/discourse/components/notification-item.js.es6 index 6818ba5a3d7..f900c1b2744 100644 --- a/app/assets/javascripts/discourse/components/notification-item.js.es6 +++ b/app/assets/javascripts/discourse/components/notification-item.js.es6 @@ -61,6 +61,10 @@ export default Ember.Component.extend({ _markRead: function(){ this.$('a').click(() => { this.set('notification.read', true); + Discourse.setTransientHeader("Discourse-Clear-Notifications", this.get('notification.id')); + if (document && document.cookie) { + document.cookie = `cn=${this.get('notification.id')}; expires=Fri, 31 Dec 9999 23:59:59 GMT`; + } return true; }); }.on('didInsertElement'), diff --git a/app/assets/javascripts/discourse/components/user-menu.js.es6 b/app/assets/javascripts/discourse/components/user-menu.js.es6 index 506fc98d781..d864f23ba49 100644 --- a/app/assets/javascripts/discourse/components/user-menu.js.es6 +++ b/app/assets/javascripts/discourse/components/user-menu.js.es6 @@ -54,7 +54,7 @@ export default Ember.Component.extend({ // TODO: It's a bit odd to use the store in a component, but this one really // wants to reach out and grab notifications const store = this.container.lookup('store:main'); - const stale = store.findStale('notification', {recent: true, limit }, {storageKey: 'recent-notifications'}); + const stale = store.findStale('notification', {recent: true, limit }, {cacheKey: 'recent-notifications'}); if (stale.hasResults) { const results = stale.results; diff --git a/app/assets/javascripts/discourse/initializers/subscribe-user-notifications.js.es6 b/app/assets/javascripts/discourse/initializers/subscribe-user-notifications.js.es6 index 79157a123a6..5923dfca878 100644 --- a/app/assets/javascripts/discourse/initializers/subscribe-user-notifications.js.es6 +++ b/app/assets/javascripts/discourse/initializers/subscribe-user-notifications.js.es6 @@ -9,10 +9,11 @@ export default { site = container.lookup('site:main'), siteSettings = container.lookup('site-settings:main'), bus = container.lookup('message-bus:main'), - keyValueStore = container.lookup('key-value-store:main'); + keyValueStore = container.lookup('key-value-store:main'), + store = container.lookup('store:main'); - // clear old cached notifications - // they could be a week old for all we know + // clear old cached notifications, we used to store in local storage + // TODO 2017 delete this line keyValueStore.remove('recent-notifications'); if (user) { @@ -40,12 +41,12 @@ export default { user.set('lastNotificationChange', new Date()); } - var stale = keyValueStore.getObject('recent-notifications'); + const stale = store.findStale('notification', {}, {cacheKey: 'recent-notifications'}); const lastNotification = data.last_notification && data.last_notification.notification; - if (stale && stale.notifications && lastNotification) { + if (stale && stale.hasResults && lastNotification) { - const oldNotifications = stale.notifications; + const oldNotifications = stale.results.get('content'); const staleIndex = _.findIndex(oldNotifications, {id: lastNotification.id}); if (staleIndex > -1) { @@ -61,8 +62,21 @@ export default { insertPosition = insertPosition === -1 ? oldNotifications.length - 1 : insertPosition; } - oldNotifications.splice(insertPosition, 0, lastNotification); - keyValueStore.setItem('recent-notifications', JSON.stringify(stale)); + oldNotifications.splice(insertPosition, 0, Em.Object.create(lastNotification)); + + var idx=0; + data.recent.forEach((info)=> { + var old = oldNotifications[idx]; + if (old) { + if (old.get('id') !== info[0]) { + oldNotifications.splice(idx, 1); + return; + } else if (old.get('read') !== info[1]) { + old.set('read', info[1]); + } + } + idx += 1; + }); } }, user.notification_channel_position); diff --git a/app/assets/javascripts/discourse/lib/stale-result.js.es6 b/app/assets/javascripts/discourse/lib/stale-result.js.es6 deleted file mode 100644 index 1ae29e58327..00000000000 --- a/app/assets/javascripts/discourse/lib/stale-result.js.es6 +++ /dev/null @@ -1,12 +0,0 @@ -const StaleResult = function() { - this.hasResults = false; -}; - -StaleResult.prototype.setResults = function(results) { - if (results) { - this.results = results; - this.hasResults = true; - } -}; - -export default StaleResult; diff --git a/app/assets/javascripts/discourse/mixins/ajax.js b/app/assets/javascripts/discourse/mixins/ajax.js index fdd58cf2fea..36e42f51225 100644 --- a/app/assets/javascripts/discourse/mixins/ajax.js +++ b/app/assets/javascripts/discourse/mixins/ajax.js @@ -3,9 +3,14 @@ respect Discourse paths and the run loop. **/ var _trackView = false; +var _transientHeader = null; Discourse.Ajax = Em.Mixin.create({ + setTransientHeader: function(k, v) { + _transientHeader = {key: k, value: v}; + }, + viewTrackingRequired: function() { _trackView = true; }, @@ -43,6 +48,11 @@ Discourse.Ajax = Em.Mixin.create({ args.headers = args.headers || {}; + if (_transientHeader) { + args.headers[_transientHeader.key] = _transientHeader.value; + _transientHeader = null; + } + if (_trackView && (!args.type || args.type === "GET")) { _trackView = false; // DON'T CHANGE: rack is prepending "HTTP_" in the header's name diff --git a/app/assets/javascripts/discourse/mixins/stale-local-storage.js.es6 b/app/assets/javascripts/discourse/mixins/stale-local-storage.js.es6 deleted file mode 100644 index 19d84951627..00000000000 --- a/app/assets/javascripts/discourse/mixins/stale-local-storage.js.es6 +++ /dev/null @@ -1,34 +0,0 @@ -import StaleResult from 'discourse/lib/stale-result'; -import { hashString } from 'discourse/lib/hash'; - -// Mix this in to an adapter to provide stale caching in our key value store -export default { - storageKey(type, findArgs) { - const hashedArgs = Math.abs(hashString(JSON.stringify(findArgs))); - return `${type}_${hashedArgs}`; - }, - - findStale(store, type, findArgs, opts) { - const staleResult = new StaleResult(); - const key = (opts && opts.storageKey) || this.storageKey(type, findArgs); - try { - const stored = this.keyValueStore.getItem(key); - if (stored) { - const parsed = JSON.parse(stored); - staleResult.setResults(parsed); - } - } catch(e) { - // JSON parsing error - } - return staleResult; - }, - - find(store, type, findArgs, opts) { - const key = (opts && opts.storageKey) || this.storageKey(type, findArgs); - - return this._super(store, type, findArgs).then((results) => { - this.keyValueStore.setItem(key, JSON.stringify(results)); - return results; - }); - } -}; diff --git a/app/assets/javascripts/discourse/models/store.js.es6 b/app/assets/javascripts/discourse/models/store.js.es6 index a7f4edcf9be..9c66da96ddd 100644 --- a/app/assets/javascripts/discourse/models/store.js.es6 +++ b/app/assets/javascripts/discourse/models/store.js.es6 @@ -73,19 +73,43 @@ export default Ember.Object.extend({ // refresh it in the background. findStale(type, findArgs, opts) { const stale = this.adapterFor(type).findStale(this, type, findArgs, opts); - if (stale.hasResults) { - stale.results = this._hydrateFindResults(stale.results, type, findArgs); - } - stale.refresh = () => this.find(type, findArgs, opts); - return stale; + return { + hasResults: (stale !== undefined), + results: stale, + refresh: () => this.find(type, findArgs, opts) + }; }, find(type, findArgs, opts) { - return this.adapterFor(type).find(this, type, findArgs, opts).then(result => { - return this._hydrateFindResults(result, type, findArgs, opts); + var adapter = this.adapterFor(type); + return adapter.find(this, type, findArgs, opts).then(result => { + var hydrated = this._hydrateFindResults(result, type, findArgs, opts); + if (adapter.cache) { + const stale = adapter.findStale(this, type, findArgs, opts); + hydrated = this._updateStale(stale, hydrated); + adapter.cacheFind(this, type, findArgs, opts, hydrated); + } + return hydrated; }); }, + _updateStale(stale, hydrated) { + if (!stale) { + return hydrated; + } + + hydrated.set('content', hydrated.get('content').map((item) => { + var staleItem = stale.content.findBy('id', item.get('id')); + if (staleItem) { + staleItem.setProperties(item); + } else { + staleItem = item; + } + return staleItem; + })); + return hydrated; + }, + refreshResults(resultSet, type, url) { const self = this; return Discourse.ajax(url).then(result => { diff --git a/app/assets/javascripts/main_include.js b/app/assets/javascripts/main_include.js index 43379cc09c9..15c4cb0747f 100644 --- a/app/assets/javascripts/main_include.js +++ b/app/assets/javascripts/main_include.js @@ -7,7 +7,6 @@ //= require ./ember-addons/macro-alias //= require ./ember-addons/ember-computed-decorators //= require ./discourse/lib/hash -//= require ./discourse/lib/stale-result //= require ./discourse/lib/load-script //= require ./discourse/lib/notification-levels //= require ./discourse/lib/app-events diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 05f850e30d9..6dd3f81ade2 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -33,6 +33,7 @@ class ApplicationController < ActionController::Base end before_filter :set_current_user_for_logs + before_filter :clear_notifications before_filter :set_locale before_filter :set_mobile_view before_filter :inject_preview_style @@ -137,6 +138,28 @@ class ApplicationController < ActionController::Base response.headers["X-Discourse-Route"] = "#{controller_name}/#{action_name}" end + def clear_notifications + if current_user && !Discourse.readonly_mode? + + cookie_notifications = cookies['cn'.freeze] + notifications = request.headers['Discourse-Clear-Notifications'.freeze] + + if cookie_notifications + if notifications.present? + notifications += "," << cookie_notifications + else + notifications = cookie_notifications + end + end + + if notifications.present? + notification_ids = notifications.split(",").map(&:to_i) + Notification.where(user_id: current_user.id, id: notification_ids).update_all(read: true) + cookies.delete('cn') + end + end + end + def set_locale I18n.locale = current_user.try(:effective_locale) || SiteSetting.default_locale I18n.ensure_all_loaded! diff --git a/app/models/notification.rb b/app/models/notification.rb index bffb70dce15..d21875c8b2c 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -130,15 +130,29 @@ class Notification < ActiveRecord::Base .to_a if notifications.present? - notifications += user - .notifications - .order('notifications.created_at DESC') - .where(read: false, notification_type: Notification.types[:private_message]) - .joins(:topic) - .where('notifications.id < ?', notifications.last.id) - .limit(count) - notifications.sort do |x,y| + ids = Notification.exec_sql(" + SELECT n.id FROM notifications n + WHERE + n.notification_type = 6 AND + n.user_id = #{user.id.to_i} AND + NOT read + ORDER BY n.id ASC + LIMIT #{count.to_i} + ").values.map do |x,_| + x.to_i + end + + if ids.length > 0 + notifications += user + .notifications + .order('notifications.created_at DESC') + .where(id: ids) + .joins(:topic) + .limit(count) + end + + notifications.uniq(&:id).sort do |x,y| if x.unread_pm? && !y.unread_pm? -1 elsif y.unread_pm? && !x.unread_pm? diff --git a/app/models/user.rb b/app/models/user.rb index 639846ee265..a1be4c16b97 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -293,15 +293,6 @@ class User < ActiveRecord::Base def saw_notification_id(notification_id) User.where("id = ? and seen_notification_id < ?", id, notification_id) .update_all ["seen_notification_id = ?", notification_id] - - # some notifications are considered read once seen - Notification.where('user_id = ? AND NOT read AND notification_type IN (?)', id, [ - Notification.types[:granted_badge], - Notification.types[:invitee_accepted], - Notification.types[:group_message_summary], - Notification.types[:liked] - ]) - .update_all ["read = ?", true] end def publish_notifications_state @@ -310,11 +301,43 @@ class User < ActiveRecord::Base notification = notifications.visible.order('notifications.id desc').first json = NotificationSerializer.new(notification).as_json if notification + + sql = " + SELECT * FROM ( + SELECT n.id, n.read FROM notifications n + LEFT JOIN topics t ON n.topic_id = t.id + WHERE + t.deleted_at IS NULL AND + n.notification_type = :type AND + n.user_id = :user_id AND + NOT read + ORDER BY n.id DESC + LIMIT 20 + ) AS x + UNION ALL + SELECT * FROM ( + SELECT n.id, n.read FROM notifications n + LEFT JOIN topics t ON n.topic_id = t.id + WHERE + t.deleted_at IS NULL AND + (n.notification_type <> :type OR read) AND + n.user_id = :user_id + ORDER BY n.id DESC + LIMIT 20 + ) AS y + " + + recent = User.exec_sql(sql, user_id: id, + type: Notification.types[:private_message]).values.map do |id, read| + [id.to_i, read == 't'.freeze] + end + MessageBus.publish("/notification/#{id}", {unread_notifications: unread_notifications, unread_private_messages: unread_private_messages, total_unread_notifications: total_unread_notifications, - last_notification: json + last_notification: json, + recent: recent }, user_ids: [id] # only publish the notification to this user ) diff --git a/db/migrate/20160215075528_add_unread_pm_index_to_notifications.rb b/db/migrate/20160215075528_add_unread_pm_index_to_notifications.rb new file mode 100644 index 00000000000..382489e2fda --- /dev/null +++ b/db/migrate/20160215075528_add_unread_pm_index_to_notifications.rb @@ -0,0 +1,6 @@ +class AddUnreadPmIndexToNotifications < ActiveRecord::Migration + def change + # create index idxtmp on notifications(user_id, id) where notification_type = 6 AND NOT read + add_index :notifications, [:user_id, :id], unique: true, where: 'notification_type = 6 AND NOT read' + end +end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index fc1911efd1b..48a217039c8 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -85,6 +85,35 @@ describe TopicsController do end + describe 'clear_notifications' do + it 'correctly clears notifications if specified via cookie' do + notification = Fabricate(:notification) + log_in_user(notification.user) + + request.cookies['cn'] = "2828,100,#{notification.id}" + + get :show, {topic_id: 100} + + expect(response.cookies['cn']).to eq nil + + notification.reload + expect(notification.read).to eq true + + end + + it 'correctly clears notifications if specified via header' do + notification = Fabricate(:notification) + log_in_user(notification.user) + + request.headers['Discourse-Clear-Notifications'] = "2828,100,#{notification.id}" + + get :show, {topic_id: 100} + + notification.reload + expect(notification.read).to eq true + end + end + describe 'set_locale' do it 'sets the one the user prefers' do SiteSetting.stubs(:allow_user_locale).returns(true)