Merge branch '2392-activate-gitlab-com-pricing-plans' into 'master'

Add migrations to enhance the association between Namespaces and GL.com plans

Closes #2391

See merge request !2609

app/controllers/admin/groups_controller.rb

@@ -89,7 +89,7 @@ class Admin::GroupsController < Admin::ApplicationController
     [
       :repository_size_limit,
       :shared_runners_minutes_limit,
-      :plan
+      :plan_id
     ]
   end
 end

app/controllers/admin/users_controller.rb

@@ -213,7 +213,7 @@ class Admin::UsersController < Admin::ApplicationController
   def user_params_ee
     [
       :note,
-      namespace_attributes: [:id, :shared_runners_minutes_limit, :plan]
+      namespace_attributes: [:id, :shared_runners_minutes_limit, :plan_id]
     ]
   end
 

app/models/plan.rb

+class Plan < ActiveRecord::Base
+  has_many :namespaces
+end

app/views/admin/_namespace_plan.html.haml

 .form-group
   = f.label :plan, class: 'control-label'
   .col-sm-10
-    = f.select :plan, options_for_select(Namespace::EE_PLANS.keys.map { |plan| [plan.titleize, plan] }, f.object.plan),
+    = f.select :plan_id, Plan.pluck(:title, :id),
       { include_blank: 'No plan' },
       class: 'form-control'

app/views/admin/_namespace_plan_badge.html.haml

 - namespace = local_assigns[:namespace]
 - if current_application_settings.should_check_namespace_plan? && namespace && namespace.plan.present?
-  %span.plan-badge.has-tooltip{ data: { plan: namespace.plan }, title: "#{namespace.plan.titleize} Plan" }
+  %span.plan-badge.has-tooltip{ data: { plan: namespace.plan.name }, title: "#{namespace.plan.title} Plan" }
     = custom_icon('icon_premium')

app/views/admin/_namespace_plan_info.html.haml

@@ -3,9 +3,9 @@
   %li
     %span.light Plan:
     - if namespace.plan.present?
-      %strong.plan-badge.inline{ data: { plan: namespace.plan } }
+      %strong.plan-badge.inline{ data: { plan: namespace.plan.name } }
         = custom_icon('icon_premium')
-        = namespace.plan.titleize
+        = namespace.plan.title
     - else
       %strong.plan-badge.inline
         = custom_icon('icon_premium')

db/fixtures/test/01_plan.rb

+require './spec/support/sidekiq'
+
+EE::Namespace::EE_PLANS.each_key do |plan|
+  Plan.create!(name: plan, title: plan.titleize)
+end

db/migrate/20170808155717_create_plans.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class CreatePlans < ActiveRecord::Migration
+  DOWNTIME = false
+
+  class Plan < ActiveRecord::Base
+    self.table_name = 'plans'
+  end
+
+  def up
+    create_table :plans do |t|
+      t.timestamps_with_timezone null: false
+      t.string :name, index: true
+      t.string :title
+    end
+
+    %w[early_adopter bronze silver gold].each do |plan|
+      Plan.create!(name: plan, title: plan.titleize)
+    end
+  end
+
+  def down
+    drop_table :plans
+  end
+end

db/migrate/20170808155912_add_plan_id_to_namespaces.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddPlanIdToNamespaces < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class Namespace < ActiveRecord::Base
+    self.table_name = 'namespaces'
+  end
+
+  class Plan < ActiveRecord::Base
+    self.table_name = 'plans'
+  end
+
+  def up
+    add_reference :namespaces, :plan
+    add_concurrent_foreign_key :namespaces, :plans, column: :plan_id, on_delete: :nullify
+    add_concurrent_index :namespaces, :plan_id
+
+    Plan.all.each do |plan|
+      Namespace.where(plan: plan.name).update_all(plan_id: plan.id)
+    end
+  end
+
+  def down
+    remove_foreign_key :namespaces, column: :plan_id
+    remove_concurrent_index :namespaces, :plan_id
+    remove_reference :namespaces, :plan
+  end
+end

db/post_migrate/20170808163512_remove_plan_from_namespaces.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemovePlanFromNamespaces < ActiveRecord::Migration
+  DOWNTIME = false
+
+  def change
+    remove_column :namespaces, :plan, :string
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170807160457) do
+ActiveRecord::Schema.define(version: 20170808163512) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1164,7 +1164,7 @@ ActiveRecord::Schema.define(version: 20170807160457) do
     t.boolean "require_two_factor_authentication", default: false, null: false
     t.integer "two_factor_grace_period", default: 48, null: false
     t.integer "cached_markdown_version"
-    t.string "plan"
+    t.integer "plan_id"
   end
 
   add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree
@@ -1177,6 +1177,7 @@ ActiveRecord::Schema.define(version: 20170807160457) do
   add_index "namespaces", ["parent_id", "id"], name: "index_namespaces_on_parent_id_and_id", unique: true, using: :btree
   add_index "namespaces", ["path"], name: "index_namespaces_on_path", using: :btree
   add_index "namespaces", ["path"], name: "index_namespaces_on_path_trigram", using: :gin, opclasses: {"path"=>"gin_trgm_ops"}
+  add_index "namespaces", ["plan_id"], name: "index_namespaces_on_plan_id", using: :btree
   add_index "namespaces", ["require_two_factor_authentication"], name: "index_namespaces_on_require_two_factor_authentication", using: :btree
   add_index "namespaces", ["type"], name: "index_namespaces_on_type", using: :btree
 
@@ -1329,6 +1330,15 @@ ActiveRecord::Schema.define(version: 20170807160457) do
   add_index "personal_access_tokens", ["token"], name: "index_personal_access_tokens_on_token", unique: true, using: :btree
   add_index "personal_access_tokens", ["user_id"], name: "index_personal_access_tokens_on_user_id", using: :btree
 
+  create_table "plans", force: :cascade do |t|
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+    t.string "name"
+    t.string "title"
+  end
+
+  add_index "plans", ["name"], name: "index_plans_on_name", using: :btree
+
   create_table "project_authorizations", id: false, force: :cascade do |t|
     t.integer "user_id"
     t.integer "project_id"
@@ -2035,6 +2045,7 @@ ActiveRecord::Schema.define(version: 20170807160457) do
   add_foreign_key "milestones", "namespaces", column: "group_id", name: "fk_95650a40d4", on_delete: :cascade
   add_foreign_key "milestones", "projects", name: "fk_9bd0a0c791", on_delete: :cascade
   add_foreign_key "namespace_statistics", "namespaces", on_delete: :cascade
+  add_foreign_key "namespaces", "plans", name: "fk_fdd12e5b80", on_delete: :nullify
   add_foreign_key "notes", "projects", name: "fk_99e097b079", on_delete: :cascade
   add_foreign_key "oauth_openid_requests", "oauth_access_grants", column: "access_grant_id", name: "fk_oauth_openid_requests_oauth_access_grants_access_grant_id"
   add_foreign_key "pages_domains", "projects", name: "fk_ea2f6dfc6f", on_delete: :cascade

ee/app/models/ee/namespace.rb

@@ -21,14 +21,20 @@ module EE
     }.freeze
 
     prepended do
+      include IgnorableColumn
+
+      ignore_column :plan
+
+      belongs_to :plan
+
       has_one :namespace_statistics
 
-      scope :with_plan, -> { where.not(plan: [nil, '']) }
+      scope :with_plan, -> { where.not(plan_id: nil) }
 
       delegate :shared_runners_minutes, :shared_runners_seconds, :shared_runners_seconds_last_reset,
         to: :namespace_statistics, allow_nil: true
 
-      validates :plan, inclusion: { in: EE_PLANS.keys }, allow_blank: true
+      validate :validate_plan_name
     end
 
     def root_ancestor
@@ -68,7 +74,7 @@ module EE
 
     def feature_available_in_plan?(feature)
       @features_available_in_plan ||= Hash.new do |h, feature|
-        h[feature] = plans.any? { |plan| License.plan_includes_feature?(EE_PLANS[plan], feature) }
+        h[feature] = plans.any? { |plan| License.plan_includes_feature?(EE_PLANS[plan&.name], feature) }
       end
 
       @features_available_in_plan[feature]
@@ -77,7 +83,7 @@ module EE
     # The main difference between the "plan" column and this method is that "plan"
     # returns nil / "" when it has no plan. Having no plan means it's a "free" plan.
     def actual_plan
-      plan.presence || FREE_PLAN
+      plan&.name || FREE_PLAN
     end
 
     def actual_shared_runners_minutes_limit
@@ -95,8 +101,25 @@ module EE
         shared_runners_minutes.to_i >= actual_shared_runners_minutes_limit
     end
 
+    # These helper methods are required to not break the Namespace API.
+    def plan=(plan_name)
+      if plan_name.is_a?(String)
+        @plan_name = plan_name
+
+        super(Plan.find_by(name: @plan_name))
+      else
+        super
+      end
+    end
+
     private
 
+    def validate_plan_name
+      if @plan_name.present? && EE_PLANS.keys.exclude?(@plan_name)
+        errors.add(:plan, 'is not included in the list')
+      end
+    end
+
     def load_feature_available(feature)
       globally_available = License.feature_available?(feature)
 
@@ -110,7 +133,7 @@ module EE
     def plans
       @ancestors_plans ||=
         if parent_id
-          ancestors.with_plan.reorder(nil).pluck('DISTINCT plan') + [plan]
+          Plan.where(id: ancestors.with_plan.reorder(nil).select('plan_id')) + [plan]
         else
           [plan]
         end

lib/api/entities.rb

@@ -616,7 +616,9 @@ module API
 
       # EE-only
       expose :shared_runners_minutes_limit, if: lambda { |_, options| options[:current_user]&.admin? }
-      expose :plan, if: -> (namespace, opts) { Ability.allowed?(opts[:current_user], :admin_namespace, namespace) }
+      expose :plan, if: -> (namespace, opts) { Ability.allowed?(opts[:current_user], :admin_namespace, namespace) } do |namespace, _|
+        namespace.plan&.name
+      end
     end
 
     class MemberAccess < Grape::Entity

spec/ee/spec/models/ee/namespace_spec.rb

@@ -4,11 +4,11 @@ describe Namespace do
   let!(:namespace) { create(:namespace) }
 
   it { is_expected.to have_one(:namespace_statistics) }
+  it { is_expected.to belong_to(:plan) }
 
   it { is_expected.to delegate_method(:shared_runners_minutes).to(:namespace_statistics) }
   it { is_expected.to delegate_method(:shared_runners_seconds).to(:namespace_statistics) }
   it { is_expected.to delegate_method(:shared_runners_seconds_last_reset).to(:namespace_statistics) }
-  it { is_expected.to validate_inclusion_of(:plan).in_array(Namespace::EE_PLANS.keys).allow_blank }
 
   context 'scopes' do
     describe '.with_plan' do
@@ -42,6 +42,29 @@ describe Namespace do
     end
   end
 
+  describe 'custom validations' do
+    describe '#validate_plan_name' do
+      let(:group) { build(:group) }
+
+      context 'with a valid plan name' do
+        it 'is valid' do
+          group.plan = Namespace::BRONZE_PLAN
+
+          expect(group).to be_valid
+        end
+      end
+
+      context 'with an invalid plan name' do
+        it 'is invalid' do
+          group.plan = 'unknown'
+
+          expect(group).not_to be_valid
+          expect(group.errors[:plan]).to include('is not included in the list')
+        end
+      end
+    end
+  end
+
   describe '#move_dir' do
     context 'when running on a primary node' do
       let!(:geo_node) { create(:geo_node, :primary, :current) }

spec/ee/spec/models/ee/project_spec.rb

@@ -115,7 +115,7 @@ describe Project do
 
             context 'allowed by Plan License AND Global License' do
               let(:allowed_on_global_license) { true }
-              let(:plan_license) { Namespace::GOLD_PLAN }
+              let(:plan_license) { Plan.find_by(name: 'gold') }
 
               it 'returns true' do
                 is_expected.to eq(true)
@@ -124,7 +124,7 @@ describe Project do
 
             context 'not allowed by Plan License but project and namespace are public' do
               let(:allowed_on_global_license) { true }
-              let(:plan_license) { Namespace::BRONZE_PLAN }
+              let(:plan_license) { Plan.find_by(name: 'bronze') }
 
               it 'returns true' do
                 allow(namespace).to receive(:public?) { true }
@@ -137,7 +137,7 @@ describe Project do
             unless License.plan_includes_feature?(License::STARTER_PLAN, feature_sym)
               context 'not allowed by Plan License' do
                 let(:allowed_on_global_license) { true }
-                let(:plan_license) { Namespace::BRONZE_PLAN }
+                let(:plan_license) { Plan.find_by(name: 'bronze') }
 
                 it 'returns false' do
                   is_expected.to eq(false)
@@ -147,7 +147,7 @@ describe Project do
 
             context 'not allowed by Global License' do
               let(:allowed_on_global_license) { false }
-              let(:plan_license) { Namespace::GOLD_PLAN }
+              let(:plan_license) { Plan.find_by(name: 'gold') }
 
               it 'returns false' do
                 is_expected.to eq(false)

spec/features/billings/billing_plans_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe 'Billing plan pages', :feature do
   let(:user) { create(:user) }
-
+  let(:bronze_plan) { Plan.find_by(name: 'bronze') }
   let(:plans_data) do
     [
       {
@@ -101,7 +101,7 @@ describe 'Billing plan pages', :feature do
 
   context 'users profile billing page' do
     before do
-      allow_any_instance_of(EE::Namespace).to receive(:plan).and_return('bronze')
+      allow_any_instance_of(EE::Namespace).to receive(:plan).and_return(bronze_plan)
 
       visit profile_billings_path
     end
@@ -123,7 +123,7 @@ describe 'Billing plan pages', :feature do
 
     context 'top-most group' do
       before do
-        expect_any_instance_of(EE::Group).to receive(:plan).at_least(:once).and_return('bronze')
+        expect_any_instance_of(EE::Group).to receive(:plan).at_least(:once).and_return(bronze_plan)
 
         visit group_billings_path(group)
       end

spec/models/plan_spec.rb

+require 'spec_helper'
+
+describe Plan do
+  describe 'associations' do
+    it { is_expected.to have_many(:namespaces) }
+  end
+end

spec/spec_helper.rb

@@ -83,6 +83,8 @@ RSpec.configure do |config|
   config.before(:all) do
     License.destroy_all
     TestLicense.init
+
+    SeedFu.seed
   end
 
   config.after(:suite) do