Merge pull request #5 from leapcode/feature/removing-email-aliases

Removing email aliases - proper error display

15 files changed, 92 insertions, 77 deletions

diff --git a/users/app/assets/javascripts/users.js.coffee b/users/app/assets/javascripts/users.js.coffee
index 0c1fb55..86bacee 100644
--- a/users/app/assets/javascripts/users.js.coffee
+++ b/users/app/assets/javascripts/users.js.coffee
@@ -24,7 +24,15 @@ srp.error = (message) ->
 pollUsers = (query, process) ->
   $.get( "/users.json", query: query).done(process)
 
+followLocationHash = ->
+  location = window.location.hash
+  if location
+    href_select = 'a[href="' + location + '"]'
+    link = $(href_select)
+    link.tab('show') if link
+
 $(document).ready ->
+  followLocationHash()
   $('#new_user').submit preventDefault
   $('#new_user').submit srp.signup
   $('#new_session').submit preventDefault
diff --git a/users/app/controllers/email_aliases_controller.rb b/users/app/controllers/email_aliases_controller.rb
index 751df85..3b0d5ac 100644
--- a/users/app/controllers/email_aliases_controller.rb
+++ b/users/app/controllers/email_aliases_controller.rb
@@ -4,29 +4,10 @@ class EmailAliasesController < ApplicationController
 
   respond_to :html
 
-  # get a list of email aliases for the given user?
-  def index
-    @aliases = @user.email_aliases
-    respond_with @aliases
-  end
-
-  def create
-    @alias = @user.add_email_alias(params[:email_alias])
-    flash[:notice] = t(:email_alias_created_successfully) unless @alias.errors
-    respond_with @alias, :location => edit_user_path(@user, :anchor => :email)
-  end
-
-  def update
-    @alias = @user.get_email_alias(params[:id])
-    @alias.set_email(params[:email_alias])
-    flash[:notice] = t(:email_alias_updated_successfully) unless @alias.errors
-    respond_with @alias, :location => edit_user_path(@user, :anchor => :email)
-  end
-
   def destroy
-    @alias = @user.get_email_alias(params[:id])
-    flash[:notice] = t(:email_alias_destroyed_successfully)
-    @alias.destroy
+    @alias = @user.email_aliases.delete(params[:id])
+    @user.save
+    flash[:notice] = t(:email_alias_destroyed_successfully, :alias => @alias)
     redirect_to edit_user_path(@user, :anchor => :email)
   end
 
diff --git a/users/app/controllers/users_controller.rb b/users/app/controllers/users_controller.rb
index 811e8e5..3d71c1a 100644
--- a/users/app/controllers/users_controller.rb
+++ b/users/app/controllers/users_controller.rb
@@ -3,6 +3,7 @@ class UsersController < ApplicationController
   skip_before_filter :verify_authenticity_token, :only => [:create]
 
   before_filter :fetch_user, :only => [:edit, :update, :destroy]
+  before_filter :set_anchor, :only => [:edit, :update]
   before_filter :authorize_admin, :only => [:index]
 
   respond_to :json, :html
@@ -27,16 +28,16 @@ class UsersController < ApplicationController
   end
 
   def edit
+    @email_alias = LocalEmail.new
   end
 
   def update
     @user.attributes = params[:user]
+    @email_alias = @user.email_aliases.last
     if @user.changed? and @user.save
       flash[:notice] = t(:user_updated_successfully)
-    else
-      flash[:error] = @user.errors.full_messages
     end
-    respond_with @user, :location => edit_user_path(@user)
+    respond_with @user, :location => edit_user_path(@user, :anchor => :email)
   end
 
   def destroy
@@ -50,4 +51,13 @@ class UsersController < ApplicationController
     @user = User.find_by_param(params[:id])
     access_denied unless admin? or (@user == current_user)
   end
+
+  def set_anchor
+    @anchor = email_settings? ? :email : :account
+  end
+
+  def email_settings?
+    params[:user] &&
+    params[:user].keys.detect{|key| key.index('email')}
+  end
 end
diff --git a/users/app/helpers/users_helper.rb b/users/app/helpers/users_helper.rb
index b017bca..45ca0e9 100644
--- a/users/app/helpers/users_helper.rb
+++ b/users/app/helpers/users_helper.rb
@@ -29,4 +29,5 @@ module UsersHelper
     classes << (@user.new_record? ? 'new' : 'edit')
     classes.compact
   end
+
 end
diff --git a/users/app/models/email.rb b/users/app/models/email.rb
index 4b01838..0745fda 100644
--- a/users/app/models/email.rb
+++ b/users/app/models/email.rb
@@ -14,4 +14,12 @@ class Email
   def to_s
     email
   end
+
+  def ==(other)
+    other.is_a?(String) ? self.email == other : super
+  end
+
+  def to_param
+    email
+  end
 end
diff --git a/users/app/models/local_email.rb b/users/app/models/local_email.rb
index 7cca4f4..c654fcb 100644
--- a/users/app/models/local_email.rb
+++ b/users/app/models/local_email.rb
@@ -1,6 +1,9 @@
 class LocalEmail < Email
 
   validate :unique_on_server
+  validate :unique_alias_for_user
+  validate :differs_from_main_email
+  validates :casted_by, :presence => true
 
   def to_partial_path
     "emails/email"
@@ -9,7 +12,23 @@ class LocalEmail < Email
   def unique_on_server
      has_email = User.find_by_email_or_alias(email)
      if has_email && has_email != self.base_doc
-      errors.add(:email, "has already been taken")
+      errors.add :email, "has already been taken"
     end
   end
+
+  def unique_alias_for_user
+    aliases = self.casted_by.email_aliases
+    if aliases.select{|a|a.email == self.email}.count > 1
+      errors.add :email, "is already your alias"
+    end
+  end
+
+  def differs_from_main_email
+    user = self.casted_by
+    if user.email == self.email
+      errors.add :email, "may not be the same as your email address"
+    end
+  end
+
+
 end
diff --git a/users/app/models/user.rb b/users/app/models/user.rb
index 10f358d..1999a28 100644
--- a/users/app/models/user.rb
+++ b/users/app/models/user.rb
@@ -35,9 +35,7 @@ class User < CouchRest::Model::Base
   validates :email_forward,
     :format => { :with => /\A(([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,}))?\Z/, :message => "needs to be a valid email address"}
 
-  validate :no_duplicate_email_aliases
-
-  validate :email_aliases_differ_from_email
+  validate :email_differs_from_email_aliases
 
   timestamps!
 
@@ -45,6 +43,7 @@ class User < CouchRest::Model::Base
     view :by_login
     view :by_created_at
     view :by_email
+
     view :by_email_alias,
       :map => <<-EOJS
     function(doc) {
@@ -56,6 +55,7 @@ class User < CouchRest::Model::Base
       });
     }
     EOJS
+
     view :by_email_or_alias,
       :map => <<-EOJS
     function(doc) {
@@ -70,6 +70,7 @@ class User < CouchRest::Model::Base
       });
     }
     EOJS
+
   end
 
   class << self
@@ -114,33 +115,20 @@ class User < CouchRest::Model::Base
     APP_CONFIG['admins'].include? self.login
   end
 
-  def add_email_alias(email)
-    email = LocalEmail.new(email) unless email.is_a? Email
-    email_aliases << email
-  end
-
   # this currently only adds the first email address submitted.
   # All the ui needs for now.
   def email_aliases_attributes=(attrs)
-    if attrs && attrs.values.first
-      add_email_alias attrs.values.first
-    end
+    email_aliases.build(attrs.values.first) if attrs
   end
 
   ##
   #  Validation Functions
   ##
 
-  # TODO: How do we handle these errors?
-  def no_duplicate_email_aliases
-    if email_aliases.count != email_aliases.map(&:email).uniq.count
-      errors.add(:email_aliases, "include a duplicate")
-    end
-  end
-
-  def email_aliases_differ_from_email
+  def email_differs_from_email_aliases
+    return if email_aliases.any? and email_aliases.last.errors.any?
     if email_aliases.map(&:email).include?(email)
-      errors.add(:email_aliases, "include the original email address")
+      errors.add(:email, "may not be the same as an alias")
     end
   end
 
diff --git a/users/app/views/emails/_email.html.haml b/users/app/views/emails/_email.html.haml
index f182ed9..3feb6f0 100644
--- a/users/app/views/emails/_email.html.haml
+++ b/users/app/views/emails/_email.html.haml
@@ -1,4 +1,6 @@
-%li.pull-right
-  %code= email
-  %i.icon-remove
-.clearfix
+- if email.valid?
+  %li.pull-right
+    %code= email
+    = link_to(user_email_alias_path(@user, email), :method => :delete) do
+      %i.icon-remove
+  .clearfix
diff --git a/users/app/views/users/_email_aliases.html.haml b/users/app/views/users/_email_aliases.html.haml
index 646480e..121acc4 100644
--- a/users/app/views/users/_email_aliases.html.haml
+++ b/users/app/views/users/_email_aliases.html.haml
@@ -2,5 +2,5 @@
   %ul.unstyled
     =render @user.email_aliases
 .clearfix
-= f.simple_fields_for :email_aliases, Email.new do |e|
+= f.simple_fields_for :email_aliases, @email_alias do |e|
   = e.input :email, :placeholder => "alias@#{request.domain}"
diff --git a/users/app/views/users/edit.html.haml b/users/app/views/users/edit.html.haml
index 92ab71b..820b80e 100644
--- a/users/app/views/users/edit.html.haml
+++ b/users/app/views/users/edit.html.haml
@@ -1,17 +1,11 @@
 .span8.offset2
   %h2=t :settings
-  %ul.nav.nav-tabs
-    %li.active
-      %a{:href => '#account', 'data-toggle' => 'tab'}Account
-    %li
-      %a{:href => '#email', 'data-toggle' => 'tab'}Email
-
-  .tab-content
-    .tab-pane.active#account
-      = user_form_with 'login_field', :legend => :change_login
-      = user_form_with 'password_fields', :legend => :change_password
-      = render 'cancel_account' if @user == current_user
-    .tab-pane#email
-      = user_form_with 'email_field', :legend => :set_email_address
-      = user_form_with 'email_forward_field', :legend => :forward_email
-      = user_form_with 'email_aliases', :legend => :add_email_alias
+  - content_for :account do
+    = user_form_with 'login_field', :legend => :change_login
+    = user_form_with 'password_fields', :legend => :change_password
+    = render 'cancel_account' if @user == current_user
+  - content_for :email do
+    = user_form_with 'email_field', :legend => :set_email_address
+    = user_form_with 'email_forward_field', :legend => :forward_email
+    = user_form_with 'email_aliases', :legend => :add_email_alias
+  = render 'tabs/tabs', :tabs => [:account, :email]
diff --git a/users/config/locales/en.yml b/users/config/locales/en.yml
index d068e70..3c71e7e 100644
--- a/users/config/locales/en.yml
+++ b/users/config/locales/en.yml
@@ -15,6 +15,7 @@ en:
   add_email_alias: "Add email alias"
   user_updated_successfully: "Settings have been updated successfully."
   user_created_successfully: "Successfully created your account."
+  email_alias_destroyed_successfully: "Successfully removed the alias '%{alias}'."
 
   activemodel:
     models:
diff --git a/users/config/routes.rb b/users/config/routes.rb
index 3c5fb73..8985502 100644
--- a/users/config/routes.rb
+++ b/users/config/routes.rb
@@ -11,7 +11,7 @@ Rails.application.routes.draw do
 
   get "signup" => "users#new", :as => "signup"
   resources :users do
-    resources :email_aliases
+    resources :email_aliases, :only => [:destroy], :id => /.*/
   end
 
 end
diff --git a/users/test/functional/users_controller_test.rb b/users/test/functional/users_controller_test.rb
index ce17500..1fa1462 100644
--- a/users/test/functional/users_controller_test.rb
+++ b/users/test/functional/users_controller_test.rb
@@ -45,11 +45,12 @@ class UsersControllerTest < ActionController::TestCase
     assert_equal user, assigns[:user]
   end
 
-  test "should process updated params" do
+  test "user can change settings" do
     user = find_record User
     user.expects(:attributes=).with(user.params)
     user.expects(:changed?).returns(true)
     user.expects(:save).returns(true)
+    user.stubs(:email_aliases).returns([])
 
     login user
     put :update, :user => user.params, :id => user.id, :format => :json
@@ -64,6 +65,7 @@ class UsersControllerTest < ActionController::TestCase
     user.expects(:attributes=).with(user.params)
     user.expects(:changed?).returns(true)
     user.expects(:save).returns(true)
+    user.stubs(:email_aliases).returns([])
 
     login :is_admin? => true
     put :update, :user => user.params, :id => user.id, :format => :json
diff --git a/users/test/unit/email_aliases_test.rb b/users/test/unit/email_aliases_test.rb
index 762aaea..f680ac6 100644
--- a/users/test/unit/email_aliases_test.rb
+++ b/users/test/unit/email_aliases_test.rb
@@ -22,7 +22,7 @@ class EmailAliasTest < ActiveSupport::TestCase
 
   test "adding email alias directly" do
     email_alias = "valid_alias@domain.net"
-    @user.add_email_alias(email_alias)
+    @user.email_aliases.build :email => email_alias
     assert @user.changed?
     assert @user.save
     assert_equal email_alias, @user.reload.email_aliases.first.to_s
@@ -30,10 +30,11 @@ class EmailAliasTest < ActiveSupport::TestCase
 
   test "duplicated email aliases are invalid" do
     email_alias = "valid_alias@domain.net"
-    @user.add_email_alias(email_alias)
+    @user.email_aliases.build :email => email_alias
     @user.save
     # add again
-    @user.add_email_alias(email_alias)
+    email_alias = @user.email_aliases.build :email => email_alias
+    assert !email_alias.valid?
     assert @user.changed?
     assert !@user.valid?
   end
@@ -41,14 +42,14 @@ class EmailAliasTest < ActiveSupport::TestCase
   test "email is invalid as email alias" do
     email_alias = "valid_alias@domain.net"
     @user.email = email_alias
-    @user.add_email_alias(email_alias)
+    @user.email_aliases.build :email => email_alias
     assert @user.changed?
     assert !@user.valid?
   end
 
   test "find user by email alias" do
     email_alias = "valid_alias@domain.net"
-    @user.add_email_alias(email_alias)
+    @user.email_aliases.build :email => email_alias
     assert @user.save
     assert_equal @user, User.find_by_email_or_alias(email_alias)
     assert_equal @user, User.find_by_email_alias(email_alias)
diff --git a/users/test/unit/email_test.rb b/users/test/unit/email_test.rb
index 1e216d6..6f9beaa 100644
--- a/users/test/unit/email_test.rb
+++ b/users/test/unit/email_test.rb
@@ -14,15 +14,15 @@ class EmailTest < ActiveSupport::TestCase
 
   teardown do
     @user.destroy if @user.persisted? # just in case
-    @other_user.destroy
+    @other_user.destroy if @other_user.persisted?
   end
 
 
   test "email aliases need to be unique" do
     email_alias = "valid_alias@domain.net"
-    @other_user.add_email_alias email_alias
+    @other_user.email_aliases.build :email => email_alias
     @other_user.save
-    @user.add_email_alias email_alias
+    @user.email_aliases.build :email => email_alias
     assert @user.changed?
     assert !@user.save
     # TODO handle errors
@@ -32,7 +32,7 @@ class EmailTest < ActiveSupport::TestCase
     email_alias = "valid_alias@domain.net"
     @other_user.email = email_alias
     @other_user.save
-    @user.add_email_alias email_alias
+    @user.email_aliases.build :email => email_alias
     assert @user.changed?
     assert !@user.save
   end