From 2c5e12268f3c9e273bd88edbb375208095f435b8 Mon Sep 17 00:00:00 2001 From: Mac Siri Date: Fri, 27 Mar 2020 15:21:40 -0400 Subject: [PATCH] Implement authentication welcome notification (#6846) --- .../welcome_notification/generator.rb | 47 ++++- spec/factories/broadcasts.rb | 26 ++- spec/requests/notifications_spec.rb | 4 +- .../welcome_notification/generator_spec.rb | 185 +++++++++++------- .../welcome_notification_worker_spec.rb | 4 +- 5 files changed, 178 insertions(+), 88 deletions(-) diff --git a/app/services/broadcasts/welcome_notification/generator.rb b/app/services/broadcasts/welcome_notification/generator.rb index 584df3b9b..143ef48fe 100644 --- a/app/services/broadcasts/welcome_notification/generator.rb +++ b/app/services/broadcasts/welcome_notification/generator.rb @@ -1,9 +1,9 @@ -# Generates a broadcast to be delivered as a notification. module Broadcasts module WelcomeNotification class Generator def initialize(receiver_id) @user = User.find(receiver_id) + @notification_enqueued = false end def self.call(*args) @@ -13,13 +13,31 @@ module Broadcasts def call # TODO: [@thepracticaldev/delightful] Move this check into the rake task logic once it has been implemented. return unless user.welcome_notifications - return if commented_on_welcome_thread? || received_notification? - Notification.send_welcome_notification(user.id, welcome_broadcast.id) + send_welcome_notification unless notification_enqueued + send_authentication_notification unless notification_enqueued end - def received_notification? - Notification.exists?(notifiable: welcome_broadcast, user: user) + private + + attr_reader :user, :notification_enqueued + + def send_welcome_notification + return if received_notification?(welcome_broadcast) || commented_on_welcome_thread? || user.created_at > 3.hours.ago + + Notification.send_welcome_notification(user.id, welcome_broadcast.id) + @notification_enqueued = true + end + + def send_authentication_notification + return if authenticated_with_all_providers? || received_notification?(authentication_broadcast) || user.created_at > 1.day.ago + + Notification.send_welcome_notification(user.id, authentication_broadcast.id) + @notification_enqueued = true + end + + def received_notification?(broadcast) + Notification.exists?(notifiable: broadcast, user: user) end def commented_on_welcome_thread? @@ -27,13 +45,28 @@ module Broadcasts Comment.where(commentable: welcome_thread, user: user).any? end - private + def authenticated_with_all_providers? + identities.count == SiteConfig.authentication_providers.count + end def welcome_broadcast @welcome_broadcast ||= Broadcast.find_by(title: "Welcome Notification: welcome_thread") end - attr_reader :user + def identities + @identities ||= user.identities.where(provider: SiteConfig.authentication_providers) + end + + def authentication_broadcast + @authentication_broadcast ||= find_broadcast + end + + def find_broadcast + missing_identities = SiteConfig.authentication_providers.map do |provider| + identities.exists?(provider: provider) ? nil : "#{provider}_connect" + end.compact + Broadcast.find_by(title: "Welcome Notification: #{missing_identities.first}") + end end end end diff --git a/spec/factories/broadcasts.rb b/spec/factories/broadcasts.rb index a91005a63..3547eb6cf 100644 --- a/spec/factories/broadcasts.rb +++ b/spec/factories/broadcasts.rb @@ -1,22 +1,30 @@ FactoryBot.define do factory :broadcast do - active { false } + active { true } factory :welcome_broadcast do - title { "Welcome Notification: welcome_thread" } - type_of { "Welcome" } + title { "Welcome Notification: welcome_thread" } + type_of { "Welcome" } processed_html { "Sloan here again! 👋 DEV is a friendly community. Why not introduce yourself by leaving a comment in the welcome thread!" } end + factory :twitter_connect_broadcast do + title { "Welcome Notification: twitter_connect" } + type_of { "Welcome" } + processed_html { "You're on a roll! 🎉 Let's connect your Twitter account to complete your identity so that we don't think you're a robot. 🤖" } + end + + factory :github_connect_broadcast do + title { "Welcome Notification: github_connect" } + type_of { "Welcome" } + processed_html { "You're on a roll! 🎉 Let's connect your GitHub account to complete your identity so that we don't think you're a robot. 🤖" } + end + # TODO: [@thepracticaldev/delightful] Remove onboarding factory once welcome notifications are live. factory :onboarding_broadcast do - title { "Welcome Notification" } - type_of { "Onboarding" } + title { "Welcome Notification" } + type_of { "Onboarding" } processed_html { "Welcome! Introduce yourself in our welcome thread!" } end end - - trait :active do - active { true } - end end diff --git a/spec/requests/notifications_spec.rb b/spec/requests/notifications_spec.rb index f2fbbe1e7..cb78a6993 100644 --- a/spec/requests/notifications_spec.rb +++ b/spec/requests/notifications_spec.rb @@ -593,8 +593,8 @@ RSpec.describe "NotificationsIndex", type: :request do context "when a user has a new welcome notification" do # TODO: [@thepracticaldev/delightful] Only test against type_of Welcome once Onbarding notifications have been removed. - let(:active_broadcast) { create(:onboarding_broadcast, :active) } - let(:inactive_broadcast) { create(:onboarding_broadcast) } + let(:active_broadcast) { create(:onboarding_broadcast) } + let(:inactive_broadcast) { create(:onboarding_broadcast, active: false) } before { sign_in user } diff --git a/spec/services/broadcasts/welcome_notification/generator_spec.rb b/spec/services/broadcasts/welcome_notification/generator_spec.rb index a29fcb882..7bf68b495 100644 --- a/spec/services/broadcasts/welcome_notification/generator_spec.rb +++ b/spec/services/broadcasts/welcome_notification/generator_spec.rb @@ -1,77 +1,126 @@ require "rails_helper" -# TODO: [@thepracticaldev/delightful] Reuse this shared example across all notifications, -# since it should be tested against every kind of broadcast we could send. -RSpec.shared_examples "unsubscribed from welcome notifications" do |_broadcast| - it "does not send a notification to an unsubscribed user" do - expect do - sidekiq_perform_enqueued_jobs { described_class.call(unsubscribed_user.id) } - end.to not_change(unsubscribed_user.notifications, :count) - end -end - RSpec.describe Broadcasts::WelcomeNotification::Generator, type: :service do + let(:mascot_account) { create(:user) } + let!(:welcome_thread) { create(:article, user: mascot_account, published: true, tags: "welcome") } + let!(:welcome_broadcast) { create(:welcome_broadcast) } + let!(:twitter_connect_broadcast) { create(:twitter_connect_broadcast) } + let!(:github_connect_broadcast) { create(:github_connect_broadcast) } + + before do + allow(User).to receive(:mascot_account).and_return(mascot_account) + SiteConfig.staff_user_id = mascot_account.id + end + + after do + # SiteConfig.clear_cache should work here but for some reason it isn't + SiteConfig.staff_user_id = 1 + end + + it "requires a valid user id" do + expect { described_class.call(1) }.to raise_error(ActiveRecord::RecordNotFound) + end + describe "::call" do - let(:user) { create(:user) } - let(:unsubscribed_user) { create(:user, welcome_notifications: false) } - let(:mascot_account) { create(:user) } - let!(:welcome_broadcast) { create(:welcome_broadcast, :active) } - - before do - allow(User).to receive(:mascot_account).and_return(mascot_account) - SiteConfig.staff_user_id = mascot_account.id + it "does not send a notification to an unsubscribed user" do + user = create(:user, :with_identity, identities: ["github"], created_at: 1.week.ago, welcome_notifications: false) + expect do + sidekiq_perform_enqueued_jobs { described_class.call(user.id) } + end.to not_change(user.notifications, :count) end - after do - SiteConfig.staff_user_id = 1 - end - - context "when sending a set_up_profile notification" do - xit "generates the appropriate broadcast to be sent to a user" - xit "it sends a welcome notification for that broadcast" - xit "it does not send duplicate welcome notification for that broadcast" - xit "does not send a notification to a user who has set up their profile" - end - - context "when sending a welcome_thread notification" do - it "generates the correct broadcast type and sends the notification to the user", :aggregate_failures do - expect do - sidekiq_perform_enqueued_jobs { described_class.call(user.id) } - end.to change(user.notifications, :count).by(1) - - expect(user.notifications.first.notifiable).to eq(welcome_broadcast) - end - - it "does not send a notification to a user who has commented in a welcome thread", :aggregate_failures do - welcome_thread_article = create(:article, title: "Welcome Thread - v0", published: true, tags: "welcome", user: mascot_account) - create(:comment, commentable: welcome_thread_article, commentable_type: "Article", user: user) - - expect do - sidekiq_perform_enqueued_jobs { described_class.call(user.id) } - end.to not_change(user.notifications, :count) - end - - it "does not send a duplicate notification" do - 2.times { sidekiq_perform_enqueued_jobs { described_class.call(user.id) } } - - expect(user.notifications.count).to eq(1) - end - - it_behaves_like "unsubscribed from welcome notifications" - end - - context "when sending a twitter_connect notification" do - xit "generates the appropriate broadcast to be sent to a user" - xit "it sends a welcome notification for that broadcast" - xit "it does not send duplicate welcome notification for that broadcast" - xit "does not send a notification to a user who is connected via twitter" - end - - context "when sending a github_connect notification" do - xit "generates the appropriate broadcast to be sent to a user" - xit "it sends a welcome notification for that broadcast" - xit "it does not send duplicate welcome notification for that broadcast" - xit "does not send a notification to a user who is connected via github" + it "sends only 1 notification at a time" do + user = create(:user, :with_identity, identities: ["github"], created_at: 1.week.ago) + expect do + sidekiq_perform_enqueued_jobs { described_class.call(user.id) } + end.to change(user.notifications, :count).by(1) end end + + describe "#send_welcome_notification" do + let(:user) { create(:user, created_at: 4.hours.ago) } + + it "does not send a notification to a newly-created user" do + user.update!(created_at: Time.zone.now) + sidekiq_perform_enqueued_jobs { described_class.new(user.id).send(:send_welcome_notification) } + expect(user.notifications.count).to eq(0) + end + + it "generates the correct broadcast type and sends the notification to the user" do + sidekiq_perform_enqueued_jobs { described_class.new(user.id).send(:send_welcome_notification) } + expect(user.notifications.first.notifiable).to eq(welcome_broadcast) + end + + it "does not send to a user who has commented in a welcome thread" do + create(:comment, commentable: welcome_thread, commentable_type: "Article", user: user) + expect do + sidekiq_perform_enqueued_jobs { described_class.new(user.id).send(:send_welcome_notification) } + end.not_to change(user.notifications, :count) + end + + it "does not send a duplicate notifications" do + 2.times do + sidekiq_perform_enqueued_jobs { described_class.new(user.id).send(:send_welcome_notification) } + end + + expect(user.notifications.count).to eq(1) + end + end + + describe "#send_authentication_notification" do + before do + allow(Notification).to receive(:send_welcome_notification) + end + + it "does not send notification if user is created less than a day ago" do + user = create(:user, :with_identity, identities: ["github"]) + sidekiq_perform_enqueued_jobs { described_class.new(user.id).send(:send_authentication_notification) } + expect(Notification).not_to have_received(:send_welcome_notification) + end + + it "generates and sends the appropriate broadcast (twitter)" do + user = create(:user, :with_identity, identities: ["github"], created_at: 1.day.ago) + sidekiq_perform_enqueued_jobs { described_class.new(user.id).send(:send_authentication_notification) } + expect(Notification).to have_received(:send_welcome_notification).with(user.id, twitter_connect_broadcast.id) + end + + it "generates and sends the appropriate broadcast (github)" do + user = create(:user, :with_identity, identities: ["twitter"], created_at: 1.day.ago) + sidekiq_perform_enqueued_jobs { described_class.new(user.id).send(:send_authentication_notification) } + expect(Notification).to have_received(:send_welcome_notification).with(user.id, github_connect_broadcast.id) + end + + it "does not send notification if user is authenticated with both services" do + user = create(:user, :with_identity, identities: %w[twitter github], created_at: 1.day.ago) + sidekiq_perform_enqueued_jobs { described_class.new(user.id).send(:send_authentication_notification) } + expect(Notification).not_to have_received(:send_welcome_notification).with(user.id, github_connect_broadcast.id) + end + + it "does not send a duplicate notifications (github)" do + allow(Notification).to receive(:send_welcome_notification).and_call_original + + user = create(:user, :with_identity, identities: ["twitter"], created_at: 1.day.ago) + 2.times do + sidekiq_perform_enqueued_jobs { described_class.new(user.id).send(:send_authentication_notification) } + end + expect(Notification).to have_received(:send_welcome_notification).with(user.id, github_connect_broadcast.id).exactly(:once) + end + + it "does not send a duplicate notifications (twitter)" do + allow(Notification).to receive(:send_welcome_notification).and_call_original + + user = create(:user, :with_identity, identities: ["github"], created_at: 1.day.ago) + 2.times do + sidekiq_perform_enqueued_jobs { described_class.new(user.id).send(:send_authentication_notification) } + end + expect(Notification).to have_received(:send_welcome_notification).with(user.id, twitter_connect_broadcast.id).exactly(:once) + end + end + + context "when sending a set_up_profile notification" do + xit "generates the appropriate broadcast to be sent to a user" + xit "it sends a welcome notification for that broadcast" + xit "it does not send duplicate welcome notification for that broadcast" + xit "does not send a notification to a user who has set up their profile" + end end diff --git a/spec/workers/notifications/welcome_notification_worker_spec.rb b/spec/workers/notifications/welcome_notification_worker_spec.rb index 7187cf4f6..665fd4f0d 100644 --- a/spec/workers/notifications/welcome_notification_worker_spec.rb +++ b/spec/workers/notifications/welcome_notification_worker_spec.rb @@ -1,8 +1,8 @@ require "rails_helper" RSpec.describe Notifications::WelcomeNotificationWorker, type: :worker do describe "#perform" do - let(:broadcast) { create(:onboarding_broadcast, :active) } - let(:inactive_broadcast) { create(:onboarding_broadcast) } + let(:broadcast) { create(:onboarding_broadcast) } + let(:inactive_broadcast) { create(:onboarding_broadcast, active: false) } let(:user) { create(:user) } let(:service) { Notifications::WelcomeNotification::Send } let(:worker) { subject }