DEV: Replace User.unstage and User#unstage API with User#unstage! (#8906)

* DEV: Replace User.unstage and User#unstage API with User#unstage!

Quoting @SamSaffron:

> User.unstage mixes concerns of both unstaging users and updating params which is fragile/surprising.
> u.unstage destroys notifications and raises a user_unstaged event prior to the user becoming unstaged and the user object being saved.

User#unstage! no longer updates user attributes and saves the object before triggering the `user_unstaged` event.

* Update one more spec

* Assign attributes after unstaging

app/controllers/users/omniauth_callbacks_controller.rb

@@ -113,8 +113,7 @@ class Users::OmniauthCallbacksController < ApplicationController
 
     # automatically activate/unstage any account if a provider marked the email valid
     if @auth_result.email_valid && @auth_result.email == user.email
-      user.unstage
-      user.save
+      user.unstage!
 
       if !user.active || !user.email_confirmed?
         user.update!(password: SecureRandom.hex)

app/controllers/users_controller.rb

@@ -436,8 +436,16 @@ class UsersController < ApplicationController
     params[:locale] ||= I18n.locale unless current_user
 
     new_user_params = user_params.except(:timezone)
-    user = User.unstage(new_user_params)
-    user = User.new(new_user_params) if user.nil?
+
+    user = User.where(staged: true).with_email(new_user_params[:email].strip.downcase).first
+
+    if user
+      user.active = false
+      user.unstage!
+    end
+
+    user ||= User.new
+    user.attributes = new_user_params
 
     # Handle API approval
     ReviewableUser.set_approved_fields!(user, current_user) if user.approved?

app/models/discourse_single_sign_on.rb

@@ -74,8 +74,7 @@ class DiscourseSingleSignOn < SingleSignOn
     end
 
     # ensure it's not staged anymore
-    user.unstage
-    user.save
+    user.unstage!
 
     change_external_attributes_and_override(sso_record, user)
 

app/models/invite_redeemer.rb

@@ -15,26 +15,27 @@ InviteRedeemer = Struct.new(:invite, :username, :name, :password, :user_custom_f
 
   # extracted from User cause it is very specific to invites
   def self.create_user_from_invite(invite, username, name, password = nil, user_custom_fields = nil, ip_address = nil)
+    user = User.where(staged: true).with_email(invite.email.strip.downcase).first
+    user.unstage! if user
+
+    user ||= User.new
+
     if username && UsernameValidator.new(username).valid_format? && User.username_available?(username)
       available_username = username
     else
       available_username = UserNameSuggester.suggest(invite.email)
     end
-    available_name = name || available_username
 
-    user_params = {
+    user.attributes = {
       email: invite.email,
       username: available_username,
-      name: available_name,
+      name: name || available_username,
       active: false,
       trust_level: SiteSetting.default_invitee_trust_level,
       ip_address: ip_address,
       registration_ip_address: ip_address
     }
 
-    user = User.unstage(user_params)
-    user = User.new(user_params) if user.nil?
-
     if !SiteSetting.must_approve_users? || (SiteSetting.must_approve_users? && invite.invited_by.staff?)
       ReviewableUser.set_approved_fields!(user, invite.invited_by)
     end

app/models/user.rb

@@ -363,22 +363,17 @@ class User < ActiveRecord::Base
     user
   end
 
-  def unstage
+  def unstage!
     if self.staged
+      ActiveRecord::Base.transaction do
         self.staged = false
         self.custom_fields[FROM_STAGED] = true
         self.notifications.destroy_all
-      DiscourseEvent.trigger(:user_unstaged, self)
-    end
+        save!
       end
 
-  def self.unstage(params)
-    if user = User.where(staged: true).with_email(params[:email].strip.downcase).first
-      params.each { |k, v| user.public_send("#{k}=", v) }
-      user.active = false
-      user.unstage
+      DiscourseEvent.trigger(:user_unstaged, self)
     end
-    user
   end
 
   def self.suggest_name(string)

lib/auth/default_current_user_provider.rb

@@ -167,7 +167,7 @@ class Auth::DefaultCurrentUserProvider
       impersonate: opts[:impersonate])
 
     cookies[TOKEN_COOKIE] = cookie_hash(@user_token.unhashed_auth_token)
-    unstage_user(user)
+    user.unstage!
     make_developer_admin(user)
     enable_bootstrap_mode(user)
 
@@ -191,13 +191,6 @@ class Auth::DefaultCurrentUserProvider
     hash
   end
 
-  def unstage_user(user)
-    if user.staged
-      user.unstage
-      user.save
-    end
-  end
-
   def make_developer_admin(user)
     if  user.active? &&
         !user.admin &&

spec/models/group_spec.rb

@@ -232,8 +232,7 @@ describe Group do
 
       expect(GroupUser.where(user_id: staged.id).count).to eq(0)
 
-      staged.unstage
-      staged.save!
+      staged.unstage!
 
       expect(GroupUser.where(user_id: staged.id).count).to eq(2)
     end

spec/models/post_spec.rb

@@ -1123,7 +1123,7 @@ describe Post do
       SiteSetting.newuser_max_links = 3
       user = Fabricate(:user, staged: true, trust_level: 0)
       user.created_at = 1.hour.ago
-      user.unstage
+      user.unstage!
       post = Fabricate(:post, raw: raw, user: user)
       expect(post.has_host_spam?).to eq(true)
     end
@@ -1133,7 +1133,7 @@ describe Post do
       SiteSetting.newuser_max_links = 3
       user = Fabricate(:user, staged: true, trust_level: 0)
       user.created_at = 2.days.ago
-      user.unstage
+      user.unstage!
       post = Fabricate(:post, raw: raw, user: user)
       expect(post.has_host_spam?).to eq(false)
     end

spec/models/user_spec.rb

@@ -1840,44 +1840,29 @@ describe User do
     end
   end
 
-  describe "#unstage" do
-    let!(:staged_user) { Fabricate(:staged, email: 'staged@account.com', active: true, username: 'staged1', name: 'Stage Name') }
-    let(:params) { { email: 'staged@account.com', active: true, username: 'unstaged1', name: 'Foo Bar' } }
+  describe "#unstage!" do
+    let!(:user) { Fabricate(:staged, email: 'staged@account.com', active: true, username: 'staged1', name: 'Stage Name') }
 
     it "correctly unstages a user" do
-      user = User.unstage(params)
-
-      expect(user.id).to eq(staged_user.id)
-      expect(user.username).to eq('unstaged1')
-      expect(user.name).to eq('Foo Bar')
-      expect(user.active).to eq(false)
-      expect(user.email).to eq('staged@account.com')
+      user.unstage!
       expect(user.staged).to eq(false)
     end
 
-    it "returns nil when the user cannot be unstaged" do
-      Fabricate(:coding_horror)
-      expect(User.unstage(email: 'jeff@somewhere.com')).to be_nil
-      expect(User.unstage(email: 'no@account.com')).to be_nil
-    end
-
     it "removes all previous notifications during unstaging" do
-      Fabricate(:notification, user: staged_user)
-      Fabricate(:private_message_notification, user: staged_user)
-      staged_user.reload
+      Fabricate(:notification, user: user)
+      Fabricate(:private_message_notification, user: user)
+      expect(user.total_unread_notifications).to eq(2)
 
-      expect(staged_user.total_unread_notifications).to eq(2)
-      user = User.unstage(params)
+      user.unstage!
+      user.reload
       expect(user.total_unread_notifications).to eq(0)
       expect(user.staged).to eq(false)
     end
 
     it "triggers an event" do
-      unstaged_user = nil
-      event = DiscourseEvent.track_events { unstaged_user = User.unstage(params) }.first
-
+      event = DiscourseEvent.track_events { user.unstage! }.first
       expect(event[:event_name]).to eq(:user_unstaged)
-      expect(event[:params].first).to eq(unstaged_user)
+      expect(event[:params].first).to eq(user)
     end
   end