From b622dd07e8359f9ae15ef25fd7ef6711c819da4c Mon Sep 17 00:00:00 2001 From: Bell Isabell Date: Tue, 3 Mar 2026 18:54:37 -0800 Subject: [PATCH] Add model-level uniqueness validation on Subscription - Add validates :email_address_id, uniqueness: { scope: :account_id } - Add find_or_create_by_with_rescue helper to handle RecordNotUnique - Update all subscription creation sites to use the rescue helper Closes #30 --- app/controllers/public_pages_controller.rb | 4 ++-- app/jobs/subscribers_import_job.rb | 4 ++-- app/models/account.rb | 5 ++--- app/models/subscription.rb | 9 +++++++++ 4 files changed, 15 insertions(+), 7 deletions(-) diff --git a/app/controllers/public_pages_controller.rb b/app/controllers/public_pages_controller.rb index b64d931..30db07c 100644 --- a/app/controllers/public_pages_controller.rb +++ b/app/controllers/public_pages_controller.rb @@ -16,8 +16,8 @@ def create @email_address = EmailAddress.find_or_create_by(email: (params.dig(:email_address, :email) || params[:email]).strip.downcase) return render :show, status: :bad_request unless @email_address.valid? - @subscription = Subscription.create_with(source: :signup).find_or_create_by(email_address: @email_address, - account: @account) + @subscription = Subscription.create_with(source: :signup).find_or_create_by_with_rescue(email_address: @email_address, + account: @account) return redirect_to '/', notice: ALREADY_SUBSCRIBED_MESSAGE if @subscription.active? diff --git a/app/jobs/subscribers_import_job.rb b/app/jobs/subscribers_import_job.rb index 79d6e59..d09981f 100644 --- a/app/jobs/subscribers_import_job.rb +++ b/app/jobs/subscribers_import_job.rb @@ -13,8 +13,8 @@ def import(subscribers_import) email_address = EmailAddress.find_or_create_by(email: email) Subscription.create_with(source: :import, subscribers_import: subscribers_import, verified_at: Time.zone.now) \ - .find_or_create_by(email_address: email_address, - account: account) + .find_or_create_by_with_rescue(email_address: email_address, + account: account) end end end diff --git a/app/models/account.rb b/app/models/account.rb index b41fe3e..1102c0f 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -159,10 +159,9 @@ def subscribe_to_updates updates = Account.find_by(slug: 'updates') return if updates.blank? - # Ok if this fails + # Ok if this fails - use rescue helper to handle race conditions Subscription.create_with(source: :signup, verified_at: Time.zone.now) \ - .where(account: updates, email_address: EmailAddress.find_or_create_by(email: email)) \ - .first_or_create + .find_or_create_by_with_rescue(account: updates, email_address: EmailAddress.find_or_create_by(email: email)) end def unsubscribe_from_updates diff --git a/app/models/subscription.rb b/app/models/subscription.rb index 220ae28..0311dc0 100644 --- a/app/models/subscription.rb +++ b/app/models/subscription.rb @@ -14,6 +14,15 @@ class Subscription < ApplicationRecord enum source: { signup: 0, invite: 1, import: 2 }, _prefix: true validates :source, presence: true + validates :email_address_id, uniqueness: { scope: :account_id, message: 'is already subscribed to this account' } + + # Handle race condition: if DB constraint fires despite find_or_create_by, + # find the existing record instead of raising + def self.find_or_create_by_with_rescue(attributes, &block) + find_or_create_by(attributes, &block) + rescue ActiveRecord::RecordNotUnique + find_by!(attributes) + end def active? !!verified_at && !unsubscribed_at