From 044de6a8b5f571a02cdca3bd79dde092afe3eb93 Mon Sep 17 00:00:00 2001 From: Andy Zhao Date: Mon, 19 Nov 2018 19:33:00 -0500 Subject: [PATCH] Make reads controller async and sanitize notification titles (#1158) --- app/labor/notification_counter.rb | 1 - .../_aggregated_reactions.html.erb | 2 +- app/views/notifications/_article.html.erb | 2 +- .../notifications/_badgeachievement.html.erb | 2 +- app/views/notifications/_comment.html.erb | 6 +- app/views/notifications/_reaction.html.erb | 2 +- spec/requests/notifications_spec.rb | 106 ++++++++++-------- 7 files changed, 67 insertions(+), 54 deletions(-) diff --git a/app/labor/notification_counter.rb b/app/labor/notification_counter.rb index 4dfcbfbd1..871bcfe35 100644 --- a/app/labor/notification_counter.rb +++ b/app/labor/notification_counter.rb @@ -11,5 +11,4 @@ class NotificationCounter def set_to_zero @user.notifications.where(read?: false).update_all(read?: true) end - handle_asynchronously :set_to_zero end diff --git a/app/views/notifications/_aggregated_reactions.html.erb b/app/views/notifications/_aggregated_reactions.html.erb index 23ff493b9..0333a18e3 100644 --- a/app/views/notifications/_aggregated_reactions.html.erb +++ b/app/views/notifications/_aggregated_reactions.html.erb @@ -35,7 +35,7 @@ reacted to " class="notification-comment-reacted-link"> <%# your article/comment or the actual title of the article/comment %> - <%= reactable_data["title"].blank? ? "your #{reactable_data["class"]["name"].downcase}" : reactable_data["title"] %> + <%= reactable_data["title"].blank? ? "your #{reactable_data["class"]["name"].downcase}" : sanitize(reactable_data["title"]) %> with diff --git a/app/views/notifications/_article.html.erb b/app/views/notifications/_article.html.erb index 321c77b94..4aba377ad 100644 --- a/app/views/notifications/_article.html.erb +++ b/app/views/notifications/_article.html.erb @@ -15,7 +15,7 @@ ">
- <%= json_data["article"]["title"] %> + <%= sanitize(json_data["article"]["title"]) %>
<% json_data["article"]["cached_tag_list_array"].each do |tag| %> diff --git a/app/views/notifications/_badgeachievement.html.erb b/app/views/notifications/_badgeachievement.html.erb index 05484ada1..591b04a8b 100644 --- a/app/views/notifications/_badgeachievement.html.erb +++ b/app/views/notifications/_badgeachievement.html.erb @@ -2,7 +2,7 @@
<% cache "activity-badge-reward-#{json_data["badge_achievement"]["badge_id"]}" do %>
- You received the <%= json_data["badge_achievement"]["badge"]["title"] %> badge + You received the <%= sanitize(json_data["badge_achievement"]["badge"]["title"]) %> badge

<%= json_data["badge_achievement"]["badge"]["description"] %> diff --git a/app/views/notifications/_comment.html.erb b/app/views/notifications/_comment.html.erb index 0adcb33a7..6e86eb146 100644 --- a/app/views/notifications/_comment.html.erb +++ b/app/views/notifications/_comment.html.erb @@ -11,7 +11,7 @@ "><%= json_data["user"]["name"] %> commented on "> - <%= json_data["comment"]["commentable"]["title"] %> + <%= sanitize(json_data["comment"]["commentable"]["title"]) %> <%= render "notifications/shared/comment_box", json_data: json_data, notification: notification, context: "default" %> <% elsif notification.action == "Moderation" %> @@ -20,7 +20,7 @@

Here's the article for context: "> - <%= json_data["comment"]["commentable"]["title"] %> + <%= sanitize(json_data["comment"]["commentable"]["title"]) %> <%= render "notifications/shared/comment_box", json_data: json_data, notification: notification, context: "moderation" %>
@@ -29,7 +29,7 @@ "><%= json_data["user"]["name"] %> wrote their first comment on: "> - <%= json_data["comment"]["commentable"]["title"] %> + <%= sanitize(json_data["comment"]["commentable"]["title"]) %> <%= render "notifications/activity/activities/shared/comment_box", activity: activity, context: "moderation" %> Give them their first reply! 🎉 diff --git a/app/views/notifications/_reaction.html.erb b/app/views/notifications/_reaction.html.erb index 7d7112d01..1e7e4dbe3 100644 --- a/app/views/notifications/_reaction.html.erb +++ b/app/views/notifications/_reaction.html.erb @@ -27,7 +27,7 @@ " class="notification-comment-reacted-link"> <%# title is blank when it's a comment with only an image, for example %> - <%= first_notification.json_data["reaction"]["reactable"]["title"].blank? ? "your #{first_notification.json_data["reaction"]["reactable_type"].downcase}" : first_notification.json_data["reaction"]["reactable"]["title"] %> + <%= first_notification.json_data["reaction"]["reactable"]["title"].blank? ? "your #{first_notification.json_data["reaction"]["reactable_type"].downcase}" : sanitize(first_notification.json_data["reaction"]["reactable"]["title"]) %> diff --git a/spec/requests/notifications_spec.rb b/spec/requests/notifications_spec.rb index f9092d0d2..181c26ffc 100644 --- a/spec/requests/notifications_spec.rb +++ b/spec/requests/notifications_spec.rb @@ -4,13 +4,13 @@ RSpec.describe "NotificationsIndex", type: :request do let(:user) { create(:user) } describe "GET notifications" do - xit "renders page with the proper heading" do + it "renders page with the proper heading" do get "/notifications" expect(response.body).to include("Notifications") end context "when signed out" do - xit "renders the signup cue" do + it "renders the signup cue" do get "/notifications" expect(response.body).to include "

#{CGI.escapeHTML(User.second_to_last.name)} followed you!" expect(response.body).to include CGI.escapeHTML(follow_message) end - xit "renders the proper message for three or more notifications in the same day" do + it "renders the proper message for three or more notifications in the same day" do mock_follow_notifications(rand(3..10)) get "/notifications" follow_message = "others followed you!" expect(response.body).to include follow_message end - xit "groups two notifications on the same day" do + it "groups two notifications on the same day" do mock_follow_notifications(2) get "/notifications" - notifications = controller.instance_variable_get(:@notifications) + grouped_notifications = controller.instance_variable_get(:@notifications)[0].grouped_notifications # only one notification object containing a group of notifications - expect(notifications.count).to eq 1 + expect(grouped_notifications.count).to eq 2 end - xit "groups three or more notifications on the same day" do + it "groups three or more notifications on the same day" do amount = rand(3..10) mock_follow_notifications(amount) get "/notifications" - notifications = controller.instance_variable_get(:@notifications) - expect(notifications.count).to eq 1 + grouped_notifications = controller.instance_variable_get(:@notifications)[0].grouped_notifications + expect(grouped_notifications.count).to eq amount end - xit "does not group notifications that occur on different days" do + it "does not group notifications that occur on different days" do mock_follow_notifications(2) Notification.last.update(created_at: Notification.last.created_at - 1.day) get "/notifications" @@ -83,8 +83,9 @@ RSpec.describe "NotificationsIndex", type: :request do end context "when a user has new reaction notifications" do - let(:article1) { create(:article, user_id: user.id) } - let(:article2) { create(:article, user_id: user.id) } + let(:article1) { create(:article, user_id: user.id) } + let(:article2) { create(:article, user_id: user.id) } + let(:special_characters_article) { create(:article, user_id: user.id, title: "Nothing like good ol' blah blah blah") } before do sign_in user @@ -104,28 +105,28 @@ RSpec.describe "NotificationsIndex", type: :request do reactions.each { |reaction| Notification.send_reaction_notification_without_delay(reaction) } end - xit "renders the proper message for a single public reaction" do + it "renders the proper message for a single public reaction" do mock_heart_reaction_notifications(1, %w(like unicorn)) get "/notifications" message = "#{CGI.escapeHTML(User.last.name)} reacted to" expect(response.body).to include message end - xit "renders the proper message for a single private reaction" do + it "renders the proper message for a single private reaction" do mock_heart_reaction_notifications(1, %w(readinglist)) get "/notifications" message = "Someone reacted to" expect(response.body).to include message end - xit "renders the proper message for two or more public reactions" do + it "renders the proper message for two or more public reactions" do mock_heart_reaction_notifications(2, %w(like unicorn)) get "/notifications" message = "#{User.last.name} and #{CGI.escapeHTML(User.second_to_last.name)}\n reacted to" expect(response.body).to include CGI.escapeHTML(message) end - xit "renders the proper message for two or more reactions where at least one is private" do + it "renders the proper message for two or more reactions where at least one is private" do mock_heart_reaction_notifications(1, %w(readinglist)) mock_heart_reaction_notifications(1, %w(unicorn like)) get "/notifications" @@ -133,29 +134,29 @@ RSpec.describe "NotificationsIndex", type: :request do expect(response.body).to include CGI.escapeHTML(message) end - xit "renders the proper message for multiple public reactions" do + it "renders the proper message for multiple public reactions" do mock_heart_reaction_notifications(3, %w(unicorn like)) get "/notifications" message = "#{User.last.name} and 2 others\n reacted to" expect(response.body).to include CGI.escapeHTML(message) end - xit "properly groups two notifications that have the same day and reactable" do + it "properly groups two notifications that have the same day and reactable" do mock_heart_reaction_notifications(2, %w(unicorn like readinglist)) get "/notifications" - notifications = controller.instance_variable_get(:@notifications) - expect(notifications.count).to eq 1 + grouped_notifications = controller.instance_variable_get(:@notifications)[0].grouped_notifications + expect(grouped_notifications.count).to eq 2 end - xit "properly groups three or more notifications that have the same day and reactable" do + it "properly groups three or more notifications that have the same day and reactable" do amount = rand(3..10) mock_heart_reaction_notifications(amount, %w(unicorn like readinglist)) get "/notifications" - notifications = controller.instance_variable_get(:@notifications) - expect(notifications.count).to eq 1 + grouped_notifications = controller.instance_variable_get(:@notifications)[0].grouped_notifications + expect(grouped_notifications.count).to eq amount end - xit "does not group notifications that are on different days but same reactable" do + it "does not group notifications that are on different days but have the same reactable" do mock_heart_reaction_notifications(2, %w(unicorn like readinglist)) Notification.last.update(created_at: Notification.last.created_at - 1.day) get "/notifications" @@ -163,13 +164,26 @@ RSpec.describe "NotificationsIndex", type: :request do expect(notifications.count).to eq 2 end - xit "does not group notifications that are on the same day but different reactables" do + it "does not group notifications that are on the same day but have different reactables" do mock_heart_reaction_notifications(1, %w(unicorn like readinglist), article1) mock_heart_reaction_notifications(1, %w(unicorn like readinglist), article2) get "/notifications" notifications = controller.instance_variable_get(:@notifications) expect(notifications.count).to eq 2 end + + it "properly renders reactable titles", focus: true do + mock_heart_reaction_notifications(1, %w(unicorn like readinglist), special_characters_article) + get "/notifications" + expect(response.body).to include special_characters_article.title + end + + it "properly renders reactable titles for multiple reactions", focus: true do + amount = rand(3..10) + mock_heart_reaction_notifications(amount, %w(unicorn like readinglist), special_characters_article) + get "/notifications" + expect(response.body).to include special_characters_article.title + end end context "when a user has a new comment notification" do @@ -183,19 +197,19 @@ RSpec.describe "NotificationsIndex", type: :request do get "/notifications" end - xit "renders the correct message" do + it "renders the correct message" do expect(response.body).to include "commented on" end - xit "does not render the moderation message" do + it "does not render the moderation message" do expect(response.body).not_to include "As a trusted member" end - xit "renders the original article's title" do - expect(response.body).to include article.title + it "renders the original article's title" do + expect(response.body).to include CGI.escapeHTML(article.title) end - xit "renders the comment's processed HTML" do + it "renders the comment's processed HTML" do expect(response.body).to include CGI.escapeHTML(comment.processed_html) end end @@ -213,15 +227,15 @@ RSpec.describe "NotificationsIndex", type: :request do get "/notifications" end - xit "renders the proper message" do + it "renders the proper message" do expect(response.body).to include "As a trusted member" end - xit "renders the article's title" do + it "renders the article's title" do expect(response.body).to include CGI.escapeHTML(article.title) end - xit "renders the comment's processed HTML" do + it "renders the comment's processed HTML" do expect(response.body).to include CGI.escapeHTML(comment.processed_html) end end @@ -232,7 +246,7 @@ RSpec.describe "NotificationsIndex", type: :request do sign_in user end - xit "renders the welcome notification" do + it "renders the welcome notification" do broadcast = create(:broadcast, :onboarding) Notification.send_welcome_notification_without_delay(user.id) get "/notifications" @@ -249,20 +263,20 @@ RSpec.describe "NotificationsIndex", type: :request do get "/notifications" end - xit "renders the proper message with the badge's title" do + it "renders the proper message with the badge's title" do message = "You received the #{Badge.first.title}" expect(response.body).to include CGI.escapeHTML(message) end - xit "renders the rewarding context message" do + it "renders the rewarding context message" do expect(response.body).to include CGI.escapeHTML(user.badge_achievements.first.rewarding_context_message) end - xit "renders the badge's description" do + it "renders the badge's description" do expect(response.body).to include CGI.escapeHTML(Badge.first.description) end - xit "renders the CHECK YOUR PROFILE button" do + it "renders the CHECK YOUR PROFILE button" do expect(response.body).to include "CHECK YOUR PROFILE" end end @@ -289,11 +303,11 @@ RSpec.describe "NotificationsIndex", type: :request do get "/notifications" end - xit "renders the proper message" do + it "renders the proper message" do expect(response.body).to include "mentioned you in a comment" end - xit "renders the processed HTML of the comment where they were mentioned" do + it "renders the processed HTML of the comment where they were mentioned" do expect(response.body).to include CGI.escapeHTML(comment.processed_html) end end @@ -309,15 +323,15 @@ RSpec.describe "NotificationsIndex", type: :request do get "/notifications" end - xit "renders the proper message" do + it "renders the proper message" do expect(response.body).to include "made a new post:" end - xit "renders the article's title" do + it "renders the article's title" do expect(response.body).to include CGI.escapeHTML(article.title) end - xit "renders the author's name" do + it "renders the author's name" do expect(response.body).to include CGI.escapeHTML(article.user.name) end end