diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb index e6aae144da60adb31015f778f6ff811ffebc53b3..9a674d02b3f4c3faea5aaad7d055ad211caf6c2e 100644 --- a/app/controllers/invites_controller.rb +++ b/app/controllers/invites_controller.rb @@ -75,7 +75,7 @@ def ensure_member_exists end def track_invite_join_click - experiment('members/invite_email', actor: member).track(:join_clicked) if member && Members::InviteEmailExperiment.initial_invite_email?(params[:invite_type]) + Gitlab::Tracking.event(self.class.name, 'join_clicked', label: 'invite_email', property: member.id.to_s) if member && initial_invite_email? end def authenticate_user! @@ -95,7 +95,11 @@ def authenticate_user! def set_session_invite_params session[:invite_email] = member.invite_email - session[:originating_member_id] = member.id if Members::InviteEmailExperiment.initial_invite_email?(params[:invite_type]) + session[:originating_member_id] = member.id if initial_invite_email? + end + + def initial_invite_email? + params[:invite_type] == Emails::Members::INITIAL_INVITE end def sign_in_redirect_params diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 7b1060eba8f0868f836df009852cf635ae7bdc82..fcb977f5ee9cab5aa6eea294cf5e5d1bcad9d777 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -199,7 +199,7 @@ def after_pending_invitations_hook return unless member - experiment('members/invite_email', actor: member).track(:accepted) + Gitlab::Tracking.event(self.class.name, 'accepted', label: 'invite_email', property: member.id.to_s) end def context_user diff --git a/app/experiments/members/invite_email_experiment.rb b/app/experiments/members/invite_email_experiment.rb deleted file mode 100644 index 893061e34f3e0b4df2aef9ce447d9d489d127bbf..0000000000000000000000000000000000000000 --- a/app/experiments/members/invite_email_experiment.rb +++ /dev/null @@ -1,93 +0,0 @@ -# frozen_string_literal: true - -module Members - class InviteEmailExperiment < ApplicationExperiment - exclude { context.actor.created_by.blank? } - exclude { context.actor.created_by.avatar_url.nil? } - - INVITE_TYPE = 'initial_email' - - def self.initial_invite_email?(invite_type) - invite_type == INVITE_TYPE - end - - def resolve_variant_name - RoundRobin.new(feature_flag_name, %i[activity control]).execute - end - end - - class RoundRobin - CacheError = Class.new(StandardError) - - COUNTER_EXPIRE_TIME = 86400 # one day - - def initialize(key, variants) - @key = key - @variants = variants - end - - def execute - increment_counter - resolve_variant_name - end - - # When the counter would expire - # - # @api private Used internally by SRE and debugging purpose - # @return [Integer] Number in seconds until expiration or false if never - def counter_expires_in - Gitlab::Redis::SharedState.with do |redis| - redis.ttl(key) - end - end - - # Return the actual counter value - # - # @return [Integer] value - def counter_value - Gitlab::Redis::SharedState.with do |redis| - (redis.get(key) || 0).to_i - end - end - - # Reset the counter - # - # @private Used internally by SRE and debugging purpose - # @return [Boolean] whether reset was a success - def reset! - redis_cmd do |redis| - redis.del(key) - end - end - - private - - attr_reader :key, :variants - - # Increase the counter - # - # @return [Boolean] whether operation was a success - def increment_counter - redis_cmd do |redis| - redis.incr(key) - redis.expire(key, COUNTER_EXPIRE_TIME) - end - end - - def resolve_variant_name - remainder = counter_value % variants.size - - variants[remainder] - end - - def redis_cmd - Gitlab::Redis::SharedState.with { |redis| yield(redis) } - - true - rescue CacheError => e - Gitlab::AppLogger.warn("GitLab: An unexpected error occurred in writing to Redis: #{e}") - - false - end - end -end diff --git a/app/mailers/emails/members.rb b/app/mailers/emails/members.rb index d18700658455ab35ec6e72fbeb0d3b40b384771f..fe2d289154773cbf2901db7cbd6f6217220f3f69 100644 --- a/app/mailers/emails/members.rb +++ b/app/mailers/emails/members.rb @@ -6,6 +6,8 @@ module Members include MembersHelper include Gitlab::Experiment::Dsl + INITIAL_INVITE = 'initial_email' + included do helper_method :member_source, :member helper_method :experiment @@ -53,6 +55,8 @@ def member_invited_email(member_source_type, member_id, token) return unless member_exists? + Gitlab::Tracking.event(self.class.name, 'invite_email_sent', label: 'invite_email', property: member_id.to_s) + mail(to: member.invite_email, subject: invite_email_subject, **invite_email_headers) do |format| format.html { render layout: 'unknown_user_mailer' } format.text { render layout: 'unknown_user_mailer' } diff --git a/app/views/notify/member_invited_email.html.haml b/app/views/notify/member_invited_email.html.haml index a4ea63e3d53f0f7267b9e232897cfee0e06f76aa..7ddc620e61834b4f1186ae6ec710e4e8c18a9a96 100644 --- a/app/views/notify/member_invited_email.html.haml +++ b/app/views/notify/member_invited_email.html.haml @@ -1,60 +1,54 @@ -- placeholders = { strong_start: '<strong>'.html_safe, strong_end: '</strong>'.html_safe, project_or_group_name: member_source.human_name, project_or_group: member_source.model_name.singular, br_tag: '<br/>'.html_safe, role: member.human_access.downcase } +- placeholders = { strong_start: '<strong>'.html_safe, + strong_end: '</strong>'.html_safe, + project_or_group_name: member_source.human_name, + project_or_group: member_source.model_name.singular, + br_tag: '<br/>'.html_safe, + role: member.human_access.downcase } -- experiment('members/invite_email', actor: member) do |experiment_instance| - - experiment_instance.use do - %tr - %td.text-content - %h2.invite-header - = s_('InviteEmail|You are invited!') - %p - - if member.created_by - = html_escape(s_("InviteEmail|%{inviter} invited you to join the %{strong_start}%{project_or_group_name}%{strong_end}%{br_tag}%{project_or_group} as a %{role}")) % placeholders.merge({ inviter: (link_to member.created_by.name, user_url(member.created_by)).html_safe }) - - else - = html_escape(s_("InviteEmail|You are invited to join the %{strong_start}%{project_or_group_name}%{strong_end}%{br_tag}%{project_or_group} as a %{role}")) % placeholders - %p.invite-actions - = link_to s_('InviteEmail|Join now'), invite_url(@token, invite_type: Members::InviteEmailExperiment::INVITE_TYPE), class: 'invite-btn-join' - - experiment_instance.try(:activity) do - %tr - %td.text-content{ colspan: 2 } - %img.mail-avatar{ height: "60", src: avatar_icon_for_user(member.created_by, 60, only_path: false), width: "60", alt: "" } - %p - = html_escape(s_("InviteEmail|%{inviter} invited you to join the %{strong_start}%{project_or_group_name}%{strong_end}%{br_tag}%{project_or_group} as a %{role}")) % placeholders.merge({ inviter: (link_to member.created_by.name, user_url(member.created_by)).html_safe }) - %p.invite-actions - = link_to s_('InviteEmail|Join now'), invite_url(@token, invite_type: Members::InviteEmailExperiment::INVITE_TYPE), class: 'invite-btn-join' - %tr.border-top - %td.text-content.mailer-align-left.half-width - %h4 - = s_('InviteEmail|%{project_or_group} details') % { project_or_group: member_source.model_name.singular.capitalize } - %ul - %li - %div - %img.mailer-icon{ alt: '', src: image_url("mailers/members/users.png") } - %span - - member_count = member_source.members.size - = n_('%{bold_start}%{count}%{bold_end} member', '%{bold_start}%{count}%{bold_end} members', - member_count).html_safe % { count: number_with_delimiter(member_count), - bold_start: '<b>'.html_safe, - bold_end: '</b>'.html_safe } - %li - %div - %img.mailer-icon{ alt: '', src: image_url("mailers/members/issues.png") } - %span - - issue_count = member_source.open_issues_count(member.created_by) - = n_('%{bold_start}%{count}%{bold_end} issue', '%{bold_start}%{count}%{bold_end} issues', - issue_count).html_safe % { count: number_with_delimiter(issue_count), - bold_start: '<b>'.html_safe, - bold_end: '</b>'.html_safe } - %li - %div - %img.mailer-icon{ alt: '', src: image_url("mailers/members/merge-request-open.png") } - %span - - mr_count = member_source.open_merge_requests_count(member.created_by) - = n_('%{bold_start}%{count}%{bold_end} opened merge request', '%{bold_start}%{count}%{bold_end} opened merge requests', - mr_count).html_safe % { count: number_with_delimiter(mr_count), - bold_start: '<b>'.html_safe, - bold_end: '</b>'.html_safe } - %td.text-content.mailer-align-left.half-width - %h4 - = s_("InviteEmail|What's it about?") - %p - = invited_to_description(member_source) +%tr + %td.text-content{ colspan: 2 } + %img.mail-avatar{ height: "60", src: avatar_icon_for_user(member.created_by, 60, only_path: false), width: "60", alt: "" } + %p + - if member.created_by + = html_escape(s_("InviteEmail|%{inviter} invited you to join the %{strong_start}%{project_or_group_name}%{strong_end}%{br_tag}%{project_or_group} as a %{role}")) % placeholders.merge({ inviter: (link_to member.created_by.name, user_url(member.created_by)).html_safe }) + - else + = html_escape(s_("InviteEmail|You are invited to join the %{strong_start}%{project_or_group_name}%{strong_end}%{br_tag}%{project_or_group} as a %{role}")) % placeholders + %p.invite-actions + = link_to s_('InviteEmail|Join now'), invite_url(@token, invite_type: Emails::Members::INITIAL_INVITE), class: 'invite-btn-join' +%tr.border-top + %td.text-content.mailer-align-left.half-width + %h4 + = s_('InviteEmail|%{project_or_group} details') % { project_or_group: member_source.model_name.singular.capitalize } + %ul + %li + %div + %img.mailer-icon{ alt: '', src: image_url("mailers/members/users.png") } + %span + - member_count = member_source.members.size + = n_('%{bold_start}%{count}%{bold_end} member', '%{bold_start}%{count}%{bold_end} members', + member_count).html_safe % { count: number_with_delimiter(member_count), + bold_start: '<b>'.html_safe, + bold_end: '</b>'.html_safe } + %li + %div + %img.mailer-icon{ alt: '', src: image_url("mailers/members/issues.png") } + %span + - issue_count = member_source.open_issues_count(member.created_by) + = n_('%{bold_start}%{count}%{bold_end} issue', '%{bold_start}%{count}%{bold_end} issues', + issue_count).html_safe % { count: number_with_delimiter(issue_count), + bold_start: '<b>'.html_safe, + bold_end: '</b>'.html_safe } + %li + %div + %img.mailer-icon{ alt: '', src: image_url("mailers/members/merge-request-open.png") } + %span + - mr_count = member_source.open_merge_requests_count(member.created_by) + = n_('%{bold_start}%{count}%{bold_end} opened merge request', '%{bold_start}%{count}%{bold_end} opened merge requests', + mr_count).html_safe % { count: number_with_delimiter(mr_count), + bold_start: '<b>'.html_safe, + bold_end: '</b>'.html_safe } + %td.text-content.mailer-align-left.half-width + %h4 + = s_("InviteEmail|What's it about?") + %p + = invited_to_description(member_source) diff --git a/config/feature_flags/experiment/members_invite_email.yml b/config/feature_flags/experiment/members_invite_email.yml deleted file mode 100644 index 5bf0b9d10d03584240543d8312649db2c173b821..0000000000000000000000000000000000000000 --- a/config/feature_flags/experiment/members_invite_email.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: members_invite_email -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/51223 -rollout_issue_url: https://gitlab.com/gitlab-org/growth/team-tasks/-/issues/325 -milestone: '13.9' -type: experiment -group: group::expansion -default_enabled: false diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 20c09e11936570728062519c8bf43cabea49f611..f3a17712faa9d32c0a8afaa7b9474ffc678a41e3 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -17908,9 +17908,6 @@ msgstr "" msgid "InviteEmail|You are invited to join the %{strong_start}%{project_or_group_name}%{strong_end}%{br_tag}%{project_or_group} as a %{role}" msgstr "" -msgid "InviteEmail|You are invited!" -msgstr "" - msgid "InviteEmail|You have been invited to join the %{project_or_group_name} %{project_or_group} as a %{role}" msgstr "" diff --git a/spec/controllers/invites_controller_spec.rb b/spec/controllers/invites_controller_spec.rb index 0d9cde88ecab27ce8bdbc564701aac063f42997e..345e8e47d1d61339b70c2518fe3040a99df4a9cb 100644 --- a/spec/controllers/invites_controller_spec.rb +++ b/spec/controllers/invites_controller_spec.rb @@ -25,37 +25,47 @@ end end - describe 'GET #show' do + describe 'GET #show', :snowplow do subject(:request) { get :show, params: params } - context 'when it is part of our invite email experiment' do + context 'when it is an initial invite email' do let(:extra_params) { { invite_type: 'initial_email' } } - it 'tracks the experiment' do - experiment = double(track: true) - allow(controller).to receive(:experiment).with('members/invite_email', actor: member).and_return(experiment) - + it 'tracks the initial join click from email' do request - expect(experiment).to have_received(:track).with(:join_clicked) + expect_snowplow_event( + category: described_class.name, + action: 'join_clicked', + label: 'invite_email', + property: member.id.to_s + ) end context 'when member does not exist' do let(:raw_invite_token) { '_bogus_token_' } - it 'does not track the experiment' do - expect(controller).not_to receive(:experiment).with('members/invite_email', actor: member) - + it 'does not track join click' do request + + expect_no_snowplow_event( + category: described_class.name, + action: 'join_clicked', + label: 'invite_email' + ) end end end - context 'when it is not part of our invite email experiment' do - it 'does not track via experiment' do - expect(controller).not_to receive(:experiment).with('members/invite_email', actor: member) - + context 'when it is not an initial email' do + it 'does not track the join click' do request + + expect_no_snowplow_event( + category: described_class.name, + action: 'join_clicked', + label: 'invite_email' + ) end end diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb index 72aa9038c3e4bbc9e6b58a094841f40b62101398..4c91d3e890aa9ebf1962746d20343f34337d9ae6 100644 --- a/spec/controllers/registrations_controller_spec.rb +++ b/spec/controllers/registrations_controller_spec.rb @@ -155,7 +155,7 @@ end context 'when registration is triggered from an accepted invite' do - context 'when it is part of our invite email experiment', :experiment do + context 'when it is part from the initial invite email', :snowplow do let_it_be(:member) { create(:project_member, :invited, invite_email: user_params.dig(:user, :email)) } let(:originating_member_id) { member.id } @@ -167,22 +167,29 @@ end context 'when member exists from the session key value' do - it 'tracks the experiment' do - expect(experiment('members/invite_email')).to track(:accepted) - .with_context(actor: member) - .on_next_instance - + it 'tracks the invite acceptance' do subject + + expect_snowplow_event( + category: 'RegistrationsController', + action: 'accepted', + label: 'invite_email', + property: member.id.to_s + ) end end context 'when member does not exist from the session key value' do let(:originating_member_id) { -1 } - it 'tracks the experiment' do - expect(experiment('members/invite_email')).not_to track(:accepted) - + it 'does not track invite acceptance' do subject + + expect_no_snowplow_event( + category: 'RegistrationsController', + action: 'accepted', + label: 'invite_email' + ) end end end diff --git a/spec/experiments/members/invite_email_experiment_spec.rb b/spec/experiments/members/invite_email_experiment_spec.rb deleted file mode 100644 index 47ae6e529a1bb5167d49ed1395021b3b4623ded1..0000000000000000000000000000000000000000 --- a/spec/experiments/members/invite_email_experiment_spec.rb +++ /dev/null @@ -1,117 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Members::InviteEmailExperiment, :clean_gitlab_redis_shared_state do - subject(:invite_email) { experiment('members/invite_email', **context) } - - let(:context) { { actor: double('Member', created_by: double('User', avatar_url: '_avatar_url_')) } } - - before do - allow(invite_email).to receive(:enabled?).and_return(true) - end - - describe ".initial_invite_email?" do - it "is an initial invite email" do - expect(described_class.initial_invite_email?('initial_email')).to be(true) - end - - it "is not an initial invite email" do - expect(described_class.initial_invite_email?('_bogus_')).to be(false) - end - end - - describe "exclusions", :experiment do - it "excludes when created by is nil" do - expect(experiment('members/invite_email')).to exclude(actor: double(created_by: nil)) - end - - it "excludes when avatar_url is nil" do - member_without_avatar_url = double('Member', created_by: double('User', avatar_url: nil)) - - expect(experiment('members/invite_email')).to exclude(actor: member_without_avatar_url) - end - end - - describe "variant resolution" do - it "proves out round robin in variant selection", :aggregate_failures do - instance_1 = described_class.new('members/invite_email', **context) - allow(instance_1).to receive(:enabled?).and_return(true) - instance_2 = described_class.new('members/invite_email', **context) - allow(instance_2).to receive(:enabled?).and_return(true) - - instance_1.try { } - - expect(instance_1.variant.name).to eq('control') - - instance_2.try { } - - expect(instance_2.variant.name).to eq('activity') - end - end - - describe Members::RoundRobin do - subject(:round_robin) { Members::RoundRobin.new('_key_', %i[variant1 variant2]) } - - describe "execute" do - context "when there are 2 variants" do - it "proves out round robin in selection", :aggregate_failures do - expect(round_robin.execute).to eq :variant2 - expect(round_robin.execute).to eq :variant1 - expect(round_robin.execute).to eq :variant2 - end - end - - context "when there are more than 2 variants" do - subject(:round_robin) { Members::RoundRobin.new('_key_', %i[variant1 variant2 variant3]) } - - it "proves out round robin in selection", :aggregate_failures do - expect(round_robin.execute).to eq :variant2 - expect(round_robin.execute).to eq :variant3 - expect(round_robin.execute).to eq :variant1 - - expect(round_robin.execute).to eq :variant2 - expect(round_robin.execute).to eq :variant3 - expect(round_robin.execute).to eq :variant1 - end - end - - context "when writing to cache fails" do - subject(:round_robin) { Members::RoundRobin.new('_key_', []) } - - it "raises an error and logs" do - allow(Gitlab::Redis::SharedState).to receive(:with).and_raise(Members::RoundRobin::CacheError) - expect(Gitlab::AppLogger).to receive(:warn) - - expect { round_robin.execute }.to raise_error(Members::RoundRobin::CacheError) - end - end - end - - describe "#counter_expires_in" do - it 'displays the expiration time in seconds' do - round_robin.execute - - expect(round_robin.counter_expires_in).to be_between(0, described_class::COUNTER_EXPIRE_TIME) - end - end - - describe '#value' do - it 'get the count' do - expect(round_robin.counter_value).to eq(0) - - round_robin.execute - - expect(round_robin.counter_value).to eq(1) - end - end - - describe '#reset!' do - it 'resets the count down to zero' do - 3.times { round_robin.execute } - - expect { round_robin.reset! }.to change { round_robin.counter_value }.from(3).to(0) - end - end - end -end diff --git a/spec/features/invites_spec.rb b/spec/features/invites_spec.rb index 93602033d7305243f9fab39a5707a40053073a1b..cf234032d336d8ee138063da988132c171afd085 100644 --- a/spec/features/invites_spec.rb +++ b/spec/features/invites_spec.rb @@ -179,7 +179,7 @@ def fill_in_welcome_form context 'when registering using invitation email' do before do - visit invite_path(group_invite.raw_invite_token, invite_type: Members::InviteEmailExperiment::INVITE_TYPE) + visit invite_path(group_invite.raw_invite_token, invite_type: Emails::Members::INITIAL_INVITE) end context 'with admin approval required enabled' do @@ -219,13 +219,16 @@ def fill_in_welcome_form end context 'email confirmation enabled' do - context 'with members/invite_email experiment', :experiment do + context 'with invite email acceptance', :snowplow do it 'tracks the accepted invite' do - expect(experiment('members/invite_email')).to track(:accepted) - .with_context(actor: group_invite) - .on_next_instance - fill_in_sign_up_form(new_user) + + expect_snowplow_event( + category: 'RegistrationsController', + action: 'accepted', + label: 'invite_email', + property: group_invite.id.to_s + ) end end diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index ae956adf563aa949871cb2b48c485d402c29ba3a..240abfc5c5334b0e36e1914f7afa402a5af3186c 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -781,7 +781,9 @@ def invite_to_project(project, inviter:, user: nil) let(:project_member) { invite_to_project(project, inviter: inviter) } let(:inviter) { maintainer } - subject { described_class.member_invited_email('project', project_member.id, project_member.invite_token) } + subject(:invite_email) do + described_class.member_invited_email('project', project_member.id, project_member.invite_token) + end it_behaves_like 'an email sent from GitLab' it_behaves_like 'it should not have Gmail Actions links' @@ -796,23 +798,10 @@ def invite_to_project(project, inviter:, user: nil) is_expected.to have_body_text project.full_name is_expected.to have_body_text project_member.human_access.downcase is_expected.to have_body_text project_member.invite_token - is_expected.to have_link('Join now', href: invite_url(project_member.invite_token, invite_type: Members::InviteEmailExperiment::INVITE_TYPE)) - end - - it 'contains invite link for the group activity' do - stub_experiments('members/invite_email': :activity) - + is_expected.to have_link('Join now', href: invite_url(project_member.invite_token, invite_type: Emails::Members::INITIAL_INVITE)) is_expected.to have_content("#{inviter.name} invited you to join the") is_expected.to have_content('Project details') is_expected.to have_content("What's it about?") - is_expected.not_to have_content('You are invited!') - is_expected.not_to have_body_text 'What is a GitLab' - end - - it 'has invite link for the control group' do - stub_experiments('members/invite_email': :control) - - is_expected.to have_content('You are invited!') end end @@ -824,6 +813,22 @@ def invite_to_project(project, inviter:, user: nil) is_expected.to have_body_text project.full_name is_expected.to have_body_text project_member.human_access.downcase is_expected.to have_body_text project_member.invite_token + is_expected.to have_link('Join now', href: invite_url(project_member.invite_token, invite_type: Emails::Members::INITIAL_INVITE)) + is_expected.to have_content('Project details') + is_expected.to have_content("What's it about?") + end + end + + context 'when invite email sent is tracked', :snowplow do + it 'tracks the sent invite' do + invite_email.deliver_now + + expect_snowplow_event( + category: 'Notify', + action: 'invite_email_sent', + label: 'invite_email', + property: project_member.id.to_s + ) end end