From 72bfa4471f1e915905ab0df7f7b3634cfa4c74e9 Mon Sep 17 00:00:00 2001 From: Scott Albertson Date: Fri, 1 Nov 2013 14:06:20 -0700 Subject: [PATCH] Move logic for updating a user into a service class --- app/controllers/users_controller.rb | 24 +------ app/services/user_updater.rb | 54 +++++++++++++++ spec/controllers/users_controller_spec.rb | 36 +--------- spec/services/user_updater_spec.rb | 83 +++++++++++++++++++++++ 4 files changed, 140 insertions(+), 57 deletions(-) create mode 100644 app/services/user_updater.rb create mode 100644 spec/services/user_updater_spec.rb diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index df19e15d18d..d237587b305 100644 --- a/app/controllers/users_controller.rb +++ b/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 diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb new file mode 100644 index 00000000000..d8c1e5047e0 --- /dev/null +++ b/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 diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index d5d2a24f0b6..46b76cdafa6 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/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 diff --git a/spec/services/user_updater_spec.rb b/spec/services/user_updater_spec.rb new file mode 100644 index 00000000000..4524c967432 --- /dev/null +++ b/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