Apply suggestions from last code review!

ee/app/models/license.rb

@@ -6,7 +6,7 @@ class License < ApplicationRecord
   STARTER_PLAN = 'starter'.freeze
   PREMIUM_PLAN = 'premium'.freeze
   ULTIMATE_PLAN = 'ultimate'.freeze
-  ALLOWED_PERCENTAGE_OF_USERS_OVERAGE = 10
+  ALLOWED_PERCENTAGE_OF_USERS_OVERAGE = (10 / 100.0).freeze
 
   EES_FEATURES = %i[
     audit_events
@@ -563,23 +563,20 @@ class License < ApplicationRecord
     end
   end
 
-  def real_restricted_user_count
-    if previous_user_count
-      restricted_user_count
-    else
-      percent = ALLOWED_PERCENTAGE_OF_USERS_OVERAGE / 100.0
+  def restricted_user_count_with_threshold
+    # overage should only be applied for new subscriptions not for renewals.
+    return restricted_user_count if previous_user_count
 
-      (restricted_user_count * (1 + percent)).to_i
-    end
+    (restricted_user_count * (1 + ALLOWED_PERCENTAGE_OF_USERS_OVERAGE)).to_i
   end
 
   def check_users_limit
     return unless restricted_user_count
 
     if previous_user_count && (prior_historical_max <= previous_user_count)
-      return if real_restricted_user_count >= current_active_users_count
+      return if restricted_user_count >= current_active_users_count
     else
-      return if real_restricted_user_count >= prior_historical_max
+      return if restricted_user_count_with_threshold >= prior_historical_max
     end
 
     user_count = prior_historical_max == 0 ? current_active_users_count : prior_historical_max

ee/spec/models/license_spec.rb

@@ -28,89 +28,91 @@ RSpec.describe License do
     end
 
     describe '#check_users_limit' do
-      before do
-        create(:group_member, :guest)
-        create(:group_member, :reporter)
-        create(:license, plan: plan)
-      end
+      context 'for each plan' do
+        before do
+          create(:group_member, :guest)
+          create(:group_member, :reporter)
+          create(:license, plan: plan)
+        end
 
-      let(:users_count) { nil }
-      let(:new_license) do
-        gl_license = build(:gitlab_license, restrictions: { plan: plan, active_user_count: users_count, previous_user_count: 1 })
-        build(:license, data: gl_license.export)
-      end
+        let(:users_count) { nil }
+        let(:new_license) do
+          gl_license = build(:gitlab_license, restrictions: { plan: plan, active_user_count: users_count, previous_user_count: 1 })
+          build(:license, data: gl_license.export)
+        end
 
-      where(:gl_plan, :valid) do
-        ::License::STARTER_PLAN  | false
-        ::License::PREMIUM_PLAN  | false
-        ::License::ULTIMATE_PLAN | true
-      end
+        where(:gl_plan, :valid) do
+          ::License::STARTER_PLAN  | false
+          ::License::PREMIUM_PLAN  | false
+          ::License::ULTIMATE_PLAN | true
+        end
 
-      with_them do
-        let(:plan) { gl_plan }
+        with_them do
+          let(:plan) { gl_plan }
 
-        context 'when license has restricted users' do
-          let(:users_count) { 1 }
+          context 'when license has restricted users' do
+            let(:users_count) { 1 }
 
-          it { expect(new_license.valid?).to eq(valid) }
-        end
+            it { expect(new_license.valid?).to eq(valid) }
+          end
 
-        context 'when license has unlimited users' do
-          let(:users_count) { nil }
+          context 'when license has unlimited users' do
+            let(:users_count) { nil }
 
-          it 'is always valid' do
-            expect(new_license.valid?).to eq(true)
+            it 'is always valid' do
+              expect(new_license.valid?).to eq(true)
+            end
           end
         end
       end
-    end
 
-    describe 'threshold for users overage' do
-      let(:current_active_users_count) { 0 }
-      let(:new_license) do
-        gl_license = build(
-          :gitlab_license,
-          starts_at: Date.today,
-          restrictions: { active_user_count: 10, previous_user_count: previous_user_count }
-        )
+      context 'threshold for users overage' do
+        let(:current_active_users_count) { 0 }
+        let(:new_license) do
+          gl_license = build(
+            :gitlab_license,
+            starts_at: Date.today,
+            restrictions: { active_user_count: 10, previous_user_count: previous_user_count }
+          )
 
-        build(:license, data: gl_license.export)
-      end
-
-      context 'when current active users count is above the limit set by the license' do
-        before do
-          create_list(:user, current_active_users_count)
-          HistoricalData.track!
+          build(:license, data: gl_license.export)
         end
 
-        context 'when license is from a fresh subscription' do
-          let(:previous_user_count) { nil }
+        context 'when current active users count is above the limit set by the license' do
+          before do
+            create_list(:user, current_active_users_count)
+            HistoricalData.track!
+          end
+
+          context 'when license is from a fresh subscription' do
+            let(:previous_user_count) { nil }
 
-          context 'when current active users count is under the threshold' do
-            let(:current_active_users_count) { 11 }
+            context 'when current active users count is under the threshold' do
+              let(:current_active_users_count) { 11 }
 
-            it 'accepts the license' do
-              expect(new_license).to be_valid
+              it 'accepts the license' do
+                expect(new_license).to be_valid
+              end
             end
-          end
 
-          context 'when current active users count is above the threshold' do
-            let(:current_active_users_count) { 12 }
+            context 'when current active users count is above the threshold' do
+              let(:current_active_users_count) { 12 }
 
-            it 'does not accept the license' do
-              expect(new_license).not_to be_valid
+              it 'does not accept the license' do
+                expect(new_license).not_to be_valid
+              end
             end
           end
-        end
 
-        context 'when license is from a renewal' do
-          let(:previous_user_count) { 1 }
+          context 'when license is from a renewal' do
+            let(:previous_user_count) { 1 }
 
-          context 'when current active users count is under the threshold' do
-            let(:current_active_users_count) { 11 }
+            context 'when current active users count is under the threshold' do
+              let(:current_active_users_count) { 11 }
 
-            it 'accepts the license' do
-              expect(new_license).not_to be_valid
+              it 'does not accept the license' do
+                expect(new_license).not_to be_valid
+              end
             end
           end
         end