From 15d1e981c86b6590d980585ee2a5755a234e4d55 Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Thu, 17 Jan 2019 23:46:04 +0100 Subject: [PATCH] DEV: Improve specs * notifications were created for the wrong user * notifications didn't have a correct data attribute --- spec/fabricators/notification_fabricator.rb | 47 ++++++- spec/mailers/user_notifications_spec.rb | 140 +++++++++++--------- 2 files changed, 120 insertions(+), 67 deletions(-) diff --git a/spec/fabricators/notification_fabricator.rb b/spec/fabricators/notification_fabricator.rb index 6c6993c0a95..804558325ea 100644 --- a/spec/fabricators/notification_fabricator.rb +++ b/spec/fabricators/notification_fabricator.rb @@ -1,8 +1,9 @@ Fabricator(:notification) do + transient :post notification_type Notification.types[:mentioned] - data '{"poison":"ivy","killer":"croc"}' user - topic { |attrs| Fabricate(:topic, user: attrs[:user]) } + topic { |attrs| attrs[:post]&.topic || Fabricate(:topic, user: attrs[:user]) } + data '{"poison":"ivy","killer":"croc"}' end Fabricator(:quote_notification, from: :notification) do @@ -13,6 +14,44 @@ end Fabricator(:private_message_notification, from: :notification) do notification_type Notification.types[:private_message] - user - topic { |attrs| Fabricate(:topic, user: attrs[:user]) } + data do |attrs| + post = attrs[:post] || Fabricate(:post, topic: attrs[:topic], user: attrs[:user]) + { + topic_title: attrs[:topic].title, + original_post_id: post.id, + original_post_type: post.post_type, + original_username: post.user.username, + revision_number: nil, + display_username: post.user.username + }.to_json + end +end + +Fabricator(:replied_notification, from: :notification) do + notification_type Notification.types[:replied] + data do |attrs| + post = attrs[:post] || Fabricate(:post, topic: attrs[:topic], user: attrs[:user]) + { + topic_title: attrs[:topic].title, + original_post_id: post.id, + original_username: post.user.username, + revision_number: nil, + display_username: post.user.username + }.to_json + end +end + +Fabricator(:posted_notification, from: :notification) do + notification_type Notification.types[:posted] + data do |attrs| + post = attrs[:post] || Fabricate(:post, topic: attrs[:topic], user: attrs[:user]) + { + topic_title: attrs[:topic].title, + original_post_id: post.id, + original_post_type: post.post_type, + original_username: post.user.username, + revision_number: nil, + display_username: post.user.username + }.to_json + end end diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index 4c350124ee9..5bd593733f7 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/spec/mailers/user_notifications_spec.rb @@ -114,7 +114,7 @@ describe UserNotifications do end context "with topics only from new users" do - let!(:new_today) { Fabricate(:topic, user: Fabricate(:user, trust_level: TrustLevel[0], created_at: 10.minutes.ago), title: "Hey everyone look at me") } + let!(:new_today) { Fabricate(:topic, user: Fabricate(:user, trust_level: TrustLevel[0], created_at: 10.minutes.ago), title: "Hey everyone look at me") } let!(:new_yesterday) { Fabricate(:topic, user: Fabricate(:user, trust_level: TrustLevel[0], created_at: 25.hours.ago), created_at: 25.hours.ago, title: "This topic is of interest to you") } it "returns topics from new users if they're more than 24 hours old" do @@ -187,14 +187,14 @@ describe UserNotifications do it "uses theme color" do cs = Fabricate(:color_scheme, name: 'Fancy', color_scheme_colors: [ - Fabricate(:color_scheme_color, name: 'header_primary', hex: 'F0F0F0'), + Fabricate(:color_scheme_color, name: 'header_primary', hex: 'F0F0F0'), Fabricate(:color_scheme_color, name: 'header_background', hex: '1E1E1E'), Fabricate(:color_scheme_color, name: 'tertiary', hex: '858585') ]) theme = Fabricate(:theme, - user_selectable: true, - user: Fabricate(:admin), - color_scheme_id: cs.id + user_selectable: true, + user: Fabricate(:admin), + color_scheme_id: cs.id ) theme.set_default! @@ -229,11 +229,11 @@ describe UserNotifications do let(:category) { Fabricate(:category, name: 'India') } let(:tag1) { Fabricate(:tag, name: 'Taggo') } let(:tag2) { Fabricate(:tag, name: 'Taggie') } - let(:topic) { Fabricate(:topic, category: category, tags: [tag1, tag2]) } + let(:topic) { Fabricate(:topic, category: category, tags: [tag1, tag2], title: "Super cool topic") } let(:post) { Fabricate(:post, topic: topic, raw: 'This is My super duper cool topic') } - let(:response) { Fabricate(:post, reply_to_post_number: 1, topic: post.topic, user: response_by_user) } + let(:response) { Fabricate(:basic_reply, topic: post.topic, user: response_by_user) } let(:user) { Fabricate(:user) } - let(:notification) { Fabricate(:notification, user: user) } + let(:notification) { Fabricate(:replied_notification, user: user, post: response) } it 'generates a correct email' do @@ -241,11 +241,13 @@ describe UserNotifications do SiteSetting.email_subject = "[%{site_name}] %{optional_pm}%{optional_cat}%{optional_tags}%{topic_title}" SiteSetting.enable_names = true SiteSetting.display_name_on_posts = true - mail = UserNotifications.user_replied(response.user, - post: response, - notification_type: notification.notification_type, - notification_data_hash: notification.data_hash - ) + mail = UserNotifications.user_replied( + user, + post: response, + notification_type: notification.notification_type, + notification_data_hash: notification.data_hash + ) + # from should include full user name expect(mail[:from].display_names).to eql(['John Doe via Discourse']) @@ -271,16 +273,17 @@ describe UserNotifications do expect(mail_html.scan(/To unsubscribe/).count).to eq(1) # side effect, topic user is updated with post number - tu = TopicUser.get(post.topic_id, response.user) + tu = TopicUser.get(post.topic_id, user) expect(tu.last_emailed_post_number).to eq(response.post_number) # no In Reply To if user opts out - response.user.user_option.email_in_reply_to = false - mail = UserNotifications.user_replied(response.user, - post: response, - notification_type: notification.notification_type, - notification_data_hash: notification.data_hash - ) + user.user_option.email_in_reply_to = false + mail = UserNotifications.user_replied( + user, + post: response, + notification_type: notification.notification_type, + notification_data_hash: notification.data_hash + ) expect(mail.html_part.to_s.scan(/In Reply To/).count).to eq(0) @@ -292,11 +295,12 @@ describe UserNotifications do response.user.name = "Bob Marley" response.user.save - mail = UserNotifications.user_replied(response.user, - post: response, - notification_type: notification.notification_type, - notification_data_hash: notification.data_hash - ) + mail = UserNotifications.user_replied( + user, + post: response, + notification_type: notification.notification_type, + notification_data_hash: notification.data_hash + ) mail_html = mail.html_part.to_s expect(mail_html.scan(/>Bob Marley/).count).to eq(1) @@ -304,11 +308,12 @@ describe UserNotifications do SiteSetting.prioritize_username_in_ux = true - mail = UserNotifications.user_replied(response.user, - post: response, - notification_type: notification.notification_type, - notification_data_hash: notification.data_hash - ) + mail = UserNotifications.user_replied( + user, + post: response, + notification_type: notification.notification_type, + notification_data_hash: notification.data_hash + ) mail_html = mail.html_part.to_s expect(mail_html.scan(/>Bob Marley/).count).to eq(0) @@ -318,7 +323,7 @@ describe UserNotifications do it "doesn't include details when private_email is enabled" do SiteSetting.private_email = true mail = UserNotifications.user_replied( - response.user, + user, post: response, notification_type: notification.notification_type, notification_data_hash: notification.data_hash @@ -333,18 +338,20 @@ describe UserNotifications do describe '.user_posted' do let(:response_by_user) { Fabricate(:user, name: "John Doe", username: "john") } - let(:post) { Fabricate(:post) } - let(:response) { Fabricate(:post, topic: post.topic, user: response_by_user) } + let(:topic) { Fabricate(:topic, title: "Super cool topic") } + let(:post) { Fabricate(:post, topic: topic) } + let(:response) { Fabricate(:post, topic: topic, user: response_by_user) } let(:user) { Fabricate(:user) } - let(:notification) { Fabricate(:notification, user: user, data: { original_username: response_by_user.username }.to_json) } + let(:notification) { Fabricate(:posted_notification, user: user, post: response) } it 'generates a correct email' do SiteSetting.enable_names = false - mail = UserNotifications.user_posted(response.user, - post: response, - notification_type: notification.notification_type, - notification_data_hash: notification.data_hash - ) + mail = UserNotifications.user_posted( + user, + post: response, + notification_type: notification.notification_type, + notification_data_hash: notification.data_hash + ) # from should not include full user name if "show user full names" is disabled expect(mail[:from].display_names).to_not eql(['John Doe']) @@ -362,14 +369,14 @@ describe UserNotifications do expect(mail.html_part.to_s.scan(/To unsubscribe/).count).to eq(1) # side effect, topic user is updated with post number - tu = TopicUser.get(post.topic_id, response.user) + tu = TopicUser.get(post.topic_id, user) expect(tu.last_emailed_post_number).to eq(response.post_number) end it "doesn't include details when private_email is enabled" do SiteSetting.private_email = true mail = UserNotifications.user_posted( - response.user, + user, post: response, notification_type: notification.notification_type, notification_data_hash: notification.data_hash @@ -385,12 +392,12 @@ describe UserNotifications do let(:topic) { Fabricate(:private_message_topic) } let(:response) { Fabricate(:post, topic: topic, user: response_by_user) } let(:user) { Fabricate(:user) } - let(:notification) { Fabricate(:notification, user: user, data: { original_username: response_by_user.username }.to_json) } + let(:notification) { Fabricate(:private_message_notification, user: user, post: response) } it 'generates a correct email' do SiteSetting.enable_names = true mail = UserNotifications.user_private_message( - response.user, + user, post: response, notification_type: notification.notification_type, notification_data_hash: notification.data_hash @@ -412,14 +419,14 @@ describe UserNotifications do expect(mail.html_part.to_s.scan(/To unsubscribe/).count).to eq(1) # side effect, topic user is updated with post number - tu = TopicUser.get(topic.id, response.user) + tu = TopicUser.get(topic.id, user) expect(tu.last_emailed_post_number).to eq(response.post_number) end it "doesn't include details when private_email is enabled" do SiteSetting.private_email = true mail = UserNotifications.user_private_message( - response.user, + user, post: response, notification_type: notification.notification_type, notification_data_hash: notification.data_hash @@ -433,9 +440,9 @@ describe UserNotifications do it "doesn't include group name in subject" do group = Fabricate(:group) - topic.allowed_groups = [ group ] + topic.allowed_groups = [group] mail = UserNotifications.user_private_message( - response.user, + user, post: response, notification_type: notification.notification_type, notification_data_hash: notification.data_hash @@ -455,7 +462,7 @@ describe UserNotifications do topic.allowed_groups = [group1, group2] mail = UserNotifications.user_private_message( - response.user, + user, post: response, notification_type: notification.notification_type, notification_data_hash: notification.data_hash @@ -470,12 +477,14 @@ describe UserNotifications do end let(:group) { Fabricate(:group, name: "my_group") } - let(:mail) { UserNotifications.user_private_message( - response.user, - post: response, - notification_type: notification.notification_type, - notification_data_hash: notification.data_hash - ) } + let(:mail) do + UserNotifications.user_private_message( + user, + post: response, + notification_type: notification.notification_type, + notification_data_hash: notification.data_hash + ) + end shared_examples "includes first group name" do it "includes first group name in subject" do @@ -493,7 +502,7 @@ describe UserNotifications do context "one group in pm" do before do - topic.allowed_groups = [ group ] + topic.allowed_groups = [group] end include_examples "includes first group name" @@ -503,7 +512,7 @@ describe UserNotifications do let(:group2) { Fabricate(:group) } before do - topic.allowed_groups = [ group, group2 ] + topic.allowed_groups = [group, group2] end include_examples "includes first group name" @@ -530,8 +539,13 @@ describe UserNotifications do post = Fabricate(:post) reply = Fabricate(:post, topic_id: post.topic_id) - notification = Fabricate(:notification, topic_id: post.topic_id, post_number: reply.post_number, - user: post.user, data: { original_username: 'bob' }.to_json) + notification = Fabricate( + :notification, + topic_id: post.topic_id, + post_number: reply.post_number, + user: post.user, + data: { original_username: 'bob' }.to_json + ) mail = UserNotifications.user_replied( user, @@ -677,11 +691,11 @@ describe UserNotifications do context "when customized" do let(:custom_body) do body = <<~BODY - You are now officially notified. - %{header_instructions} - %{message} %{respond_instructions} - %{topic_title_url_encoded} - %{site_title_url_encoded} + You are now officially notified. + %{header_instructions} + %{message} %{respond_instructions} + %{topic_title_url_encoded} + %{site_title_url_encoded} BODY body << "%{context}" if notification_type != :invited_to_topic