Merge branch...

Merge branch '325813-inproductmarketingemails-activerecord-recordnotunique-pg-uniqueviolation-duplicate-key-value' into 'master'

Fix issue with InProductMarketingEmailService sending duplicate emails

See merge request gitlab-org/gitlab!57478

app/models/users/in_product_marketing_email.rb

@@ -21,8 +21,19 @@ module Users
       team: 3
     }, _suffix: true
 
-    scope :without_track_or_series, -> (track, series) do
-      where.not(track: track).or(where.not(series: series))
+    scope :without_track_and_series, -> (track, series) do
+      users = User.arel_table
+      product_emails = arel_table
+
+      join_condition = users[:id].eq(product_emails[:user_id])
+        .and(product_emails[:track]).eq(tracks[track])
+        .and(product_emails[:series]).eq(series)
+
+      arel_join = users.join(product_emails, Arel::Nodes::OuterJoin).on(join_condition)
+
+      joins(arel_join.join_sources)
+        .where(in_product_marketing_emails: { id: nil })
+        .select(Arel.sql("DISTINCT ON(#{users.table_name}.id) #{users.table_name}.*"))
     end
   end
 end

app/services/namespaces/in_product_marketing_emails_service.rb

@@ -23,7 +23,6 @@ module Namespaces
     def initialize(track, interval)
       @track = track
       @interval = interval
-      @current_batch_user_ids = []
       @in_product_marketing_email_records = []
     end
 
@@ -35,13 +34,11 @@ module Namespaces
           send_email_for_group(group)
         end
       end
-
-      record_sent_emails
     end
 
     private
 
-    attr_reader :track, :interval, :current_batch_user_ids, :in_product_marketing_email_records
+    attr_reader :track, :interval, :in_product_marketing_email_records
 
     def send_email_for_group(group)
       experiment_enabled_for_group = experiment_enabled_for_group?(group)
@@ -49,8 +46,13 @@ module Namespaces
       return unless experiment_enabled_for_group
 
       users_for_group(group).each do |user|
-        send_email(user, group) if can_perform_action?(user, group)
+        if can_perform_action?(user, group)
+          send_email(user, group)
+          track_sent_email(user, track, series)
+        end
       end
+
+      save_tracked_emails!
     end
 
     def experiment_enabled_for_group?(group)
@@ -75,13 +77,9 @@ module Namespaces
     end
 
     def users_for_group(group)
-      group.users.where(email_opted_in: true)
-        .where.not(id: current_batch_user_ids)
-        .left_outer_joins(:in_product_marketing_emails)
-        .merge(
-          Users::InProductMarketingEmail.without_track_or_series(track, series)
-            .or(Users::InProductMarketingEmail.where(id: nil))
-        )
+      group.users
+        .where(email_opted_in: true)
+        .merge(Users::InProductMarketingEmail.without_track_and_series(track, series))
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
@@ -100,8 +98,6 @@ module Namespaces
 
     def send_email(user, group)
       NotificationService.new.in_product_marketing(user.id, group.id, track, series)
-
-      track_sent_email(user, group, track, series)
     end
 
     def completed_actions
@@ -122,13 +118,12 @@ module Namespaces
       INTERVAL_DAYS.index(interval)
     end
 
-    def record_sent_emails
+    def save_tracked_emails!
       Users::InProductMarketingEmail.bulk_insert!(in_product_marketing_email_records)
+      @in_product_marketing_email_records = []
     end
 
-    def track_sent_email(user, group, track, series)
-      current_batch_user_ids << user.id
-
+    def track_sent_email(user, track, series)
       in_product_marketing_email_records << Users::InProductMarketingEmail.new(
         user: user,
         track: track,

spec/models/users/in_product_marketing_email_spec.rb

@@ -16,28 +16,45 @@ RSpec.describe Users::InProductMarketingEmail, type: :model do
     it { is_expected.to validate_uniqueness_of(:user_id).scoped_to([:track, :series]).with_message('has already been sent') }
   end
 
-  describe '.without_track_or_series' do
-    let(:track) { 0 }
+  describe '.without_track_and_series' do
+    let(:track) { :create }
     let(:series) { 0 }
 
-    let_it_be(:in_product_marketing_email) { create(:in_product_marketing_email, series: 0, track: 0) }
+    let_it_be(:user) { create(:user) }
 
-    subject(:without_track_or_series) { described_class.without_track_or_series(track, series) }
+    subject(:without_track_and_series) { User.merge(described_class.without_track_and_series(track, series)) }
 
-    context 'for the same track and series' do
-      it { is_expected.to be_empty }
+    before do
+      create(:in_product_marketing_email, track: :create, series: 0, user: user)
+      create(:in_product_marketing_email, track: :create, series: 1, user: user)
+      create(:in_product_marketing_email, track: :verify, series: 0, user: user)
     end
 
-    context 'for a different track' do
-      let(:track) { 1 }
+    context 'when given track and series already exists' do
+      it { expect(without_track_and_series).to be_empty }
+    end
+
+    context 'when track does not exist' do
+      let(:track) { :trial }
+
+      it { expect(without_track_and_series).to eq [user] }
+    end
 
-      it { is_expected.to eq([in_product_marketing_email])}
+    context 'when series does not exist' do
+      let(:series) { 2 }
+
+      it { expect(without_track_and_series).to eq [user] }
     end
 
-    context 'for a different series' do
-      let(:series) { 1 }
+    context 'when no track or series for a user exists' do
+      let(:track) { :create }
+      let(:series) { 0 }
+
+      before do
+        @other_user = create(:user)
+      end
 
-      it { is_expected.to eq([in_product_marketing_email])}
+      it { expect(without_track_and_series).to eq [@other_user] }
     end
   end
 end

spec/services/namespaces/in_product_marketing_emails_service_spec.rb

@@ -138,38 +138,64 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do
     it { is_expected.not_to send_in_product_marketing_email }
   end
 
-  context 'when the user has already received any marketing email in this batch' do
+  describe 'do not send emails twice' do
+    subject { described_class.send_for_all_tracks_and_intervals }
+
+    let(:user) { create(:user, email_opted_in: true) }
+
+    context 'when user already got a specific email' do
       before do
-      other_group = create(:group)
-      other_group.add_developer(user)
-      create(:onboarding_progress, namespace: other_group, created_at: previous_action_completed_at, git_write_at: current_action_completed_at)
+        create(:in_product_marketing_email, user: user, track: track, series: 0)
       end
 
-    # For any group Notify is called exactly once
-    it { is_expected.to send_in_product_marketing_email(user.id, anything, :create, 0) }
+      it { is_expected.not_to send_in_product_marketing_email(user.id, anything, track, 0) }
     end
 
-  context 'when user has already received a specific series in a track before' do
+    context 'when user already got sent the whole track' do
       before do
-      described_class.new(:create, described_class::INTERVAL_DAYS.index(interval)).execute
+        0.upto(2) do |series|
+          create(:in_product_marketing_email, user: user, track: track, series: series)
+        end
       end
 
-    # For any group Notify is called exactly once
-    it { is_expected.to send_in_product_marketing_email(user.id, anything, :create, described_class::INTERVAL_DAYS.index(interval)) }
+      it 'does not send any of the emails anymore', :aggregate_failures do
+        0.upto(2) do |series|
+          expect(subject).not_to send_in_product_marketing_email(user.id, anything, track, series)
+        end
+      end
+    end
 
-    context 'when different series' do
-      let(:interval) { 5 }
-      let(:actions_completed) { { created_at: frozen_time - 6.days } }
+    context 'when user is in two groups' do
+      let(:other_group) { create(:group) }
 
-      it { is_expected.to send_in_product_marketing_email(user.id, anything, :create, described_class::INTERVAL_DAYS.index(interval)) }
+      before do
+        other_group.add_developer(user)
       end
 
-    context 'when different track' do
-      let(:track) { :verify }
-      let(:interval) { 1 }
-      let(:actions_completed) { { created_at: frozen_time - 2.days, git_write_at: frozen_time - 2.days } }
+      context 'when both groups would get the same email' do
+        before do
+          create(:onboarding_progress, namespace: other_group, **actions_completed)
+        end
+
+        it 'does not send the same email twice' do
+          subject
+
+          expect(Notify).to have_received(:in_product_marketing_email).with(user.id, anything, :create, 0).once
+        end
+      end
 
-      it { is_expected.to send_in_product_marketing_email(user.id, anything, :verify, described_class::INTERVAL_DAYS.index(interval)) }
+      context 'when other group gets a different email' do
+        before do
+          create(:onboarding_progress, namespace: other_group, created_at: previous_action_completed_at, git_write_at: frozen_time - 2.days)
+        end
+
+        it 'sends both emails' do
+          subject
+
+          expect(Notify).to have_received(:in_product_marketing_email).with(user.id, group.id, :create, 0)
+          expect(Notify).to have_received(:in_product_marketing_email).with(user.id, other_group.id, :verify, 0)
+        end
+      end
     end
   end