Merge pull request #98 from coopdevs/user_email_validation

Added email validation format for user and related test. Fixed validation errors when creating users to provide i18n feedback.

app/controllers/users_controller.rb

@@ -8,9 +8,11 @@ class UsersController < ApplicationController
 
   def index
     @users = scoped_users
-    @memberships = current_organization.members.where(user_id: @users.map(&:id)).includes(:account).each_with_object({}) do |mem, ob|
-      ob[mem.user_id] = mem
-    end
+    @memberships = current_organization.members.
+                   where(user_id: @users.map(&:id)).
+                   includes(:account).each_with_object({}) do |mem, ob|
+                     ob[mem.user_id] = mem
+                   end
   end
 
   def show
@@ -30,25 +32,24 @@ class UsersController < ApplicationController
 
   def create
     authorize User
+
+    empty_email = false
+
     if @user = User.find_by_email(user_params[:email])
-      if !@user.active?(current_organization)
-        # Deactivated user is registered again (overwrite new attributes)
-        @user.attributes = user_params.merge(:active => true)
-        @user.save!
-      end
+      reactivate_user
     else
       # New User
       @user = User.new(user_params)
-      @user.skip_confirmation! #auto-confirm, not sending confirmation email
-      @user.save!
+      empty_email = @user.email.empty?
+      @user.setup_and_save_user
     end
 
     if @user.persisted?
-      @user.add_to_organization current_organization
-
+      @user.tune_after_persisted(current_organization)
       redirect_to users_path
     else
-      redirect_to :action => "new"
+      @user.email = "" if empty_email
+      render action: "new"
     end
   end
 
@@ -64,12 +65,19 @@ class UsersController < ApplicationController
 
   def give_time
     @user = scoped_users.find(params[:id])
-    @destination = @user.members.find_by(organization: current_organization).account.id
-    @source = current_user.members.find_by(organization: current_organization).account.id
-    @offer = current_organization.offers.find(params[:offer]) if params[:offer].present?
-    @transfer = Transfer.new(source: @source, destination: @destination, post: @offer)
+    @destination = @user.members.
+                   find_by(organization: current_organization).account.id
+    @source = current_user.members.
+              find_by(organization: current_organization).account.id
+    @offer = current_organization.offers.
+             find(params[:offer]) if params[:offer].present?
+    @transfer = Transfer.new(source: @source,
+                             destination: @destination,
+                             post: @offer)
     if admin?
-      @sources = [current_organization.account] + current_organization.member_accounts.where("members.active is true")
+      @sources = [current_organization.account] +
+                 current_organization.
+                 member_accounts.where("members.active is true")
     end
   end
 
@@ -84,17 +92,26 @@ class UsersController < ApplicationController
       # TODO - Inquiries and Offers
     end
 
-    redirect_to :action => "index"
+    redirect_to action: "index"
   end
 
   private
 
+  def reactivate_user
+    if !@user.active?(current_organization)
+      # Deactivated user is registered again (overwrite new attributes)
+      @user.attributes = user_params.merge(active: true)
+      @user.save!
+    end
+  end
+
   def user_params
-    fields_to_permit = %w"gender username email date_of_birth phone alt_phone active description"
-    fields_to_permit += %w"admin registration_number registration_date" if admin?
+    fields_to_permit = %w"gender username email date_of_birth phone
+                          alt_phone active description"
+    fields_to_permit += %w"admin registration_number
+                           registration_date" if admin?
     fields_to_permit += %w"organization_id superadmin" if superadmin?
     # params[:user].permit(*fields_to_permit).tap &method(:ap)
     params.require(:user).permit *fields_to_permit
   end
-
 end

app/helpers/application_helper.rb

@@ -1,22 +1,24 @@
-require 'date'
+require "date"
 
 module ApplicationHelper
-
   # froom http://railscasts.com/episodes/244-gravatar?language=en&view=asciicast
-  def avatar_url(user, size=32)
+  def avatar_url(user, size = 32)
     gravatar_id = Digest::MD5::hexdigest(user.email).downcase
-    gravatar_options = Hash[s: size, d: 'identicon']
-    "http://gravatar.com/avatar/#{gravatar_id}.png?#{Rack::Utils.build_query(gravatar_options)}"
+    gravatar_options = Hash[s: size, d: "identicon"]
+    "http://gravatar.com/avatar/#{gravatar_id}.png?" +
+      "#{Rack::Utils.build_query(gravatar_options)}"
   end
 
-
   def theme_stylesheet_link_tag
     theme = current_organization.try(:theme)
+    main_url = "//netdna.bootstrapcdn.com/"
+    bs_version = "3.1.0"
+    bs_css_file = "bootstrap.min.css"
     url = if Organization::BOOTSWATCH_THEMES.include? theme
-      "//netdna.bootstrapcdn.com/bootswatch/3.1.0/#{theme}/bootstrap.min.css"
-    else
-      "//netdna.bootstrapcdn.com/bootstrap/3.1.0/css/bootstrap.min.css"
-    end
+            "#{main_url}/bootswatch/#{bs_version}/#{theme}/#{bs_css_file}"
+          else
+            "#{main_url}/bootstrap/#{bs_version}/css/#{bs_css_file}"
+          end
     stylesheet_link_tag url
   end
 
@@ -27,7 +29,7 @@ module ApplicationHelper
   def seconds_to_hm(seconds)
     sign = seconds <=> 0
     if sign.try :nonzero?
-      minutes, seconds = seconds.abs.divmod(60)
+      minutes, _seconds = seconds.abs.divmod(60)
       hours, minutes = minutes.divmod(60)
       raw format("%s%d:%02d", ("-" if sign < 0), hours, minutes)
     else
@@ -39,4 +41,18 @@ module ApplicationHelper
     document_path(Document.terms_and_conditions || 0, modal: true)
   end
 
+  def show_error_messages!(resource)
+    return "" if resource.errors.empty?
+
+    messages = resource.errors.
+               full_messages.map { |msg| content_tag(:li, msg) }.join
+    html = <<-HTML
+    <div class="alert alert-error alert-block"> <button type="button"
+      class="close" data-dismiss="alert">x</button>
+      #{messages}
+    </div>
+    HTML
+
+    html.html_safe
+  end
 end

app/helpers/devise_helper.rb

@@ -1,15 +1,2 @@
 module DeviseHelper
-  def devise_error_messages!
-    return '' if resource.errors.empty?
-
-    messages = resource.errors.full_messages.map { |msg| content_tag(:li, msg) }.join
-    html = <<-HTML
-    <div class="alert alert-error alert-block"> <button type="button"
-    class="close" data-dismiss="alert">x</button>
-      #{messages}
-    </div>
-    HTML
-
-    html.html_safe
-  end
 end

app/models/user.rb

@@ -1,6 +1,7 @@
-require 'textacular/searchable'
+require "textacular/searchable"
 
 class User < ActiveRecord::Base
+  attr_accessor :empty_email
 
   devise *[
     :database_authenticatable,
@@ -16,12 +17,18 @@ class User < ActiveRecord::Base
 
   GENDERS = %w[male female]
 
-  default_scope ->{ order('users.id ASC') }
+  default_scope -> { order("users.id ASC") }
 
-  scope :actives, -> { where({ members: { active: true } }) }
+  scope :actives, -> { where(members: { active: true }) }
 
   validates :username, presence: true, uniqueness: true
   validates :email, presence: true, uniqueness: true
+
+  # Allows @domain.com for dummy emails but does not allow pure invalid
+  # emails like 'without email'
+  validates_format_of :email,
+                      with: /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i
+
   # validates :gender, presence: true, inclusion: {in: GENDERS}
 
   has_many :members
@@ -54,8 +61,9 @@ class User < ActiveRecord::Base
     "#{username}"
   end
 
-  def add_to_organization organization
-    organization && members.find_or_create_by(organization: organization) do |member|
+  def add_to_organization(organization)
+    organization && members.
+      find_or_create_by(organization: organization) do |member|
       member.entry_date = DateTime.now.utc
     end
   end
@@ -67,4 +75,27 @@ class User < ActiveRecord::Base
   def member(organization)
     members.where(organization_id: organization).first
   end
+
+  def set_dummy_email
+    self.email = "user#{id}@example.com"
+    skip_reconfirmation! # auto-reconfirm
+  end
+
+  def setup_and_save_user
+    # check if email is provided or not. If not, flag it and generate
+    # temporary valid email with current time milliseconds
+    # this will be updated to user.id@example.com later on
+    self.empty_email = email.strip.empty?
+    self.email = "user#{DateTime.now.strftime('%Q')}@example.com" if empty_email
+    skip_confirmation! # auto-confirm, not sending confirmation email
+    save
+  end
+
+  def tune_after_persisted(organization)
+    add_to_organization organization
+
+    # If email was empty, udpate again with user.id just generated
+    set_dummy_email if empty_email
+    save
+  end
 end

app/views/devise/confirmations/new.html.haml

@@ -5,13 +5,13 @@
     .panel.panel-primary
       .panel-heading
         %h2.panel-title
-          =t(".resend_instructions")
+          = t(".resend_instructions")
 
       .panel-body
-        = devise_error_messages! 
+        = show_error_messages!(resource)
         = form_for resource, url: confirmation_path(resource_name), html: { method: :post } do |f|
           .form-group
             = f.label :email, class: "control-label"
-            = f.text_field :email, :required => true, :autofocus => true, class: "form-control"
+            = f.text_field :email, required: true, autofocus: true, class: "form-control"
           .form-group
             = f.submit t(".resend_instructions"), class: "btn btn-primary"

app/views/devise/passwords/edit.html.haml

@@ -5,10 +5,10 @@
     .panel.panel-primary
       .panel-heading
         %h2.panel-title
-          =t(".change_password")
+          = t(".change_password")
 
       .panel-body
-        = devise_error_messages!
+        = show_error_messages!(resource)
         = form_for resource, url: password_path(resource_name), html: { method: :put } do |f|
           = f.hidden_field :reset_password_token
           .form-group

app/views/devise/passwords/new.html.haml

@@ -5,13 +5,13 @@
     .panel.panel-primary
       .panel-heading
         %h2.panel-title
-          =t(".forgot_question")
+          = t(".forgot_question")
 
       .panel-body
-        = devise_error_messages! 
+        = show_error_messages!(resource)
         = form_for resource, url: password_path(resource_name), html: { method: :post } do |f|
           .form-group
             = f.label :email, class: "control-label"
-            = f.text_field :email, :required => true, :autofocus => true, class: "form-control"
+            = f.text_field :email, required: true, autofocus: true, class: "form-control"
           .form-group
             = f.submit t(".send_instructions"), class: "btn btn-primary"

app/views/devise/registrations/edit.html.haml

@@ -3,9 +3,9 @@
     .panel.panel-primary
       .panel-heading
         %h2.panel-title
-          =t(".edit_user")
+          = t(".edit_user")
       .panel-body
-        = devise_error_messages! 
+        = show_error_messages!(resource)
 
         = form_for(resource, as: resource_name, url: registration_path(resource_name), html: { method: :put }) do |f|
           .form-group
@@ -19,7 +19,7 @@
             = f.label t(".password"), class: "control-label"
             = f.password_field :password, required: true, autocomplete: "off", class: "form-control"
             %p.help-block
-              =t(".help_password")
+              = t(".help_password")
           .form-group
             = f.label t(".password_confirmation"), class: "control-label"
             = f.password_field :password_confirmation, required: true, class: "form-control"
@@ -34,7 +34,7 @@
     .panel.panel-primary
       .panel-heading
         %h2.panel-title
-          =t(".cancel_account")
+          = t(".cancel_account")
       .panel-body
         %p
           = t(".unhappy")

app/views/devise/registrations/new.html.haml

@@ -5,22 +5,22 @@
     .panel.panel-primary
       .panel-heading
         %h2.panel-title
-          =t(".sign_up")
+          = t(".sign_up")
 
       .panel-body
-        = devise_error_messages! 
+        = show_error_messages!(resource)
         = form_for resource, url: registration_path(resource_name) do |f|
           .form-group
             = f.label :username, class: "control-label"
-            = f.text_field :username, :required => true, :autofocus => true, class: "form-control"
+            = f.text_field :username, required: true, autofocus: true, class: "form-control"
           .form-group
             = f.label :email, class: "control-label"
-            = f.text_field :email, :required => true, :autofocus => true, class: "form-control"
+            = f.text_field :email, required: true, autofocus: true, class: "form-control"
           .form-group
             = f.label t(".password"), class: "control-label"
-            = f.password_field :password, :required => true, class: "form-control"
+            = f.password_field :password, required: true, class: "form-control"
           .form-group
             = f.label t(".password_confirmation"), class: "control-label"
-            = f.password_field :password_confirmation, :required => true, class: "form-control"
+            = f.password_field :password_confirmation, required: true, class: "form-control"
           .form-group
             = f.submit t(".sign_me_up"), class: "btn btn-primary"

app/views/devise/sessions/new.html.haml

@@ -5,17 +5,17 @@
     .panel.panel-primary
       .panel-heading
         %h2.panel-title
-          =t(".sign_in")
+          = t(".sign_in")
 
       .panel-body
-        = devise_error_messages! 
+        = show_error_messages!(resource)
         = form_for resource, url: session_path(resource_name) do |f|
           .form-group
             = f.label :email, class: "control-label"
-            = f.text_field :email, :required => false, :autofocus => true, class: "form-control"
+            = f.text_field :email, required: false, autofocus: true, class: "form-control"
           .form-group
             = f.label :password, class: "control-label"
-            = f.password_field :password, :required => false, :autofocus => true, class: "form-control"
+            = f.password_field :password, required: false, autofocus: true, class: "form-control"
           - if devise_mapping.rememberable?
             .form-group
               .checkbox

app/views/devise/unlocks/new.html.haml

@@ -5,13 +5,13 @@
     .panel.panel-primary
       .panel-heading
         %h2.panel-title
-          =t(".resend_instructions")
+          = t(".resend_instructions")
 
       .panel-body
-        = devise_error_messages!
+        = show_error_messages!(resource)
         = form_for resource, url: unlock_path(resource_name), html: { method: :post } do |f|
           .form-group
             = f.label :email, class: "control-label"
-            = f.text_field :email, :required => false, :autofocus => true, class: "form-control"
+            = f.text_field :email, required: false, autofocus: true, class: "form-control"
           .form-group
             = f.submit t(".resend_instructions"), class: "btn btn-primary"

app/views/users/_form.html.haml

@@ -1,4 +1,5 @@
 .well
+  = show_error_messages!(@user)
   = simple_form_for @user do |f|
     .form-inputs
 
@@ -9,7 +10,7 @@
         - if @user.unconfirmed_email.present?
           = f.input :unconfirmed_email, readonly: true
       - else
-        = f.input :email, :readonly => true
+        = f.input :email, readonly: true
 
       / - if current_user.try :superadmin?
       /   = f.association :organization, include_blank: false
@@ -21,7 +22,7 @@
       = f.input :alt_phone
       = f.input :date_of_birth, start_year: Date.today.year - 90, end_year: Date.today.year - 12, include_blank: :true
 
-      = f.input :description, as: 'text'
+      = f.input :description, as: "text"
 
       / = f.association :categories, label_method: :fqn, input_html: {style: 'width: 100%'}
 

app/views/users/edit.html.haml

@@ -1,6 +1,6 @@
 %h1
   = @user.username
   %small
-    = t ".edit"
+    = t ".edit_user"
 
-= render "form"
+= render "form"

config/locales/ca.yml

@@ -87,7 +87,12 @@ ca:
         end_on: Finalitza al
       account:
         balance: Balanç
-
+    errors:
+      models:
+        user:
+          attributes:
+            email:
+              invalid: invàlid
   # ETIQUETAS VARIADAS EN PLANTILLAS
 
   application:
@@ -141,6 +146,8 @@ ca:
       actions: Accions
     new:
       new_user: Nou usuari
+    edit:
+      edit_user: Canviar usuari
     show:
       phone:
         one: Telèfon

config/locales/en.yml

@@ -87,7 +87,12 @@ en:
         end_on: Ends on
       account:
         balance: Balance
-
+    errors:
+      models:
+        user:
+          attributes:
+            email:
+              invalid: invalid
   # ETIQUETAS VARIADAS EN PLANTILLAS
 
   application:
@@ -141,6 +146,8 @@ en:
       actions: Actions
     new:
       new_user: New user
+    edit:
+      edit_user: Update user
     show:
       phone:
         one: Phone

config/locales/es.yml

@@ -87,7 +87,12 @@ es:
         end_on: Termina el
       account:
         balance: Balance
-
+    errors:
+      models:
+        user:
+          attributes:
+            email:
+              invalid: inválido
   # ETIQUETAS VARIADAS EN PLANTILLAS
 
   application:
@@ -141,6 +146,8 @@ es:
       actions: Acciones
     new:
       new_user: Nuevo usuario
+    edit:
+      edit_user: Cambiar usuario
     show:
       phone:
         one: Teléfono

spec/controllers/users_controller_spec.rb

@@ -17,27 +17,45 @@ describe UsersController do
               organization: test_organization,
               manager: false)
   end
+  let (:wrong_email_member) do
+    Fabricate(:member,
+              organization: test_organization,
+              manager: false)
+  end
+  let (:empty_email_member) do
+    Fabricate(:member,
+              organization: test_organization,
+              manager: false)
+  end
+
   let! (:user) { member.user }
   let! (:another_user) { another_member.user }
   let! (:admin_user) { member_admin.user }
+  let! (:wrong_user) { wrong_email_member.user }
+  let! (:empty_email_user) { empty_email_member.user }
+
   include_context "stub browser locale"
   before { set_browser_locale("ca") }
 
   describe "GET #index" do
     context "with an normal logged user" do
       it "populates and array of users" do
-        login(member.user)
+        login(user)
 
         get "index"
-        expect(assigns(:users)).to eq([user, another_user, admin_user])
+        expect(assigns(:users)).to eq([user, another_user,
+                                       admin_user, wrong_user,
+                                       empty_email_user])
       end
     end
     context "with an admin logged user" do
       it "populates and array of users" do
-        login(member_admin.user)
+        login(admin_user)
 
         get "index"
-        expect(assigns(:users)).to eq([user, another_user, admin_user])
+        expect(assigns(:users)).to eq([user, another_user,
+                                       admin_user, wrong_user,
+                                       empty_email_user])
       end
     end
   end
@@ -46,7 +64,7 @@ describe UsersController do
     context "with valid params" do
       context "with a normal logged user" do
         it "assigns the requested user to @user" do
-          login(member.user)
+          login(user)
 
           get "show", id: user.id
           expect(assigns(:user)).to eq(user)
@@ -54,7 +72,7 @@ describe UsersController do
       end
       context "with an admin logged user" do
         it "assigns the requested user to @user" do
-          login(member_admin.user)
+          login(admin_user)
 
           get "show", id: user.id
           expect(assigns(:user)).to eq(user)
@@ -76,13 +94,29 @@ describe UsersController do
       end
 
       context "with an admin logged user" do
+        before { login(admin_user) }
+
         it "creates a new user" do
-          login(member_admin.user)
-
           expect { subject }.to change(User, :count).by(1)
-
           subject.should redirect_to("/members")
+        end
 
+        it "can create a user with a valid email" do
+          subject { post "create", user: user }
+          user.valid?
+          user.errors[:email].count.should == 0
+        end
+
+        # TODO: To complete, now failing
+        it "can create a user with empty email and generates dummy email" do
+          empty_email_user[:email] = ""
+          subject { post "create", user: empty_email_user }
+          empty_email_user.valid?
+          #expect { subject }.to change(User, :count).by(1)
+          #empty_email_user.email.should match(/(user)\d+(@example.com)/)
+          #subject.should redirect_to("/members")
+          empty_email_user.errors[:email].count.should == 0
+          #user.errors[:email].count.should == 0
         end
       end
     end
@@ -91,18 +125,14 @@ describe UsersController do
   describe "PUT #update" do
     context "with valid params" do
       context "with a logged" do
-
         context "normal user" do
+          before { login(member.user) }
           it "locates the requested @user" do
-            login(member.user)
-
             put "update", id: user.id, user: Fabricate.to_params(:user)
             expect(assigns(:user)).to eq(user)
           end
 
           it "changes @user's own attributes" do
-            login(member.user)
-
             put "update",
                 id: user.id,
                 user: Fabricate.to_params(:user,
@@ -117,8 +147,6 @@ describe UsersController do
           end
 
           it "cannot change another user's attributes" do
-            login(member.user)
-
             put "update",
                 id: another_user.id,
                 user: Fabricate.to_params(:user,
@@ -134,16 +162,14 @@ describe UsersController do
         end
 
         context "admin user" do
-          it "locates the requested @user" do
-            login(member_admin.user)
+          before { login(admin_user) }
 
+          it "locates the requested @user" do
             put "update", id: user.id, user: Fabricate.to_params(:user)
             expect(assigns(:user)).to eq(user)
           end
 
           it "changes @user's attributes" do
-            login(member_admin.user)
-
             put "update",
                 id: user.id,
                 user: Fabricate.to_params(:user,
@@ -162,9 +188,9 @@ describe UsersController do
 
     context "with invalid params" do
       context "with a logged admin user" do
-        it "does not change @user's attributes" do
-          login(member_admin.user)
+        before { login(admin_user) }
 
+        it "does not change @user's attributes" do
           put :update,
               id: user.id,
               user: Fabricate.to_params(:user,
@@ -176,6 +202,20 @@ describe UsersController do
           expect(user.phone).not_to eq("1234")
           expect(user.alt_phone).not_to eq("4321")
         end
+
+        it "cannot create a user with invalid email" do
+          wrong_user[:email] = "sin mail"
+          subject { post "create", user: wrong_user }
+          wrong_user.valid?
+          wrong_user.errors[:email].count.should > 0
+        end
+
+        it "cannot create a user with dummy @example.com" do
+          user[:email] = "@example.com"
+          subject { post "create", user: user }
+          user.valid?
+          user.errors[:email].count.should > 0
+        end
       end
     end
   end

spec/models/user_spec.rb

@@ -0,0 +1,5 @@
+require 'spec_helper'
+
+describe User do
+  pending "add some examples to (or delete) #{__FILE__}"
+end