Merge branch '57538-normalize-users-private-profile-field' into 'master'

Migrate null values for users.private_profile column

See merge request gitlab-org/gitlab-ce!29888

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

changelogs/unreleased/57538-normalize-users-private-profile-field.yml

+---
+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

db/migrate/20190620105427_change_null_private_profile_to_false.rb

+# 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

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

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)**
 

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

lib/gitlab/background_migration/migrate_null_private_profile_to_false.rb

+# 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

spec/lib/gitlab/background_migration/migrate_null_private_profile_to_false_spec.rb

+# 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

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
 

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 }