maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Notifications: increase efficiency and fix pagination offset issue (#3986)

Browse source 
* 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
This commit is contained in:
rhymes 2019-09-10 15:43:45 +02:00 • committed by Ben Halpern
parent 8d4991f906
commit 80e6cf4801
6 changed files with 70 additions and 43 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

17
app/assets/javascripts/initializers/initNotifications.js
View file

@ -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();
}
});
}

4
app/assets/stylesheets/notifications.scss
View file

@ -410,5 +410,9 @@
.load-more-wrapper {
@include load-more;
button {
margin-top: 0;
}
}
}

24
app/controllers/notifications_controller.rb
View file

@ -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

26
app/models/notification.rb
View file

@ -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)

22
app/views/notifications/_notifications_list.html.erb
View file

@ -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| %>
<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("Notification error - #{e.message} - #{notification.id}") %>
@ -30,3 +17,10 @@
If you see too many of these, please report it to <a href="mailto:yo@dev.to">yo@dev.to</a>!
</div>
<% end %>
<% if @notifications.any? %>
<% 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=<%= @notifications.last.id %>"></div>
<% end %>

20
app/views/notifications/index.html.erb
View file

@ -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 %>
<div class="load-more-wrapper">
<button id="load-more-button" type="button">
Load More
</button>
</div>
<% end %>
</div>
<%# 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 %>
<div class="load-more-wrapper">
<button id="load-more-button" type="button">
Load More
</button>
</div>
<% end %>
<div class="side-bar sidebar-additional"></div>
</div>
<%= render "articles/fitvids" %>