Commit 75cf5f5b authored by Mario de la Ossa's avatar Mario de la Ossa

User#projects_limit remove DB default and added NOT NULL constraint

This change is required because otherwise if a user is created with a
value for `projects_limit` that matches the DB default, it gets
overwritten by `current_application_settings.default_projects_limit`. By
removing the default we once again can allow a user to be created with a
limit of 10 projects without the risk that it'll change to 10000
parent 1b447b16
......@@ -794,10 +794,7 @@ class User < ActiveRecord::Base
# `User.select(:id)` raises
# `ActiveModel::MissingAttributeError: missing attribute: projects_limit`
# without this safeguard!
return unless has_attribute?(:projects_limit)
connection_default_value_defined = new_record? && !projects_limit_changed?
return unless projects_limit.nil? || connection_default_value_defined
return unless has_attribute?(:projects_limit) && projects_limit.nil?
self.projects_limit = current_application_settings.default_projects_limit
end
......
---
title: User#projects_limit remove DB default and added NOT NULL constraint
merge_request: 16165
author: Mario de la Ossa
type: fixed
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class ChangeUserProjectLimitNotNullAndRemoveDefault < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
# When a migration requires downtime you **must** uncomment the following
# constant and define a short and easy to understand explanation as to why the
# migration requires downtime.
# DOWNTIME_REASON = ''
# When using the methods "add_concurrent_index", "remove_concurrent_index" or
# "add_column_with_default" you must disable the use of transactions
# as these methods can not run in an existing transaction.
# When using "add_concurrent_index" or "remove_concurrent_index" methods make sure
# that either of them is the _only_ method called in the migration,
# any other changes should go in a separate migration.
# This ensures that upon failure _only_ the index creation or removing fails
# and can be retried or reverted easily.
#
# To disable transactions uncomment the following line and remove these
# comments:
# disable_ddl_transaction!
def up
# Set Users#projects_limit to NOT NULL and remove the default value
change_column_null :users, :projects_limit, false
change_column_default :users, :projects_limit, nil
end
def down
change_column_null :users, :projects_limit, true
change_column_default :users, :projects_limit, 10
end
end
......@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20171220191323) do
ActiveRecord::Schema.define(version: 20171229225929) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
......@@ -1805,7 +1805,7 @@ ActiveRecord::Schema.define(version: 20171220191323) do
t.datetime "updated_at"
t.string "name"
t.boolean "admin", default: false, null: false
t.integer "projects_limit", default: 10
t.integer "projects_limit", null: false
t.string "skype", default: "", null: false
t.string "linkedin", default: "", null: false
t.string "twitter", default: "", null: false
......
......@@ -134,6 +134,16 @@ describe User do
end
end
it 'has a DB-level NOT NULL constraint on projects_limit' do
user = create(:user)
expect(user.persisted?).to eq(true)
expect do
user.update_columns(projects_limit: nil)
end.to raise_error(ActiveRecord::StatementInvalid)
end
it { is_expected.to validate_presence_of(:projects_limit) }
it { is_expected.to validate_numericality_of(:projects_limit) }
it { is_expected.to allow_value(0).for(:projects_limit) }
......@@ -805,6 +815,13 @@ describe User do
expect(user.can_create_group).to be_falsey
expect(user.theme_id).to eq(1)
end
it 'does not undo projects_limit setting if it matches old DB default of 10' do
# If the real default project limit is 10 then this test is worthless
expect(Gitlab.config.gitlab.default_projects_limit).not_to eq(10)
user = described_class.new(projects_limit: 10)
expect(user.projects_limit).to eq(10)
end
end
context 'when current_application_settings.user_default_external is true' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment