Fixes logic for when we reset subscription seats

Resets the subscription seats if the last time it was updated was before
the current term started

Changelog: fixed
EE: true

ee/app/models/gitlab_subscription.rb

@@ -13,7 +13,7 @@ class GitlabSubscription < ApplicationRecord
 
   before_update :set_max_seats_used_changed_at
   before_update :log_previous_state_for_update
-  before_update :reset_seats_for_new_term
+  before_update :reset_seat_statistics
   before_update :publish_subscription_renewed_event
 
   after_commit :index_namespace, on: [:create, :update]
@@ -88,9 +88,9 @@ def seats_remaining
   end
 
   # Refresh seat related attribute (without persisting them)
-  def refresh_seat_attributes
+  def refresh_seat_attributes(reset_max: false)
     self.seats_in_use = calculate_seats_in_use
-    self.max_seats_used = [max_seats_used, seats_in_use].max
+    self.max_seats_used = reset_max ? seats_in_use : [max_seats_used, seats_in_use].max
     self.seats_owed = calculate_seats_owed
   end
 
@@ -167,14 +167,13 @@ def index_namespace
     ElasticsearchIndexedNamespace.safe_find_or_create_by!(namespace_id: namespace_id)
   end
 
-  # If the subscription starts a new term, the dates will change. We reset max_seats_used
-  # and seats_owed so that we don't carry it over from the previous term
-  def reset_seats_for_new_term
-    return unless new_term?
+  # If the subscription changes, we reset max_seats_used and seats_owed
+  # if they're out of date, so that we don't carry them over from the previous term/subscription.
+  def reset_seat_statistics
+    return unless reset_seat_statistics?
 
-    self.max_seats_used = attributes['seats_in_use']
-    self.seats_owed = calculate_seats_owed
-    self.max_seats_used_changed_at = nil
+    refresh_seat_attributes(reset_max: true)
+    self.max_seats_used_changed_at = Time.current
   end
 
   def publish_subscription_renewed_event
@@ -195,4 +194,8 @@ def new_term?
     persisted? && start_date_changed? && end_date_changed? &&
       (end_date_was.nil? || start_date >= end_date_was)
   end
+
+  def reset_seat_statistics?
+    new_term? || (max_seats_used_changed_at.present? && max_seats_used_changed_at.to_date < start_date)
+  end
 end

ee/spec/models/gitlab_subscription_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe GitlabSubscription, :saas do
+RSpec.describe GitlabSubscription, :saas, feature_category: :subscription_management do
   using RSpec::Parameterized::TableSyntax
 
   %i[free_plan bronze_plan premium_plan ultimate_plan].each do |plan|
@@ -199,10 +199,8 @@
   end
 
   describe '#calculate_seats_owed' do
-    let!(:gitlab_subscription) { create(:gitlab_subscription, subscription_attrs) }
-
-    before do
-      gitlab_subscription.update!(seats: 5, max_seats_used: 10)
+    let!(:gitlab_subscription) do
+      create(:gitlab_subscription, subscription_attrs.merge(max_seats_used: 10, seats: 5))
     end
 
     shared_examples 'always returns a total of 0' do
@@ -267,10 +265,10 @@
   end
 
   describe '#refresh_seat_attributes' do
-    subject(:gitlab_subscription) { create(:gitlab_subscription, seats: 3, max_seats_used: 2) }
+    subject(:gitlab_subscription) { create(:gitlab_subscription, seats: 3, max_seats_used: 2, seats_owed: 0) }
 
     before do
-      expect(subject).to receive(:calculate_seats_in_use).and_return(calculate_seats_in_use)
+      expect(gitlab_subscription).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
@@ -296,6 +294,18 @@
           .and change(gitlab_subscription, :seats_owed).from(0).to(1)
       end
     end
+
+    context 'when resetting the max seats' do
+      let(:calculate_seats_in_use) { 1 }
+
+      it 'sets max_seats_used to the current seats in use' do
+        expect do
+          gitlab_subscription.refresh_seat_attributes(reset_max: 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
 
   describe '#seats_in_use' do
@@ -609,21 +619,24 @@
         end
       end
 
-      context 'when start and end dates change' do
+      shared_examples 'resets seats' do
+        it 'resets seats attributes' do
+          expect do
+            gitlab_subscription.update!(start_date: new_start, end_date: new_end)
+          end.to change(gitlab_subscription, :max_seats_used).from(42).to(1)
+            .and change(gitlab_subscription, :seats_owed).from(29).to(0)
+            .and change(gitlab_subscription, :seats_in_use).from(20).to(1)
+
+          expect(gitlab_subscription.max_seats_used_changed_at).to be_like_time(Time.current)
+        end
+      end
+
+      context 'when starting a new term' do
         context 'when start_date is after the old end_date' do
           let(:new_start) { gitlab_subscription.end_date + 1.year }
           let(:new_end) { new_start + 1.year }
 
-          it 'resets seats attributes' do
-            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)
-              .and change(gitlab_subscription, :max_seats_used_changed_at).to(nil)
-              .and not_change(gitlab_subscription, :seats_in_use)
-          end
+          it_behaves_like 'resets seats'
 
           it 'triggers subscription started event' do
             expect { gitlab_subscription.update!(start_date: new_start, end_date: new_end) }
@@ -633,44 +646,31 @@
         end
 
         context 'when the end_date was nil' do
-          it 'resets seats attributes' do
-            gitlab_subscription.update!(end_date: nil)
+          let(:new_start) { Date.today + 1.year }
+          let(:new_end) { new_start + 1.year }
 
-            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, :max_seats_used_changed_at).to(nil)
-              .and change(gitlab_subscription, :seats_owed).from(29).to(7)
-              .and not_change(gitlab_subscription, :seats_in_use)
+          before do
+            gitlab_subscription.update!(end_date: nil)
           end
+
+          it_behaves_like 'resets seats'
         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, :max_seats_used_changed_at)
-              .and not_change(gitlab_subscription, :seats_owed)
-          end
+        context 'when the start_date is before the old end_date' do
+          let(:new_start) { gitlab_subscription.end_date - 1.month }
+          let(:new_end) { new_start + 1.year }
+
+          it_behaves_like 'resets seats'
         end
       end
 
-      context 'when only one date is changed' do
+      context 'when dates are changed but not for a new term' do
         it 'does not reset seats attributes' do
+          new_start_date = (gitlab_subscription.max_seats_used_changed_at - 1.day).to_date
+
           expect do
-            gitlab_subscription.update!(start_date: Date.today)
-          end.to change(gitlab_subscription, :start_date).to(Date.today)
+            gitlab_subscription.update!(start_date: new_start_date)
+          end.to change(gitlab_subscription, :start_date).to(new_start_date)
             .and not_change(gitlab_subscription, :max_seats_used)
             .and not_change(gitlab_subscription, :max_seats_used_changed_at)
             .and not_change(gitlab_subscription, :seats_owed)
@@ -692,6 +692,46 @@
             .and not_change(gitlab_subscription, :seats_owed)
         end
       end
+
+      context 'when max_seats_used_changed_at is not set' do
+        it 'does not reset seats attributes' do
+          gitlab_subscription.update!(max_seats_used_changed_at: nil)
+
+          expect do
+            gitlab_subscription.update!(seats_in_use: 99)
+          end.to not_change(gitlab_subscription, :max_seats_used)
+            .and not_change(gitlab_subscription, :max_seats_used_changed_at)
+            .and not_change(gitlab_subscription, :seats_owed)
+        end
+      end
+
+      context 'when max_seats_used_changed_at is on the start_date' do
+        let(:new_start) { Date.today }
+        let(:new_end) { new_start + 1.year }
+
+        before do
+          gitlab_subscription.update!(max_seats_used_changed_at: Time.current)
+        end
+
+        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, :max_seats_used_changed_at)
+            .and not_change(gitlab_subscription, :seats_owed)
+        end
+      end
+
+      context 'when max_seats_used_changed_at is before the start_date' do
+        let(:new_start) { Date.today }
+        let(:new_end) { new_start + 1.year }
+
+        before do
+          gitlab_subscription.update!(max_seats_used_changed_at: Time.current - 1.day)
+        end
+
+        it_behaves_like 'resets seats'
+      end
     end
 
     context 'after_destroy_commit' do

ee/spec/workers/update_max_seats_used_for_gitlab_com_subscriptions_worker_spec.rb

@@ -17,7 +17,8 @@
         namespace: create(:namespace, :with_namespace_settings),
         seats: 11,
         max_seats_used: 11,
-        max_seats_used_changed_at: 1.month.ago.to_s(:db)
+        max_seats_used_changed_at: 1.month.ago.to_s(:db),
+        start_date: 2.months.ago.to_s(:db)
       )
     end