Fix sending in-product marketing emails twice

When there were other emails for a given user already sent, we queried
the user twice due to the OUTER JOIN. This also raised an exception for
the service since it violated the unique constraint on `user`, `series`
and `track` when we record the sent emails.

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

@@ -75,13 +75,10 @@ module Namespaces
     end
 
     def users_for_group(group)
-      group.users.where(email_opted_in: true)
+      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))
-        )
+        .merge(Users::InProductMarketingEmail.without_track_and_series(track, series))
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
@@ -101,7 +98,7 @@ 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)
+      track_sent_email(user, track, series)
     end
 
     def completed_actions
@@ -126,7 +123,7 @@ module Namespaces
       Users::InProductMarketingEmail.bulk_insert!(in_product_marketing_email_records)
     end
 
-    def track_sent_email(user, group, track, series)
+    def track_sent_email(user, track, series)
       current_batch_user_ids << user.id
 
       in_product_marketing_email_records << Users::InProductMarketingEmail.new(

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
+
+    context 'when series does not exist' do
+      let(:series) { 2 }
 
-      it { is_expected.to eq([in_product_marketing_email])}
+      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
-    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)
+  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
+        create(:in_product_marketing_email, user: user, track: track, series: 0)
+      end
+
+      it { is_expected.not_to send_in_product_marketing_email(user.id, anything, track, 0) }
     end
 
-    # For any group Notify is called exactly once
-    it { is_expected.to send_in_product_marketing_email(user.id, anything, :create, 0) }
-  end
+    context 'when user already got sent the whole track' do
+      before do
+        0.upto(2) do |series|
+          create(:in_product_marketing_email, user: user, track: track, series: series)
+        end
+      end
 
-  context 'when user has already received a specific series in a track before' do
-    before do
-      described_class.new(:create, described_class::INTERVAL_DAYS.index(interval)).execute
+      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
 
-    # 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)) }
+    context 'when user is in two groups' do
+      let(:other_group) { create(:group) }
+
+      before do
+        other_group.add_developer(user)
+      end
 
-    context 'when different series' do
-      let(:interval) { 5 }
-      let(:actions_completed) { { created_at: frozen_time - 6.days } }
+      context 'when both groups would get the same email' do
+        before do
+          create(:onboarding_progress, namespace: other_group, **actions_completed)
+        end
 
-      it { is_expected.to send_in_product_marketing_email(user.id, anything, :create, described_class::INTERVAL_DAYS.index(interval)) }
-    end
+        it 'does not send the same email twice' do
+          subject
 
-    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 } }
+          expect(Notify).to have_received(:in_product_marketing_email).with(user.id, anything, :create, 0).once
+        end
+      end
+
+      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 { is_expected.to send_in_product_marketing_email(user.id, anything, :verify, described_class::INTERVAL_DAYS.index(interval)) }
+        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