Merge branch 'add-not-null-constraint-to-gitlab-subscriptions-namespace-id' into 'master'

Add NOT NULL constraint to gitlab_subscriptions namespace_id

See merge request gitlab-org/gitlab!54319

changelogs/unreleased/add-not-null-constraint-to-gitlab-subscriptions-namespace-id.yml

+---
+title: Add NOT NULL constraint to gitlab_subscriptions namespace_id
+merge_request: 54319
+author:
+type: other

db/post_migrate/20210303064112_add_not_null_constraints_to_gitlab_subscriptions_namespace_id.rb

+# frozen_string_literal: true
+
+class AddNotNullConstraintsToGitlabSubscriptionsNamespaceId < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    # This will add the `NOT NULL` constraint WITHOUT validating it
+    add_not_null_constraint :gitlab_subscriptions, :namespace_id, validate: false
+  end
+
+  def down
+    # Down is required as `add_not_null_constraint` is not reversible
+    remove_not_null_constraint :gitlab_subscriptions, :namespace_id
+  end
+end

db/post_migrate/20210303064142_cleanup_gitlab_subscriptions_with_null_namespace_id.rb

+# frozen_string_literal: true
+
+class CleanupGitlabSubscriptionsWithNullNamespaceId < ActiveRecord::Migration[6.0]
+  disable_ddl_transaction!
+
+  class GitlabSubscription < ActiveRecord::Base
+    self.table_name = 'gitlab_subscriptions'
+  end
+
+  def up
+    # As of today, there is 0 records whose namespace_id is null on GitLab.com.
+    # And we expect no such records on non GitLab.com instance.
+    # So this post-migration cleanup script is just for extra safe.
+    #
+    # This will be fast on GitLab.com, because:
+    #   - gitlab_subscriptions.count=5021850
+    #   - namespace_id is indexed, so the query is pretty fast. Try on database-lab, this uses 5.931 ms
+    GitlabSubscription.where('namespace_id IS NULL').delete_all
+  end
+
+  def down
+    # no-op : can't go back to `NULL` without first dropping the `NOT NULL` constraint
+  end
+end

db/schema_migrations/20210303064112

+5b8f32bafe4bffd30b9235f9b6ba5774a26d5c4c9f1e987d3e840056f8abdd52
\ No newline at end of file

db/schema_migrations/20210303064142

+70c5b76788460bd098c3ae3f75c7b18194c7765e1462f5305feaf2400f7dd4ff
\ No newline at end of file

db/structure.sql

@@ -19955,6 +19955,9 @@ ALTER TABLE ONLY chat_teams
 ALTER TABLE vulnerability_scanners
     ADD CONSTRAINT check_37608c9db5 CHECK ((char_length(vendor) <= 255)) NOT VALID;
 
+ALTER TABLE gitlab_subscriptions
+    ADD CONSTRAINT check_77fea3f0e7 CHECK ((namespace_id IS NOT NULL)) NOT VALID;
+
 ALTER TABLE sprints
     ADD CONSTRAINT check_ccd8a1eae0 CHECK ((start_date IS NOT NULL)) NOT VALID;
 

ee/app/models/gitlab_subscription.rb

@@ -15,7 +15,7 @@ class GitlabSubscription < ApplicationRecord
   belongs_to :hosted_plan, class_name: 'Plan'
 
   validates :seats, :start_date, presence: true
-  validates :namespace_id, uniqueness: true, allow_blank: true
+  validates :namespace_id, uniqueness: true, presence: true
 
   delegate :name, :title, to: :hosted_plan, prefix: :plan, allow_nil: true
 
@@ -74,7 +74,6 @@ class GitlabSubscription < ApplicationRecord
 
   def has_a_paid_hosted_plan?(include_trials: false)
     (include_trials || !trial?) &&
-      hosted? &&
       seats > 0 &&
       Plan::PAID_HOSTED_PLANS.include?(plan_name)
   end
@@ -103,7 +102,7 @@ class GitlabSubscription < ApplicationRecord
   # in order to make it easy for customers to get this information.
   def seats_in_use
     return super unless Feature.enabled?(:seats_in_use_for_free_or_trial)
-    return super if has_a_paid_hosted_plan? || !hosted?
+    return super if has_a_paid_hosted_plan?
 
     seats_in_use_now
   end
@@ -153,10 +152,6 @@ class GitlabSubscription < ApplicationRecord
     GitlabSubscriptionHistory.create(attrs.except(*omitted_attrs))
   end
 
-  def hosted?
-    namespace_id.present?
-  end
-
   def automatically_index_in_elasticsearch?
     return false unless ::Gitlab.dev_env_or_com?
     return false if expired?

ee/app/workers/update_max_seats_used_for_gitlab_com_subscriptions_worker.rb

@@ -16,6 +16,9 @@ class UpdateMaxSeatsUsedForGitlabComSubscriptionsWorker # rubocop:disable Scalab
       tuples = []
 
       subscriptions.each do |subscription|
+        # This should be removed after https://gitlab.com/gitlab-org/gitlab/-/issues/243496
+        # Because there should not be any NULL namespace_id gitlab_subscription after the merge
+        # request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40537
         unless subscription.namespace
           track_error(subscription)
           next

ee/spec/helpers/billing_plans_helper_spec.rb

@@ -99,10 +99,7 @@ RSpec.describe BillingPlansHelper do
 
     with_them do
       let_it_be(:user) { create(:user) }
-      let(:namespace) do
-        create :namespace, type: type,
-          gitlab_subscription: create(:gitlab_subscription, hosted_plan: create("#{plan}_plan".to_sym))
-      end
+      let(:namespace) { create(:namespace_with_plan, plan: "#{plan}_plan".to_sym, type: type) }
 
       before do
         allow(helper).to receive(:current_user).and_return(user)
@@ -119,13 +116,7 @@ RSpec.describe BillingPlansHelper do
     end
 
     context 'when the group is on a plan eligible for the new purchase flow' do
-      let(:namespace) do
-        create(
-          :namespace,
-          type: Group,
-          gitlab_subscription: create(:gitlab_subscription, hosted_plan: create(:free_plan))
-        )
-      end
+      let(:namespace) { create(:namespace_with_plan, plan: :free_plan, type: Group) }
 
       before do
         allow(helper).to receive(:current_user).and_return(user)

ee/spec/lib/ee/gitlab/application_context_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe Gitlab::ApplicationContext do
   describe '#to_lazy_hash' do
     let(:user) { build(:user) }
-    let(:project) { build(:project) }
+    let(:project) { create(:project) }
     let(:namespace) { create(:group) }
     let(:subgroup) { create(:group, parent: namespace) }
 

ee/spec/models/ee/user_spec.rb

@@ -1003,11 +1003,11 @@ RSpec.describe User do
 
   describe '#manageable_groups_eligible_for_subscription' do
     let_it_be(:user) { create(:user) }
-    let_it_be(:licensed_group) { create(:group, gitlab_subscription: create(:gitlab_subscription, :bronze)) }
-    let_it_be(:free_group_z) { create(:group, name: 'AZ', gitlab_subscription: create(:gitlab_subscription, :free)) }
-    let_it_be(:free_group_a) { create(:group, name: 'AA', gitlab_subscription: create(:gitlab_subscription, :free)) }
+    let_it_be(:licensed_group) { create(:group_with_plan, plan: :bronze_plan) }
+    let_it_be(:free_group_z) { create(:group_with_plan, plan: :free_plan, name: 'AZ') }
+    let_it_be(:free_group_a) { create(:group_with_plan, plan: :free_plan, name: 'AA') }
     let_it_be(:sub_group) { create(:group, name: 'SubGroup', parent: free_group_a) }
-    let_it_be(:trial_group) { create(:group, name: 'AB', gitlab_subscription: create(:gitlab_subscription, :active_trial, :ultimate)) }
+    let_it_be(:trial_group) { create(:group_with_plan, plan: :ultimate_plan, trial_ends_on: Date.current + 1.day, name: 'AB') }
 
     subject { user.manageable_groups_eligible_for_subscription }
 
@@ -1085,10 +1085,10 @@ RSpec.describe User do
 
   describe '#manageable_groups_eligible_for_trial' do
     let_it_be(:user) { create :user }
-    let_it_be(:non_trialed_group_z) { create :group, name: 'Zeta', gitlab_subscription: create(:gitlab_subscription, :free) }
-    let_it_be(:non_trialed_group_a) { create :group, name: 'Alpha', gitlab_subscription: create(:gitlab_subscription, :free) }
-    let_it_be(:trialed_group) { create :group, name: 'Omitted', gitlab_subscription: create(:gitlab_subscription, :free, trial: true) }
-    let_it_be(:non_trialed_subgroup) { create :group, name: 'Sub-group', gitlab_subscription: create(:gitlab_subscription, :free), parent: non_trialed_group_a }
+    let_it_be(:non_trialed_group_z) { create :group_with_plan, name: 'Zeta', plan: :free_plan }
+    let_it_be(:non_trialed_group_a) { create :group_with_plan, name: 'Alpha', plan: :free_plan }
+    let_it_be(:trialed_group) { create :group_with_plan, name: 'Omitted', plan: :free_plan, trial_ends_on: Date.today + 1.day }
+    let_it_be(:non_trialed_subgroup) { create :group_with_plan, name: 'Sub-group', plan: :free_plan, parent: non_trialed_group_a }
 
     subject { user.manageable_groups_eligible_for_trial }
 

ee/spec/models/gitlab_subscription_spec.rb

@@ -257,16 +257,6 @@ RSpec.describe GitlabSubscription do
 
       it_behaves_like 'a disabled feature'
     end
-
-    context 'with a self hosted plan' do
-      before do
-        gitlab_subscription.update!(namespace: nil)
-      end
-
-      it 'returns the previously calculated seats in use' do
-        expect(subject).to eq(5)
-      end
-    end
   end
 
   describe '#expired?' do
@@ -298,17 +288,15 @@ RSpec.describe GitlabSubscription do
 
     let(:subscription) { build(:gitlab_subscription) }
 
-    where(:plan_name, :seats, :hosted, :result) do
-      'bronze'        | 0 | true  | false
-      'bronze'        | 1 | true  | true
-      'bronze'        | 1 | false | false
-      'premium'       | 1 | true  | true
+    where(:plan_name, :seats, :result) do
+      'bronze'        | 0 | false
+      'bronze'        | 1 | true
+      'premium'       | 1 | true
     end
 
     with_them do
       before do
         plan = build(:plan, name: plan_name)
-        allow(subscription).to receive(:hosted?).and_return(hosted)
         subscription.assign_attributes(hosted_plan: plan, seats: seats)
       end
 
@@ -394,18 +382,6 @@ RSpec.describe GitlabSubscription do
         end
       end
 
-      context 'when it is not a hosted plan' do
-        before do
-          gitlab_subscription.namespace_id = nil
-        end
-
-        it 'does not index anything' do
-          expect(ElasticsearchIndexedNamespace).not_to receive(:safe_find_or_create_by!)
-
-          gitlab_subscription.save!
-        end
-      end
-
       context 'when it is a free plan' do
         let(:plan) { :free }
 
@@ -429,12 +405,13 @@ RSpec.describe GitlabSubscription do
 
     context 'before_update' do
       it 'logs previous state to gitlab subscription history' do
-        subject.update! max_seats_used: 42, seats: 13
-        subject.update! max_seats_used: 32
+        gitlab_subscription = create(:gitlab_subscription, max_seats_used: 42, seats: 13, namespace: create(:namespace))
+
+        gitlab_subscription.update! max_seats_used: 32
 
         expect(GitlabSubscriptionHistory.count).to eq(1)
         expect(GitlabSubscriptionHistory.last.attributes).to include(
-          'gitlab_subscription_id' => subject.id,
+          'gitlab_subscription_id' => gitlab_subscription.id,
           'change_type' => 'gitlab_subscription_updated',
           'max_seats_used' => 42,
           'seats' => 13

ee/spec/workers/update_max_seats_used_for_gitlab_com_subscriptions_worker_spec.rb

@@ -96,28 +96,6 @@ RSpec.describe UpdateMaxSeatsUsedForGitlabComSubscriptionsWorker do
           .and change(gitlab_subscription_2, :seats_in_use).from(0).to(13)
           .and change(gitlab_subscription_2, :seats_owed).from(0).to(12)
       end
-
-      context 'when namespace is absent' do
-        before do
-          gitlab_subscription.update!(namespace_id: nil)
-        end
-
-        it 'skips the subscription' do
-          expect(Gitlab::ErrorTracking).to receive(:track_exception).with(
-            an_instance_of(StandardError),
-            { gitlab_subscription_id: gitlab_subscription.id, namespace_id: nil }
-          )
-
-          expect do
-            perform_and_reload
-          end.to not_change(gitlab_subscription, :max_seats_used).from(0)
-            .and not_change(gitlab_subscription, :seats_in_use).from(0)
-            .and not_change(gitlab_subscription, :seats_owed).from(0)
-            .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
   end