Migrate null values for users.private_profile57538-normalize-users-private-profile-field

- Background migration for changing null values to false
- Set false as default value for private_profile DB column

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 9d28c5d2383..8138e1749a2 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 }