Merge pull request #1610 from salbertson/sa-refactor-users-update

Move logic for updating a user to a service class

app/controllers/users_controller.rb

@@ -43,28 +43,8 @@ class UsersController < ApplicationController
     user = User.where(username_lower: params[:username].downcase).first
     guardian.ensure_can_edit!(user)
     json_result(user, serializer: UserSerializer) do |u|
-
-      website = params[:website]
-      if website
-        website = "http://" + website unless website =~ /^http/
-      end
-
-      u.bio_raw = params[:bio_raw] || u.bio_raw
-      u.name = params[:name] || u.name
-      u.website = website || u.website
-      u.digest_after_days = params[:digest_after_days] || u.digest_after_days
-      u.auto_track_topics_after_msecs = params[:auto_track_topics_after_msecs].to_i if params[:auto_track_topics_after_msecs]
-      u.new_topic_duration_minutes = params[:new_topic_duration_minutes].to_i if params[:new_topic_duration_minutes]
-      u.title = params[:title] || u.title if guardian.can_grant_title?(u)
-
-      [:email_digests, :email_always, :email_direct, :email_private_messages,
-       :external_links_in_new_tab, :enable_quoting, :dynamic_favicon].each do |i|
-        if params[i].present?
-          u.send("#{i.to_s}=", params[i] == 'true')
-        end
-      end
-
-      u.save ? u : nil
+      updater = UserUpdater.new(user)
+      updater.update(params)
     end
   end
 

app/services/user_updater.rb

@@ -0,0 +1,54 @@
+class UserUpdater
+  def initialize(user)
+    @user = user
+    @guardian = Guardian.new(user)
+  end
+
+  def update(attributes = {})
+    user.website = format_url(attributes[:website]) || user.website
+
+    user.bio_raw = attributes[:bio_raw] || user.bio_raw
+    user.name = attributes[:name] || user.name
+    user.digest_after_days = attributes[:digest_after_days] || user.digest_after_days
+
+    if attributes[:auto_track_topics_after_msecs]
+      user.auto_track_topics_after_msecs = attributes[:auto_track_topics_after_msecs].to_i 
+    end
+
+    if attributes[:new_topic_duration_minutes]
+      user.new_topic_duration_minutes = attributes[:new_topic_duration_minutes].to_i
+    end
+
+    if guardian.can_grant_title?(user)
+      user.title = attributes[:title] || user.title
+    end
+
+    [
+      :email_digests,
+      :email_always,
+      :email_direct,
+      :email_private_messages,
+      :external_links_in_new_tab,
+      :enable_quoting,
+      :dynamic_favicon
+    ].each do |attribute|
+      if attributes[attribute].present?
+        user.send("#{attribute.to_s}=", attributes[attribute] == 'true')
+      end
+    end
+
+    user.save
+  end
+
+  private
+
+  attr_reader :user, :guardian
+
+  def format_url(website)
+    if website =~ /^http/
+      website
+    else
+      "http://#{website}"
+    end
+  end
+end

spec/controllers/users_controller_spec.rb

@@ -853,29 +853,9 @@ describe UsersController do
           json = JSON.parse(response.body)
           expect(json['user']['id']).to eq user.id
         end
-
-        context 'when website includes http' do
-          it 'does not add http before updating' do
-            user = log_in
-
-            put :update, username: user.username, website: 'http://example.com'
-
-            expect(user.reload.website).to eq 'http://example.com'
-          end
-        end
-
-        context 'when website does not include http' do
-          it 'adds http before updating' do
-            user = log_in
-
-            put :update, username: user.username, website: 'example.com'
-
-            expect(user.reload.website).to eq 'http://example.com'
-          end
-        end
       end
 
-      context 'without permission to update any attributes' do
+      context 'without permission to update' do
         it 'does not allow the update' do
           user = Fabricate(:user, name: 'Billy Bob')
           log_in_user(user)
@@ -889,20 +869,6 @@ describe UsersController do
           expect(user.reload.name).not_to eq 'Jim Tom'
         end
       end
-
-      context 'without permission to update title' do
-        it 'does not allow the user to update their title' do
-          user = Fabricate(:user, title: 'Emperor')
-          log_in_user(user)
-          guardian = Guardian.new(user)
-          guardian.stubs(can_grant_title?: false).with(user)
-          Guardian.stubs(new: guardian).with(user)
-
-          put :update, username: user.username, title: 'Minion'
-
-          expect(user.reload.title).not_to eq 'Minion'
-        end
-      end
     end
   end
 

spec/services/user_updater_spec.rb

@@ -0,0 +1,83 @@
+require 'spec_helper'
+
+describe UserUpdater do
+  describe '#update' do
+    it 'saves user' do
+      user = Fabricate(:user, name: 'Billy Bob')
+      updater = UserUpdater.new(user)
+
+      updater.update(name: 'Jim Tom')
+
+      expect(user.reload.name).to eq 'Jim Tom'
+    end
+
+    context 'when update succeeds' do
+      it 'returns true' do
+        user = Fabricate(:user)
+        updater = UserUpdater.new(user)
+
+        expect(updater.update).to be_true
+      end
+    end
+
+    context 'when update fails' do
+      it 'returns false' do
+        user = Fabricate(:user)
+        user.stubs(save: false)
+        updater = UserUpdater.new(user)
+
+        expect(updater.update).to be_false
+      end
+    end
+
+    context 'with permission to update title' do
+      it 'allows user to change title' do
+        user = Fabricate(:user, title: 'Emperor')
+        guardian = stub
+        guardian.stubs(:can_grant_title?).with(user).returns(true)
+        Guardian.stubs(:new).with(user).returns(guardian)
+        updater = UserUpdater.new(user)
+
+        updater.update(title: 'Minion')
+
+        expect(user.reload.title).to eq 'Minion'
+      end
+    end
+
+    context 'without permission to update title' do
+      it 'does not allow user to change title' do
+        user = Fabricate(:user, title: 'Emperor')
+        guardian = stub
+        guardian.stubs(:can_grant_title?).with(user).returns(false)
+        Guardian.stubs(:new).with(user).returns(guardian)
+        updater = UserUpdater.new(user)
+
+        updater.update(title: 'Minion')
+
+        expect(user.reload.title).not_to eq 'Minion'
+      end
+    end
+
+    context 'when website includes http' do
+      it 'does not add http before updating' do
+        user = Fabricate(:user)
+        updater = UserUpdater.new(user)
+
+        updater.update(website: 'http://example.com')
+
+        expect(user.reload.website).to eq 'http://example.com'
+      end
+    end
+
+    context 'when website does not include http' do
+      it 'adds http before updating' do
+        user = Fabricate(:user)
+        updater = UserUpdater.new(user)
+
+        updater.update(website: 'example.com')
+
+        expect(user.reload.website).to eq 'http://example.com'
+      end
+    end
+  end
+end