Merge branch '550-improve-trueup-validation' into 'master'

Read true-up info from license and validate it

Closes #550

See merge request !1159

app/models/license.rb

@@ -2,7 +2,8 @@ class License < ActiveRecord::Base
   include ActionView::Helpers::NumberHelper
 
   validate :valid_license
-  validate :active_user_count, unless: :persisted?
+  validate :active_user_count, if: :new_record?, unless: :validate_with_trueup?
+  validate :check_trueup, unless: :persisted?, if: :validate_with_trueup?
   validate :not_expired, unless: :persisted?
 
   before_validation :reset_license, if: :data_changed?
@@ -83,23 +84,35 @@ class License < ActiveRecord::Base
   end
 
   def add_ons
-    return {} unless license? && restricted?(:add_ons)
-
-    restrictions[:add_ons]
+    restricted_attr(:add_ons, {})
   end
 
   def add_on?(code)
     add_ons[code].to_i > 0
   end
 
-  def user_count
-    return unless self.license? && self.restricted?(:active_user_count)
+  def restricted_user_count
+    restricted_attr(:active_user_count)
+  end
+
+  def previous_user_count
+    restricted_attr(:previous_user_count)
+  end
 
-    self.restrictions[:active_user_count]
+  def validate_with_trueup?
+    [restricted_attr(:trueup_quantity),
+     restricted_attr(:trueup_from),
+     restricted_attr(:trueup_to)].all?(&:present?)
   end
 
   private
 
+  def restricted_attr(name, default = nil)
+    return default unless license? && restricted?(name)
+
+    restrictions[name]
+  end
+
   def reset_current
     self.class.reset_current
   end
@@ -114,27 +127,54 @@ class License < ActiveRecord::Base
     self.errors.add(:base, "The license key is invalid. Make sure it is exactly as you received it from GitLab Inc.")
   end
 
-  def active_user_count
-    restricted_user_count = user_count
+  def historical_max(from, to)
+    HistoricalData.during(from..to).maximum(:active_user_count) || 0
+  end
 
+  def active_user_count
     return unless restricted_user_count
 
-    date_range = (self.starts_at - 1.year)..self.starts_at
-    active_user_count = HistoricalData.during(date_range).maximum(:active_user_count) || 0
+    historical_user_count = historical_max((starts_at - 1.year), starts_at)
+    overage               = historical_user_count - restricted_user_count
 
-    return unless active_user_count
+    return if historical_user_count <= restricted_user_count
+
+    add_limit_error(user_count: historical_user_count, restricted_user_count: restricted_user_count, overage: overage)
+  end
 
-    return if active_user_count <= restricted_user_count
+  def check_trueup
+    trueup_qty          = restrictions[:trueup_quantity]
+    trueup_from         = Date.parse(restrictions[:trueup_from]) rescue (starts_at - 1.year)
+    trueup_to           = Date.parse(restrictions[:trueup_to]) rescue starts_at
+    active_user_count   = User.active.count
+    max_historical      = historical_max(trueup_from, trueup_to)
+    overage             = active_user_count - restricted_user_count
+    expected_trueup_qty = if previous_user_count
+                            max_historical - previous_user_count
+                          else
+                            max_historical - active_user_count
+                          end
+
+    if trueup_qty >= expected_trueup_qty
+      if restricted_user_count < active_user_count
+        add_limit_error(trueup: true, user_count: active_user_count, restricted_user_count: restricted_user_count, overage: overage)
+      end
+    else
+      message = "You have applied a True-up for #{trueup_qty} #{"user".pluralize(trueup_qty)} "
+      message << "but you need one for #{expected_trueup_qty} #{"user".pluralize(expected_trueup_qty)}. "
+      message << "Please contact sales at renewals@gitlab.com"
 
-    overage = active_user_count - restricted_user_count
+      self.errors.add(:base, message)
+    end
+  end
 
-    message = ""
-    message << "During the year before this license started, this GitLab installation had "
-    message << "#{number_with_delimiter active_user_count} active #{"user".pluralize(active_user_count)}, "
-    message << "exceeding this license's limit of #{number_with_delimiter restricted_user_count} by "
-    message << "#{number_with_delimiter overage} #{"user".pluralize(overage)}. "
+  def add_limit_error(trueup: false, user_count:, restricted_user_count:, overage:)
+    message =  trueup ? "This GitLab installation currently has " : "During the year before this license started, this GitLab installation had "
+    message << "#{number_with_delimiter(user_count)} active #{"user".pluralize(user_count)}, "
+    message << "exceeding this license's limit of #{number_with_delimiter(restricted_user_count)} by "
+    message << "#{number_with_delimiter(overage)} #{"user".pluralize(overage)}. "
     message << "Please upload a license for at least "
-    message << "#{number_with_delimiter active_user_count} #{"user".pluralize(active_user_count)}."
+    message << "#{number_with_delimiter(user_count)} #{"user".pluralize(user_count)} or contact sales at renewals@gitlab.com"
 
     self.errors.add(:base, message)
   end

changelogs/unreleased/550-fix-historical-max-validation

+---
+title: Read true-up info from license and validate it
+merge_request: 1159
+author:

lib/gitlab/usage_data.rb

@@ -62,7 +62,7 @@ module Gitlab
           usage_data[:license_md5] = Digest::MD5.hexdigest(license.data)
           usage_data[:historical_max_users] = ::HistoricalData.max_historical_user_count
           usage_data[:licensee] = license.licensee
-          usage_data[:license_user_count] = license.user_count
+          usage_data[:license_user_count] = license.restricted_user_count
           usage_data[:license_starts_at] = license.starts_at
           usage_data[:license_expires_at] = license.expires_at
           usage_data[:license_add_ons] = license.add_ons

spec/lib/gitlab/usage_data_spec.rb

@@ -77,7 +77,7 @@ describe Gitlab::UsageData do
       expect(subject[:licensee]).to eq(license.licensee)
       expect(subject[:active_user_count]).to eq(User.active.count)
       expect(subject[:licensee]).to eq(license.licensee)
-      expect(subject[:license_user_count]).to eq(license.user_count)
+      expect(subject[:license_user_count]).to eq(license.restricted_user_count)
       expect(subject[:license_starts_at]).to eq(license.starts_at)
       expect(subject[:license_expires_at]).to eq(license.expires_at)
       expect(subject[:license_add_ons]).to eq(license.add_ons)

spec/models/license_spec.rb

@@ -88,6 +88,74 @@ describe License do
           expect(license).to be_valid
         end
       end
+
+      context 'with true-up info' do
+        def set_restrictions(opts)
+          gl_license.restrictions = {
+            active_user_count: opts[:restricted_user_count],
+            previous_user_count: opts[:previous_user_count],
+            trueup_quantity: opts[:trueup_quantity],
+            trueup_from: (Date.today - 1.year).to_s,
+            trueup_to: Date.today.to_s
+          }
+        end
+
+        context 'when quantity is ok' do
+          before do
+            set_restrictions(restricted_user_count: 5, trueup_quantity: 10)
+          end
+
+          it 'is valid' do
+            expect(license).to be_valid
+          end
+
+          context 'but active users exceeds restricted user count' do
+            it 'is invalid' do
+              6.times { create(:user) }
+
+              expect(license).not_to be_valid
+            end
+          end
+        end
+
+        context 'when quantity is wrong' do
+          it 'is invalid' do
+            set_restrictions(restricted_user_count: 5, trueup_quantity: 8)
+
+            expect(license).not_to be_valid
+          end
+        end
+
+        context 'when previous user count is not present' do
+          before do
+            set_restrictions(restricted_user_count: 5, trueup_quantity: 7)
+          end
+
+          it 'uses current active user count to calculate the expected true-up' do
+            3.times { create(:user) }
+
+            expect(license).to be_valid
+          end
+
+          context 'with wrong true-up quantity' do
+            it 'is invalid' do
+              2.times { create(:user) }
+
+              expect(license).not_to be_valid
+            end
+          end
+        end
+
+        context 'when previous user count is present' do
+          before do
+            set_restrictions(restricted_user_count: 5, trueup_quantity: 6, previous_user_count: 4)
+          end
+
+          it 'uses it to calculate the expected true-up' do
+            expect(license).to be_valid
+          end
+        end
+      end
     end
 
     describe "Not expired" do