Merge branch...

Merge branch '296967-update-new-user-invitation-email-test-variation-3-group-and-permission-information' into 'master'

Update new user invitation email with permission information

See merge request gitlab-org/gitlab!53275

app/assets/stylesheets/mailer.scss

@@ -14,13 +14,15 @@ $mailer-line-cell-bg-color: #6b4fbb;
 $mailer-wrapper-cell-bg-color: #fff;
 $mailer-wrapper-cell-border-color: #ededed;
 $mailer-header-footer-text-color: #5c5c5c;
+$full-width: 640px;
+$half-width: 320px;
 
 body {
   margin: 0 !important;
   background-color: $mailer-bg-color;
   padding: 0;
   text-align: center;
-  min-width: 640px;
+  min-width: $full-width;
   width: 100%;
   height: 100%;
   font-family: $mailer-font;
@@ -31,7 +33,7 @@ table#body {
   margin: 0;
   padding: 0;
   text-align: center;
-  min-width: 640px;
+  min-width: $full-width;
   width: 100%;
 }
 
@@ -44,6 +46,11 @@ a {
   }
 }
 
+.mail-avatar {
+  border-radius: 50%;
+  display: block;
+}
+
 .highlight {
   font-weight: 500;
 }
@@ -77,10 +84,18 @@ a {
   margin-left: 4px;
 }
 
+.half-width {
+  min-width: $half-width;
+}
+
 tr td {
   font-family: $mailer-font;
 }
 
+tr.border-top td {
+  border-top: 2px solid $gray-100;
+}
+
 tr.line td {
   font-family: $mailer-font;
   background-color: $mailer-line-cell-bg-color;
@@ -100,7 +115,7 @@ td.footer-message {
 }
 
 table.wrapper {
-  width: 640px;
+  width: $full-width;
   margin: 0 auto;
   border-collapse: separate;
   border-spacing: 0;
@@ -149,7 +164,7 @@ tr.footer td {
   }
 
   .gitlab-info-text {
-    max-width: 640px;
+    max-width: $full-width;
     margin: 0 auto;
     text-align: center;
     color: $gray-400;

app/experiments/application_experiment.rb

@@ -23,16 +23,41 @@ class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/Namesp
     ))
   end
 
+  def rollout_strategy
+    # no-op override in inherited class as desired
+  end
+
+  def variants
+    # override as desired in inherited class with all variants + control
+    # %i[variant1 variant2 control]
+    #
+    # this will make sure we supply variants as these go together - rollout_strategy of :round_robin must have variants
+    raise NotImplementedError, "Inheriting class must supply variants as an array if :round_robin strategy is used" if rollout_strategy == :round_robin
+  end
+
   private
 
+  def feature_flag_name
+    name.tr('/', '_')
+  end
+
   def resolve_variant_name
-    return variant_names.first if Feature.enabled?(feature_flag_name, self, type: :experiment, default_enabled: :yaml)
+    case rollout_strategy
+    when :round_robin
+      round_robin_rollout
+    else
+      percentage_rollout
+    end
+  end
 
-    nil # Returning nil vs. :control is important for not caching and rollouts.
+  def round_robin_rollout
+    Strategy::RoundRobin.new(feature_flag_name, variants).execute
   end
 
-  def feature_flag_name
-    name.tr('/', '_')
+  def percentage_rollout
+    return variant_names.first if Feature.enabled?(feature_flag_name, self, type: :experiment, default_enabled: :yaml)
+
+    nil # Returning nil vs. :control is important for not caching and rollouts.
   end
 
   # Cache is an implementation on top of Gitlab::Redis::SharedState that also

app/experiments/members/invite_email_experiment.rb

@@ -7,16 +7,12 @@ module Members
 
     INVITE_TYPE = 'initial_email'
 
-    private
+    def rollout_strategy
+      :round_robin
+    end
 
-    def resolve_variant_name
-      # we are overriding here so that when we add another experiment
-      # we can merely add that variant and check of feature flag here
-      if Feature.enabled?(feature_flag_name, self, type: :experiment, default_enabled: :yaml)
-        :avatar
-      else
-        nil # :control
-      end
+    def variants
+      %i[avatar permission_info control]
     end
   end
 end

app/experiments/strategy/round_robin.rb

+# frozen_string_literal: true
+
+module Strategy
+  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

app/helpers/notify_helper.rb

@@ -8,4 +8,30 @@ module NotifyHelper
   def issue_reference_link(entity, *args, full: false)
     link_to(entity.to_reference(full: full), issue_url(entity, *args))
   end
+
+  def invited_role_description(role_name)
+    case role_name
+    when "Guest"
+      s_("InviteEmail|As a guest, you can view projects, leave comments, and create issues.")
+    when "Reporter"
+      s_("InviteEmail|As a reporter, you can view projects and reports, and leave comments on issues.")
+    when "Developer"
+      s_("InviteEmail|As a developer, you have full access to projects, so you can take an idea from concept to production.")
+    when "Maintainer"
+      s_("InviteEmail|As a maintainer, you have full access to projects. You can push commits to master and deploy to production.")
+    when "Owner"
+      s_("InviteEmail|As an owner, you have full access to projects and can manage access to the group, including inviting new members.")
+    when "Minimal Access"
+      s_("InviteEmail|As a user with minimal access, you can view the high-level group from the UI and API.")
+    end
+  end
+
+  def invited_to_description(source)
+    case source
+    when "project"
+      s_('InviteEmail|Projects can be used to host your code, track issues, collaborate on code, and continuously build, test, and deploy your app with built-in GitLab CI/CD.')
+    when "group"
+      s_('InviteEmail|Groups assemble related projects together and grant members access to several projects at once.')
+    end
+  end
 end

app/views/notify/member_invited_email.html.haml

 - 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 |e|
-  - e.use do
+- experiment('members/invite_email', actor: member) do |experiment_instance|
+  - experiment_instance.use do
     %tr
       %td.text-content
         %h2.invite-header
@@ -13,11 +13,28 @@
             = 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'
-  - e.try(:avatar) do
+  - experiment_instance.try(:avatar) do
     %tr
       %td.text-content
-        %img.avatar{ height: "60", src: avatar_icon_for_user(member.created_by, 60, only_path: false), style: "display: block; border-radius: 30px; margin: -2px 0;", width: "60", alt: "" }
+        %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'
+  - experiment_instance.try(:permission_info) 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} with the %{role} permission level.")) % 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.half-width
+        %h4
+          = s_('InviteEmail|What is a GitLab %{project_or_group}?') % { project_or_group: member_source.model_name.singular }
+        %p= invited_to_description(member_source.model_name.singular)
+      %td.text-content.half-width
+        %h4
+          = s_('InviteEmail|What can I do with the %{role} permission level?') % { role: member.human_access.downcase }
+        %p= invited_role_description(member.human_access)

locale/gitlab.pot

@@ -16295,9 +16295,42 @@ msgstr ""
 msgid "InviteEmail|%{inviter} invited you to join the %{strong_start}%{project_or_group_name}%{strong_end}%{br_tag}%{project_or_group} as a %{role}"
 msgstr ""
 
+msgid "InviteEmail|%{inviter} invited you to join the %{strong_start}%{project_or_group_name}%{strong_end}%{br_tag}%{project_or_group} with the %{role} permission level."
+msgstr ""
+
+msgid "InviteEmail|As a developer, you have full access to projects, so you can take an idea from concept to production."
+msgstr ""
+
+msgid "InviteEmail|As a guest, you can view projects, leave comments, and create issues."
+msgstr ""
+
+msgid "InviteEmail|As a maintainer, you have full access to projects. You can push commits to master and deploy to production."
+msgstr ""
+
+msgid "InviteEmail|As a reporter, you can view projects and reports, and leave comments on issues."
+msgstr ""
+
+msgid "InviteEmail|As a user with minimal access, you can view the high-level group from the UI and API."
+msgstr ""
+
+msgid "InviteEmail|As an owner, you have full access to projects and can manage access to the group, including inviting new members."
+msgstr ""
+
+msgid "InviteEmail|Groups assemble related projects together and grant members access to several projects at once."
+msgstr ""
+
 msgid "InviteEmail|Join now"
 msgstr ""
 
+msgid "InviteEmail|Projects can be used to host your code, track issues, collaborate on code, and continuously build, test, and deploy your app with built-in GitLab CI/CD."
+msgstr ""
+
+msgid "InviteEmail|What can I do with the %{role} permission level?"
+msgstr ""
+
+msgid "InviteEmail|What is a GitLab %{project_or_group}?"
+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 ""
 

spec/experiments/application_experiment_spec.rb

@@ -115,24 +115,77 @@ RSpec.describe ApplicationExperiment, :experiment do
   end
 
   describe "variant resolution" do
-    it "uses the default value as specified in the yaml" do
-      expect(Feature).to receive(:enabled?).with('namespaced_stub', subject, type: :experiment, default_enabled: :yaml)
+    context "when using the default feature flag percentage rollout" do
+      it "uses the default value as specified in the yaml" do
+        expect(Feature).to receive(:enabled?).with('namespaced_stub', subject, type: :experiment, default_enabled: :yaml)
 
-      expect(subject.variant.name).to eq('control')
-    end
+        expect(subject.variant.name).to eq('control')
+      end
+
+      it "returns nil when not rolled out" do
+        stub_feature_flags(namespaced_stub: false)
+
+        expect(subject.variant.name).to eq('control')
+      end
 
-    it "returns nil when not rolled out" do
-      stub_feature_flags(namespaced_stub: false)
+      context "when rolled out to 100%" do
+        it "returns the first variant name" do
+          subject.try(:variant1) {}
+          subject.try(:variant2) {}
 
-      expect(subject.variant.name).to eq('control')
+          expect(subject.variant.name).to eq('variant1')
+        end
+      end
     end
 
-    context "when rolled out to 100%" do
-      it "returns the first variant name" do
-        subject.try(:variant1) {}
-        subject.try(:variant2) {}
+    context "when using the round_robin strategy", :clean_gitlab_redis_shared_state do
+      context "when variants aren't supplied" do
+        subject :inheriting_class do
+          Class.new(described_class) do
+            def rollout_strategy
+              :round_robin
+            end
+          end.new('namespaced/stub')
+        end
+
+        it "raises an error" do
+          expect { inheriting_class.variants }.to raise_error(NotImplementedError)
+        end
+      end
+
+      context "when variants are supplied" do
+        let(:inheriting_class) do
+          Class.new(described_class) do
+            def rollout_strategy
+              :round_robin
+            end
+
+            def variants
+              %i[variant1 variant2 control]
+            end
+          end
+        end
+
+        it "proves out round robin in variant selection", :aggregate_failures do
+          instance_1 = inheriting_class.new('namespaced/stub')
+          allow(instance_1).to receive(:enabled?).and_return(true)
+          instance_2 = inheriting_class.new('namespaced/stub')
+          allow(instance_2).to receive(:enabled?).and_return(true)
+          instance_3 = inheriting_class.new('namespaced/stub')
+          allow(instance_3).to receive(:enabled?).and_return(true)
+
+          instance_1.try {}
+
+          expect(instance_1.variant.name).to eq('variant2')
+
+          instance_2.try {}
+
+          expect(instance_2.variant.name).to eq('control')
+
+          instance_3.try {}
 
-        expect(subject.variant.name).to eq('variant1')
+          expect(instance_3.variant.name).to eq('variant1')
+        end
       end
     end
   end

spec/experiments/members/invite_email_experiment_spec.rb

@@ -3,27 +3,23 @@
 require 'spec_helper'
 
 RSpec.describe Members::InviteEmailExperiment do
-  subject do
+  subject :invite_email do
     experiment('members/invite_email', actor: double('Member', created_by: double('User', avatar_url: '_avatar_url_')))
   end
 
   before do
-    allow(subject).to receive(:enabled?).and_return(true)
+    allow(invite_email).to receive(:enabled?).and_return(true)
   end
 
-  describe "variant resolution" do
-    it "returns nil when not rolled out" do
-      stub_feature_flags(members_invite_email: false)
-
-      expect(subject.variant.name).to eq('control')
+  describe "#rollout_strategy" do
+    it "resolves to round_robin" do
+      expect(invite_email.rollout_strategy).to eq(:round_robin)
     end
+  end
 
-    context "when rolled out to 100%" do
-      it "returns the first variant name" do
-        subject.try(:avatar) {}
-
-        expect(subject.variant.name).to eq('avatar')
-      end
+  describe "#variants" do
+    it "has all the expected variants" do
+      expect(invite_email.variants).to match(%i[avatar permission_info control])
     end
   end
 

spec/experiments/strategy/round_robin_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Strategy::RoundRobin, :clean_gitlab_redis_shared_state do
+  subject(:round_robin) { described_class.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) { described_class.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) { described_class.new('_key_', []) }
+
+      it "raises an error and logs" do
+        allow(Gitlab::Redis::SharedState).to receive(:with).and_raise(Strategy::RoundRobin::CacheError)
+        expect(Gitlab::AppLogger).to receive(:warn)
+
+        expect { round_robin.execute }.to raise_error(Strategy::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

spec/helpers/notify_helper_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 RSpec.describe NotifyHelper do
   include ActionView::Helpers::UrlHelper
+  using RSpec::Parameterized::TableSyntax
 
   describe 'merge_request_reference_link' do
     let(:project) { create(:project) }
@@ -27,6 +28,36 @@ RSpec.describe NotifyHelper do
     end
   end
 
+  describe '#invited_role_description' do
+    where(:role, :description) do
+      "Guest"          | /As a guest/
+      "Reporter"       | /As a reporter/
+      "Developer"      | /As a developer/
+      "Maintainer"     | /As a maintainer/
+      "Owner"          | /As an owner/
+      "Minimal Access" | /As a user with minimal access/
+    end
+
+    with_them do
+      specify do
+        expect(helper.invited_role_description(role)).to match description
+      end
+    end
+  end
+
+  describe '#invited_to_description' do
+    where(:source, :description) do
+      "project" | /Projects can/
+      "group"   | /Groups assemble/
+    end
+
+    with_them do
+      specify do
+        expect(helper.invited_to_description(source)).to match description
+      end
+    end
+  end
+
   def reference_link(entity, url)
     "<a href=\"#{url}\">#{entity.to_reference}</a>"
   end

spec/mailers/notify_spec.rb

@@ -912,6 +912,15 @@ RSpec.describe Notify do
           stub_experiments('members/invite_email': :avatar)
 
           is_expected.not_to have_content('You are invited!')
+          is_expected.not_to have_body_text 'What is a GitLab'
+        end
+
+        it 'contains invite link for the avatar', :experiment do
+          stub_experiments('members/invite_email': :permission_info)
+
+          is_expected.not_to have_content('You are invited!')
+          is_expected.to have_body_text 'What is a GitLab'
+          is_expected.to have_body_text 'What can I do with'
         end
 
         it 'has invite link for the control group' do