Commit c85c0ccc authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets

Merge branch 'mdelaossa/gitlab-ce-31995-project-limit-default-fix'

Signed-off-by: default avatarDmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
parents 335f02a5 75cf5f5b
...@@ -794,10 +794,7 @@ class User < ActiveRecord::Base ...@@ -794,10 +794,7 @@ class User < ActiveRecord::Base
# `User.select(:id)` raises # `User.select(:id)` raises
# `ActiveModel::MissingAttributeError: missing attribute: projects_limit` # `ActiveModel::MissingAttributeError: missing attribute: projects_limit`
# without this safeguard! # without this safeguard!
return unless has_attribute?(:projects_limit) return unless has_attribute?(:projects_limit) && projects_limit.nil?
connection_default_value_defined = new_record? && !projects_limit_changed?
return unless projects_limit.nil? || connection_default_value_defined
self.projects_limit = current_application_settings.default_projects_limit self.projects_limit = current_application_settings.default_projects_limit
end 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 @@ ...@@ -11,7 +11,7 @@
# #
# It's strongly recommended that you check this file into your version control system. # It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20171221140220) do ActiveRecord::Schema.define(version: 20171229225929) do
# These are extensions that must be enabled in order to support this database # These are extensions that must be enabled in order to support this database
enable_extension "plpgsql" enable_extension "plpgsql"
...@@ -1808,7 +1808,7 @@ ActiveRecord::Schema.define(version: 20171221140220) do ...@@ -1808,7 +1808,7 @@ ActiveRecord::Schema.define(version: 20171221140220) do
t.datetime "updated_at" t.datetime "updated_at"
t.string "name" t.string "name"
t.boolean "admin", default: false, null: false 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 "skype", default: "", null: false
t.string "linkedin", default: "", null: false t.string "linkedin", default: "", null: false
t.string "twitter", default: "", null: false t.string "twitter", default: "", null: false
......
...@@ -136,6 +136,16 @@ describe User do ...@@ -136,6 +136,16 @@ describe User do
end end
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_presence_of(:projects_limit) }
it { is_expected.to validate_numericality_of(:projects_limit) } it { is_expected.to validate_numericality_of(:projects_limit) }
it { is_expected.to allow_value(0).for(:projects_limit) } it { is_expected.to allow_value(0).for(:projects_limit) }
...@@ -807,6 +817,13 @@ describe User do ...@@ -807,6 +817,13 @@ describe User do
expect(user.can_create_group).to be_falsey expect(user.can_create_group).to be_falsey
expect(user.theme_id).to eq(1) expect(user.theme_id).to eq(1)
end 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 end
context 'when current_application_settings.user_default_external is true' do 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