Merge branch '3326-fix-license_helper-memoization-issue' into 'master'

Resolve "LicenseHelper uses "extend self" inapproriately"

Closes #3326

See merge request gitlab-org/gitlab!28723

ee/app/helpers/ee/dashboard_helper.rb

@@ -28,7 +28,7 @@ module EE
     end
 
     def has_start_trial?
-      !current_license && current_user.admin?
+      !current_user.has_current_license? && current_user.admin?
     end
 
     def analytics_nav_url

ee/app/helpers/ee/gitlab_routing_helper.rb

@@ -42,6 +42,12 @@ module EE
       project_security_vulnerability_path(entity.project, entity, *args)
     end
 
+    def upgrade_plan_path(group)
+      return profile_billings_path if group.blank?
+
+      group_billings_path(group)
+    end
+
     def self.url_helper(route_name)
       define_method("#{route_name}_url") do |*args|
         path = public_send(:"#{route_name}_path", *args) # rubocop:disable GitlabSecurity/PublicSend

ee/app/helpers/license_helper.rb

@@ -18,30 +18,25 @@ module LicenseHelper
     License.current&.maximum_user_count || 0
   end
 
-  def license_message(signed_in: signed_in?, is_admin: current_user&.admin?)
+  def license_message(signed_in: signed_in?, is_admin: current_user&.admin?, license: License.current)
     Gitlab::ExpiringSubscriptionMessage.new(
-      subscribable: current_license,
+      subscribable: license,
       signed_in: signed_in,
       is_admin: is_admin
     ).message
   end
 
-  def seats_calculation_message
-    if current_license&.exclude_guests_from_active_count?
-      content_tag :p do
-        "Users with a Guest role or those who don't belong to a Project or Group will not use a seat from your license."
-      end
-    end
-  end
+  def seats_calculation_message(license)
+    return unless license.present?
+    return unless license.exclude_guests_from_active_count?
 
-  def current_license
-    return @current_license if defined?(@current_license)
-
-    @current_license = License.current
+    content_tag :p do
+      "Users with a Guest role or those who don't belong to a Project or Group will not use a seat from your license."
+    end
   end
 
   def current_license_title
-    @current_license_title ||= License.current ? License.current.plan.titleize : 'Core'
+    License.current&.plan&.titleize || 'Core'
   end
 
   def new_trial_url
@@ -52,15 +47,6 @@ module LicenseHelper
     uri.to_s
   end
 
-  def upgrade_plan_url
-    group = @project&.group || @group
-    if group
-      group_billings_path(group)
-    else
-      profile_billings_path
-    end
-  end
-
   def show_promotions?(selected_user = current_user)
     return false unless selected_user
 

ee/app/views/admin/licenses/_breakdown.html.haml

@@ -32,7 +32,7 @@
               = _(' %{start} to %{end}') % { start: @license.starts_at, end: @license.expires_at }
             \.
           = _('The %{link_start}true-up model%{link_end} allows having more users, and additional users will incur a retroactive charge on renewal.').html_safe % { link_start: true_up_link_start, link_end: link_end }
-          = seats_calculation_message
+          = seats_calculation_message(@license)
     .col-sm-6.d-flex.pr-0
       .info-well.dark-well
         .well-segment.well-centered

ee/app/views/admin/licenses/show.html.haml

@@ -5,7 +5,7 @@
 - else
   %h3.page-title
     Your License
-    - if current_license.trial?
+    - if @license.trial?
       = render "upload_buy_license"
     - else
       = link_to 'Upload New License', new_admin_license_path, class: "btn btn-success float-right"

ee/app/views/layouts/header/_ee_subscribable_banner.html.haml

@@ -2,8 +2,8 @@
   - subscribable = decorated_subscription
   - message = subscription_message
 - else
-  - subscribable = current_license
-  - message = license_message
+  - subscribable = License.current
+  - message = license_message(license: subscribable)
 
 - if message.present?
   .container-fluid.container-limited.pt-3

ee/app/views/shared/promotions/_promotion_link_project.html.haml

@@ -3,7 +3,8 @@
 - if Gitlab::CurrentSettings.should_check_namespace_plan?
   - namespace = @project&.namespace || @group
   - if can?(current_user, :admin_namespace, namespace)
-    = link_to _('Upgrade your plan'), upgrade_plan_url, class: 'btn btn-primary', target: target_blank ? '_blank' : '_self'
+    - current_group = @project&.group || @group
+    = link_to _('Upgrade your plan'), upgrade_plan_path(current_group), class: 'btn btn-primary', target: target_blank ? '_blank' : '_self'
   - elsif namespace.is_a?(Group)
     %p= _('Contact an owner of group %{namespace_name} to upgrade the plan.') % { namespace_name: namespace.name }
   - else

ee/spec/helpers/ee/dashboard_helper_spec.rb

@@ -208,7 +208,7 @@ describe DashboardHelper, type: :helper do
 
       before do
         allow(helper).to receive(:current_user).and_return(user)
-        allow(helper).to receive(:current_license).and_return(license)
+        allow(License).to receive(:current).and_return(license)
       end
 
       it { is_expected.to eq(output) }

ee/spec/helpers/ee/gitlab_routing_helper_spec.rb

@@ -114,4 +114,24 @@ describe EE::GitlabRoutingHelper do
       expect(subject).to start_with 'http://localhost/users/auth/group_saml/metadata?group_path=foo&token='
     end
   end
+
+  describe '#upgrade_plan_path' do
+    subject { upgrade_plan_path(group) }
+
+    context 'when the group is present' do
+      let(:group) { build_stubbed(:group) }
+
+      it "returns the group billing path" do
+        expect(subject).to eq(group_billings_path(group))
+      end
+    end
+
+    context 'when the group is blank' do
+      let(:group) { nil }
+
+      it "returns the profile billing path" do
+        expect(subject).to eq(profile_billings_path)
+      end
+    end
+  end
 end

ee/spec/helpers/license_helper_spec.rb

@@ -90,4 +90,67 @@ describe LicenseHelper do
       end
     end
   end
+
+  describe '#current_license_title' do
+    context 'when there is a current license' do
+      context 'and it has a plan associated to it' do
+        it 'returns the plan titleized' do
+          custom_plan = 'custom plan'
+          license = double('License', plan: custom_plan)
+          allow(License).to receive(:current).and_return(license)
+
+          expect(current_license_title).to eq(custom_plan.titleize)
+        end
+      end
+
+      context 'and it does not have a plan associated to it' do
+        it 'returns the default title' do
+          license = double('License', plan: nil)
+          allow(License).to receive(:current).and_return(license)
+
+          expect(current_license_title).to eq('Core')
+        end
+      end
+    end
+
+    context 'when there is NOT a current license' do
+      it 'returns the default title' do
+        allow(License).to receive(:current).and_return(nil)
+
+        expect(current_license_title).to eq('Core')
+      end
+    end
+  end
+
+  describe '#seats_calculation_message' do
+    subject { seats_calculation_message(license) }
+
+    context 'with a license' do
+      let(:license) { double("License", 'exclude_guests_from_active_count?' => exclude_guests) }
+
+      context 'and guest are excluded from the active count' do
+        let(:exclude_guests) { true }
+
+        it 'returns a tag with the message' do
+          expect(subject).to eq("<p>Users with a Guest role or those who don&#39;t belong to a Project or Group will not use a seat from your license.</p>")
+        end
+      end
+
+      context 'and guest are NOT excluded from the active count' do
+        let(:exclude_guests) { false }
+
+        it 'returns nil' do
+          expect(subject).to be_blank
+        end
+      end
+    end
+
+    context 'when the license is blank' do
+      let(:license) { nil }
+
+      it 'returns nil' do
+        expect(subject).to be_blank
+      end
+    end
+  end
 end