Merge branch 'rd-5693-in-ultimate-don-t-count-guest-users-to-license-limit' into 'master'

Exclude Guest users or users without a membership from seats count for Ultimate plan

Closes #5693

See merge request gitlab-org/gitlab-ee!5816

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: 20180529093006) do
+ActiveRecord::Schema.define(version: 20180531031410) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -2691,6 +2691,7 @@ ActiveRecord::Schema.define(version: 20180529093006) do
   add_index "users", ["name"], name: "index_users_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"}
   add_index "users", ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true, using: :btree
   add_index "users", ["state"], name: "index_users_on_state", using: :btree
+  add_index "users", ["state"], name: "index_users_on_state_and_internal_attrs", where: "((ghost <> true) AND (support_bot <> true))", using: :btree
   add_index "users", ["support_bot"], name: "index_users_on_support_bot", using: :btree
   add_index "users", ["username"], name: "index_users_on_username", using: :btree
   add_index "users", ["username"], name: "index_users_on_username_trigram", using: :gin, opclasses: {"username"=>"gin_trgm_ops"}

ee/app/helpers/license_helper.rb

@@ -61,6 +61,14 @@ module LicenseHelper
     end
   end
 
+  def seats_calculation_message
+    if current_license&.exclude_guests_from_active_count?
+      content_tag :p do
+        "Users with a Guest role or those who don't belong to a Project or Group will not use a seat from your license."
+      end
+    end
+  end
+
   def current_license
     return @current_license if defined?(@current_license)
 

ee/app/models/ee/user.rb

@@ -33,6 +33,8 @@ module EE
       has_many :protected_branch_merge_access_levels, dependent: :destroy, class_name: ::ProtectedBranch::MergeAccessLevel # rubocop:disable Cop/ActiveRecordDependent
       has_many :protected_branch_push_access_levels, dependent: :destroy, class_name: ::ProtectedBranch::PushAccessLevel # rubocop:disable Cop/ActiveRecordDependent
       has_many :protected_branch_unprotect_access_levels, dependent: :destroy, class_name: ::ProtectedBranch::UnprotectAccessLevel # rubocop:disable Cop/ActiveRecordDependent
+
+      scope :excluding_guests, -> { joins(:members).where('members.access_level > ?', ::Gitlab::Access::GUEST).distinct }
     end
 
     module ClassMethods

ee/app/models/historical_data.rb

@@ -10,7 +10,7 @@ class HistoricalData < ActiveRecord::Base
     def track!
       create!(
         date:               Date.today,
-        active_user_count:  User.active.count
+        active_user_count:  License.load_license&.current_active_users_count
       )
     end
 

ee/app/models/license.rb

@@ -299,7 +299,13 @@ class License < ActiveRecord::Base
   end
 
   def current_active_users_count
-    @current_active_users_count ||= User.active.count
+    @current_active_users_count ||= begin
+      if exclude_guests_from_active_count?
+        User.active.excluding_guests.count
+      else
+        User.active.count
+      end
+    end
   end
 
   def validate_with_trueup?
@@ -316,6 +322,10 @@ class License < ActiveRecord::Base
     !expired?
   end
 
+  def exclude_guests_from_active_count?
+    plan == License::ULTIMATE_PLAN
+  end
+
   def remaining_days
     return 0 if expired?
 

ee/app/views/admin/licenses/_breakdown.html.haml

@@ -31,6 +31,7 @@
           The
           %a{ href: 'https://about.gitlab.com/license-faq/' } true-up model
           allows having more users, and additional users will incur a retroactive charge on renewal.
+          = seats_calculation_message
     .col-sm-4
       .info-well.dark-well
         .well-segment.well-centered

ee/changelogs/unreleased/rd-5693-in-ultimate-don-t-count-guest-users-to-license-limit.yml

+---
+title: Guest users will not consume seats quote in Ultimate plan
+merge_request: 5816
+author:
+type: added

ee/db/migrate/20180531031410_add_index_for_active_users.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddIndexForActiveUsers < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:users, :state, name: 'index_users_on_state_and_internal_attrs', where: "ghost <> true AND support_bot <> true")
+  end
+
+  def down
+    remove_concurrent_index(:users, name: :index_users_on_state_and_internal_attrs)
+  end
+end

ee/spec/models/ee/user_spec.rb

@@ -7,6 +7,27 @@ describe EE::User do
     it { is_expected.to have_many(:vulnerability_feedback) }
   end
 
+  describe "scopes" do
+    describe '.excluding_guests' do
+      let!(:user_without_membership) { create(:user).id }
+      let!(:project_guest_user)      { create(:project_member, :guest).user_id }
+      let!(:project_reporter_user)   { create(:project_member, :reporter).user_id }
+      let!(:group_guest_user)        { create(:group_member, :guest).user_id }
+      let!(:group_reporter_user)     { create(:group_member, :reporter).user_id }
+
+      it 'exclude users with a Guest role in a Project/Group' do
+        user_ids = User.excluding_guests.pluck(:id)
+
+        expect(user_ids).to include(project_reporter_user)
+        expect(user_ids).to include(group_reporter_user)
+
+        expect(user_ids).not_to include(user_without_membership)
+        expect(user_ids).not_to include(project_guest_user)
+        expect(user_ids).not_to include(group_guest_user)
+      end
+    end
+  end
+
   describe '#access_level=' do
     let(:user) { build(:user) }
 

ee/spec/models/historical_data_spec.rb

@@ -50,4 +50,30 @@ describe HistoricalData do
       expect(described_class.max_historical_user_count).to eq(300)
     end
   end
+
+  describe '.max_historical_user_count with different plans' do
+    using RSpec::Parameterized::TableSyntax
+
+    before do
+      create(:group_member, :guest)
+      create(:group_member, :reporter)
+      create(:license, plan: plan)
+
+      described_class.track!
+    end
+
+    where(:gl_plan, :expected_count) do
+      ::License::STARTER_PLAN  | 2
+      ::License::PREMIUM_PLAN  | 2
+      ::License::ULTIMATE_PLAN | 1
+    end
+
+    with_them do
+      let(:plan) { gl_plan }
+
+      it 'does not count guest users' do
+        expect(described_class.max_historical_user_count).to eq(expected_count)
+      end
+    end
+  end
 end

ee/spec/models/license_spec.rb

@@ -23,6 +23,46 @@ describe License do
       end
     end
 
+    describe '#check_users_limit' do
+      using RSpec::Parameterized::TableSyntax
+
+      before do
+        create(:group_member, :guest)
+        create(:group_member, :reporter)
+        create(:license, plan: plan)
+      end
+
+      let(:users_count) { nil }
+      let(:new_license) do
+        gl_license = build(:gitlab_license, restrictions: { plan: plan, active_user_count: users_count, previous_user_count: 1 })
+        build(:license, data: gl_license.export)
+      end
+
+      where(:gl_plan, :valid) do
+        ::License::STARTER_PLAN  | false
+        ::License::PREMIUM_PLAN  | false
+        ::License::ULTIMATE_PLAN | true
+      end
+
+      with_them do
+        let(:plan) { gl_plan }
+
+        context 'when license has restricted users' do
+          let(:users_count) { 1 }
+
+          it { expect(new_license.valid?).to eq(valid) }
+        end
+
+        context 'when license has unlimited users' do
+          let(:users_count) { nil }
+
+          it 'is always valid' do
+            expect(new_license.valid?).to eq(true)
+          end
+        end
+      end
+    end
+
     describe "Historical active user count" do
       let(:active_user_count) { User.active.count + 10 }
       let(:date)              { described_class.current.starts_at }