diff options
| -rw-r--r-- | app/models/user.rb | 7 | ||||
| -rw-r--r-- | changelogs/unreleased/57538-normalize-users-private-profile-field.yml | 5 | ||||
| -rw-r--r-- | db/migrate/20190620105427_change_null_private_profile_to_false.rb | 33 | ||||
| -rw-r--r-- | db/schema.rb | 2 | ||||
| -rw-r--r-- | doc/api/users.md | 8 | ||||
| -rw-r--r-- | lib/api/users.rb | 2 | ||||
| -rw-r--r-- | lib/gitlab/background_migration/migrate_null_private_profile_to_false.rb | 17 | ||||
| -rw-r--r-- | spec/lib/gitlab/background_migration/migrate_null_private_profile_to_false_spec.rb | 23 | ||||
| -rw-r--r-- | spec/models/user_spec.rb | 12 | ||||
| -rw-r--r-- | spec/requests/api/users_spec.rb | 8 |
10 files changed, 111 insertions, 6 deletions
diff --git a/app/models/user.rb b/app/models/user.rb index 02637b70f03..0fd3daa3383 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -185,6 +185,7 @@ class User < ApplicationRecord before_validation :set_notification_email, if: :new_record? before_validation :set_public_email, if: :public_email_changed? before_validation :set_commit_email, if: :commit_email_changed? + before_save :default_private_profile_to_false before_save :set_public_email, if: :public_email_changed? # in case validation is skipped before_save :set_commit_email, if: :commit_email_changed? # in case validation is skipped before_save :ensure_incoming_email_token @@ -1491,6 +1492,12 @@ class User < ApplicationRecord private + def default_private_profile_to_false + return unless private_profile_changed? && private_profile.nil? + + self.private_profile = false + end + def has_current_license? false end diff --git a/changelogs/unreleased/57538-normalize-users-private-profile-field.yml b/changelogs/unreleased/57538-normalize-users-private-profile-field.yml new file mode 100644 index 00000000000..85fc4cbf33c --- /dev/null +++ b/changelogs/unreleased/57538-normalize-users-private-profile-field.yml @@ -0,0 +1,5 @@ +--- +title: Migrate NULL values for users.private_profile column and update users API to reject null value for private_profile. +merge_request: 29888 +author: +type: changed diff --git a/db/migrate/20190620105427_change_null_private_profile_to_false.rb b/db/migrate/20190620105427_change_null_private_profile_to_false.rb new file mode 100644 index 00000000000..d820dbbe9d3 --- /dev/null +++ b/db/migrate/20190620105427_change_null_private_profile_to_false.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +class ChangeNullPrivateProfileToFalse < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + DELAY = 30.seconds.to_i + BATCH_SIZE = 1_000 + + disable_ddl_transaction! + + class User < ActiveRecord::Base + self.table_name = 'users' + + include ::EachBatch + end + + def up + change_column_default :users, :private_profile, false + + # Migration will take about 120 hours + User.where(private_profile: nil).each_batch(of: BATCH_SIZE) do |batch, index| + range = batch.pluck('MIN(id)', 'MAX(id)').first + delay = index * DELAY + + BackgroundMigrationWorker.perform_in(delay.seconds, 'MigrateNullPrivateProfileToFalse', [*range]) + end + end + + def down + change_column_default :users, :private_profile, nil + end +end diff --git a/db/schema.rb b/db/schema.rb index e6a52e691c1..644ca1fe970 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -3391,7 +3391,7 @@ ActiveRecord::Schema.define(version: 2019_07_03_130053) do t.integer "theme_id", limit: 2 t.integer "accepted_term_id" t.string "feed_token" - t.boolean "private_profile" + t.boolean "private_profile", default: false t.boolean "include_private_contributions" t.string "commit_email" t.boolean "auditor", default: false, null: false diff --git a/doc/api/users.md b/doc/api/users.md index b43b4de823b..54641f4c862 100644 --- a/doc/api/users.md +++ b/doc/api/users.md @@ -357,9 +357,9 @@ Parameters: - `admin` (optional) - User is admin - true or false (default) - `can_create_group` (optional) - User can create groups - true or false - `skip_confirmation` (optional) - Skip confirmation - true or false (default) -- `external` (optional) - Flags the user as external - true or false(default) +- `external` (optional) - Flags the user as external - true or false (default) - `avatar` (optional) - Image file for user's avatar -- `private_profile` (optional) - User's profile is private - true or false +- `private_profile` (optional) - User's profile is private - true or false (default) - `shared_runners_minutes_limit` (optional) - Pipeline minutes quota for this user **(STARTER)** - `extra_shared_runners_minutes_limit` (optional) - Extra pipeline minutes quota for this user **(STARTER)** @@ -392,11 +392,11 @@ Parameters: - `admin` (optional) - User is admin - true or false (default) - `can_create_group` (optional) - User can create groups - true or false - `skip_reconfirmation` (optional) - Skip reconfirmation - true or false (default) -- `external` (optional) - Flags the user as external - true or false(default) +- `external` (optional) - Flags the user as external - true or false (default) - `shared_runners_minutes_limit` (optional) - Pipeline minutes quota for this user - `extra_shared_runners_minutes_limit` (optional) - Extra pipeline minutes quota for this user - `avatar` (optional) - Image file for user's avatar -- `private_profile` (optional) - User's profile is private - true or false +- `private_profile` (optional) - User's profile is private - true or false (default) - `shared_runners_minutes_limit` (optional) - Pipeline minutes quota for this user **(STARTER)** - `extra_shared_runners_minutes_limit` (optional) - Extra pipeline minutes quota for this user **(STARTER)** diff --git a/lib/api/users.rb b/lib/api/users.rb index 41418aa216c..30a278fdff1 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -51,7 +51,7 @@ module API optional :can_create_group, type: Boolean, desc: 'Flag indicating the user can create groups' optional :external, type: Boolean, desc: 'Flag indicating the user is an external user' optional :avatar, type: File, desc: 'Avatar image for user' - optional :private_profile, type: Boolean, desc: 'Flag indicating the user has a private profile' + optional :private_profile, type: Boolean, default: false, desc: 'Flag indicating the user has a private profile' all_or_none_of :extern_uid, :provider use :optional_params_ee diff --git a/lib/gitlab/background_migration/migrate_null_private_profile_to_false.rb b/lib/gitlab/background_migration/migrate_null_private_profile_to_false.rb new file mode 100644 index 00000000000..32ed6a2756d --- /dev/null +++ b/lib/gitlab/background_migration/migrate_null_private_profile_to_false.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + # This class is responsible for migrating a range of users with private_profile == NULL to false + class MigrateNullPrivateProfileToFalse + # Temporary AR class for users + class User < ActiveRecord::Base + self.table_name = 'users' + end + + def perform(start_id, stop_id) + User.where(private_profile: nil, id: start_id..stop_id).update_all(private_profile: false) + end + end + end +end diff --git a/spec/lib/gitlab/background_migration/migrate_null_private_profile_to_false_spec.rb b/spec/lib/gitlab/background_migration/migrate_null_private_profile_to_false_spec.rb new file mode 100644 index 00000000000..c45c64f6a23 --- /dev/null +++ b/spec/lib/gitlab/background_migration/migrate_null_private_profile_to_false_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::BackgroundMigration::MigrateNullPrivateProfileToFalse, :migration, schema: 20190620105427 do + let(:users) { table(:users) } + + it 'correctly migrates nil private_profile to false' do + private_profile_true = users.create!(private_profile: true, projects_limit: 1, email: 'a@b.com') + private_profile_false = users.create!(private_profile: false, projects_limit: 1, email: 'b@c.com') + private_profile_nil = users.create!(private_profile: nil, projects_limit: 1, email: 'c@d.com') + + described_class.new.perform(private_profile_true.id, private_profile_nil.id) + + private_profile_true.reload + private_profile_false.reload + private_profile_nil.reload + + expect(private_profile_true.private_profile).to eq(true) + expect(private_profile_false.private_profile).to eq(false) + expect(private_profile_nil.private_profile).to eq(false) + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e4f84172215..5cfa64fd764 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -530,6 +530,17 @@ describe User do end describe 'before save hook' do + context '#default_private_profile_to_false' do + let(:user) { create(:user, private_profile: true) } + + it 'converts nil to false' do + user.private_profile = nil + user.save! + + expect(user.private_profile).to eq false + end + end + context 'when saving an external user' do let(:user) { create(:user) } let(:external_user) { create(:user, external: true) } @@ -1127,6 +1138,7 @@ describe User do expect(user.can_create_group).to eq(Gitlab.config.gitlab.default_can_create_group) expect(user.theme_id).to eq(Gitlab.config.gitlab.default_theme) expect(user.external).to be_falsey + expect(user.private_profile).to eq false end end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 46925daf40a..0ad50e5347a 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -745,6 +745,14 @@ describe API::Users do expect(user.reload.private_profile).to eq(true) end + it "updates private profile when nil is given to false" do + admin.update(private_profile: true) + + put api("/users/#{user.id}", admin), params: { private_profile: nil } + + expect(user.reload.private_profile).to eq(false) + end + it "does not update admin status" do put api("/users/#{admin_user.id}", admin), params: { can_create_group: false } |