From 69bc552054278b8a0627b8a2d1f19ba1ce94f481 Mon Sep 17 00:00:00 2001
From: riking <rikingcoding@gmail.com>
Date: Tue, 2 Sep 2014 18:32:27 -0700
Subject: [PATCH] FEATURE: Actually show more notifications

The "Show more notifications..." link in the notifications dropdown now
links to /my/notifications, which is a historical view of all
notifications you have recieved.

Notification history is loaded in blocks of 60 at a time.

Admins can see others' notification history. (This was requested for
'debugging purposes', though that's what impersonation is for, IMO.)
---
 .../discourse/controllers/header.js.es6       |  8 +++-
 .../controllers/user-notifications.js.es6     | 27 +++++++++++
 .../discourse/controllers/user.js.es6         |  2 +
 .../discourse/models/notification.js          | 48 +++++++++++++++++++
 .../discourse/routes/application_routes.js    |  1 +
 .../routes/user-notifications.js.es6          | 19 ++++++++
 .../templates/notifications.js.handlebars     |  8 +++-
 .../user/notifications.js.handlebars          | 24 ++++++++++
 .../templates/user/user.js.handlebars         | 10 ++++
 .../views/user-notification-history.js.es6    |  5 ++
 app/assets/stylesheets/common/base/user.scss  |  5 ++
 app/assets/stylesheets/desktop/user.scss      | 24 ++++++++++
 app/assets/stylesheets/mobile/user.scss       | 20 ++++++++
 app/controllers/notifications_controller.rb   | 23 ++++++++-
 app/serializers/user_serializer.rb            |  6 +++
 config/locales/client.en.yml                  |  2 +
 config/routes.rb                              |  4 +-
 lib/guardian/user_guardian.rb                 |  4 ++
 .../notifications_controller_spec.rb          | 15 ++++--
 .../controllers/header-test.js.es6            |  3 +-
 20 files changed, 246 insertions(+), 12 deletions(-)
 create mode 100644 app/assets/javascripts/discourse/controllers/user-notifications.js.es6
 create mode 100644 app/assets/javascripts/discourse/models/notification.js
 create mode 100644 app/assets/javascripts/discourse/routes/user-notifications.js.es6
 create mode 100644 app/assets/javascripts/discourse/templates/user/notifications.js.handlebars
 create mode 100644 app/assets/javascripts/discourse/views/user-notification-history.js.es6

diff --git a/app/assets/javascripts/discourse/controllers/header.js.es6 b/app/assets/javascripts/discourse/controllers/header.js.es6
index d7cebb23f01..846b1441f7e 100644
--- a/app/assets/javascripts/discourse/controllers/header.js.es6
+++ b/app/assets/javascripts/discourse/controllers/header.js.es6
@@ -45,12 +45,16 @@ export default DiscourseController.extend({
     if (self.get("loadingNotifications")) { return; }
 
     self.set("loadingNotifications", true);
-    Discourse.ajax("/notifications").then(function(result) {
+    Discourse.NotificationContainer.loadRecent().then(function(result) {
       self.setProperties({
         'currentUser.unread_notifications': 0,
         notifications: result
       });
-    }).finally(function(){
+    }).catch(function() {
+      self.setProperties({
+        notifications: null
+      });
+    }).finally(function() {
       self.set("loadingNotifications", false);
     });
   },
diff --git a/app/assets/javascripts/discourse/controllers/user-notifications.js.es6 b/app/assets/javascripts/discourse/controllers/user-notifications.js.es6
new file mode 100644
index 00000000000..6bb5aee5201
--- /dev/null
+++ b/app/assets/javascripts/discourse/controllers/user-notifications.js.es6
@@ -0,0 +1,27 @@
+
+export default Ember.ArrayController.extend({
+  canLoadMore: true,
+  loading: false,
+
+  actions: {
+    loadMore: function() {
+      if (this.get('canLoadMore') && !this.get('loading')) {
+        this.set('loading', true);
+        var self = this;
+        Discourse.NotificationContainer.loadHistory(
+            self.get('model.lastObject.created_at'),
+            self.get('user.username')).then(function(result) {
+          self.set('loading', false);
+          self.pushObjects(result);
+          // Stop trying if it's the end
+          if (result.length === 0) {
+            self.set('canLoadMore', false);
+          }
+        }).catch(function(error) {
+          self.set('loading', false);
+          Em.Logger.error(error);
+        });
+      }
+    }
+  }
+});
diff --git a/app/assets/javascripts/discourse/controllers/user.js.es6 b/app/assets/javascripts/discourse/controllers/user.js.es6
index 2db276896eb..651347c2f8b 100644
--- a/app/assets/javascripts/discourse/controllers/user.js.es6
+++ b/app/assets/javascripts/discourse/controllers/user.js.es6
@@ -20,6 +20,8 @@ export default ObjectController.extend({
     return this.get('viewingSelf') || Discourse.User.currentProp('admin');
   }.property('viewingSelf'),
 
+  canSeeNotificationHistory: Em.computed.alias('canSeePrivateMessages'),
+
   showBadges: function() {
     return Discourse.SiteSettings.enable_badges && (this.get('content.badge_count') > 0);
   }.property('content.badge_count'),
diff --git a/app/assets/javascripts/discourse/models/notification.js b/app/assets/javascripts/discourse/models/notification.js
new file mode 100644
index 00000000000..90bc5fe4f1c
--- /dev/null
+++ b/app/assets/javascripts/discourse/models/notification.js
@@ -0,0 +1,48 @@
+Discourse.NotificationContainer = Ember.ArrayProxy.extend({
+
+});
+
+Discourse.NotificationContainer.reopenClass({
+
+  createFromJson: function(json_array) {
+    return Discourse.NotificationContainer.create({content: json_array});
+  },
+
+  createFromError: function(error) {
+    return Discourse.NotificationContainer.create({
+      content: [],
+      error: true,
+      forbidden: error.status === 403
+    });
+  },
+
+  loadRecent: function() {
+    return Discourse.ajax('/notifications').then(function(result) {
+      return Discourse.NotificationContainer.createFromJson(result);
+    }).catch(function(error) {
+      // HeaderController can't handle it properly
+      throw error;
+    });
+  },
+
+  loadHistory: function(beforeDate, username) {
+    var url = '/notifications/history.json',
+        params = [
+          beforeDate ? ('before=' + beforeDate) : null,
+          username ? ('user=' + username) : null
+        ];
+
+    // Remove nulls
+    params = params.filter(function(param) { return !!param; });
+    // Build URL
+    params.forEach(function(param, idx) {
+      url = url + (idx === 0 ? '?' : '&') + param;
+    });
+
+    return Discourse.ajax(url).then(function(result) {
+      return Discourse.NotificationContainer.createFromJson(result);
+    }).catch(function(error) {
+      return Discourse.NotificationContainer.createFromError(error);
+    });
+  }
+});
diff --git a/app/assets/javascripts/discourse/routes/application_routes.js b/app/assets/javascripts/discourse/routes/application_routes.js
index e2539177499..2a0cd141c54 100644
--- a/app/assets/javascripts/discourse/routes/application_routes.js
+++ b/app/assets/javascripts/discourse/routes/application_routes.js
@@ -69,6 +69,7 @@ Discourse.Route.buildRoutes(function() {
     });
 
     this.route('badges');
+    this.route('notifications');
     this.route('flaggedPosts', { path: '/flagged-posts' });
     this.route('deletedPosts', { path: '/deleted-posts' });
 
diff --git a/app/assets/javascripts/discourse/routes/user-notifications.js.es6 b/app/assets/javascripts/discourse/routes/user-notifications.js.es6
new file mode 100644
index 00000000000..91d273a04da
--- /dev/null
+++ b/app/assets/javascripts/discourse/routes/user-notifications.js.es6
@@ -0,0 +1,19 @@
+export default Discourse.Route.extend({
+  model: function() {
+    var user = this.modelFor('user');
+    return Discourse.NotificationContainer.loadHistory(undefined, user.get('username'));
+  },
+
+  setupController: function(controller, model) {
+    this.controllerFor('user').set('indexStream', false);
+    if (this.controllerFor('user_activity').get('content')) {
+      this.controllerFor('user_activity').set('userActionType', -1);
+    }
+    controller.set('model', model);
+    controller.set('user', this.modelFor('user'));
+  },
+
+  renderTemplate: function() {
+    this.render('user-notification-history', {into: 'user', outlet: 'userOutlet'});
+  }
+});
diff --git a/app/assets/javascripts/discourse/templates/notifications.js.handlebars b/app/assets/javascripts/discourse/templates/notifications.js.handlebars
index e2220200519..b7117b7af60 100644
--- a/app/assets/javascripts/discourse/templates/notifications.js.handlebars
+++ b/app/assets/javascripts/discourse/templates/notifications.js.handlebars
@@ -6,11 +6,15 @@
           {{notification-item notification=this scope=scope}}
         {{/each}}
         <li class="read last">
-          <a {{bind-attr href="currentUser.path"}}>{{i18n notifications.more}} &hellip;</a>
+          <a href="/my/notifications">{{i18n notifications.more}}&hellip;</a>
         </li>
       </ul>
     {{else}}
-      <div class="none">{{i18n notifications.none}}</div>
+      {{#if error}}
+        <div class="none error">{{i18n notifications.none}}</div>
+      {{else}}
+        <div class="none">{{i18n notifications.none}}</div>
+      {{/if}}
     {{/if}}
   {{else}}
     <div class='loading'><i class='fa fa-spinner fa-spin'></i></div>
diff --git a/app/assets/javascripts/discourse/templates/user/notifications.js.handlebars b/app/assets/javascripts/discourse/templates/user/notifications.js.handlebars
new file mode 100644
index 00000000000..9c66d9f7a06
--- /dev/null
+++ b/app/assets/javascripts/discourse/templates/user/notifications.js.handlebars
@@ -0,0 +1,24 @@
+{{#if model.error}}
+  <div class="item error">
+    {{#if model.forbidden}}
+      {{i18n errors.reasons.forbidden}}
+    {{else}}
+      {{i18n errors.desc.unknown}}
+    {{/if}}
+  </div>
+{{/if}}
+
+{{#each itemController="notification"}}
+  <div {{bind-attr class=":item :notification read::unread"}}>
+    {{notification-item notification=this scope=scope}}
+    <span class="time">
+      {{date path="created_at" leaveAgo="true"}}
+    </span>
+  </div>
+{{/each}}
+{{#if loading}}
+  <div class='spinner'>{{i18n loading}}</div>
+{{/if}}
+{{#unless canLoadMore}}
+  <div class='end-of-stream'></div>
+{{/unless}}
diff --git a/app/assets/javascripts/discourse/templates/user/user.js.handlebars b/app/assets/javascripts/discourse/templates/user/user.js.handlebars
index 509424f6cdf..9532d1f027f 100644
--- a/app/assets/javascripts/discourse/templates/user/user.js.handlebars
+++ b/app/assets/javascripts/discourse/templates/user/user.js.handlebars
@@ -23,6 +23,16 @@
             {{/link-to}}
           {{/link-to}}
         {{/if}}
+        {{#if canSeeNotificationHistory}}
+          {{#link-to 'user.notifications' tagName="li"}}
+            {{#link-to 'user.notifications'}}
+              <i class='glyph fa fa-comment'></i>
+              {{i18n user.notification_history}}
+              <span class='count'>({{unread_notification_count}})</span>
+              <span class='fa fa-chevron-right'></span>
+            {{/link-to}}
+          {{/link-to}}
+        {{/if}}
       </ul>
 
       {{#if canSeePrivateMessages}}
diff --git a/app/assets/javascripts/discourse/views/user-notification-history.js.es6 b/app/assets/javascripts/discourse/views/user-notification-history.js.es6
new file mode 100644
index 00000000000..a31bea4931d
--- /dev/null
+++ b/app/assets/javascripts/discourse/views/user-notification-history.js.es6
@@ -0,0 +1,5 @@
+export default Ember.View.extend(Discourse.LoadMore, {
+  eyelineSelector: '.user-stream .notification',
+  classNames: ['user-stream', 'notification-history'],
+  templateName: 'user/notifications'
+});
diff --git a/app/assets/stylesheets/common/base/user.scss b/app/assets/stylesheets/common/base/user.scss
index 4d09ac6180f..e2b9a56e3e6 100644
--- a/app/assets/stylesheets/common/base/user.scss
+++ b/app/assets/stylesheets/common/base/user.scss
@@ -15,6 +15,11 @@
   }
 }
 
+.end-of-stream {
+  border: 3px solid $primary;
+  width: 100%;
+}
+
 .user-navigation {
 
     .map {
diff --git a/app/assets/stylesheets/desktop/user.scss b/app/assets/stylesheets/desktop/user.scss
index fe6f7144399..caefbf72f53 100644
--- a/app/assets/stylesheets/desktop/user.scss
+++ b/app/assets/stylesheets/desktop/user.scss
@@ -367,6 +367,30 @@
       float: right;
       margin-top: -4px;
     }
+    .notification {
+      &.unread {
+        background-color: dark-light-diff($tertiary, $secondary, 90%, -60%);
+      }
+
+      li { display: inline-block; }
+      p {
+        display: inline-block;
+        margin-left: 10px;
+        span {
+          color: $primary;
+        }
+      }
+      .time {
+        display: inline-block;
+        margin-left: 10px;
+        float: none;
+      }
+      // common/base/header.scss
+      .fa, .icon {
+        color: scale-color($primary, $lightness: 50%);
+        font-size: 24px;
+      }
+    }
   }
 
   .staff-counters {
diff --git a/app/assets/stylesheets/mobile/user.scss b/app/assets/stylesheets/mobile/user.scss
index 77de38d4f58..cabbc38fed4 100644
--- a/app/assets/stylesheets/mobile/user.scss
+++ b/app/assets/stylesheets/mobile/user.scss
@@ -284,6 +284,26 @@
       float: right !important;
       margin-top: -8px;
     }
+    .notification {
+      padding: 0 8px;
+      li { display: inline-block; }
+      p {
+        display: inline-block;
+        margin: 7px;
+        span {
+          color: $primary;
+        }
+      }
+      .time {
+        display: inline-block;
+        margin: 0;
+        float: none;
+      }
+      // common/base/header.scss
+      .fa, .icon {
+        color: scale-color($primary, $lightness: 50%);
+      }
+    }
   }
 
   .staff-counters {
diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb
index 46c4374c117..40c0b6227e6 100644
--- a/app/controllers/notifications_controller.rb
+++ b/app/controllers/notifications_controller.rb
@@ -2,7 +2,7 @@ class NotificationsController < ApplicationController
 
   before_filter :ensure_logged_in
 
-  def index
+  def recent
     notifications = Notification.recent_report(current_user, 10)
 
     if notifications.present?
@@ -16,4 +16,25 @@ class NotificationsController < ApplicationController
     render_serialized(notifications, NotificationSerializer)
   end
 
+  def history
+    params.permit(:before, :user)
+    params[:before] ||= 1.day.from_now
+
+    user = current_user
+    if params[:user]
+      user = User.find_by_username(params[:user].to_s)
+    end
+
+    unless guardian.can_see_notifications?(user)
+      return render json: {errors: [I18n.t('js.errors.reasons.forbidden')]}, status: 403
+    end
+
+    notifications = Notification.where(user_id: user.id)
+        .includes(:topic)
+        .limit(60)
+        .where('created_at < ?', params[:before])
+        .order(created_at: :desc)
+
+    render_serialized(notifications, NotificationSerializer)
+  end
 end
diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb
index 1edb4034d2f..327c7952614 100644
--- a/app/serializers/user_serializer.rb
+++ b/app/serializers/user_serializer.rb
@@ -43,6 +43,7 @@ class UserSerializer < BasicUserSerializer
              :suspended_till,
              :uploaded_avatar_id,
              :badge_count,
+             :unread_notification_count,
              :has_title_badges,
              :edit_history_public,
              :custom_fields
@@ -76,6 +77,7 @@ class UserSerializer < BasicUserSerializer
                      :tracked_category_ids,
                      :watched_category_ids,
                      :private_messages_stats,
+                     :unread_notification_count,
                      :disable_jump_reply,
                      :gravatar_avatar_upload_id,
                      :custom_avatar_upload_id,
@@ -242,6 +244,10 @@ class UserSerializer < BasicUserSerializer
     object.badges.where(allow_title: true).count > 0
   end
 
+  def unread_notification_count
+    Notification.where(user_id: object.id, read: false).count
+  end
+
   def include_edit_history_public?
     can_edit && !SiteSetting.edit_history_visible_to_public
   end
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index 49355a5c016..5b18c2fa076 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -306,6 +306,7 @@ en:
       invited_by: "Invited By"
       trust_level: "Trust Level"
       notifications: "Notifications"
+      notification_history: "Notification History"
       disable_jump_reply: "Don't jump to your new post after replying"
       dynamic_favicon: "Show incoming message notifications on favicon (experimental)"
       edit_history_public: "Let other users view my post revisions"
@@ -520,6 +521,7 @@ en:
         network: "Please check your connection."
         network_fixed: "Looks like it's back."
         server: "Error code: {{status}}"
+        forbidden: "You're not allowed to view that."
         unknown: "Something went wrong."
       buttons:
         back: "Go Back"
diff --git a/config/routes.rb b/config/routes.rb
index 786b510fc20..654839be257 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -232,6 +232,7 @@ Discourse::Application.routes.draw do
   get "users/:username/activity" => "users#show", constraints: {username: USERNAME_ROUTE_FORMAT}
   get "users/:username/activity/:filter" => "users#show", constraints: {username: USERNAME_ROUTE_FORMAT}
   get "users/:username/badges" => "users#show", constraints: {username: USERNAME_ROUTE_FORMAT}
+  get "users/:username/notifications" => "users#show", constraints: {username: USERNAME_ROUTE_FORMAT}
   delete "users/:username" => "users#destroy", constraints: {username: USERNAME_ROUTE_FORMAT}
   get "users/by-external/:external_id" => "users#show"
   get "users/:username/flagged-posts" => "users#show", constraints: {username: USERNAME_ROUTE_FORMAT}
@@ -270,7 +271,8 @@ Discourse::Application.routes.draw do
     end
   end
 
-  resources :notifications
+  get "notifications" => "notifications#recent"
+  get "notifications/history" => "notifications#history"
 
   match "/auth/:provider/callback", to: "users/omniauth_callbacks#complete", via: [:get, :post]
   match "/auth/failure", to: "users/omniauth_callbacks#failure", via: [:get, :post]
diff --git a/lib/guardian/user_guardian.rb b/lib/guardian/user_guardian.rb
index 2b624c0012b..af6596b9bb8 100644
--- a/lib/guardian/user_guardian.rb
+++ b/lib/guardian/user_guardian.rb
@@ -26,6 +26,10 @@ module UserGuardian
     can_edit?(user)
   end
 
+  def can_see_notifications?(user)
+    is_me?(user) || is_admin?
+  end
+
   def can_block_user?(user)
     user && is_staff? && not(user.staff?)
   end
diff --git a/spec/controllers/notifications_controller_spec.rb b/spec/controllers/notifications_controller_spec.rb
index 1259bda2270..71326786866 100644
--- a/spec/controllers/notifications_controller_spec.rb
+++ b/spec/controllers/notifications_controller_spec.rb
@@ -5,29 +5,34 @@ describe NotificationsController do
   context 'when logged in' do
     let!(:user) { log_in }
 
-    it 'should succeed' do
-      xhr :get, :index
+    it 'should succeed for recent' do
+      xhr :get, :recent
+      response.should be_success
+    end
+
+    it 'should succeed for history' do
+      xhr :get, :history
       response.should be_success
     end
 
     it 'should mark notifications as viewed' do
       notification = Fabricate(:notification, user: user)
       user.reload.unread_notifications.should == 1
-      xhr :get, :index
+      xhr :get, :recent
       user.reload.unread_notifications.should == 0
     end
 
     it 'should not mark notifications as viewed if silent param is present' do
       notification = Fabricate(:notification, user: user)
       user.reload.unread_notifications.should == 1
-      xhr :get, :index, silent: true
+      xhr :get, :recent, silent: true
       user.reload.unread_notifications.should == 1
     end
   end
 
   context 'when not logged in' do
     it 'should raise an error' do
-      lambda { xhr :get, :index }.should raise_error(Discourse::NotLoggedIn)
+      lambda { xhr :get, :recent }.should raise_error(Discourse::NotLoggedIn)
     end
   end
 
diff --git a/test/javascripts/controllers/header-test.js.es6 b/test/javascripts/controllers/header-test.js.es6
index aabbd9ad5ed..d5bdbe501f5 100644
--- a/test/javascripts/controllers/header-test.js.es6
+++ b/test/javascripts/controllers/header-test.js.es6
@@ -29,7 +29,8 @@ test("showNotifications action", function() {
     resolveRequestWith(["notification"]);
   });
 
-  deepEqual(controller.get("notifications"), ["notification"], "notifications are set correctly after data has finished loading");
+  // Can't use deepEquals because controller.get("notifications") is an ArrayProxy, not an Array
+  ok(controller.get("notifications").indexOf("notification") !== -1, "notification is in the controller");
   equal(Discourse.User.current().get("unread_notifications"), 0, "current user's unread notifications count is zeroed after data has finished loading");
   ok(viewSpy.showDropdownBySelector.calledWith("#user-notifications"), "dropdown with notifications is shown after data has finished loading");
 });