Add collection for notification #6952

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@AugierLe42e
Contributor

This supersedes #6493, solves #5780 and partly solves #3890.

@denschub denschub referenced this pull request Aug 11, 2016
Closed

Fetch notification counts #6493

0 of 4 tasks complete
@AugierLe42e AugierLe42e and 1 other commented on an outdated diff Aug 15, 2016
app/assets/javascripts/app/views/header_view.js
@@ -12,12 +12,18 @@ app.views.Header = app.views.Base.extend({
});
},
- postRenderTemplate: function(){
- new app.views.Notifications({ el: "#notification-dropdown" });
- this.notificationDropdown = new app.views.NotificationDropdown({ el: "#notification-dropdown" });
- new app.views.Search({ el: "#header-search-form" });
+ postRenderTemplate: function() {
+ if (!app.collections.Notifications.instance) {
+ app.collections.Notifications.instance = new app.collections.Notifications();
+ }
@AugierLe42e
AugierLe42e Aug 15, 2016 Contributor

I'd like an advice on this. The collection needs to be shared with the notification view of the /notifications page so I create a class instance but I'm not really satisfied with this solution.

@svbergerem
svbergerem Sep 10, 2016 Member
setupHeader: function() {
  if(app.currentUser.authenticated()) {
    app.notificationsCollection = new app.collections.Notifications();
    app.header = new app.views.Header();
    $("header").prepend(app.header.el);
    app.header.render();
  }
},

in app/assets/javascripts/app/app.js?

@AugierLe42e
AugierLe42e Sep 11, 2016 Contributor

Ah, yes! It's nicer. Thx.

@AugierLe42e AugierLe42e changed the title from [WIP] Add collection for notification to [Add collection for notification Sep 19, 2016
@AugierLe42e
Contributor
AugierLe42e commented Sep 19, 2016 edited

Ready for review. I added an option for browser notification when there are unread diaspora notifications.

@AugierLe42e AugierLe42e changed the title from [Add collection for notification to Add collection for notification Sep 19, 2016
@AugierLe42e
Contributor

Ping.

+ unreadCountByType: {},
+
+ initialize: function() {
+ this.pollNotifications();
@svbergerem
svbergerem Nov 11, 2016 Member

Maybe

setInterval(this.pollNotifications.bind(this), 30000)

instead?

@svbergerem
svbergerem Nov 11, 2016 Member

I'd also increase the time between two fetches to a minute or even 5 minutes. This feature increases the load for servers with a lot of users and I'd like to keep the additional load small. We can go back to 30 seconds in a later PR when this has been tested on bigger pods.

@denschub It would be great if you could add your opinion here.

@svbergerem
svbergerem Nov 11, 2016 Member

In combination with the browser notification stuff I wrote a few lines below the new code could be

this.fetch();
this.timeout = 300000 // 5 minutes
setTimeout(setInterval(this.pollNotifications.bind(this), this.timeout).bind(this), this.timeout);
Diaspora.BrowserNotification.requestPermission();
@AugierLe42e
AugierLe42e Nov 13, 2016 Contributor

It has been in production on diaspote for 3 months now with no problem. Maybe @Flaburgan can merge it on d-fr before it is merged on develop to see what happens?

@Flaburgan
Flaburgan Nov 13, 2016 Contributor

I can, even if d-fr is not a big pod like frama or jd so I would tend to pick the careful way here and choose 1 or 2 minutes at the moment.

+ this.fetch();
+ if (unreadCountBefore < this.unreadCount) {
+ Diaspora.BrowserNotification.spawnNotification(Diaspora.I18n.t("notifications.new_notifications"));
+ }
@svbergerem
svbergerem Nov 11, 2016 Member

At least while testing this feature it didn't work because the check unreadCountBefore < this.unreadCount was done before fetch finished. You can fix this by listening to the finishedLoading event:

   pollNotifications: function() {
     var unreadCountBefore = this.unreadCount;
+    this.once("finishedLoading", function() {
+      if (unreadCountBefore < this.unreadCount) {
+        Diaspora.BrowserNotification.spawnNotification(Diaspora.I18n.t("notifications.new_notifications"));
+      }
+    }, this);
     this.fetch();
-    if (unreadCountBefore < this.unreadCount) {
-      Diaspora.BrowserNotification.spawnNotification(Diaspora.I18n.t("notifications.new_notifications"));
-    }

This would also show a browser notification immediately after loading the page when there are unread notifications. To fix this call this.fetch() in initialize and call pollNotifications the first time after some timeout.

+ if (unreadCountBefore < this.unreadCount) {
+ Diaspora.BrowserNotification.spawnNotification(Diaspora.I18n.t("notifications.new_notifications"));
+ }
+ setTimeout(this.pollNotifications.bind(this), 30000);
@svbergerem
svbergerem Nov 11, 2016 Member

Remove this line when using setInterval.

+
+ /**
+ * Add new models to the collection at the end or at the beginning of the collection and
+ * then fires an event for each model of the collection. It will fire a different event
@svbergerem
svbergerem Nov 11, 2016 Member

Either

Adds new models to the collection at the end or at the beginning of the collection and then fires an event for each model of the collection.

or

Add new models to the collection at the end or at the beginning of the collection and then fire an event for each model of the collection.

+ var accu = function(model) { models.push(model); };
+ this.on("add", accu);
+ Backbone.Collection.prototype.set.apply(this, [items, options]);
+ this.off("add", accu);
@svbergerem
svbergerem Nov 11, 2016 Member

http://backbonejs.org/#Collection-set

Returns the touched models in the collection.

Have you already tried

var models = Backbone.Collection.prototype.set.apply(this, [items, options]);

?

@AugierLe42e
AugierLe42e Nov 13, 2016 Contributor

Hmm... Nope, I didn't. Thanks for highlighting that.

@AugierLe42e
AugierLe42e Nov 13, 2016 edited Contributor

Ok, it doesn't work. It also return models that haven't been newly added and creates duplications in the dropdown.

+ /* eslint-disable new-cap */
+ var model = new this.model(item);
+ /* eslint-enable new-cap */
+ model.on("change:unread", this.onChagedUnreadStatus.bind(this));
@svbergerem
svbergerem Nov 11, 2016 Member

this.onChangedUnreadStatus.bind(this)

+ this.find(function(model) { return model.getGUID() === guid; }).setUnread();
+ },
+
+ onChagedUnreadStatus: function(model) {
@svbergerem
svbergerem Nov 11, 2016 Member

onChangedUnreadStatus

+ constructor: function(attributes, options) {
+ options = options || {};
+ options.parse = true;
+ Backbone.Model.prototype.constructor.apply(this, [attributes, options]);
@svbergerem
svbergerem Nov 11, 2016 Member

http://backbonejs.org/#Model-constructor
Looks to me like

Backbone.Model.apply(this, [attributes, options]);

would be sufficient.

+
+ getGUID: function() {
+ return this.get("id");
+ },
@svbergerem
svbergerem Nov 11, 2016 Member

I'd prefer setting

this.guid = this.get("id");

in the constructor and using this.guid instead of this.getGUID().

- [allNotes, typeNotes].forEach(function(badge) {
- if(badge.text() > 0) {
+ var updateBadge = function(badge, count) {
@svbergerem
svbergerem Nov 11, 2016 Member

Why not make this function a “normal” view function?

@AugierLe42e
AugierLe42e Nov 13, 2016 Contributor

What do you mean?

@svbergerem
svbergerem Nov 13, 2016 Member

You assigned the function to a variable inside oft a function. I suggested to make this a function of the view.

One might want to use the function later for something else and one would be able to test the function.

+ updateBadge(notificationsContainer.find("a[data-type=" + notificationType + "] .badge"), count);
+ }.bind(this));
+
+ updateBadge(notificationsContainer.find(".list-group > a:eq(0) .badge"), this.collection.unreadCount);
@svbergerem
svbergerem Nov 11, 2016 Member

The links for the different notification types have a data-type attribute and I'd add data-type="all" to the first link so you can use this here instead of using a:eq(0). Then it should be more obvious what this line does.

+ }.bind(this));
+
+ updateBadge(notificationsContainer.find(".list-group > a:eq(0) .badge"), this.collection.unreadCount);
+ updateBadge($(".notifications-link .badge"), this.collection.unreadCount);
@svbergerem
svbergerem Nov 11, 2016 Member

A comment stating that this updates the notification count in the header would be great.

@svbergerem
svbergerem Nov 11, 2016 Member

Maybe even

// update notification counts in the sidebar
Object.keys(...);
updateBadge(notificationsContainer.find("a[data-type=all] .badge"), this.collection.unreadCount);

// update notification count in the header
updateBadge($(".notifications-link .badge"), this.collection.unreadCount);
+ } else {
+ /* eslint-disable no-console */
+ console.warn("Browser do not support notifications");
+ /* eslint-enable no-console */
@svbergerem
svbergerem Nov 11, 2016 Member

I'd just drop this.
@denschub What do you think?

@denschub
denschub Nov 14, 2016 Member

Yeah, I'd rather not have console logging. Believe it or not, there are browsers who don't even have the console object, so this would raise. Also, this is not a hard requirement that would break anything, so remove it.

+ if (!_.isString(title)) {
+ /* eslint-disable no-console */
+ console.error("No title given; aborting...");
+ /* eslint-enable no-console */
@svbergerem
svbergerem Nov 11, 2016 Member

I'd like to see

throw new Error("No notification title given.");

here instead.

@@ -246,6 +246,7 @@ en:
notifications:
mark_read: "Mark read"
mark_unread: "Mark unread"
+ new_notifications: "You have new notifications"
@svbergerem
svbergerem Nov 11, 2016 Member

How about

"You have <=% count %> unread notifications"

?

@AugierLe42e
AugierLe42e Nov 13, 2016 Contributor

Hmm... There is a problem using "count" as a variable name for localization. I suppose probably because of this line. I throws Error: Missing translation: notifications.new_notifications.other i18n.js:48:15. I used number instead.

@svbergerem
svbergerem Nov 13, 2016 Member

Thanks for pointing this out. Actually we should be using count with one -> "... notification" other -> "... notifications".

@AugierLe42e
AugierLe42e Nov 13, 2016 Contributor

Oh, so that's what the error was about!

+ expect(Backbone.Collection.prototype.set).toHaveBeenCalledWith([], {at: 0});
+ });
+
+ it("inserts the items at the begining of the collection with option 'pushBack' is false", function() {
@svbergerem
svbergerem Nov 11, 2016 Member

inserts the items at the beginning of the collection if option 'pushBack' is false

+ expect(Backbone.Collection.prototype.set).toHaveBeenCalledWith([], {pushBack: false, at: 0});
+ });
+
+ it("inserts the items at the end of the collection with option 'pushBack' is true", function() {
@svbergerem
svbergerem Nov 11, 2016 Member

inserts the items at the end of the collection if option 'pushBack' is true

+ expect(calls.argsFor(index + 3)).toEqual(["pushFront", this.model1]);
+ });
+
+ it("triggers a 'pushBack' event for each model in when option 'pushBack' is true", function() {
@svbergerem
svbergerem Nov 11, 2016 Member

triggers a 'pushBack' event for each model in normal order when option 'pushBack' is true

+ it("calls calls ajax with correct parameters and sets 'unread' attribute", function() {
+ this.target.setUnreadStatus(true);
+ jasmine.Ajax.requests.mostRecent().respondWith({status: 200, responseText: '{"guid": 16, "unread": true}'});
+ var call = jQuery.ajax.calls.mostRecent();
@svbergerem
svbergerem Nov 11, 2016 Member

Have you tried

var call = jasmine.Ajax.requests.mostRecent();

?

Then you wouldn't have to spy on jQuery.ajax.

+ var call = jQuery.ajax.calls.mostRecent();
+
+ expect(jQuery.ajax).toHaveBeenCalled();
+ expect(call.args[0].url).toBe("/notifications/16");
@svbergerem
svbergerem Nov 11, 2016 Member

With jasmine.Ajax.requests.mostRecent() this would be

expect(jasmine.Ajax.requests.mostRecent().url).toBe('/notifications/16')

or

expect(call.url).toBe('/notifications/16')
+ icon: ImagePaths.get("branding/logos/asterisk_white_mobile.png")
+ });
+
+ setTimeout(notification.close.bind(notification), 5000);
@svbergerem
svbergerem Nov 11, 2016 Member

I think I'd just remove this. Users can close the notifications themselves. When I'm afk for a few minutes then I'd like to see the notification when I come back.

Any other opinions about this?

@AugierLe42e
Contributor

It's ok on my side.

+ /* eslint-enable camelcase */
+ type: "PUT",
+ context: this,
+ success: function() { this.set("unread", state); }.bind(this)
@svbergerem
svbergerem Nov 17, 2016 Member

Binding this shouldn't be needed since you already set context: this.

+ this.bindCollectionEvents();
+ },
+
+ bindCollectionEvents: function() {
@svbergerem
svbergerem Nov 17, 2016 Member

Some tests would be great.

- this.dropdownNotifications.scroll(function(){
- self.dropdownScroll();
- });
+ this.dropdownNotifications.scroll(function() { this.dropdownScroll(); }.bind(this));
@svbergerem
svbergerem Nov 17, 2016 Member

This would attach the dropdownScroll function to the scroll event again and again. Doing this once at the end of the initialize function should work, too.

+ expect(Backbone.Collection.prototype.set).toHaveBeenCalledWith([], {at: 0});
+ });
+
+ it(" the items at the beginning of the collection if option 'pushBack' is false", function() {
@svbergerem
svbergerem Nov 17, 2016 Member

it("inserts the items...

- this.readN.find(".unread-toggle").trigger("click");
-
- expect(this.view.setUnread).toHaveBeenCalledWith(this.guid);
- });
@svbergerem
svbergerem Nov 17, 2016 Member

Am I missing something or are there now no tests for marking notifications as read/unread via click?

@AugierLe42e
AugierLe42e Nov 17, 2016 Contributor

The tests happens in notifications_collection_spec.js. I can reinstanciate these ones for the click event.

- this.view.updateView(this.guid, this.type, false);
@svbergerem
svbergerem Nov 17, 2016 Member

This decreased the notifications count before which is now missing. Now you just call updateView twice without any changes.

- this.view.updateView(this.guid, this.type, false);
@svbergerem
svbergerem Nov 17, 2016 Member

Again: you just call updateView twice without any changes.

- expect(this.readN.hasClass("unread")).toBeFalsy();
- expect(this.readN.find(".unread-toggle .entypo-eye").attr("data-original-title")).toBe(
- Diaspora.I18n.t("notifications.mark_unread")
- );
@svbergerem
svbergerem Nov 17, 2016 Member

This is now done by onChangedUnreadStatus but there seems to be no test for this. Would be great if you could add that.

+ it("hides badge count when notification count is zero", function() {
+ Object.keys(this.collection.unreadCountByType).forEach(function(notificationType) {
+ this.collection.unreadCountByType[notificationType] = 0;
+ this.collection.unreadCount = 0;
@svbergerem
svbergerem Nov 17, 2016 Member

This should be outside of the forEach loop.

- expect(parseInt(badge2.text(), 10)).toBe(count + 1);
+ this.view.updateView();
+ expect(parseInt(badge2.text(), 10)).toBe(this.collection.unreadCount);
+ });
@svbergerem
svbergerem Nov 17, 2016 Member

Before we checked both increase and decrease of the unread counts. Calling updateView once should also be sufficient for changes to badge1 and badge2, so just do "increase", updateView, "check badge1 and badge2", "decrease", updateView, "check badge1 and badge2".

+ this.bindCollectionEvents();
+ },
+
+ bindCollectionEvents: function() {
@svbergerem
svbergerem Nov 17, 2016 Member

Some tests would be great.

+ },
+
+ bindCollectionEvents: function() {
+ this.collection.on("change", this.onChagedUnreadStatus.bind(this));
@svbergerem
svbergerem Nov 17, 2016 Member

onChangedUnreadStatus

- this.getAllUnread().each(function(i, el){
- self.setRead($(el).data("guid"));
- });
+ onChagedUnreadStatus: function(model) {
@svbergerem
svbergerem Nov 17, 2016 Member

onChangedUnreadStatus

- this.view.hasMoreNotifs = true;
- spyOn(this.view, "getNotifications");
+ describe("dropdownScroll", function() {
+ it("Calls collection#fetchMore if is at the bottom and has more notifications to load", function() {
@svbergerem
svbergerem Nov 17, 2016 Member

Calls collection#fetchMore if it is at the bottom and has more notifications to load

(missing "it" and the check for more notifications is done in fetchMore)

- this.view.isBottom = function(){ return true; };
- this.view.hasMoreNotifs = false;
- spyOn(this.view, "getNotifications");
+ it("Doesn't call collection#fetchMore if is not at the bottom", function() {
@svbergerem
svbergerem Nov 17, 2016 Member

Doesn't call collection#fetchMore if it is not at the bottom

@AugierLe42e
Contributor

@svbergerem : I don't understand why this test and this one won't pass. According to the test failure report the spied methods seem not mocked at all and called anyway. I use strickly the same method in this one with no problem :/

@svbergerem
Member

@AugierLe42e Here you already bind the events and you bind them here a second time with new functions. Thus both old and new functions are called and calling the old ones makes the tests fail.

diff --git a/spec/javascripts/app/views/notification_dropdown_view_spec.js b/spec/javascripts/app/views/notification_dropdown_view_spec.js
index 3b01319..f47e945 100644
--- a/spec/javascripts/app/views/notification_dropdown_view_spec.js
+++ b/spec/javascripts/app/views/notification_dropdown_view_spec.js
@@ -12,6 +12,9 @@ describe("app.views.NotificationDropdown", function() {

   describe("bindCollectionEvents", function() {
     it("binds collection events", function() {
+      this.view.collection.off("pushFront");
+      this.view.collection.off("pushBack");
+      this.view.collection.off("finishedLoading");
       spyOn(this.view, "onPushFront");
       spyOn(this.view, "onPushBack");
       spyOn(this.view, "finishLoading");
@@ -20,11 +23,11 @@ describe("app.views.NotificationDropdown", function() {

       this.collection.trigger("pushFront");
       this.collection.trigger("pushBack");
-      this.collection.trigger("finishLoading");
+      this.collection.trigger("finishedLoading");

       expect(this.view.onPushFront).toHaveBeenCalled();
       expect(this.view.onPushBack).toHaveBeenCalled();
-      expect(this.view.pushBack).toHaveBeenCalled();
+      expect(this.view.finishLoading).toHaveBeenCalled();
     });
   });

You should be able to fix your second problem in a similar way.

@AugierLe42e
Contributor

JS is so tricky sometimes. Thx, @svbergerem.

@AugierLe42e AugierLe42e Add collection to app.views.NotificationDropdown and app.views.Notifi…
…cations

:q
59181e2
@AugierLe42e
Contributor

Done.

@svbergerem svbergerem added this to the 0.6.2.0 milestone Nov 18, 2016
@svbergerem svbergerem added a commit that closed this pull request Nov 18, 2016
@AugierLe42e @svbergerem AugierLe42e + svbergerem Add collection to app.views.NotificationDropdown and app.views.Notifi…
…cations

closes #6952
af331bf
@svbergerem
Member

Thank you! :)

@Flaburgan
Contributor

I just pulled this on d-fr, looks like it works nicely. I'll report here the load usage if I see some significant changes. If not, I'll try to set it to 1 minute instead of 5 and then test again. This setting could probably be added in the config file btw.

@AugierLe42e AugierLe42e deleted the AugierLe42e:notifications-header-use-collection branch Nov 18, 2016
@AugierLe42e
Contributor

@svbergerem: Thanks for your careful reviewing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment