From 80e6cf4801dc6a0c956ded9d64ab8b525e4d4429 Mon Sep 17 00:00:00 2001 From: rhymes Date: Tue, 10 Sep 2019 15:43:45 +0200 Subject: [PATCH] Notifications: increase efficiency and fix pagination offset issue (#3986) * Increase efficiency and fix pagination offset issue Right now we filter recently aggregated notifications at runtime, which worked before when there was no pagination, but now filtering is not a good idea combined with offset pagination because it throws off the calculations. The filtering should be done in the SQL query before paginating, which is what we've switched to. I added a bunch of scopes to make the code clearer as well. To avoid nesting tags I've also updated the HTML and the JS logic to insert it in the right place. * Add explanation for hiding the load more button for new users --- .../initializers/initNotifications.js | 17 ++++++++---- app/assets/stylesheets/notifications.scss | 4 +++ app/controllers/notifications_controller.rb | 24 ++++++++--------- app/models/notification.rb | 26 ++++++++++++++++--- .../_notifications_list.html.erb | 22 ++++++---------- app/views/notifications/index.html.erb | 20 +++++++------- 6 files changed, 70 insertions(+), 43 deletions(-) diff --git a/app/assets/javascripts/initializers/initNotifications.js b/app/assets/javascripts/initializers/initNotifications.js index 7b65387dd..ab7de6e1e 100644 --- a/app/assets/javascripts/initializers/initNotifications.js +++ b/app/assets/javascripts/initializers/initNotifications.js @@ -156,8 +156,7 @@ function listenForNotificationsBellClick() { } function initPagination() { - // 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 + // paginators appear after each block of HTML notifications sent by the server const paginators = document.getElementsByClassName('notifications-paginator'); if (paginators && paginators.length > 0) { const paginator = paginators[paginators.length - 1]; @@ -171,10 +170,17 @@ function initPagination() { .then(function(response) { if (response.status === 200) { response.text().then(function(html) { - const notificationsList = html.trim(); + const markup = html.trim(); + + if (markup) { + const container = document.getElementById('articles-list'); + + const newNotifications = document.createElement('div'); + newNotifications.innerHTML = markup; + + paginator.remove(); + container.append(newNotifications); - if (notificationsList) { - paginator.innerHTML = notificationsList; initReactions(); } else { // no more notifications to load, we hide the load more wrapper @@ -182,6 +188,7 @@ function initPagination() { if (button) { button.style.display = 'none'; } + paginator.remove(); } }); } diff --git a/app/assets/stylesheets/notifications.scss b/app/assets/stylesheets/notifications.scss index ae2c7ccb4..d4c7a18a0 100644 --- a/app/assets/stylesheets/notifications.scss +++ b/app/assets/stylesheets/notifications.scss @@ -410,5 +410,9 @@ .load-more-wrapper { @include load-more; + + button { + margin-top: 0; + } } } diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index 555bfe674..f82deeefc 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -6,6 +6,8 @@ class NotificationsController < ApplicationController @notifications_index = true @user = user_to_view + @initial_page_size = 8 + # 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 @@ -13,7 +15,7 @@ class NotificationsController < ApplicationController num = 30 notified_at_offset = Notification.find(params[:offset])&.notified_at else - num = 8 + num = @initial_page_size end @notifications = if (params[:org_id].present? || params[:filter] == "org") && allowed_user? @@ -21,9 +23,11 @@ class NotificationsController < ApplicationController elsif params[:org_id].blank? && params[:filter].present? filtered_notifications else - Notification.where(user_id: @user.id).order("notified_at DESC") + @user.notifications end + @notifications = @notifications.without_recently_aggregated_reactions_and_follows.order(notified_at: :desc) + # 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 @@ -53,23 +57,19 @@ class NotificationsController < ApplicationController def filtered_notifications if params[:filter].to_s.casecmp("posts").zero? - Notification.where(user_id: @user.id, notifiable_type: "Article", action: "Published"). - order("notified_at DESC") + @user.notifications.for_published_articles elsif params[:filter].to_s.casecmp("comments").zero? - Notification.where(user_id: @user.id, notifiable_type: "Comment", action: nil). # Nil action means not reaction in this context - or(Notification.where(user_id: @user.id, notifiable_type: "Mention")). - order("notified_at DESC") + @user.notifications.for_comments.or(@user.notifications.for_mentions) end end def organization_notifications + org_id = params[:org_id] + if params[:filter].to_s.casecmp("comments").zero? - Notification.where(organization_id: params[:org_id], notifiable_type: "Comment", action: nil, user_id: nil). # Nil action means not reaction in this context - or(Notification.where(organization_id: params[:org_id], notifiable_type: "Mention", user_id: nil)). - order("notified_at DESC") + Notification.for_organization_comments(org_id).or(Notification.for_organization_mentions(org_id)) else - Notification.where(organization_id: params[:org_id], user_id: nil). - order("notified_at DESC") + Notification.for_organization(org_id) end end diff --git a/app/models/notification.rb b/app/models/notification.rb index 052475a84..b0f2c5aae 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -3,12 +3,32 @@ class Notification < ApplicationRecord belongs_to :user, optional: true belongs_to :organization, optional: true - validates :user_id, presence: true, if: proc { |notifcation| notifcation.organization_id.nil? } - validates :organization_id, presence: true, if: proc { |notifcation| notifcation.user_id.nil? } + validates :user_id, presence: true, if: proc { |notification| notification.organization_id.nil? } + validates :organization_id, presence: true, if: proc { |notification| notification.user_id.nil? } + validates :user_id, uniqueness: { scope: %i[organization_id notifiable_id notifiable_type action] } before_create :mark_notified_at_time - validates :user_id, uniqueness: { scope: %i[organization_id notifiable_id notifiable_type action] } + scope :for_published_articles, -> { where(notifiable_type: "Article", action: "Published") } + scope :for_comments, -> { where(notifiable_type: "Comment", action: nil) } # nil action means "not a reaction" + scope :for_mentions, -> { where(notifiable_type: "Mention") } + + scope :for_organization, ->(org_id) { where(organization_id: org_id, user_id: nil) } + scope :for_organization_comments, lambda { |org_id| + # nil action means "not a reaction" + where(organization_id: org_id, notifiable_type: "Comment", action: nil, user_id: nil) + } + scope :for_organization_mentions, lambda { |org_id| + where(organization_id: org_id, notifiable_type: "Mention", user_id: nil) + } + + scope :with_recently_aggregated_reactions_and_follows, lambda { + where("notified_at < ?", 24.hours.ago). + where(action: %w[Reaction Follow]) + } + scope :without_recently_aggregated_reactions_and_follows, lambda { + where.not(with_recently_aggregated_reactions_and_follows.arel.exists) + } class << self def send_new_follower_notification(follow, is_read = false) diff --git a/app/views/notifications/_notifications_list.html.erb b/app/views/notifications/_notifications_list.html.erb index 3ba382ebc..7924d6196 100644 --- a/app/views/notifications/_notifications_list.html.erb +++ b/app/views/notifications/_notifications_list.html.erb @@ -1,22 +1,9 @@ -<% num_notifications = @notifications.size %> - -<% @notifications.each_with_index do |notification, index| %> - <% next if notification.notified_at < 24.hours.ago && notification.aggregated? %> - +<% @notifications.each do |notification| %>
" data-notification-id="<%= notification.id %>"> <%= render notification.notifiable_type.downcase.to_s, notification: notification %>
- - <%# 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 %> -
- <% end %> <% rescue => e %> <% logger.error("Notification error - #{e.message} - #{notification.id}") %> @@ -30,3 +17,10 @@ If you see too many of these, please report it to yo@dev.to! <% end %> + +<% if @notifications.any? %> + <% sub_path = params[:org_id].present? ? "#{params[:filter]}/#{params[:org_id]}" : params[:filter].to_s %> +
+<% end %> diff --git a/app/views/notifications/index.html.erb b/app/views/notifications/index.html.erb index 9519df54f..f5eb73cc0 100644 --- a/app/views/notifications/index.html.erb +++ b/app/views/notifications/index.html.erb @@ -64,16 +64,18 @@ <% end %> <%= render "notifications_list", params: params %> - - <%# not using "any?"" here because "notifications_list" already asks for ".size", a little optimization :) %> - <% if @notifications.size > 7 %> -
- -
- <% end %> + + <%# new or less active users that don't have a page worth of notifications + won't be shown the "load more" button %> + <% if @notifications.size >= @initial_page_size %> +
+ +
+ <% end %> +
<%= render "articles/fitvids" %>