From 01a2df9e69ca284887fec641a10a9690bc8ad1db Mon Sep 17 00:00:00 2001 From: Douwe Maan <douwe@gitlab.com> Date: Tue, 7 Feb 2017 02:25:35 +0000 Subject: [PATCH 1/2] 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 | 80 ++++++++++++++----- .../550-fix-historical-max-validation | 4 + lib/gitlab/usage_data.rb | 2 +- spec/lib/gitlab/usage_data_spec.rb | 2 +- spec/models/license_spec.rb | 68 ++++++++++++++++ 5 files changed, 134 insertions(+), 22 deletions(-) create mode 100644 changelogs/unreleased/550-fix-historical-max-validation diff --git a/app/models/license.rb b/app/models/license.rb index 4df5e6647cbf6..91506780adb48 100644 --- a/app/models/license.rb +++ b/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 @@ def respond_to_missing?(method_name, include_private = false) 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 @@ def valid_license 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 diff --git a/changelogs/unreleased/550-fix-historical-max-validation b/changelogs/unreleased/550-fix-historical-max-validation new file mode 100644 index 0000000000000..15faa547d9a3b --- /dev/null +++ b/changelogs/unreleased/550-fix-historical-max-validation @@ -0,0 +1,4 @@ +--- +title: Read true-up info from license and validate it +merge_request: 1159 +author: diff --git a/lib/gitlab/usage_data.rb b/lib/gitlab/usage_data.rb index a83397a74f623..a2c2366f067ea 100644 --- a/lib/gitlab/usage_data.rb +++ b/lib/gitlab/usage_data.rb @@ -62,7 +62,7 @@ def license_usage_data 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 diff --git a/spec/lib/gitlab/usage_data_spec.rb b/spec/lib/gitlab/usage_data_spec.rb index ab906e463bd64..0375882459eae 100644 --- a/spec/lib/gitlab/usage_data_spec.rb +++ b/spec/lib/gitlab/usage_data_spec.rb @@ -77,7 +77,7 @@ 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) diff --git a/spec/models/license_spec.rb b/spec/models/license_spec.rb index d623be0de3525..48826c3aad98c 100644 --- a/spec/models/license_spec.rb +++ b/spec/models/license_spec.rb @@ -88,6 +88,74 @@ 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 -- GitLab From 093d5534f2670376848fa8586aa227e7b6e07eee Mon Sep 17 00:00:00 2001 From: Ruben Davila <rdavila84@gmail.com> Date: Thu, 9 Feb 2017 17:24:51 -0500 Subject: [PATCH 2/2] Specify if downtime is required sor some migrations. Build was failing due to this. --- .../20151215132013_add_pages_size_to_application_settings.rb | 2 ++ db/migrate/20160210105555_create_pages_domain.rb | 2 ++ ...0519203051_add_developers_can_merge_to_protected_branches.rb | 2 ++ 3 files changed, 6 insertions(+) diff --git a/db/migrate/20151215132013_add_pages_size_to_application_settings.rb b/db/migrate/20151215132013_add_pages_size_to_application_settings.rb index e7fb73190e83f..4854710e3f953 100644 --- a/db/migrate/20151215132013_add_pages_size_to_application_settings.rb +++ b/db/migrate/20151215132013_add_pages_size_to_application_settings.rb @@ -1,4 +1,6 @@ class AddPagesSizeToApplicationSettings < ActiveRecord::Migration + DOWNTIME = false + def up add_column :application_settings, :max_pages_size, :integer, default: 100, null: false end diff --git a/db/migrate/20160210105555_create_pages_domain.rb b/db/migrate/20160210105555_create_pages_domain.rb index 9af206143bd91..0e8507c7e9ad6 100644 --- a/db/migrate/20160210105555_create_pages_domain.rb +++ b/db/migrate/20160210105555_create_pages_domain.rb @@ -1,4 +1,6 @@ class CreatePagesDomain < ActiveRecord::Migration + DOWNTIME = false + def change create_table :pages_domains do |t| t.integer :project_id diff --git a/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb b/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb index 15ad8e8bcbb6b..285e798cfeadd 100644 --- a/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb +++ b/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb @@ -1,6 +1,8 @@ class AddDevelopersCanMergeToProtectedBranches < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers + DOWNTIME = false + disable_ddl_transaction! def change -- GitLab