Merge branch 'nicolasdular/record-product-email-campaign-emails' into 'master'

Do not send emails for in-product marketing twice to a user

See merge request gitlab-org/gitlab!55840

app/models/user.rb

@@ -199,6 +199,8 @@ class User < ApplicationRecord
 
   has_many :reviews, foreign_key: :author_id, inverse_of: :author
 
+  has_many :in_product_marketing_emails, class_name: '::Users::InProductMarketingEmail'
+
   #
   # Validations
   #

app/models/users/in_product_marketing_email.rb

+# frozen_string_literal: true
+
+module Users
+  class InProductMarketingEmail < ApplicationRecord
+    include BulkInsertSafe
+
+    belongs_to :user
+
+    validates :user, presence: true
+    validates :track, presence: true
+    validates :series, presence: true
+    validates :user_id, uniqueness: {
+      scope: [:track, :series],
+      message: 'has already been sent'
+    }
+
+    enum track: {
+      create: 0,
+      verify: 1,
+      trial: 2,
+      team: 3
+    }, _suffix: true
+
+    scope :without_track_or_series, -> (track, series) do
+      where.not(track: track).or(where.not(series: series))
+    end
+  end
+end

app/services/namespaces/in_product_marketing_emails_service.rb

@@ -23,20 +23,25 @@ module Namespaces
     def initialize(track, interval)
       @track = track
       @interval = interval
-      @sent_email_user_ids = []
+      @current_batch_user_ids = []
+      @in_product_marketing_email_records = []
     end
 
     def execute
+      raise NotImplementedError, "Track #{track} not defined" unless TRACKS.key?(track)
+
       groups_for_track.each_batch do |groups|
         groups.each do |group|
           send_email_for_group(group)
         end
       end
+
+      record_sent_emails
     end
 
     private
 
-    attr_reader :track, :interval, :sent_email_user_ids
+    attr_reader :track, :interval, :current_batch_user_ids, :in_product_marketing_email_records
 
     def send_email_for_group(group)
       experiment_enabled_for_group = experiment_enabled_for_group?(group)
@@ -71,7 +76,12 @@ module Namespaces
 
     def users_for_group(group)
       group.users.where(email_opted_in: true)
-        .where.not(id: sent_email_user_ids)
+        .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))
+        )
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
@@ -85,14 +95,13 @@ module Namespaces
         user.can?(:start_trial, group)
       when :team
         user.can?(:admin_group_member, group)
-      else
-        raise NotImplementedError, "No ability defined for track #{track}"
       end
     end
 
     def send_email(user, group)
       NotificationService.new.in_product_marketing(user.id, group.id, track, series)
-      sent_email_user_ids << user.id
+
+      track_sent_email(user, group, track, series)
     end
 
     def completed_actions
@@ -111,5 +120,21 @@ module Namespaces
     def series
       INTERVAL_DAYS.index(interval)
     end
+
+    def record_sent_emails
+      Users::InProductMarketingEmail.bulk_insert!(in_product_marketing_email_records)
+    end
+
+    def track_sent_email(user, group, track, series)
+      current_batch_user_ids << user.id
+
+      in_product_marketing_email_records << Users::InProductMarketingEmail.new(
+        user: user,
+        track: track,
+        series: series,
+        created_at: Time.zone.now,
+        updated_at: Time.zone.now
+      )
+    end
   end
 end

changelogs/unreleased/nicolasdular-record-product-email-campaign-emails.yml

+---
+title: Record sent in-product marketing emails and don't send the same email twice
+merge_request: 55840
+author:
+type: changed

db/migrate/20210317104301_create_in_product_marketing_emails.rb

+# frozen_string_literal: true
+
+class CreateInProductMarketingEmails < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  UNIQUE_INDEX_NAME = 'index_in_product_marketing_emails_on_user_track_series'
+
+  disable_ddl_transaction!
+
+  def up
+    with_lock_retries do
+      create_table :in_product_marketing_emails do |t|
+        t.bigint :user_id, null: false
+        t.datetime_with_timezone :cta_clicked_at
+        t.integer :track, null: false, limit: 2
+        t.integer :series, null: false, limit: 2
+
+        t.timestamps_with_timezone
+      end
+    end
+
+    add_index :in_product_marketing_emails, :user_id
+    add_index :in_product_marketing_emails, [:user_id, :track, :series], unique: true, name: UNIQUE_INDEX_NAME
+  end
+
+  def down
+    with_lock_retries do
+      drop_table :in_product_marketing_emails
+    end
+  end
+end

db/migrate/20210317105904_add_user_foreign_key_to_in_product_marketing_emails.rb

+# frozen_string_literal: true
+
+class AddUserForeignKeyToInProductMarketingEmails < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :in_product_marketing_emails, :users, column: :user_id, on_delete: :cascade
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :in_product_marketing_emails, column: :user_id
+    end
+  end
+end

db/schema_migrations/20210317104301

+c502a539a50ef1b8f07a8c22426a23cf974ee663fc80a99abe0d658e2a07f52b
\ No newline at end of file

db/schema_migrations/20210317105904

+64675f43f66d90158147c62699c0d2a48dc74d017c81b30ce3847408d0dad3cb
\ No newline at end of file

db/structure.sql

@@ -13460,6 +13460,25 @@ CREATE SEQUENCE import_failures_id_seq
 
 ALTER SEQUENCE import_failures_id_seq OWNED BY import_failures.id;
 
+CREATE TABLE in_product_marketing_emails (
+    id bigint NOT NULL,
+    user_id bigint NOT NULL,
+    cta_clicked_at timestamp with time zone,
+    track smallint NOT NULL,
+    series smallint NOT NULL,
+    created_at timestamp with time zone NOT NULL,
+    updated_at timestamp with time zone NOT NULL
+);
+
+CREATE SEQUENCE in_product_marketing_emails_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE in_product_marketing_emails_id_seq OWNED BY in_product_marketing_emails.id;
+
 CREATE TABLE incident_management_oncall_participants (
     id bigint NOT NULL,
     oncall_rotation_id bigint NOT NULL,
@@ -19367,6 +19386,8 @@ ALTER TABLE ONLY import_export_uploads ALTER COLUMN id SET DEFAULT nextval('impo
 
 ALTER TABLE ONLY import_failures ALTER COLUMN id SET DEFAULT nextval('import_failures_id_seq'::regclass);
 
+ALTER TABLE ONLY in_product_marketing_emails ALTER COLUMN id SET DEFAULT nextval('in_product_marketing_emails_id_seq'::regclass);
+
 ALTER TABLE ONLY incident_management_oncall_participants ALTER COLUMN id SET DEFAULT nextval('incident_management_oncall_participants_id_seq'::regclass);
 
 ALTER TABLE ONLY incident_management_oncall_rotations ALTER COLUMN id SET DEFAULT nextval('incident_management_oncall_rotations_id_seq'::regclass);
@@ -20670,6 +20691,9 @@ ALTER TABLE ONLY import_export_uploads
 ALTER TABLE ONLY import_failures
     ADD CONSTRAINT import_failures_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY in_product_marketing_emails
+    ADD CONSTRAINT in_product_marketing_emails_pkey PRIMARY KEY (id);
+
 ALTER TABLE ONLY incident_management_oncall_shifts
     ADD CONSTRAINT inc_mgmnt_no_overlapping_oncall_shifts EXCLUDE USING gist (rotation_id WITH =, tstzrange(starts_at, ends_at, '[)'::text) WITH &&);
 
@@ -22740,6 +22764,10 @@ CREATE INDEX index_import_failures_on_project_id_not_null ON import_failures USI
 
 CREATE INDEX index_imported_projects_on_import_type_creator_id_created_at ON projects USING btree (import_type, creator_id, created_at) WHERE (import_type IS NOT NULL);
 
+CREATE INDEX index_in_product_marketing_emails_on_user_id ON in_product_marketing_emails USING btree (user_id);
+
+CREATE UNIQUE INDEX index_in_product_marketing_emails_on_user_track_series ON in_product_marketing_emails USING btree (user_id, track, series);
+
 CREATE INDEX index_inc_mgmnt_oncall_participants_on_oncall_user_id ON incident_management_oncall_participants USING btree (user_id);
 
 CREATE UNIQUE INDEX index_inc_mgmnt_oncall_participants_on_user_id_and_rotation_id ON incident_management_oncall_participants USING btree (user_id, oncall_rotation_id);
@@ -24679,6 +24707,9 @@ ALTER TABLE ONLY ci_group_variables
 ALTER TABLE ONLY namespaces
     ADD CONSTRAINT fk_3448c97865 FOREIGN KEY (push_rule_id) REFERENCES push_rules(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY in_product_marketing_emails
+    ADD CONSTRAINT fk_35c9101b63 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY epics
     ADD CONSTRAINT fk_3654b61b03 FOREIGN KEY (author_id) REFERENCES users(id) ON DELETE CASCADE;
 

spec/factories/users/in_product_marketing_email.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :in_product_marketing_email, class: 'Users::InProductMarketingEmail' do
+    user
+
+    track { 'create' }
+    series { 0 }
+  end
+end

spec/models/user_spec.rb

@@ -108,6 +108,7 @@ RSpec.describe User do
     it { is_expected.to have_many(:merge_request_assignees).inverse_of(:assignee) }
     it { is_expected.to have_many(:merge_request_reviewers).inverse_of(:reviewer) }
     it { is_expected.to have_many(:created_custom_emoji).inverse_of(:creator) }
+    it { is_expected.to have_many(:in_product_marketing_emails) }
 
     describe "#user_detail" do
       it 'does not persist `user_detail` by default' do

spec/models/users/in_product_marketing_email_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::InProductMarketingEmail, type: :model do
+  describe 'associations' do
+    it { is_expected.to belong_to(:user) }
+  end
+
+  describe 'validations' do
+    subject { build(:in_product_marketing_email) }
+
+    it { is_expected.to validate_presence_of(:user) }
+    it { is_expected.to validate_presence_of(:track) }
+    it { is_expected.to validate_presence_of(:series) }
+    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 }
+    let(:series) { 0 }
+
+    let_it_be(:in_product_marketing_email) { create(:in_product_marketing_email, series: 0, track: 0) }
+
+    subject(:without_track_or_series) { described_class.without_track_or_series(track, series) }
+
+    context 'for the same track and series' do
+      it { is_expected.to be_empty }
+    end
+
+    context 'for a different track' do
+      let(:track) { 1 }
+
+      it { is_expected.to eq([in_product_marketing_email])}
+    end
+
+    context 'for a different series' do
+      let(:series) { 1 }
+
+      it { is_expected.to eq([in_product_marketing_email])}
+    end
+  end
+end

spec/services/namespaces/in_product_marketing_emails_service_spec.rb

@@ -139,7 +139,7 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do
     it { is_expected.not_to send_in_product_marketing_email }
   end
 
-  context 'when the user has already received a marketing email as part of another group' do
+  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)
@@ -150,14 +150,50 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do
     it { is_expected.to send_in_product_marketing_email(user.id, anything, :create, 0) }
   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
+    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 different series' do
+      let(:interval) { 5 }
+      let(:actions_completed) { { created_at: frozen_time - 6.days } }
+
+      it { is_expected.to send_in_product_marketing_email(user.id, anything, :create, described_class::INTERVAL_DAYS.index(interval)) }
+    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 } }
+
+      it { is_expected.to send_in_product_marketing_email(user.id, anything, :verify, described_class::INTERVAL_DAYS.index(interval)) }
+    end
+  end
+
+  it 'records sent emails' do
+    expect { subject }.to change { Users::InProductMarketingEmail.count }.by(1)
+
+    expect(
+      Users::InProductMarketingEmail.where(
+        user: user,
+        track: Users::InProductMarketingEmail.tracks[:create],
+        series: 0
+      )
+    ).to exist
+  end
+
   context 'when invoked with a non existing track' do
     let(:track) { :foo }
 
     before do
-      stub_const("#{described_class}::TRACKS", { foo: :git_write })
+      stub_const("#{described_class}::TRACKS", { bar: :git_write })
     end
 
-    it { expect { subject }.to raise_error(NotImplementedError, 'No ability defined for track foo') }
+    it { expect { subject }.to raise_error(NotImplementedError, 'Track foo not defined') }
   end
 
   context 'when group is a sub-group' do