Notification duplicate removal should favour the longer notification

[julianlam]

If a notification gets created and pushed to a user with the same uniqueId, it should favour the notification with the longer message text. Right now I believe it just favours the later one.

Also dbl check mentions vs. reply uniqueId:

[a5mith]

Would longer really be the best way to tackle this? In the image, I would rather know someone mentioned me than replied to a topic that I started.

Would it not be possible to give each type of notification a sort of importance rating. So if two notifications are possible, it will just display whichever is higher on that rating.

[julianlam]

An importance rating may work better 👍

[julianlam]

There's a bit of an async bug which explains why duplicates occur.

When multiple notifications are added at the same time, both calls to check whether the notification exists returns false, and they both get added.

Might remove that check and just remove duplicates during the "getNotifications" method.

[preeefix]

Why not have have "iamcardinal replied and mentioned you in: Random Freaking Posts"

[julianlam]

@crdnl One's core, another is a plugin, so it would be a little tougher to have them play nicely 😄

[julianlam]

Duplicate detection is currently achieved through notifications' unique ids, if set.

An idea:

• if duplicate discovered in unread notifs, append to read notifs (no notification alert)

Unfortunately there's no easy way to combine both notification texts into one...

[psychobunny]

@barisusakli didn't you fix something to do with this recently?

[barisusakli]

I changed the user made a new post into user posted in <topic_title>. And made sure two notifications aren't sent sent when person A posts in topic B in case you are following both. The issue here could still happen ie the person you follow posts and mentions you = you get two notifications as in the first screen shot.

[julianlam]

Yes, it does still happen. Right now the follower notification ("julian has posted a reply to themes") is done at the same time as the hook for topic posting, and due to a race condition, both notifications end up making it into the database as the check for notifications returns false.

Easy fix would be to delay the firing of the post hook until after the follower notification has returned. I initially was hesitant to do this as it would be a delay incurred that could possibly be avoided, but it seems to be an acceptable workaround upon second glance.

[barisusakli]

Merged the notification branch, here are some notable changes.

• introduced a new hash uid:<uid>:notifications:uniqueId:nid that maps uniqueIds to notification ids.
• 'uid:' + uid + ':notifications:unread' and 'uid:' + uid + ':notifications:read' no longer store nids instead they store uniqueIds.
• uniqueId needs to be unique per user. ie topic:1768 is not a good uniqueId use topic:1768:uid:<uid> instead. (sent a PR to mentions plugin for this)
• Notifications.get is error first now
• Notifications.create is error first now
• Higher importance overwrites lower importance notifications (it was the other way around)
• Notifications.get no longer takes uid as param Notifications.get = function(nid, callback)
• getUnreadByUniqueId is removed, replaced by getUnreadByField, can pass any notification field instead of just uniqueId ie getUnreadByField(uid, 'uniqueId', uniqueId, callback) or getUnreadByField(uid, 'tid', tid, callback)
• new notification field tid(optional), if the notification is related to a topic it has this field. used for marking topic notifications read.