Paginate notifications (#3948)

* Use offset instead of page and explain things * Make load more button work * Add tests for admin view * Restore the previous logged out behavior * Modernize initNotifications.js * Revert "Modernize initNotifications.js" This reverts commit 4a112b797d7911c4ab63ad0c0a07111b4e7abe25. * Fix elements presence errors * Change load-more-button to only appear if 7 or more posts (and some notification copy adjustments)
2019-09-07 17:54:31 +02:00 · 2019-09-07 17:54:31 +02:00 · f7d6874956
commit f7d6874956
parent 61494d2343
9 changed files with 184 additions and 70 deletions
--- a/app/assets/javascripts/initializers/initNotifications.js
+++ b/app/assets/javascripts/initializers/initNotifications.js
@ -4,28 +4,35 @@ function initNotifications() {
  initReactions();
  listenForNotificationsBellClick();
  initPagination();
+  initLoadMoreButton();
 }

 function markNotificationsAsRead() {
-  setTimeout(function () {
+  setTimeout(function() {
    if (document.getElementById('notifications-container')) {
      var xmlhttp;
-      var locationAsArray = window.location.pathname.split("/");
+      var locationAsArray = window.location.pathname.split('/');
      // Use regex to ensure only numbers in the original string are converted to integers
-      var parsedLastParam = parseInt(locationAsArray[locationAsArray.length - 1].replace(/[^0-9]/g, ''), 10);
+      var parsedLastParam = parseInt(
+        locationAsArray[locationAsArray.length - 1].replace(/[^0-9]/g, ''),
+        10,
+      );

      if (window.XMLHttpRequest) {
        xmlhttp = new XMLHttpRequest();
      } else {
        xmlhttp = new ActiveXObject('Microsoft.XMLHTTP');
      }
-      xmlhttp.onreadystatechange = function () {
-      };
+      xmlhttp.onreadystatechange = function() {};

      var csrfToken = document.querySelector("meta[name='csrf-token']").content;

-      if(Number.isInteger(parsedLastParam)) {
-        xmlhttp.open('Post', '/notifications/reads?org_id=' + parsedLastParam, true);
+      if (Number.isInteger(parsedLastParam)) {
+        xmlhttp.open(
+          'Post',
+          '/notifications/reads?org_id=' + parsedLastParam,
+          true,
+        );
      } else {
        xmlhttp.open('Post', '/notifications/reads', true);
      }
@ -36,29 +43,42 @@ function markNotificationsAsRead() {
 }

 function fetchNotificationsCount() {
-  if (document.getElementById('notifications-container') == null && checkUserLoggedIn()) {
+  if (
+    document.getElementById('notifications-container') == null &&
+    checkUserLoggedIn()
+  ) {
    var xmlhttp;
    if (window.XMLHttpRequest) {
      xmlhttp = new XMLHttpRequest();
    } else {
      xmlhttp = new ActiveXObject('Microsoft.XMLHTTP');
    }
-    xmlhttp.onreadystatechange = function () {
+    xmlhttp.onreadystatechange = function() {
      if (xmlhttp.readyState == XMLHttpRequest.DONE) {
        var count = xmlhttp.response;
        if (isNaN(count)) {
-          document.getElementById('notifications-number').classList.remove('showing');
-        } else if (count != '0' && count != undefined && count != "") {
-          document.getElementById('notifications-number').innerHTML = xmlhttp.response;
-          document.getElementById('notifications-number').classList.add('showing');
-          if(instantClick){
-            InstantClick.removeExpiredKeys("force");
-            setTimeout(function(){
-              InstantClick.preload(document.getElementById("notifications-link").href, "force");
-            },30)
+          document
+            .getElementById('notifications-number')
+            .classList.remove('showing');
+        } else if (count != '0' && count != undefined && count != '') {
+          document.getElementById('notifications-number').innerHTML =
+            xmlhttp.response;
+          document
+            .getElementById('notifications-number')
+            .classList.add('showing');
+          if (instantClick) {
+            InstantClick.removeExpiredKeys('force');
+            setTimeout(function() {
+              InstantClick.preload(
+                document.getElementById('notifications-link').href,
+                'force',
+              );
+            }, 30);
          }
        } else {
-          document.getElementById('notifications-number').classList.remove('showing');
+          document
+            .getElementById('notifications-number')
+            .classList.remove('showing');
        }
      }
    };
@ -69,12 +89,12 @@ function fetchNotificationsCount() {
 }

 function initReactions() {
-  setTimeout(function () {
+  setTimeout(function() {
    if (document.getElementById('notifications-container')) {
      var butts = document.getElementsByClassName('reaction-button');
      for (var i = 0; i < butts.length; i++) {
        var butt = butts[i];
-        butt.onclick = function (event) {
+        butt.onclick = function(event) {
          event.preventDefault();
          sendHapticMessage('medium');
          var thisButt = this;
@ -95,7 +115,7 @@ function initReactions() {

          getCsrfToken()
            .then(sendFetch('reaction-creation', formData))
-            .then(function (response) {
+            .then(function(response) {
              if (response.status === 200) {
                response.json().then(successCb);
              }
@ -105,13 +125,19 @@ function initReactions() {
      var butts = document.getElementsByClassName('toggle-reply-form');
      for (var i = 0; i < butts.length; i++) {
        var butt = butts[i];
-        butt.onclick = function (event) {
+        butt.onclick = function(event) {
          event.preventDefault();
          var thisButt = this;
-          document.getElementById('comment-form-for-' + thisButt.dataset.reactableId).classList.add('showing');
+          document
+            .getElementById('comment-form-for-' + thisButt.dataset.reactableId)
+            .classList.add('showing');
          thisButt.innerHTML = '';
-          setTimeout(function () {
-            document.getElementById('comment-textarea-for-' + thisButt.dataset.reactableId).focus();
+          setTimeout(function() {
+            document
+              .getElementById(
+                'comment-textarea-for-' + thisButt.dataset.reactableId,
+              )
+              .focus();
          }, 30);
        };
      }
@ -120,26 +146,53 @@ function initReactions() {
 }

 function listenForNotificationsBellClick() {
-  setTimeout(function () {
-    document.getElementById('notifications-link').onclick = function () {
-      document.getElementById('notifications-number').classList.remove('showing');
+  setTimeout(function() {
+    document.getElementById('notifications-link').onclick = function() {
+      document
+        .getElementById('notifications-number')
+        .classList.remove('showing');
    };
  }, 180);
 }

 function initPagination() {
-  var el = document.getElementById("notifications-pagination")
-  if (el) {
-    window.fetch(el.dataset.paginationPath, {
-      method: 'GET',
-      credentials: 'same-origin'
-    }).then(function (response) {
-      if (response.status === 200) {
-        response.text().then(function(html){
-          el.innerHTML = html
-          initReactions();
+  // paginators appear at the end of each block of HTML notifications sent by
+  // the server, each time we paginate we're only interested in the last one
+  const paginators = document.getElementsByClassName('notifications-paginator');
+  if (paginators && paginators.length > 0) {
+    const paginator = paginators[paginators.length - 1];
+
+    if (paginator) {
+      window
+        .fetch(paginator.dataset.paginationPath, {
+          method: 'GET',
+          credentials: 'same-origin',
+        })
+        .then(function(response) {
+          if (response.status === 200) {
+            response.text().then(function(html) {
+              const notificationsList = html.trim();
+
+              if (notificationsList) {
+                paginator.innerHTML = notificationsList;
+                initReactions();
+              } else {
+                // no more notifications to load, we hide the load more wrapper
+                const button = document.getElementById('load-more-button');
+                if (button) {
+                  button.style.display = 'none';
+                }
+              }
+            });
+          }
        });
-      }
-    });
+    }
+  }
+}
+
+function initLoadMoreButton() {
+  const button = document.getElementById('load-more-button');
+  if (button) {
+    button.addEventListener('click', initPagination);
  }
 }
--- a/app/assets/stylesheets/_mixins.scss
+++ b/app/assets/stylesheets/_mixins.scss
@ -25,3 +25,21 @@
  #{$property}: #{$fallback} !important;
  #{$property}: var(--#{$cssVariable}, #{$fallback}) !important;
 }
+
+
+/* Mixin for a load more wrapper, made by a container div and child button */
+@mixin load-more() {
+  text-align: center;
+
+  button {
+    background: transparent;
+    @include themeable(border, theme-border, 1px solid $light-medium-gray);
+    font-size: 17px;
+    padding: 14px 5px;
+    margin: 40px auto 70px;
+    width: 320px;
+    max-width: 80%;
+    border-radius: 100px;
+    font-weight: bold;
+  }
+}
--- a/app/assets/stylesheets/item-list.scss
+++ b/app/assets/stylesheets/item-list.scss
@ -198,19 +198,7 @@
    }

    .load-more-wrapper {
-      text-align: center;
-
-      button {
-        background: transparent;
-        @include themeable(border, theme-border, 1px solid $light-medium-gray);
-        font-size: 17px;
-        padding: 14px 5px;
-        margin: 40px auto 70px;
-        width: 320px;
-        max-width: 80%;
-        border-radius: 100px;
-        font-weight: bold;
-      }
+      @include load-more;
    }
  }
 }
--- a/app/assets/stylesheets/notifications.scss
+++ b/app/assets/stylesheets/notifications.scss
@ -2,6 +2,7 @@
@import 'mixins';
 .notifications-index {
  @include themeable(background, theme-background, $lightest-gray);
+
  .home {
    .articles-list {
      .signup-cue {
@ -406,4 +407,8 @@
      }
    }
  }
+
+  .load-more-wrapper {
+    @include load-more;
+  }
 }
--- a/app/controllers/notifications_controller.rb
+++ b/app/controllers/notifications_controller.rb
@ -5,12 +5,17 @@ class NotificationsController < ApplicationController

    @notifications_index = true
    @user = user_to_view
-    if params[:page]
-      num = 45
-      notified_at_offset = Notification.find(params[:page])&.notified_at
+
+    # NOTE: this controller is using offset based pagination by assuming that
+    # the id of the last notification also corresponds to the newest `notified_at`
+    # this might not be forever true but it's good enough for now
+    if params[:offset]
+      num = 30
+      notified_at_offset = Notification.find(params[:offset])&.notified_at
    else
      num = 8
    end
+
    @notifications = if (params[:org_id].present? || params[:filter] == "org") && allowed_user?
                       organization_notifications
                     elsif params[:org_id].blank? && params[:filter].present?
@ -18,11 +23,21 @@ class NotificationsController < ApplicationController
                     else
                       Notification.where(user_id: @user.id).order("notified_at DESC")
                     end
+
+    # if offset based pagination is invoked by the frontend code, we filter out all earlier ones
+    @notifications = @notifications.where("notified_at < ?", notified_at_offset) if notified_at_offset
+
+    @notifications = NotificationDecorator.decorate_collection(@notifications.limit(num))
+
    @last_user_reaction = @user.reactions.last&.id
    @last_user_comment = @user.comments.last&.id
-    @notifications = @notifications.where("notified_at < ?", notified_at_offset) if notified_at_offset
-    @notifications = NotificationDecorator.decorate_collection(@notifications.limit(num))
+
    @organizations = @user.member_organizations if @user.organizations
+
+    # The first call, the one coming from the browser URL bar will render the "index" view, which renders
+    # the first few notifications. After that the JS frontend code (see `initNotification.js`)
+    # will call this action again by sending the offset id for the last known notifications, the result
+    # will be the partial rendering of only the list of notifications that will be attached to the DOM by JS
    render partial: "notifications_list" if notified_at_offset
  end

--- a/app/views/notifications/_comment.html.erb
+++ b/app/views/notifications/_comment.html.erb
@ -25,9 +25,7 @@
      </div>
      <%= render "notifications/shared/comment_box", json_data: json_data, notification: notification, context: "default" %>
    <% elsif notification.action == "Moderation" %>
-      Hey there! <%= image_tag("emoji/apple-hugging-face.png", class: "reaction-image", alt: "Hugging face emoji") %><br /><br />
      <a href="/<%= json_data["comment"]["path"].split("/")[1] %>">@<%= json_data["comment"]["path"].split("/")[1] %></a> just left a comment. Since they are new to the community, could you leave a nice reply to help them feel welcome?
-      <br /><br />
      <b>Thank you!</b>
      <br /><br />
      <em style="font-size: 0.9em">Alternatively, if this comment violates the code of conduct, please downvote/report as appropriate.</em>
@ -38,7 +36,7 @@
        </a></b>
      <%= render "notifications/shared/comment_box", json_data: json_data, notification: notification, context: "moderation" %>
      <br>
-      <div class="footnote">All negative reactions are 100% private. Thank you for being a trusted <%= ApplicationConfig["COMMUNITY_NAME"] %> member.</div>
+      <div class="footnote">All negative reactions are 100% private.</div>
    <% elsif notification.action == "First" %>
      <a href="<%= json_data["user"]["path"] %>"><%= json_data["user"]["name"] %></a>
      wrote their first comment on:
--- a/app/views/notifications/_notifications_list.html.erb
+++ b/app/views/notifications/_notifications_list.html.erb
@ -1,12 +1,25 @@
-<% notification_count = 0 %>
-<% @notifications.each do |notification| %>
+<% num_notifications = @notifications.size %>
+
+<% @notifications.each_with_index do |notification, index| %>
  <% next if notification.notified_at < 24.hours.ago && notification.aggregated? %>
-  <% notification_count += 1 %>
-  <div class="single-article single-article-small-pic single-notification <%= "unseen" unless notification.read? %>" data-notification-id="<%= notification.id %>">
+
+  <div
+    class="single-article single-article-small-pic single-notification <%= "unseen" unless notification.read? %>"
+    data-notification-id="<%= notification.id %>">
    <%= render notification.notifiable_type.downcase.to_s, notification: notification %>
  </div>
+
+  <%# Since pagination is offset based, the fastest way to retrieve the last known notification id is to %>
+  <%# ask Ruby when it is at the end of the loop %>
+  <% if index == num_notifications - 1 %>
+    <% sub_path = params[:org_id].present? ? "#{params[:filter]}/#{params[:org_id]}" : params[:filter].to_s %>
+    <div
+      class="notifications-paginator"
+      data-pagination-path="/notifications/<%= sub_path %>?offset=<%= notification.id %>"></div>
+  <% end %>
 <% rescue => e %>
-  <% logger.error("Notifification error - #{e.message} - #{notification.id}") %>
+  <% logger.error("Notification error - #{e.message} - #{notification.id}") %>
+
  <div class="small-pic">
    <img src="https://res.cloudinary.com/practicaldev/image/fetch/s--LnVw15KE--/c_fill,f_auto,fl_progressive,h_90,q_auto,w_90/https://thepracticaldev.s3.amazonaws.com/uploads/user/profile_image/31047/af153cd6-9994-4a68-83f4-8ddf3e13f0bf.jpg"
         alt="Sloan, the sloth mascot">
--- a/app/views/notifications/index.html.erb
+++ b/app/views/notifications/index.html.erb
@ -62,9 +62,16 @@
          </div>
        <% end %>
      <% end %>
-      <%= render "notifications_list" %>
-      <% if @notifications.any? %>
-        <div id="notifications-pagination" data-pagination-path="/notifications/<%= params[:org_id].present? ? "#{params[:filter]}/#{params[:org_id]}" : params[:filter].to_s %>?page=<%= @notifications.last.id %>"></div>
+
+      <%= render "notifications_list", params: params %>
+
+      <%# not using "any?"" here because "notifications_list" already asks for ".size", a little optimization :) %>
+      <% if @notifications.size > 7 %>
+        <div class="load-more-wrapper">
+          <button id="load-more-button" type="button">
+            Load More
+          </button>
+        </div>
      <% end %>
    </div>
    <div class="side-bar sidebar-additional"></div>
--- a/spec/requests/notifications_spec.rb
+++ b/spec/requests/notifications_spec.rb
@ -391,5 +391,22 @@ RSpec.describe "NotificationsIndex", type: :request do
        expect(response.body).to include time_ago_in_words(article.published_at)
      end
    end
+
+    context "when a user is an admin" do
+      let(:admin) { create(:user, :super_admin) }
+      let(:user2)    { create(:user) }
+      let(:article)  { create(:article, user_id: user.id) }
+
+      before do
+        user2.follow(user)
+        Notification.send_to_followers_without_delay(article, "Published")
+        sign_in admin
+      end
+
+      it "can view other people's notifications" do
+        get "/notifications?username=#{user2.username}"
+        expect(response.body).to include "made a new post:"
+      end
+    end
  end
 end