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)
This commit is contained in:
Sam 2016-02-15 19:29:35 +11:00
parent 1f062ae2fd
commit dd6ebde824
15 changed files with 203 additions and 86 deletions

View File

@ -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});

View File

@ -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) {

View File

@ -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'),

View File

@ -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;

View File

@ -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);

View File

@ -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;

View File

@ -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

View File

@ -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;
});
}
};

View File

@ -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 => {

View File

@ -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

View File

@ -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!

View File

@ -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?

View File

@ -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
)

View File

@ -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

View File

@ -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)