From c4820da588d0d76b4b6378f821211a00f20048e3 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Mon, 7 Sep 2020 15:01:25 +0800
Subject: [PATCH] Extract attribute refresh logic
Add spec
---
ee/app/models/gitlab_subscription.rb | 7 ++
...sed_for_gitlab_com_subscriptions_worker.rb | 7 +-
ee/spec/models/gitlab_subscription_spec.rb | 32 ++++++
...or_gitlab_com_subscriptions_worker_spec.rb | 97 +++++++++++--------
4 files changed, 95 insertions(+), 48 deletions(-)
diff --git a/ee/app/models/gitlab_subscription.rb b/ee/app/models/gitlab_subscription.rb
index e58699fa9185..03e6cd439052 100644
--- a/ee/app/models/gitlab_subscription.rb
+++ b/ee/app/models/gitlab_subscription.rb
@@ -56,6 +56,13 @@ def calculate_seats_owed
[0, max_seats_used - seats].max
end
+ # Refresh seat related attribute (without persisting them)
+ def refresh_seat_attributes!
+ self.seats_in_use = calculate_seats_in_use
+ self.max_seats_used = [max_seats_used, seats_in_use].max
+ self.seats_owed = calculate_seats_owed
+ end
+
def has_a_paid_hosted_plan?(include_trials: false)
(include_trials || !trial?) &&
hosted? &&
diff --git a/ee/app/workers/update_max_seats_used_for_gitlab_com_subscriptions_worker.rb b/ee/app/workers/update_max_seats_used_for_gitlab_com_subscriptions_worker.rb
index e7f341885fcf..2079b0ef595f 100644
--- a/ee/app/workers/update_max_seats_used_for_gitlab_com_subscriptions_worker.rb
+++ b/ee/app/workers/update_max_seats_used_for_gitlab_com_subscriptions_worker.rb
@@ -16,12 +16,9 @@ def perform
tuples = []
subscriptions.each do |subscription|
- seats_in_use = subscription.calculate_seats_in_use
- max_seats_used = [subscription.max_seats_used, seats_in_use].max
- subscription.max_seats_used = max_seats_used
- seats_owed = subscription.calculate_seats_owed
+ subscription.refresh_seat_attributes!
- tuples << [subscription.id, max_seats_used, seats_in_use, seats_owed]
+ tuples << [subscription.id, subscription.max_seats_used, subscription.seats_in_use, subscription.seats_owed]
end
if tuples.present?
diff --git a/ee/spec/models/gitlab_subscription_spec.rb b/ee/spec/models/gitlab_subscription_spec.rb
index 87258baf947f..4db38cda207d 100644
--- a/ee/spec/models/gitlab_subscription_spec.rb
+++ b/ee/spec/models/gitlab_subscription_spec.rb
@@ -176,6 +176,38 @@
end
end
+ describe '#refresh_seat_attributes!' do
+ subject { create(:gitlab_subscription, seats: 3, max_seats_used: 2) }
+
+ before do
+ expect(subject).to receive(:calculate_seats_in_use).and_return(calculate_seats_in_use)
+ end
+
+ context 'when current seats in use is lower than recorded max_seats_used' do
+ let(:calculate_seats_in_use) { 1 }
+
+ it 'does not increase max_seats_used' do
+ expect do
+ subject.refresh_seat_attributes!
+ end.to change(subject, :seats_in_use).from(0).to(1)
+ .and not_change(subject, :max_seats_used)
+ .and not_change(subject, :seats_owed)
+ end
+ end
+
+ context 'when current seats in use is higher than seats and max_seats_used' do
+ let(:calculate_seats_in_use) { 4 }
+
+ it 'increases seats and max_seats_used' do
+ expect do
+ subject.refresh_seat_attributes!
+ end.to change(subject, :seats_in_use).from(0).to(4)
+ .and change(subject, :max_seats_used).from(2).to(4)
+ .and change(subject, :seats_owed).from(0).to(1)
+ end
+ end
+ end
+
describe '#expired?' do
let(:gitlab_subscription) { create(:gitlab_subscription, end_date: end_date) }
diff --git a/ee/spec/workers/update_max_seats_used_for_gitlab_com_subscriptions_worker_spec.rb b/ee/spec/workers/update_max_seats_used_for_gitlab_com_subscriptions_worker_spec.rb
index 62b38771f1a1..a518335c8289 100644
--- a/ee/spec/workers/update_max_seats_used_for_gitlab_com_subscriptions_worker_spec.rb
+++ b/ee/spec/workers/update_max_seats_used_for_gitlab_com_subscriptions_worker_spec.rb
@@ -5,11 +5,10 @@
RSpec.describe UpdateMaxSeatsUsedForGitlabComSubscriptionsWorker do
subject { described_class.new }
- let_it_be(:user) { create(:user) }
- let_it_be(:group) { create(:group) }
- let_it_be(:bronze_plan) { create(:bronze_plan) }
- let_it_be(:early_adopter_plan) { create(:early_adopter_plan) }
- let_it_be(:gitlab_subscription, refind: true) { create(:gitlab_subscription, namespace: group, seats: 1) }
+ let_it_be(:bronze_plan) { create(:bronze_plan) }
+ let_it_be(:early_adopter_plan) { create(:early_adopter_plan) }
+ let_it_be(:gitlab_subscription, refind: true) { create(:gitlab_subscription, seats: 1) }
+ let_it_be(:gitlab_subscription_2, refind: true) { create(:gitlab_subscription, seats: 11) }
let(:db_is_read_only) { false }
let(:subscription_attrs) { nil }
@@ -18,22 +17,54 @@
allow(Gitlab::Database).to receive(:read_only?) { db_is_read_only }
allow(Gitlab::CurrentSettings).to receive(:should_check_namespace_plan?) { true }
- group.add_developer(user)
+ allow_next_found_instance_of(GitlabSubscription) do |subscription|
+ allow(subscription).to receive(:refresh_seat_attributes!) do
+ subscription.max_seats_used = subscription.seats + 3
+ subscription.seats_in_use = subscription.seats + 2
+ subscription.seats_owed = subscription.seats + 1
+ end
+ end
+ end
+
+ def perform_and_reload
+ subject.perform
+ gitlab_subscription.reload
+ gitlab_subscription_2.reload
+ end
+
+ shared_examples 'updates nothing' do
+ it 'does not update seat columns' do
+ expect do
+ perform_and_reload
+ end.to not_change(gitlab_subscription, :max_seats_used)
+ .and not_change(gitlab_subscription, :seats_in_use)
+ .and not_change(gitlab_subscription, :seats_owed)
+ .and not_change(gitlab_subscription_2, :max_seats_used)
+ .and not_change(gitlab_subscription_2, :seats_in_use)
+ .and not_change(gitlab_subscription_2, :seats_owed)
+ end
end
- shared_examples 'keeps original max_seats_used value' do
- it 'does not update max_seats_used' do
- expect { subject.perform }.not_to change { gitlab_subscription.reload.max_seats_used }
+ shared_examples 'updates only paid plans' do
+ it "persists seat attributes after refresh_seat_attributes! for only paid plans" do
+ expect do
+ perform_and_reload
+ end.to not_change(gitlab_subscription, :max_seats_used)
+ .and not_change(gitlab_subscription, :seats_in_use)
+ .and not_change(gitlab_subscription, :seats_owed)
+ .and change(gitlab_subscription_2, :max_seats_used).from(0).to(14)
+ .and change(gitlab_subscription_2, :seats_in_use).from(0).to(13)
+ .and change(gitlab_subscription_2, :seats_owed).from(0).to(12)
end
end
context 'where the DB is read only' do
let(:db_is_read_only) { true }
- include_examples 'keeps original max_seats_used value'
+ include_examples 'updates nothing'
end
- context 'when the DB PostgreSQK AND is not read only' do
+ context 'when the DB is not read only' do
before do
gitlab_subscription.update!(subscription_attrs) if subscription_attrs
end
@@ -41,56 +72,36 @@
context 'with a free plan' do
let(:subscription_attrs) { { hosted_plan: nil } }
- include_examples 'keeps original max_seats_used value'
+ include_examples 'updates only paid plans'
end
context 'with a trial plan' do
let(:subscription_attrs) { { hosted_plan: bronze_plan, trial: true } }
- include_examples 'keeps original max_seats_used value'
+ include_examples 'updates only paid plans'
end
context 'with an early adopter plan' do
let(:subscription_attrs) { { hosted_plan: early_adopter_plan } }
- include_examples 'keeps original max_seats_used value'
+ include_examples 'updates only paid plans'
end
context 'with a paid plan', :aggregate_failures do
- let_it_be(:other_user) { create(:user) }
- let_it_be(:other_group) { create(:group) }
- let_it_be(:other_gitlab_subscription, refind: true) { create(:gitlab_subscription, namespace: other_group, seats: 1) }
-
before do
- group.add_developer(other_user)
- other_group.add_developer(other_user)
-
gitlab_subscription.update!(hosted_plan: bronze_plan)
- other_gitlab_subscription.update!(hosted_plan: bronze_plan)
- end
-
- it 'only updates max_seats_used if active users count is greater than it' do
- expect do
- subject.perform
- gitlab_subscription.reload
- other_gitlab_subscription.reload
- end.to change(gitlab_subscription, :max_seats_used).from(0).to(2)
- .and change(gitlab_subscription, :seats_in_use).from(0).to(2)
- .and change(gitlab_subscription, :seats_owed).from(0).to(1)
- .and change(other_gitlab_subscription, :max_seats_used).from(0).to(1)
- .and change(other_gitlab_subscription, :seats_in_use).from(0).to(1)
- .and not_change(other_gitlab_subscription, :seats_owed).from(0)
+ gitlab_subscription_2.update!(hosted_plan: bronze_plan)
end
- it 'does not update max_seats_used if active users count is lower than it' do
- gitlab_subscription.update_column(:max_seats_used, 5)
-
+ it 'persists seat attributes after refresh_seat_attributes' do
expect do
- subject.perform
- gitlab_subscription.reload
- end.to change(gitlab_subscription, :seats_in_use).from(0).to(2)
- .and change(gitlab_subscription, :seats_owed).from(0).to(4)
- .and not_change(gitlab_subscription, :max_seats_used).from(5)
+ perform_and_reload
+ end.to change(gitlab_subscription, :max_seats_used).from(0).to(4)
+ .and change(gitlab_subscription, :seats_in_use).from(0).to(3)
+ .and change(gitlab_subscription, :seats_owed).from(0).to(2)
+ .and change(gitlab_subscription_2, :max_seats_used).from(0).to(14)
+ .and change(gitlab_subscription_2, :seats_in_use).from(0).to(13)
+ .and change(gitlab_subscription_2, :seats_owed).from(0).to(12)
end
end
end
--
GitLab