From 7dd2ae9fe092c0415e6bf85fa687142de4453e6e Mon Sep 17 00:00:00 2001 From: rhymes Date: Thu, 9 Jan 2020 16:19:45 +0100 Subject: [PATCH] Remove notifiable access from articles and comments notifications (#5415) [deploy] * Add created_at to comment notification data * Remove notifiable usage from notifications partials * Fix typo --- app/controllers/notifications_controller.rb | 6 ----- app/services/notifications.rb | 1 + app/services/notifications/update.rb | 4 +++- app/views/notifications/_article.html.erb | 6 +---- app/views/notifications/_comment.html.erb | 8 ++++--- .../notifications/new_comment/send_spec.rb | 24 +++++++++++++++++++ 6 files changed, 34 insertions(+), 15 deletions(-) diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index 18768ca2e..1db06617f 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -33,12 +33,6 @@ class NotificationsController < ApplicationController @notifications = @notifications.limit(num) - # after notifications are paginated/limited, we check if any of the notifiables is "Comment" with no action - # because its respective view is the only one that actually uses ".notifiable" to render, - # this way we save useless eager loadings - notifiable_eager_loading_params = { notifiable_type: %w[Comment], action: [nil, ""] } - @notifications = @notifications.includes(:notifiable) if @notifications.exists?(notifiable_eager_loading_params) - @notifications = NotificationDecorator.decorate_collection(@notifications) @last_user_reaction = @user.reactions.last&.id diff --git a/app/services/notifications.rb b/app/services/notifications.rb index a4468cf36..fc73cbb01 100644 --- a/app/services/notifications.rb +++ b/app/services/notifications.rb @@ -18,6 +18,7 @@ module Notifications class: { name: "Comment" }, path: comment.path, processed_html: comment.processed_html, + created_at: comment.created_at, updated_at: comment.updated_at, ancestry: comment.ancestry, depth: comment.depth, diff --git a/app/services/notifications/update.rb b/app/services/notifications/update.rb index a5f4c2492..3ea4a555a 100644 --- a/app/services/notifications/update.rb +++ b/app/services/notifications/update.rb @@ -24,7 +24,9 @@ module Notifications new_json_data = notifications.first.json_data || {} new_json_data[notifiable.class.name.downcase] = public_send("#{notifiable.class.name.downcase}_data", notifiable) new_json_data[:user] = user_data(notifiable.user) - new_json_data[:organization] = organization_data(notifiable.organization) if notifiable.is_a?(Article) && notifiable.organization_id + add_organization_data = notifiable.is_a?(Article) && notifiable.organization_id + new_json_data[:organization] = organization_data(notifiable.organization) if add_organization_data + notifications.update_all(json_data: new_json_data) end diff --git a/app/views/notifications/_article.html.erb b/app/views/notifications/_article.html.erb index ba0d46cf2..fde275ee7 100644 --- a/app/views/notifications/_article.html.erb +++ b/app/views/notifications/_article.html.erb @@ -29,11 +29,7 @@ <% end %>
- <% if json_data["article"]["published_at"] %> - <%= time_ago_in_words json_data["article"]["published_at"] %> ago - <% else %> - <%= time_ago_in_words notification.notifiable.published_at %> ago - <% end %> + <%= time_ago_in_words json_data["article"]["published_at"] %> ago
"> diff --git a/app/views/notifications/_comment.html.erb b/app/views/notifications/_comment.html.erb index bf791e6da..688e89d7f 100644 --- a/app/views/notifications/_comment.html.erb +++ b/app/views/notifications/_comment.html.erb @@ -20,9 +20,11 @@ "> <%= h(json_data["comment"]["commentable"]["title"]) %> -
- <%= time_ago_in_words notification.notifiable.created_at %> ago -
+ <% if json_data["comment"]["created_at"] %> +
+ <%= time_ago_in_words json_data["comment"]["created_at"] %> ago +
+ <% end %> <%= render "notifications/shared/comment_box", json_data: json_data, notification: notification, context: "default" %> <% elsif notification.action == "Moderation" %> ">@<%= json_data["comment"]["path"].split("/")[1] %> just left a comment. Since they are new to the community, could you leave a nice reply to help them feel welcome? diff --git a/spec/services/notifications/new_comment/send_spec.rb b/spec/services/notifications/new_comment/send_spec.rb index 737662272..781717378 100644 --- a/spec/services/notifications/new_comment/send_spec.rb +++ b/spec/services/notifications/new_comment/send_spec.rb @@ -17,6 +17,30 @@ RSpec.describe Notifications::NewComment::Send, type: :service do end.to change(Notification, :count).by(2) end + it "creates a correct user notification" do + described_class.call(child_comment) + + notification = child_comment.notifications.last + + expect(notification.action).to be_nil + expect(notification.json_data["user"]["id"]).to eq(child_comment.user.id) + expect(notification.json_data["user"]["username"]).to eq(child_comment.user.username) + end + + it "creates the correct comment data for the notification" do + described_class.call(child_comment) + + notification = child_comment.notifications.last + json_data = notification.json_data + + expect(json_data["comment"]["id"]).to eq(child_comment.id) + expect(Time.zone.parse(json_data["comment"]["created_at"]).to_i).to eq(child_comment.created_at.to_i) + expect(Time.zone.parse(json_data["comment"]["updated_at"]).to_i).to eq(child_comment.updated_at.to_i) + expect(json_data["comment"]["ancestors"]).to be_present + expect(json_data["comment"]["commentable"]).to be_present + expect(json_data["comment"]["processed_html"]).to be_present + end + it "creates notifications for the article author and the parent comment author" do described_class.call(child_comment) child_comment.reload