separate signup and settings service objects for user

9 files changed, 105 insertions, 80 deletions

diff --git a/users/app/controllers/v1/users_controller.rb b/users/app/controllers/v1/users_controller.rb
index 467c9b4..f380c19 100644
--- a/users/app/controllers/v1/users_controller.rb
+++ b/users/app/controllers/v1/users_controller.rb
@@ -18,14 +18,23 @@ module V1
     end
 
     def create
-      @user = User.create(params[:user])
+      @user = signup_service.register(params[:user])
       respond_with @user # return ID instead?
     end
 
     def update
-      @user.update_attributes params[:user]
+      account_settings.update params[:user]
       respond_with @user
     end
 
+    protected
+
+    def account_settings
+      AccountSettings.new(@user)
+    end
+
+    def signup_service
+      SignupService.new
+    end
   end
 end
diff --git a/users/app/models/account_settings.rb b/users/app/models/account_settings.rb
new file mode 100644
index 0000000..a73a95a
--- /dev/null
+++ b/users/app/models/account_settings.rb
@@ -0,0 +1,36 @@
+class AccountSettings
+
+  def initialize(user)
+    @user = user
+  end
+
+  def update(attrs)
+    if attrs[:password_verifier].present?
+      update_login(attrs[:login])
+      @user.update_attributes attrs.slice(:password_verifier, :password_salt)
+    end
+    # TODO: move into identity controller
+    update_pgp_key(attrs[:public_key]) if attrs.has_key? :public_key
+    @user.save && save_identities
+  end
+
+  protected
+
+  def update_login(login, verifier)
+    return unless login.present?
+    @old_identity = Identity.for(@user)
+    @user.login = login
+    @new_identity = Identity.for(@user) # based on the new login
+    @old_identity.destination = @user.email_address # alias old -> new
+  end
+
+  def update_pgp_key(key)
+    @new_identity ||= Identity.for(@user)
+    @new_identity.set_key(:pgp, key)
+  end
+
+  def save_identities
+    @new_identity.try(:save) && @old_identity.try(:save)
+  end
+
+end
diff --git a/users/app/models/identity.rb b/users/app/models/identity.rb
index 73531ec..355f67a 100644
--- a/users/app/models/identity.rb
+++ b/users/app/models/identity.rb
@@ -27,6 +27,33 @@ class Identity < CouchRest::Model::Base
 
   end
 
+  def self.for(user, attributes = {})
+    find_for(user, attributes) || build_for(user, attributes)
+  end
+
+  def self.find_for(user, attributes = {})
+    attributes.reverse_merge! attributes_from_user(user)
+    find_by_address_and_destination [attributes[:address], attributes[:destination]]
+  end
+
+  def self.build_for(user, attributes = {})
+    attributes.reverse_merge! attributes_from_user(user)
+    Identity.new(attributes)
+  end
+
+  def self.create_for(user, attributes = {})
+    identity = build_for(user, attributes)
+    identity.save
+    identity
+  end
+
+  def self.attributes_from_user(user)
+    { user_id: user.id,
+      address: user.email_address,
+      destination: user.email_address
+    }
+  end
+
   def keys
     read_attribute('keys') || HashWithIndifferentAccess.new
   end
diff --git a/users/app/models/signup_service.rb b/users/app/models/signup_service.rb
new file mode 100644
index 0000000..f316ca9
--- /dev/null
+++ b/users/app/models/signup_service.rb
@@ -0,0 +1,9 @@
+class SignupService
+
+  def register(attrs)
+    User.create(attrs).tap do |user|
+      Identity.create_for user
+    end
+  end
+
+end
diff --git a/users/app/models/user.rb b/users/app/models/user.rb
index c791069..f78f290 100644
--- a/users/app/models/user.rb
+++ b/users/app/models/user.rb
@@ -47,63 +47,10 @@ class User < CouchRest::Model::Base
     view :by_created_at
   end # end of design
 
-  # We proxy access to the pgp_key. So we need to make sure
-  # the updated identity actually gets saved.
-  def save(*args)
-    super
-    identity.user_id ||= self.id
-    identity.save if identity.changed?
-  end
-
-  # So far this only works for creating a new user.
-  # TODO: Create an alias for the old login when changing the login
-  def login=(value)
-    write_attribute 'login', value
-    if @identity
-      @identity.address = email_address
-      @identity.destination = email_address
-    else
-      build_identity
-    end
-  end
-
-  # DEPRECATED
-  #
-  # Please set the key on the identity directly
-  def public_key=(value)
-    identity.set_key(:pgp, value)
-  end
-
-  # DEPRECATED
-  #
-  # Please access identity.keys[:pgp] directly
-  def public_key
-    identity.keys[:pgp]
-  end
-
   class << self
     alias_method :find_by_param, :find
   end
 
-  # this is the main identity. login@domain.tld
-  # aliases and forwards are represented in other identities.
-  def identity
-    @identity ||= find_identity || build_identity
-  end
-
-  def create_identity(attribs = {}, &block)
-    new_identity = build_identity(attribs, &block)
-    new_identity.save
-    new_identity
-  end
-
-  def build_identity(attribs = {}, &block)
-    attribs.reverse_merge! user_id: self.id,
-      address: self.email_address,
-      destination: self.email_address
-    Identity.new(attribs, &block)
-  end
-
   def to_param
     self.id
   end
@@ -142,10 +89,6 @@ class User < CouchRest::Model::Base
 
   protected
 
-  def find_identity
-    Identity.find_by_address_and_destination([email_address, email_address])
-  end
-
   ##
   #  Validation Functions
   ##
diff --git a/users/test/functional/v1/users_controller_test.rb b/users/test/functional/v1/users_controller_test.rb
index 0d44e50..a330bf3 100644
--- a/users/test/functional/v1/users_controller_test.rb
+++ b/users/test/functional/v1/users_controller_test.rb
@@ -5,7 +5,9 @@ class V1::UsersControllerTest < ActionController::TestCase
   test "user can change settings" do
     user = find_record :user
     changed_attribs = record_attributes_for :user_with_settings
-    user.expects(:update_attributes).with(changed_attribs)
+    account_settings = stub
+    account_settings.expects(:update).with(changed_attribs)
+    AccountSettings.expects(:new).with(user).returns(account_settings)
 
     login user
     put :update, :user => changed_attribs, :id => user.id, :format => :json
@@ -18,7 +20,9 @@ class V1::UsersControllerTest < ActionController::TestCase
   test "admin can update user" do
     user = find_record :user
     changed_attribs = record_attributes_for :user_with_settings
-    user.expects(:update_attributes).with(changed_attribs)
+    account_settings = stub
+    account_settings.expects(:update).with(changed_attribs)
+    AccountSettings.expects(:new).with(user).returns(account_settings)
 
     login :is_admin? => true
     put :update, :user => changed_attribs, :id => user.id, :format => :json
diff --git a/users/test/integration/api/account_flow_test.rb b/users/test/integration/api/account_flow_test.rb
index 93b6507..c09dcb6 100644
--- a/users/test/integration/api/account_flow_test.rb
+++ b/users/test/integration/api/account_flow_test.rb
@@ -84,20 +84,17 @@ class AccountFlowTest < RackTest
       identity.destroy
     end
     put "http://api.lvh.me:3000/1/users/" + @user.id + '.json', :user => {:public_key => test_public_key, :login => new_login}, :format => :json
-    @user.reload
     assert last_response.successful?
-    assert_equal test_public_key, @user.public_key
-    assert_equal new_login, @user.login
+    assert_equal test_public_key, Identity.for(@user).keys[:pgp]
+    # does not change login if no password_verifier is present
+    assert_equal original_login, @user.login
     # eventually probably want to remove most of this into a non-integration functional test
     # should not overwrite public key:
     put "http://api.lvh.me:3000/1/users/" + @user.id + '.json', :user => {:blee => :blah}, :format => :json
-    @user.reload
-    assert_equal test_public_key, @user.public_key
+    assert_equal test_public_key, Identity.for(@user).keys[:pgp]
     # should overwrite public key:
     put "http://api.lvh.me:3000/1/users/" + @user.id + '.json', :user => {:public_key => nil}, :format => :json
-    # TODO: not sure why i need this, but when public key is removed, the DB is updated but @user.reload doesn't seem to actually reload.
-    @user = User.find(@user.id) # @user.reload
-    assert_nil @user.public_key
+    assert_nil Identity.for(@user).keys[:pgp]
   end
 
 end
diff --git a/users/test/unit/identity_test.rb b/users/test/unit/identity_test.rb
index 40b1296..bf24f02 100644
--- a/users/test/unit/identity_test.rb
+++ b/users/test/unit/identity_test.rb
@@ -11,28 +11,28 @@ class IdentityTest < ActiveSupport::TestCase
   end
 
   test "initial identity for a user" do
-    id = @user.identity
+    id = Identity.for(@user)
     assert_equal @user.email_address, id.address
     assert_equal @user.email_address, id.destination
     assert_equal @user, id.user
   end
 
   test "add alias" do
-    id = @user.build_identity address: alias_name
+    id = Identity.for @user, address: alias_name
     assert_equal LocalEmail.new(alias_name), id.address
     assert_equal @user.email_address, id.destination
     assert_equal @user, id.user
   end
 
   test "add forward" do
-    id = @user.build_identity destination: forward_address
+    id = Identity.for @user, destination: forward_address
     assert_equal @user.email_address, id.address
     assert_equal Email.new(forward_address), id.destination
     assert_equal @user, id.user
   end
 
   test "forward alias" do
-    id = @user.build_identity address: alias_name, destination: forward_address
+    id = Identity.for @user, address: alias_name, destination: forward_address
     assert_equal LocalEmail.new(alias_name), id.address
     assert_equal Email.new(forward_address), id.destination
     assert_equal @user, id.user
@@ -40,29 +40,29 @@ class IdentityTest < ActiveSupport::TestCase
   end
 
   test "prevents duplicates" do
-    id = @user.create_identity address: alias_name, destination: forward_address
-    dup = @user.build_identity address: alias_name, destination: forward_address
+    id = Identity.create_for @user, address: alias_name, destination: forward_address
+    dup = Identity.build_for @user, address: alias_name, destination: forward_address
     assert !dup.valid?
     assert_equal ["This alias already exists"], dup.errors[:base]
   end
 
   test "validates availability" do
     other_user = FactoryGirl.create(:user)
-    id = @user.create_identity address: alias_name, destination: forward_address
-    taken = other_user.build_identity address: alias_name
+    id = Identity.create_for @user, address: alias_name, destination: forward_address
+    taken = Identity.build_for other_user, address: alias_name
     assert !taken.valid?
     assert_equal ["This email has already been taken"], taken.errors[:base]
     other_user.destroy
   end
 
   test "setting and getting pgp key" do
-    id = @user.identity
+    id = Identity.for(@user)
     id.set_key(:pgp, pgp_key_string)
     assert_equal pgp_key_string, id.keys[:pgp]
   end
 
   test "querying pgp key via couch" do
-    id = @user.identity
+    id = Identity.for(@user)
     id.set_key(:pgp, pgp_key_string)
     id.save
     view = Identity.pgp_key_by_email.key(id.address)
diff --git a/users/test/unit/user_test.rb b/users/test/unit/user_test.rb
index f303c8d..477c727 100644
--- a/users/test/unit/user_test.rb
+++ b/users/test/unit/user_test.rb
@@ -58,7 +58,7 @@ class UserTest < ActiveSupport::TestCase
 
   test "login needs to be unique amongst aliases" do
     other_user = FactoryGirl.create :user
-    other_user.create_identity address: @user.login
+    Identity.create_for other_user, address: @user.login
     assert !@user.valid?
     other_user.destroy
   end