Reset subscription max seats used on new term

When starting a new term on a subscription, the max seats used
should reset to the current seats in use rather than carry over from the
previous term

Changelog: fixed
EE: true

ee/app/models/gitlab_subscription.rb

@@ -9,7 +9,10 @@ class GitlabSubscription < ApplicationRecord
   enum trial_extension_type: { extended: 1, reactivated: 2 }
 
   default_value_for(:start_date) { Date.today }
+
   before_update :log_previous_state_for_update
+  before_update :reset_seats_for_new_term
+
   after_commit :index_namespace, on: [:create, :update]
   after_destroy_commit :log_previous_state_for_destroy
 
@@ -68,9 +71,9 @@ class GitlabSubscription < ApplicationRecord
   end
 
   # Refresh seat related attribute (without persisting them)
-  def refresh_seat_attributes!
+  def refresh_seat_attributes!(new_term_reset: false)
     self.seats_in_use = calculate_seats_in_use
-    self.max_seats_used = [max_seats_used, seats_in_use].max
+    self.max_seats_used = new_term_reset ? seats_in_use : [max_seats_used, seats_in_use].max
     self.seats_owed = calculate_seats_owed
   end
 
@@ -175,4 +178,17 @@ class GitlabSubscription < ApplicationRecord
 
     ElasticsearchIndexedNamespace.safe_find_or_create_by!(namespace_id: namespace_id)
   end
+
+  # If the subscription starts a new term, the dates will change. We reset seat counts
+  # so that we don't carry over max_seats_used/owed from the previous term
+  def reset_seats_for_new_term
+    return unless new_term?
+
+    refresh_seat_attributes!(new_term_reset: true)
+  end
+
+  def new_term?
+    persisted? && start_date_changed? && end_date_changed? &&
+      (end_date_was.nil? || start_date >= end_date_was)
+  end
 end

ee/spec/models/gitlab_subscription_spec.rb

@@ -163,7 +163,7 @@ RSpec.describe GitlabSubscription, :saas do
   end
 
   describe '#refresh_seat_attributes!' do
-    subject { create(:gitlab_subscription, seats: 3, max_seats_used: 2) }
+    subject(:gitlab_subscription) { 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)
@@ -174,10 +174,20 @@ RSpec.describe GitlabSubscription, :saas do
 
       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)
+          gitlab_subscription.refresh_seat_attributes!
+        end.to change(gitlab_subscription, :seats_in_use).from(0).to(1)
+          .and not_change(gitlab_subscription, :max_seats_used)
+          .and not_change(gitlab_subscription, :seats_owed)
+      end
+
+      context 'when resetting for a new term' do
+        it 'sets max_seats_used to seats_in_use' do
+          expect do
+            gitlab_subscription.refresh_seat_attributes!(new_term_reset: true)
+          end.to change(gitlab_subscription, :seats_in_use).from(0).to(1)
+            .and change(gitlab_subscription, :max_seats_used).from(2).to(1)
+            .and not_change(gitlab_subscription, :seats_owed)
+        end
       end
     end
 
@@ -186,10 +196,20 @@ RSpec.describe GitlabSubscription, :saas do
 
       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)
+          gitlab_subscription.refresh_seat_attributes!
+        end.to change(gitlab_subscription, :seats_in_use).from(0).to(4)
+          .and change(gitlab_subscription, :max_seats_used).from(2).to(4)
+          .and change(gitlab_subscription, :seats_owed).from(0).to(1)
+      end
+
+      context 'when resetting for a new term' do
+        it 'sets max_seats_used to seats_in_use' do
+          expect do
+            gitlab_subscription.refresh_seat_attributes!(new_term_reset: true)
+          end.to change(gitlab_subscription, :seats_in_use).from(0).to(4)
+            .and change(gitlab_subscription, :max_seats_used).from(2).to(4)
+            .and change(gitlab_subscription, :seats_owed).from(0).to(1)
+        end
       end
     end
   end
@@ -432,11 +452,13 @@ RSpec.describe GitlabSubscription, :saas do
       expect(GitlabSubscriptionHistory.attribute_names - described_class.attribute_names).to eq(diff_attrs)
     end
 
-    context 'before_update' do
-      it 'logs previous state to gitlab subscription history' do
-        gitlab_subscription = create(:gitlab_subscription, max_seats_used: 42, seats: 13, namespace: create(:namespace))
+    context 'before_update', :freeze_time do
+      let(:gitlab_subscription) do
+        create(:gitlab_subscription, max_seats_used: 42, seats: 13, seats_owed: 29, start_date: Date.today - 1.year)
+      end
 
-        gitlab_subscription.update! max_seats_used: 32
+      it 'logs previous state to gitlab subscription history' do
+        gitlab_subscription.update!(max_seats_used: 32)
 
         expect(GitlabSubscriptionHistory.count).to eq(1)
         expect(GitlabSubscriptionHistory.last.attributes).to include(
@@ -446,6 +468,75 @@ RSpec.describe GitlabSubscription, :saas do
           'seats' => 13
         )
       end
+
+      context 'when start and end dates change' do
+        before do
+          allow(gitlab_subscription).to receive(:seats_in_use).and_return(20)
+        end
+
+        context 'when start_date is after the old end_date' do
+          it 'resets seats attributes' do
+            new_start = gitlab_subscription.end_date + 1.year
+            new_end = new_start + 1.year
+
+            expect do
+              gitlab_subscription.update!(start_date: new_start, end_date: new_end)
+            end.to change(gitlab_subscription, :start_date).to(new_start)
+              .and change(gitlab_subscription, :end_date).to(new_end)
+              .and change(gitlab_subscription, :max_seats_used).from(42).to(20)
+              .and change(gitlab_subscription, :seats_owed).from(29).to(7)
+          end
+        end
+
+        context 'when the end_date was nil' do
+          it 'resets seats attributes' do
+            gitlab_subscription.update!(end_date: nil)
+
+            new_start = Date.today + 1.year
+            new_end = new_start + 1.year
+
+            expect do
+              gitlab_subscription.update!(start_date: new_start, end_date: new_end)
+            end.to change(gitlab_subscription, :start_date).to(new_start)
+              .and change(gitlab_subscription, :end_date).to(new_end)
+              .and change(gitlab_subscription, :max_seats_used).from(42).to(20)
+              .and change(gitlab_subscription, :seats_owed).from(29).to(7)
+          end
+        end
+
+        context 'when start_date is not after the old end_date' do
+          it 'does not reset seats attributes' do
+            new_start = gitlab_subscription.end_date - 1.month
+            new_end = new_start + 1.year
+
+            expect do
+              gitlab_subscription.update!(start_date: new_start, end_date: new_end)
+            end.to change(gitlab_subscription, :start_date).to(new_start)
+              .and change(gitlab_subscription, :end_date).to(new_end)
+              .and not_change(gitlab_subscription, :max_seats_used)
+              .and not_change(gitlab_subscription, :seats_owed)
+          end
+        end
+      end
+
+      context 'when only one date is changed' do
+        it 'does not reset seats attributes' do
+          expect do
+            gitlab_subscription.update!(start_date: Date.today)
+          end.to change(gitlab_subscription, :start_date).to(Date.today)
+            .and not_change(gitlab_subscription, :max_seats_used)
+            .and not_change(gitlab_subscription, :seats_owed)
+        end
+      end
+
+      context 'when no dates are changed' do
+        it 'does not reset seats attributes' do
+          expect do
+            gitlab_subscription.update!(seats_in_use: 99)
+          end.to not_change(gitlab_subscription, :max_seats_used)
+            .and not_change(gitlab_subscription, :seats_owed)
+        end
+      end
     end
 
     context 'after_destroy_commit' do

ee/spec/requests/api/namespaces_spec.rb

@@ -590,14 +590,14 @@ RSpec.describe API::Namespaces do
 
     let_it_be(:premium_plan) { create(:premium_plan) }
     let_it_be(:namespace) { create(:group, name: 'test.test-group.22') }
-    let_it_be(:gitlab_subscription) { create(:gitlab_subscription, namespace: namespace) }
+    let_it_be(:gitlab_subscription) { create(:gitlab_subscription, namespace: namespace, start_date: '2018/01/01', end_date: '2019/01/01') }
 
     let(:params) do
       {
         seats: 150,
         plan_code: 'premium',
-        start_date: '01/01/2018',
-        end_date: '01/01/2019'
+        start_date: '2018/01/01',
+        end_date: '2019/01/01'
       }
     end
 
@@ -679,6 +679,26 @@ RSpec.describe API::Namespaces do
             do_put(namespace.id, admin, gitlab_subscription.attributes)
           end.to change { gitlab_subscription.reload.updated_at }
         end
+
+        context 'when starting a new term' do
+          it 'resets the seat attributes for the subscription' do
+            gitlab_subscription.update!(seats: 20, max_seats_used: 42, seats_owed: 22)
+
+            new_start = gitlab_subscription.end_date + 1.year
+            new_end = gitlab_subscription.end_date + 1.year
+            new_term_params = params.merge(start_date: new_start, end_date: new_end)
+
+            expect(gitlab_subscription.seats_in_use).to eq 0
+
+            do_put(namespace.id, admin, new_term_params)
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(gitlab_subscription.reload).to have_attributes(
+              max_seats_used: 0,
+              seats_owed: 0
+            )
+          end
+        end
       end
     end