Merge branch 'clean-up-invite-team-email-experiment' into 'master'

Clean up code for invite_team_email experiment

See merge request gitlab-org/gitlab!82799

app/controllers/groups/email_campaigns_controller.rb

@@ -40,7 +40,7 @@ class Groups::EmailCampaignsController < Groups::ApplicationController
       project_pipelines_url(group.projects.first)
     when :trial, :trial_short
       'https://about.gitlab.com/free-trial/'
-    when :team, :team_short, :invite_team
+    when :team, :team_short
       group_group_members_url(group)
     when :admin_verify
       project_settings_ci_cd_path(group.projects.first, ci_runner_templates: true, anchor: 'js-runners-settings')
@@ -59,11 +59,6 @@ class Groups::EmailCampaignsController < Groups::ApplicationController
     @track = params[:track]&.to_sym
     @series = params[:series]&.to_i
 
-    # There is only one email that will be sent for invite team track so series
-    # should only have the value 0. Return early if track is invite team and
-    # condition for series value is met
-    return if @track == Namespaces::InviteTeamEmailService::TRACK && @series == 0
-
     track_valid = @track.in?(Namespaces::InProductMarketingEmailsService::TRACKS.keys)
     return render_404 unless track_valid
 

app/models/users/in_product_marketing_email.rb

@@ -26,12 +26,17 @@ module Users
       invite_team: 8
     }, _suffix: true
 
+    # Tracks we don't send emails for (e.g. unsuccessful experiment). These
+    # are kept since we already have DB records that use the enum value.
+    INACTIVE_TRACK_NAMES = %w(invite_team).freeze
+    ACTIVE_TRACKS = tracks.except(*INACTIVE_TRACK_NAMES)
+
     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[:track]).eq(ACTIVE_TRACKS[track])
         .and(product_emails[:series]).eq(series)
 
       arel_join = users.join(product_emails, Arel::Nodes::OuterJoin).on(join_condition)

app/services/groups/create_service.rb

@@ -57,11 +57,6 @@ module Groups
     end
 
     def after_create_hook
-      if group.persisted? && group.root?
-        delay = Namespaces::InviteTeamEmailService::DELIVERY_DELAY_IN_MINUTES
-        Namespaces::InviteTeamEmailWorker.perform_in(delay, group.id, current_user.id)
-      end
-
       track_experiment_event
     end
 

app/services/namespaces/in_product_marketing_emails_service.rb

@@ -45,6 +45,11 @@ module Namespaces
       }
     }.freeze
 
+    def self.email_count_for_track(track)
+      interval_days = TRACKS.dig(track.to_sym, :interval_days)
+      interval_days&.count || 0
+    end
+
     def self.send_for_all_tracks_and_intervals
       TRACKS.each_key do |track|
         TRACKS[track][:interval_days].each do |interval|

app/services/namespaces/invite_team_email_service.rb

-# frozen_string_literal: true
-
-module Namespaces
-  class InviteTeamEmailService
-    include Gitlab::Experiment::Dsl
-
-    TRACK = :invite_team
-    DELIVERY_DELAY_IN_MINUTES = 20.minutes
-
-    def self.send_email(user, group)
-      new(user, group).execute
-    end
-
-    def initialize(user, group)
-      @group = group
-      @user = user
-      @sent_email_records = InProductMarketingEmailRecords.new
-    end
-
-    def execute
-      return unless user.email_opted_in?
-      return unless group.root?
-      return unless group.setup_for_company
-
-      # Exclude group if users other than the creator have already been
-      # added/invited
-      return unless group.member_count == 1
-
-      return if email_for_track_sent_to_user?
-
-      experiment(:invite_team_email, group: group) do |e|
-        e.publish_to_database
-        e.candidate do
-          send_email(user, group)
-          sent_email_records.add(user, track, series)
-          sent_email_records.save!
-        end
-      end
-    end
-
-    private
-
-    attr_reader :user, :group, :sent_email_records
-
-    def send_email(user, group)
-      NotificationService.new.in_product_marketing(user.id, group.id, track, series)
-    end
-
-    def track
-      TRACK
-    end
-
-    def series
-      0
-    end
-
-    def email_for_track_sent_to_user?
-      Users::InProductMarketingEmail.for_user_with_track_and_series(user, track, series).present?
-    end
-  end
-end

app/workers/all_queues.yml

@@ -2587,15 +2587,6 @@
   :weight: 1
   :idempotent:
   :tags: []
-- :name: namespaces_invite_team_email
-  :worker_name: Namespaces::InviteTeamEmailWorker
-  :feature_category: :experimentation_activation
-  :has_external_dependencies:
-  :urgency: :low
-  :resource_boundary: :unknown
-  :weight: 1
-  :idempotent:
-  :tags: []
 - :name: namespaces_onboarding_issue_created
   :worker_name: Namespaces::OnboardingIssueCreatedWorker
   :feature_category: :onboarding

app/workers/namespaces/invite_team_email_worker.rb

-# frozen_string_literal: true
-
-module Namespaces
-  class InviteTeamEmailWorker # rubocop:disable Scalability/IdempotentWorker
-    include ApplicationWorker
-
-    data_consistency :always
-
-    feature_category :experimentation_activation
-    urgency :low
-
-    def perform(group_id, user_id)
-      # rubocop: disable CodeReuse/ActiveRecord
-      user = User.find_by(id: user_id)
-      group = Group.find_by(id: group_id)
-      # rubocop: enable CodeReuse/ActiveRecord
-      return unless user && group
-
-      Namespaces::InviteTeamEmailService.send_email(user, group)
-    end
-  end
-end

config/feature_flags/experiment/invite_team_email.yml

----
-name: invite_team_email
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/72470
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/345553
-milestone: '14.5'
-type: experiment
-group: group::activation
-default_enabled: false

config/sidekiq_queues.yml

@@ -281,8 +281,6 @@
   - 1
 - - namespaceless_project_destroy
   - 1
-- - namespaces_invite_team_email
-  - 1
 - - namespaces_onboarding_issue_created
   - 1
 - - namespaces_onboarding_pipeline_created

lib/gitlab/email/message/in_product_marketing.rb

@@ -7,7 +7,7 @@ module Gitlab
         UnknownTrackError = Class.new(StandardError)
 
         def self.for(track)
-          valid_tracks = [Namespaces::InviteTeamEmailService::TRACK, Namespaces::InProductMarketingEmailsService::TRACKS.keys].flatten
+          valid_tracks = Namespaces::InProductMarketingEmailsService::TRACKS.keys
           raise UnknownTrackError unless valid_tracks.include?(track)
 
           "Gitlab::Email::Message::InProductMarketing::#{track.to_s.classify}".constantize

lib/gitlab/email/message/in_product_marketing/invite_team.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Email
-    module Message
-      module InProductMarketing
-        class InviteTeam < Base
-          def subject_line
-            s_('InProductMarketing|Invite your teammates to GitLab')
-          end
-
-          def tagline
-            ''
-          end
-
-          def title
-            s_('InProductMarketing|GitLab is better with teammates to help out!')
-          end
-
-          def subtitle
-            ''
-          end
-
-          def body_line1
-            s_('InProductMarketing|Invite your teammates today and build better code together. You can even assign tasks to new teammates such as setting up CI/CD, to help get projects up and running.')
-          end
-
-          def body_line2
-            ''
-          end
-
-          def cta_text
-            s_('InProductMarketing|Invite your teammates to help')
-          end
-
-          def logo_path
-            'mailers/in_product_marketing/team-0.png'
-          end
-
-          def series?
-            false
-          end
-
-          private
-
-          def validate_series!
-            raise ArgumentError, "Only one email is sent for this track. Value of `series` should be 0." unless @series == 0
-          end
-        end
-      end
-    end
-  end
-end

lib/gitlab/usage_data.rb

@@ -795,14 +795,9 @@ module Gitlab
         sent_emails = count(Users::InProductMarketingEmail.group(:track, :series))
         clicked_emails = count(Users::InProductMarketingEmail.where.not(cta_clicked_at: nil).group(:track, :series))
 
-        Users::InProductMarketingEmail.tracks.keys.each_with_object({}) do |track, result|
+        Users::InProductMarketingEmail::ACTIVE_TRACKS.keys.each_with_object({}) do |track, result|
+          series_amount = Namespaces::InProductMarketingEmailsService.email_count_for_track(track)
           # rubocop: enable UsageData/LargeTable:
-          series_amount =
-            if track.to_sym == Namespaces::InviteTeamEmailService::TRACK
-              0
-            else
-              Namespaces::InProductMarketingEmailsService::TRACKS[track.to_sym][:interval_days].count
-            end
 
           0.upto(series_amount - 1).map do |series|
             # When there is an error with the query and it's not the Hash we expect, we return what we got from `count`.

locale/gitlab.pot

@@ -19238,9 +19238,6 @@ msgstr ""
 msgid "InProductMarketing|GitHub Enterprise projects to GitLab"
 msgstr ""
 
-msgid "InProductMarketing|GitLab is better with teammates to help out!"
-msgstr ""
-
 msgid "InProductMarketing|GitLab is infrastructure agnostic (supporting GCP, AWS, Azure, OpenShift, VMWare, On Prem, Bare Metal, and more), offering a consistent workflow experience – irrespective of the environment."
 msgstr ""
 
@@ -19319,15 +19316,6 @@ msgstr ""
 msgid "InProductMarketing|Invite your team today to build better code (and processes) together"
 msgstr ""
 
-msgid "InProductMarketing|Invite your teammates to GitLab"
-msgstr ""
-
-msgid "InProductMarketing|Invite your teammates to help"
-msgstr ""
-
-msgid "InProductMarketing|Invite your teammates today and build better code together. You can even assign tasks to new teammates such as setting up CI/CD, to help get projects up and running."
-msgstr ""
-
 msgid "InProductMarketing|It's all in the stats"
 msgstr ""
 

spec/lib/gitlab/email/message/in_product_marketing/base_spec.rb

@@ -100,7 +100,6 @@ RSpec.describe Gitlab::Email::Message::InProductMarketing::Base do
       :trial        | true
       :team         | true
       :experience   | true
-      :invite_team  | false
     end
 
     with_them do

spec/lib/gitlab/email/message/in_product_marketing/invite_team_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::Email::Message::InProductMarketing::InviteTeam do
-  let_it_be(:group) { build(:group) }
-  let_it_be(:user) { build(:user) }
-
-  let(:series) { 0 }
-
-  subject(:message) { described_class.new(group: group, user: user, series: series) }
-
-  describe 'initialize' do
-    context 'when series is valid' do
-      it 'does not raise error' do
-        expect { subject }.not_to raise_error(ArgumentError)
-      end
-    end
-
-    context 'when series is invalid' do
-      let(:series) { 1 }
-
-      it 'raises error' do
-        expect { subject }.to raise_error(ArgumentError)
-      end
-    end
-  end
-
-  it 'contains the correct message', :aggregate_failures do
-    expect(message.subject_line).to eq 'Invite your teammates to GitLab'
-    expect(message.tagline).to be_empty
-    expect(message.title).to eq 'GitLab is better with teammates to help out!'
-    expect(message.subtitle).to be_empty
-    expect(message.body_line1).to eq 'Invite your teammates today and build better code together. You can even assign tasks to new teammates such as setting up CI/CD, to help get projects up and running.'
-    expect(message.body_line2).to be_empty
-    expect(message.cta_text).to eq 'Invite your teammates to help'
-    expect(message.logo_path).to eq 'mailers/in_product_marketing/team-0.png'
-  end
-end

spec/lib/gitlab/email/message/in_product_marketing_spec.rb

@@ -18,7 +18,6 @@ RSpec.describe Gitlab::Email::Message::InProductMarketing do
         :trial        | described_class::Trial
         :team         | described_class::Team
         :experience   | described_class::Experience
-        :invite_team  | described_class::InviteTeam
       end
 
       with_them do

spec/mailers/emails/in_product_marketing_spec.rb

@@ -69,7 +69,6 @@ RSpec.describe Emails::InProductMarketing do
       :team_short   | 0
       :trial_short  | 0
       :admin_verify | 0
-      :invite_team  | 0
     end
 
     with_them do
@@ -99,11 +98,7 @@ RSpec.describe Emails::InProductMarketing do
             is_expected.not_to have_body_text(CGI.unescapeHTML(message.invite_link))
           end
 
-          if track == :invite_team
-            is_expected.not_to have_body_text(/This is email \d of \d/)
-          else
-            is_expected.to have_body_text(message.progress)
-          end
+          is_expected.to have_body_text(message.progress)
         end
       end
     end

spec/models/users/in_product_marketing_email_spec.rb

@@ -19,13 +19,6 @@ 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 '.tracks' do
-    it 'has an entry for every track' do
-      tracks = [Namespaces::InviteTeamEmailService::TRACK, Namespaces::InProductMarketingEmailsService::TRACKS.keys].flatten
-      expect(tracks).to match_array(described_class.tracks.keys.map(&:to_sym))
-    end
-  end
-
   describe '.without_track_and_series' do
     let_it_be(:user) { create(:user) }
 
@@ -135,4 +128,15 @@ RSpec.describe Users::InProductMarketingEmail, type: :model do
       end
     end
   end
+
+  describe '.ACTIVE_TRACKS' do
+    it 'has an entry for every track' do
+      tracks = Namespaces::InProductMarketingEmailsService::TRACKS.keys
+      expect(tracks).to match_array(described_class::ACTIVE_TRACKS.keys.map(&:to_sym))
+    end
+
+    it 'does not include INACTIVE_TRACK_NAMES' do
+      expect(described_class::ACTIVE_TRACKS.keys).not_to include(*described_class::INACTIVE_TRACK_NAMES)
+    end
+  end
 end

spec/requests/groups/email_campaigns_controller_spec.rb

@@ -94,7 +94,7 @@ RSpec.describe Groups::EmailCampaignsController do
 
     describe 'track parameter' do
       context 'when valid' do
-        where(track: [Namespaces::InProductMarketingEmailsService::TRACKS.keys.without(:experience), Namespaces::InviteTeamEmailService::TRACK].flatten)
+        where(track: Namespaces::InProductMarketingEmailsService::TRACKS.keys.without(:experience))
 
         with_them do
           it_behaves_like 'track and redirect'
@@ -117,10 +117,6 @@ RSpec.describe Groups::EmailCampaignsController do
         with_them do
           it_behaves_like 'track and redirect'
         end
-
-        it_behaves_like 'track and redirect' do
-          let(:track) { Namespaces::InviteTeamEmailService::TRACK.to_s }
-        end
       end
 
       context 'when invalid' do
@@ -128,10 +124,6 @@ RSpec.describe Groups::EmailCampaignsController do
 
         with_them do
           it_behaves_like 'no track and 404'
-
-          it_behaves_like 'no track and 404' do
-            let(:track) { Namespaces::InviteTeamEmailService::TRACK.to_s }
-          end
         end
       end
     end

spec/services/groups/create_service_spec.rb

@@ -263,43 +263,6 @@ RSpec.describe Groups::CreateService, '#execute' do
     end
   end
 
-  describe 'invite team email' do
-    let(:service) { described_class.new(user, group_params) }
-
-    before do
-      allow(Namespaces::InviteTeamEmailWorker).to receive(:perform_in)
-    end
-
-    it 'is sent' do
-      group = service.execute
-      delay = Namespaces::InviteTeamEmailService::DELIVERY_DELAY_IN_MINUTES
-      expect(Namespaces::InviteTeamEmailWorker).to have_received(:perform_in).with(delay, group.id, user.id)
-    end
-
-    context 'when group has not been persisted' do
-      let(:service) { described_class.new(user, group_params.merge(name: '<script>alert("Attack!")</script>')) }
-
-      it 'not sent' do
-        expect(Namespaces::InviteTeamEmailWorker).not_to receive(:perform_in)
-        service.execute
-      end
-    end
-
-    context 'when group is not root' do
-      let(:parent_group) { create :group }
-      let(:service) { described_class.new(user, group_params.merge(parent_id: parent_group.id)) }
-
-      before do
-        parent_group.add_owner(user)
-      end
-
-      it 'not sent' do
-        expect(Namespaces::InviteTeamEmailWorker).not_to receive(:perform_in)
-        service.execute
-      end
-    end
-  end
-
   describe 'logged_out_marketing_header experiment', :experiment do
     let(:service) { described_class.new(user, group_params) }
 

spec/services/namespaces/in_product_marketing_email_records_spec.rb

@@ -15,7 +15,7 @@ RSpec.describe Namespaces::InProductMarketingEmailRecords do
     before do
       allow(Users::InProductMarketingEmail).to receive(:bulk_insert!)
 
-      records.add(user, :invite_team, 0)
+      records.add(user, :team_short, 0)
       records.add(user, :create, 1)
     end
 
@@ -33,13 +33,13 @@ RSpec.describe Namespaces::InProductMarketingEmailRecords do
   describe '#add' do
     it 'adds a Users::InProductMarketingEmail record to its records' do
       freeze_time do
-        records.add(user, :invite_team, 0)
+        records.add(user, :team_short, 0)
         records.add(user, :create, 1)
 
         first, second = records.records
 
         expect(first).to be_a Users::InProductMarketingEmail
-        expect(first.track.to_sym).to eq :invite_team
+        expect(first.track.to_sym).to eq :team_short
         expect(first.series).to eq 0
         expect(first.created_at).to eq Time.zone.now
         expect(first.updated_at).to eq Time.zone.now

spec/services/namespaces/in_product_marketing_emails_service_spec.rb

@@ -183,7 +183,7 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do
     expect(
       Users::InProductMarketingEmail.where(
         user: user,
-        track: Users::InProductMarketingEmail.tracks[:create],
+        track: Users::InProductMarketingEmail::ACTIVE_TRACKS[:create],
         series: 0
       )
     ).to exist

spec/services/namespaces/invite_team_email_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Namespaces::InviteTeamEmailService do
-  let_it_be(:user) { create(:user, email_opted_in: true) }
-
-  let(:track) { described_class::TRACK }
-  let(:series) { 0 }
-
-  let(:setup_for_company) { true }
-  let(:parent_group) { nil }
-  let(:group) { create(:group, parent: parent_group) }
-
-  subject(:action) { described_class.send_email(user, group) }
-
-  before do
-    group.add_owner(user)
-    allow(group).to receive(:setup_for_company).and_return(setup_for_company)
-    allow(Notify).to receive(:in_product_marketing_email).and_return(double(deliver_later: nil))
-  end
-
-  RSpec::Matchers.define :send_invite_team_email do |*args|
-    match do
-      expect(Notify).to have_received(:in_product_marketing_email).with(*args).once
-    end
-
-    match_when_negated do
-      expect(Notify).not_to have_received(:in_product_marketing_email)
-    end
-  end
-
-  shared_examples 'unexperimented' do
-    it { is_expected.not_to send_invite_team_email }
-
-    it 'does not record sent email' do
-      expect { subject }.not_to change { Users::InProductMarketingEmail.count }
-    end
-  end
-
-  shared_examples 'candidate' do
-    it { is_expected.to send_invite_team_email(user.id, group.id, track, 0) }
-
-    it 'records sent email' do
-      expect { subject }.to change { Users::InProductMarketingEmail.count }.by(1)
-
-      expect(
-        Users::InProductMarketingEmail.where(
-          user: user,
-          track: track,
-          series: 0
-        )
-      ).to exist
-    end
-
-    it_behaves_like 'tracks assignment and records the subject', :invite_team_email, :group do
-      subject { group }
-    end
-  end
-
-  context 'when group is in control path' do
-    before do
-      stub_experiments(invite_team_email: :control)
-    end
-
-    it { is_expected.not_to send_invite_team_email }
-
-    it 'does not record sent email' do
-      expect { subject }.not_to change { Users::InProductMarketingEmail.count }
-    end
-
-    it_behaves_like 'tracks assignment and records the subject', :invite_team_email, :group do
-      subject { group }
-    end
-  end
-
-  context 'when group is in candidate path' do
-    before do
-      stub_experiments(invite_team_email: :candidate)
-    end
-
-    it_behaves_like 'candidate'
-
-    context 'when the user has not opted into marketing emails' do
-      let(:user) { create(:user, email_opted_in: false ) }
-
-      it_behaves_like 'unexperimented'
-    end
-
-    context 'when group is not top level' do
-      it_behaves_like 'unexperimented' do
-        let(:parent_group) do
-          create(:group).tap { |g| g.add_owner(user) }
-        end
-      end
-    end
-
-    context 'when group is not set up for a company' do
-      it_behaves_like 'unexperimented' do
-        let(:setup_for_company) { nil }
-      end
-    end
-
-    context 'when other users have already been added to the group' do
-      before do
-        group.add_developer(create(:user))
-      end
-
-      it_behaves_like 'unexperimented'
-    end
-
-    context 'when other users have already been invited to the group' do
-      before do
-        group.add_developer('not_a_user_yet@example.com')
-      end
-
-      it_behaves_like 'unexperimented'
-    end
-
-    context 'when the user already got sent the email' do
-      before do
-        create(:in_product_marketing_email, user: user, track: track, series: 0)
-      end
-
-      it_behaves_like 'unexperimented'
-    end
-  end
-end

spec/workers/namespaces/invite_team_email_worker_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Namespaces::InviteTeamEmailWorker do
-  let_it_be(:user) { create(:user) }
-  let_it_be(:group) { create(:group) }
-
-  it 'sends the email' do
-    expect(Namespaces::InviteTeamEmailService).to receive(:send_email).with(user, group).once
-    subject.perform(group.id, user.id)
-  end
-
-  context 'when user id is non-existent' do
-    it 'does not send the email' do
-      expect(Namespaces::InviteTeamEmailService).not_to receive(:send_email)
-      subject.perform(group.id, non_existing_record_id)
-    end
-  end
-
-  context 'when group id is non-existent' do
-    it 'does not send the email' do
-      expect(Namespaces::InviteTeamEmailService).not_to receive(:send_email)
-      subject.perform(non_existing_record_id, user.id)
-    end
-  end
-end