From ce5ab07a6f92c1186915a8919cbf7d9bd1049e96 Mon Sep 17 00:00:00 2001 From: Peter Hartshorn Date: Tue, 14 Nov 2023 13:39:42 +0000 Subject: [PATCH] Stop sending duplicate confirmation emails We've received a report of duplicate email confirmations being sent out when a user has subscribed to a number of subscriptions. This change prevents a confirmation email being sent if the user is already subscribed and the frequency is the same. The confirmation page in Email Alert Frontend remains unchanged. As the CreateSubscriptionServive uses a lock on the subscriber record, this should prevent duplicate emails being sent out for issues like double clicks as the lock will only be released when the record has been created. Trello: https://trello.com/c/hJ5up59M/2249-stop-sending-duplicate-signup-emails-if-user-clicks-confirmation-more-than-once --- app/controllers/subscriptions_controller.rb | 17 +++++---- app/services/create_subscription_service.rb | 8 +++-- app/services/merge_subscribers_service.rb | 3 +- spec/integration/create_subscription_spec.rb | 4 +-- .../create_subscription_service_spec.rb | 35 ++++++++++--------- 5 files changed, 39 insertions(+), 28 deletions(-) diff --git a/app/controllers/subscriptions_controller.rb b/app/controllers/subscriptions_controller.rb index c6e798022..51431208f 100644 --- a/app/controllers/subscriptions_controller.rb +++ b/app/controllers/subscriptions_controller.rb @@ -12,12 +12,9 @@ def create current_user, ) - unless params[:skip_confirmation_email] - email = SubscriptionConfirmationEmailBuilder.call(subscription:) - SendEmailWorker.perform_async_in_queue(email.id, queue: :send_email_transactional) - end + send_confirmation_email(subscription) - render json: { subscription: } + render json: { subscription: subscription[:record] } end def show @@ -37,7 +34,7 @@ def update current_user, ) - render json: { subscription: new_subscription } + render json: { subscription: new_subscription[:record] } end def latest_matching @@ -63,4 +60,12 @@ def frequency def subscription_params params.permit(:id, :address, :subscriber_list_id, :frequency) end + + def send_confirmation_email(subscription) + return if params[:skip_confirmation_email] + return unless subscription[:new_record] + + email = SubscriptionConfirmationEmailBuilder.call(subscription: subscription[:record]) + SendEmailWorker.perform_async_in_queue(email.id, queue: :send_email_transactional) + end end diff --git a/app/services/create_subscription_service.rb b/app/services/create_subscription_service.rb index 82639de64..dcbb80f12 100644 --- a/app/services/create_subscription_service.rb +++ b/app/services/create_subscription_service.rb @@ -20,18 +20,22 @@ def call ) if subscription - return subscription if subscription.frequency == frequency + if subscription.frequency == frequency + return { record: subscription, new_record: false } + end subscription.end(reason: :frequency_changed) end - Subscription.create!( + new_subscription = Subscription.create!( subscriber:, subscriber_list:, frequency:, signon_user_uid: current_user.uid, source: subscription ? :frequency_changed : :user_signed_up, ) + + { record: new_subscription, new_record: true } rescue ArgumentError # This happens if a frequency is provided that isn't included # in the enum which is in the Subscription model diff --git a/app/services/merge_subscribers_service.rb b/app/services/merge_subscribers_service.rb index 6191db940..164c5192a 100644 --- a/app/services/merge_subscribers_service.rb +++ b/app/services/merge_subscribers_service.rb @@ -49,7 +49,8 @@ def keep_most_frequent(original, other) subscriber_to_keep, other.frequency, current_user, - ) + )[:record] + new_subscription.update!(source: :subscriber_merged) new_subscription end diff --git a/spec/integration/create_subscription_spec.rb b/spec/integration/create_subscription_spec.rb index 0d7d60354..dbc9047d1 100644 --- a/spec/integration/create_subscription_spec.rb +++ b/spec/integration/create_subscription_spec.rb @@ -100,10 +100,10 @@ def create_subscription(extra_params: {}) expect(subscription.reload.ended?).to be false end - it "sends a confirmation email" do + it "doesn't send a confirmation email" do stub_notify create_subscription - expect(a_request(:post, /notifications/)).to have_been_made.at_least_once + expect(a_request(:post, /notifications/)).to_not have_been_made end end diff --git a/spec/services/create_subscription_service_spec.rb b/spec/services/create_subscription_service_spec.rb index c9bf65a49..364bd2b5a 100644 --- a/spec/services/create_subscription_service_spec.rb +++ b/spec/services/create_subscription_service_spec.rb @@ -4,14 +4,16 @@ let(:frequency) { "daily" } let(:user) { create :user } let(:args) { [subscriber_list, subscriber, frequency, user] } + let(:new_subscription) { described_class.call(*args) } + let(:subscription_record) { new_subscription[:record] } describe ".call" do it "creates a subscription if one does not exist" do - new_subscription = described_class.call(*args) - expect(new_subscription.subscriber_list).to eq subscriber_list - expect(new_subscription.subscriber).to eq subscriber - expect(new_subscription.frequency).to eq frequency - expect(new_subscription.source).to eq "user_signed_up" + expect(subscription_record.subscriber_list).to eq subscriber_list + expect(subscription_record.subscriber).to eq subscriber + expect(subscription_record.frequency).to eq frequency + expect(subscription_record.source).to eq "user_signed_up" + expect(new_subscription[:new_record]).to eq true end it "replaces a subscription if the frequencies differ" do @@ -22,15 +24,14 @@ frequency: "weekly", ) - new_subscription = described_class.call(*args) + expect(subscription_record.subscriber_list).to eq subscriber_list + expect(subscription_record.subscriber).to eq subscriber + expect(subscription_record.frequency).to eq frequency + expect(subscription_record.source).to eq "frequency_changed" + expect(new_subscription[:new_record]).to eq true expect(subscription.reload).to be_ended expect(subscription.ended_reason).to eq "frequency_changed" - - expect(new_subscription.subscriber_list).to eq subscriber_list - expect(new_subscription.subscriber).to eq subscriber - expect(new_subscription.frequency).to eq frequency - expect(new_subscription.source).to eq "frequency_changed" end it "preserves a subscription if the frequency is unchanged" do @@ -41,8 +42,8 @@ frequency:, ) - new_subscription = described_class.call(*args) - expect(new_subscription).to eq subscription + expect(subscription_record).to eq subscription + expect(new_subscription[:new_record]).to eq false end it "ignores subscriptions that were previously ended" do @@ -54,10 +55,10 @@ frequency:, ) - new_subscription = described_class.call(*args) - expect(new_subscription).to_not be_ended - expect(new_subscription.frequency).to eq frequency - expect(new_subscription.source).to eq "user_signed_up" + expect(subscription_record).to_not be_ended + expect(subscription_record.frequency).to eq frequency + expect(subscription_record.source).to eq "user_signed_up" + expect(new_subscription[:new_record]).to eq true end it "raises a RecordInvalid error if the frequency is invalid" do