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

@@ -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

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

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

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
 

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