Add email aliases for users

Emails are used to associate commits with users. The emails
are not verified and don't have to be valid email addresses. They
are assigned on a first come, first serve basis.

Notifications are sent when an email is added.

app/controllers/profiles/emails_controller.rb

+class Profiles::EmailsController < ApplicationController
+  layout "profile"
+
+  def index
+    @primary = current_user.email
+    @emails = current_user.emails
+  end
+
+  def create
+    @email = current_user.emails.new(params[:email])
+    
+    flash[:alert] = @email.errors.full_messages.first unless @email.save
+
+    redirect_to profile_emails_url
+  end
+
+  def destroy
+    @email = current_user.emails.find(params[:id])
+    @email.destroy
+
+    respond_to do |format|
+      format.html { redirect_to profile_emails_url }
+      format.js { render nothing: true }
+    end
+  end
+end

app/helpers/commits_helper.rb

@@ -122,17 +122,18 @@ module CommitsHelper
   def commit_person_link(commit, options = {})
     source_name = commit.send "#{options[:source]}_name".to_sym
     source_email = commit.send "#{options[:source]}_email".to_sym
+    
+    user = User.find_for_commit(source_email, source_name)
+    person_name = user.nil? ? source_name : user.name
+    person_email = user.nil? ? source_email : user.email
+    
     text = if options[:avatar]
-            avatar = image_tag(avatar_icon(source_email, options[:size]), class: "avatar #{"s#{options[:size]}" if options[:size]}", width: options[:size], alt: "")
-            %Q{#{avatar} <span class="commit-#{options[:source]}-name">#{source_name}</span>}
+            avatar = image_tag(avatar_icon(person_email, options[:size]), class: "avatar #{"s#{options[:size]}" if options[:size]}", width: options[:size], alt: "")
+            %Q{#{avatar} <span class="commit-#{options[:source]}-name">#{person_name}</span>}
           else
-            source_name
+            person_name
           end
 
-    # Prefer email match over name match
-    user = User.where(email: source_email).first
-    user ||= User.where(name: source_name).first
-
     options = {
       class: "commit-#{options[:source]}-link has_tooltip",
       data: { :'original-title' => sanitize(source_email) }

app/mailers/emails/profile.rb

@@ -6,6 +6,12 @@ module Emails
       mail(to: @user.email, subject: subject("Account was created for you"))
     end
 
+    def new_email_email(email_id)
+      @email = Email.find(email_id)
+      @user = @email.user
+      mail(to: @user.email, subject: subject("Email was added to your account"))
+    end
+
     def new_ssh_key_email(key_id)
       @key = Key.find(key_id)
       @user = @key.user

app/models/email.rb

+# == Schema Information
+#
+# Table name: emails
+#
+#  id          :integer          not null, primary key
+#  user_id     :integer          not null
+#  email       :string           not null
+#  created_at  :datetime         not null
+class Email < ActiveRecord::Base
+  attr_accessible :email, :user_id
+
+  #
+  # Relations
+  #
+  belongs_to :user
+  
+  #
+  # Validations
+  #
+  validates :user_id, presence: true
+  validates :email, presence: true, email: { strict_mode: true }, uniqueness: true
+  validate :unique_email, if: ->(email) { email.email_changed? }
+  
+  before_validation :cleanup_email
+
+  def cleanup_email
+    self.email = self.email.downcase.strip
+  end
+
+  def unique_email
+    self.errors.add(:email, 'has already been taken') if User.exists?(email: self.email)
+  end
+end
\ No newline at end of file

app/models/user.rb

@@ -78,6 +78,7 @@ class User < ActiveRecord::Base
 
   # Profile
   has_many :keys, dependent: :destroy
+  has_many :emails, dependent: :destroy
 
   # Groups
   has_many :users_groups, dependent: :destroy
@@ -116,6 +117,7 @@ class User < ActiveRecord::Base
   validates :notification_level, inclusion: { in: Notification.notification_levels }, presence: true
   validate :namespace_uniq, if: ->(user) { user.username_changed? }
   validate :avatar_type, if: ->(user) { user.avatar_changed? }
+  validate :unique_email, if: ->(user) { user.email_changed? }
   validates :avatar, file_size: { maximum: 100.kilobytes.to_i }
 
   before_validation :generate_password, on: :create
@@ -184,6 +186,13 @@ class User < ActiveRecord::Base
       end
     end
     
+    def find_for_commit(email, name)
+      # Prefer email match over name match
+      User.where(email: email).first ||
+        User.joins(:emails).where(emails: { email: email }).first ||
+        User.where(name: name).first
+    end
+
     def filter filter_name
       case filter_name
       when "admins"; self.admins
@@ -250,6 +259,10 @@ class User < ActiveRecord::Base
     end
   end
 
+  def unique_email
+    self.errors.add(:email, 'has already been taken') if Email.exists?(email: self.email)
+  end
+
   # Groups user has access to
   def authorized_groups
     @authorized_groups ||= begin

app/observers/email_observer.rb

+class EmailObserver < BaseObserver
+  def after_create(email)
+    notification.new_email(email)
+  end
+end

app/services/git_push_service.rb

@@ -188,8 +188,6 @@ class GitPushService
   end
 
   def commit_user commit
-    User.where(email: commit.author_email).first ||
-      User.where(name: commit.author_name).first ||
-      user
+    User.find_for_commit(commit.author_email, commit.author_name) || user
   end
 end

app/services/notification_service.rb

@@ -17,6 +17,13 @@ class NotificationService
     end
   end
 
+  # Always notify user about email added to profile
+  def new_email(email)
+    if email.user
+      mailer.new_email_email(email.id)
+    end
+  end
+
   # When create an issue we should send next emails:
   #
   #  * issue assignee if their notification level is not Disabled

app/views/layouts/nav/_profile.html.haml

@@ -4,6 +4,10 @@
       %i.icon-home
   = nav_link(controller: :accounts) do
     = link_to "Account", profile_account_path
+  = nav_link(controller: :emails) do
+    = link_to profile_emails_path do
+      Emails
+      %span.count= current_user.emails.count + 1
   - unless current_user.ldap_user?
     = nav_link(controller: :passwords) do
       = link_to "Password", edit_profile_password_path

app/views/notify/new_email_email.html.haml

+%p
+  Hi #{@user.name}!
+%p
+  A new email was added to your account:
+%p
+  email:
+  %code= @email.email
+%p
+  If this email was added in error, you can remove it here:
+  = link_to "Emails", profile_emails_url

app/views/notify/new_email_email.text.erb

+Hi <%= @user.name %>!
+
+A new email was added to your account:
+
+email.................. <%= @email.email %>
+
+If this email was added in error, you can remove it here: <%= profile_emails_url %>

app/views/profiles/emails/index.html.haml

+%h3.page-title
+  My Email Addresses
+%p.light
+  Your
+  %b Primary Email
+  will be used for account notifications, avatar detection and web based operations, such as edits and merges. All email addresses will be used to identify your commits.
+
+.ui-box
+  .title
+    Emails (#{@emails.count + 1})
+  %ul.well-list#emails-table
+    %li
+      %strong= @primary
+      %span.label.label-success Primary Email
+    - @emails.each do |email|
+      %li
+        %strong= email.email
+        %span.cgray
+          added #{time_ago_with_tooltip(email.created_at)}
+        = link_to 'Remove', profile_email_path(email), data: { confirm: 'Are you sure?'}, method: :delete, class: 'btn btn-small btn-remove pull-right'
+
+%h3.page-title Add Email Address
+= form_for 'email', url: profile_emails_path, html: { class: 'form-horizontal' } do |f|
+  .form-group
+    = f.label :email, class: 'control-label'
+    .col-sm-10
+      = f.text_field :email, class: 'form-control'
+  .form-actions
+    = f.submit 'Add', class: 'btn btn-create'
\ No newline at end of file

config/routes.rb

@@ -124,6 +124,7 @@ Gitlab::Application.routes.draw do
         end
       end
       resources :keys
+      resources :emails, only: [:index, :create, :destroy]
       resources :groups, only: [:index] do
         member do
           delete :leave

db/migrate/20140209025651_create_emails.rb

+class CreateEmails < ActiveRecord::Migration
+  def change
+    create_table :emails do |t|
+      t.integer  :user_id, null: false
+      t.string   :email, null: false
+      
+      t.timestamps
+    end
+
+    add_index :emails, :user_id
+    add_index :emails, :email, unique: true
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20140127170938) do
+ActiveRecord::Schema.define(version: 20140209025651) do
 
   create_table "broadcast_messages", force: true do |t|
     t.text     "message",    null: false
@@ -33,6 +33,16 @@ ActiveRecord::Schema.define(version: 20140127170938) do
 
   add_index "deploy_keys_projects", ["project_id"], name: "index_deploy_keys_projects_on_project_id", using: :btree
 
+  create_table "emails", force: true do |t|
+    t.integer  "user_id",    null: false
+    t.string   "email",      null: false
+    t.datetime "created_at"
+    t.datetime "updated_at"
+  end
+
+  add_index "emails", ["email"], name: "index_emails_on_email", unique: true, using: :btree
+  add_index "emails", ["user_id"], name: "index_emails_on_user_id", using: :btree
+
   create_table "events", force: true do |t|
     t.string   "target_type"
     t.integer  "target_id"
@@ -66,8 +76,8 @@ ActiveRecord::Schema.define(version: 20140127170938) do
     t.integer  "assignee_id"
     t.integer  "author_id"
     t.integer  "project_id"
-    t.datetime "created_at"
-    t.datetime "updated_at"
+    t.datetime "created_at",               null: false
+    t.datetime "updated_at",               null: false
     t.integer  "position",     default: 0
     t.string   "branch_name"
     t.text     "description"
@@ -85,8 +95,8 @@ ActiveRecord::Schema.define(version: 20140127170938) do
 
   create_table "keys", force: true do |t|
     t.integer  "user_id"
-    t.datetime "created_at"
-    t.datetime "updated_at"
+    t.datetime "created_at",  null: false
+    t.datetime "updated_at",  null: false
     t.text     "key"
     t.string   "title"
     t.string   "type"
@@ -111,8 +121,8 @@ ActiveRecord::Schema.define(version: 20140127170938) do
     t.integer  "author_id"
     t.integer  "assignee_id"
     t.string   "title"
-    t.datetime "created_at"
-    t.datetime "updated_at"
+    t.datetime "created_at",        null: false
+    t.datetime "updated_at",        null: false
     t.integer  "milestone_id"
     t.string   "state"
     t.string   "merge_status"
@@ -164,8 +174,8 @@ ActiveRecord::Schema.define(version: 20140127170938) do
     t.text     "note"
     t.string   "noteable_type"
     t.integer  "author_id"
-    t.datetime "created_at"
-    t.datetime "updated_at"
+    t.datetime "created_at",                    null: false
+    t.datetime "updated_at",                    null: false
     t.integer  "project_id"
     t.string   "attachment"
     t.string   "line_code"
@@ -187,8 +197,8 @@ ActiveRecord::Schema.define(version: 20140127170938) do
     t.string   "name"
     t.string   "path"
     t.text     "description"
-    t.datetime "created_at"
-    t.datetime "updated_at"
+    t.datetime "created_at",                                null: false
+    t.datetime "updated_at",                                null: false
     t.integer  "creator_id"
     t.boolean  "issues_enabled",         default: true,     null: false
     t.boolean  "wall_enabled",           default: true,     null: false
@@ -239,8 +249,8 @@ ActiveRecord::Schema.define(version: 20140127170938) do
     t.text     "content",    limit: 2147483647
     t.integer  "author_id",                                    null: false
     t.integer  "project_id"
-    t.datetime "created_at"
-    t.datetime "updated_at"
+    t.datetime "created_at",                                   null: false
+    t.datetime "updated_at",                                   null: false
     t.string   "file_name"
     t.datetime "expires_at"
     t.boolean  "private",                       default: true, null: false
@@ -262,13 +272,16 @@ ActiveRecord::Schema.define(version: 20140127170938) do
     t.datetime "created_at"
   end
 
+  add_index "taggings", ["tag_id"], name: "index_taggings_on_tag_id", using: :btree
+  add_index "taggings", ["taggable_id", "taggable_type", "context"], name: "index_taggings_on_taggable_id_and_taggable_type_and_context", using: :btree
+
   create_table "tags", force: true do |t|
     t.string "name"
   end
 
   create_table "users", force: true do |t|
     t.string   "email",                  default: "",    null: false
-    t.string   "encrypted_password",     limit: 128, default: "",    null: false
+    t.string   "encrypted_password",     default: "",    null: false
     t.string   "reset_password_token"
     t.datetime "reset_password_sent_at"
     t.datetime "remember_created_at"
@@ -277,8 +290,8 @@ ActiveRecord::Schema.define(version: 20140127170938) do
     t.datetime "last_sign_in_at"
     t.string   "current_sign_in_ip"
     t.string   "last_sign_in_ip"
-    t.datetime "created_at"
-    t.datetime "updated_at"
+    t.datetime "created_at",                             null: false
+    t.datetime "updated_at",                             null: false
     t.string   "name"
     t.boolean  "admin",                  default: false, null: false
     t.integer  "projects_limit",         default: 10
@@ -313,6 +326,7 @@ ActiveRecord::Schema.define(version: 20140127170938) do
   add_index "users", ["authentication_token"], name: "index_users_on_authentication_token", unique: true, using: :btree
   add_index "users", ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true, using: :btree
   add_index "users", ["email"], name: "index_users_on_email", unique: true, using: :btree
+  add_index "users", ["extern_uid", "provider"], name: "index_users_on_extern_uid_and_provider", unique: true, using: :btree
   add_index "users", ["name"], name: "index_users_on_name", using: :btree
   add_index "users", ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true, using: :btree
   add_index "users", ["username"], name: "index_users_on_username", using: :btree
@@ -331,8 +345,8 @@ ActiveRecord::Schema.define(version: 20140127170938) do
   create_table "users_projects", force: true do |t|
     t.integer  "user_id",                        null: false
     t.integer  "project_id",                     null: false
-    t.datetime "created_at"
-    t.datetime "updated_at"
+    t.datetime "created_at",                     null: false
+    t.datetime "updated_at",                     null: false
     t.integer  "project_access",     default: 0, null: false
     t.integer  "notification_level", default: 3, null: false
   end
@@ -344,8 +358,8 @@ ActiveRecord::Schema.define(version: 20140127170938) do
   create_table "web_hooks", force: true do |t|
     t.string   "url"
     t.integer  "project_id"
-    t.datetime "created_at"
-    t.datetime "updated_at"
+    t.datetime "created_at",                                    null: false
+    t.datetime "updated_at",                                    null: false
     t.string   "type",                  default: "ProjectHook"
     t.integer  "service_id"
     t.boolean  "push_events",           default: true,          null: false

features/profile/emails.feature

+Feature: Profile Emails
+  Background:
+    Given I sign in as a user
+    And I visit profile emails page
+
+  Scenario: I should see emails
+    Then I should see my emails
+
+  Scenario: Add new email
+    Given I submit new email "my@email.com"
+    Then I should see new email "my@email.com"
+    And I should see my emails
+
+  Scenario: Add duplicate email
+    Given I submit duplicate email @user.email
+    Then I should not have @user.email added
+    And I should see my emails
+
+  Scenario: Remove email
+    Given I submit new email "my@email.com"
+    Then I should see new email "my@email.com"
+    And I should see my emails
+    Then I click link "Remove" for "my@email.com"
+    Then I should not see email "my@email.com"
+    And I should see my emails

features/project/commits/commits_user_lookup.feature

+Feature: Project Browse Commits User Lookup
+  Background:
+    Given I sign in as a user
+    And I own a project
+    And I have the user that authored the commits
+    And I visit my project's commits page
+
+  Scenario: I browse commit from list
+    Given I click on commit link
+    Then I see commit info
+
+  Scenario: I browse another commit from list
+    Given I click on another commit link
+    Then I see other commit info
\ No newline at end of file

features/steps/profile/profile_emails.rb

+class ProfileEmails < Spinach::FeatureSteps
+  include SharedAuthentication
+
+  Then 'I visit profile emails page' do
+    visit profile_emails_path
+  end
+
+  Then 'I should see my emails' do
+    page.should have_content(@user.email)
+    @user.emails.each do |email|
+      page.should have_content(email.email)
+    end
+  end
+
+  And 'I submit new email "my@email.com"' do
+    fill_in "email_email", with: "my@email.com"
+    click_button "Add"
+  end
+
+  Then 'I should see new email "my@email.com"' do
+    email = @user.emails.find_by(email: "my@email.com")
+    email.should_not be_nil
+    page.should have_content("my@email.com")
+  end
+
+  Then 'I should not see email "my@email.com"' do
+    email = @user.emails.find_by(email: "my@email.com")
+    email.should be_nil
+    page.should_not have_content("my@email.com")
+  end
+  
+  Then 'I click link "Remove" for "my@email.com"' do
+    # there should only be one remove button at this time
+    click_link "Remove"
+    # force these to reload as they have been cached
+    @user.emails.reload
+  end
+
+  And 'I submit duplicate email @user.email' do
+    fill_in "email_email", with: @user.email
+    click_button "Add"
+  end
+
+  Then 'I should not have @user.email added' do
+    email = @user.emails.find_by(email: @user.email)
+    email.should be_nil
+  end
+end

features/steps/project/project_browse_commits_user_lookup.rb

+class ProjectBrowseCommitsUserLookup < Spinach::FeatureSteps
+  include SharedAuthentication
+  include SharedProject
+  include SharedPaths
+  
+  Given 'I have the user that authored the commits' do
+    @user = create(:user, email: 'dmitriy.zaporozhets@gmail.com')
+    create(:email, { user: @user, email: 'dzaporozhets@sphereconsultinginc.com' })
+  end
+
+  Given 'I click on commit link' do
+    visit project_commit_path(@project, ValidCommit::ID)
+  end
+
+  Given 'I click on another commit link' do
+    visit project_commit_path(@project, ValidCommitWithAltEmail::ID)
+  end
+
+  Then 'I see commit info' do
+    page.should have_content ValidCommit::MESSAGE
+    check_author_link(ValidCommit::AUTHOR_EMAIL)
+  end
+  
+  Then 'I see other commit info' do
+    page.should have_content ValidCommitWithAltEmail::MESSAGE
+    check_author_link(ValidCommitWithAltEmail::AUTHOR_EMAIL)
+  end
+
+  def check_author_link(email)
+    author_link = find('.commit-author-link')
+    author_link['href'].should == user_path(@user)
+    author_link['data-original-title'].should == email
+    find('.commit-author-name').text.should == @user.name
+  end
+end

features/support/env.rb

@@ -15,7 +15,7 @@ require 'spinach/capybara'
 require 'sidekiq/testing/inline'
 
 
-%w(valid_commit big_commits select2_helper test_env).each do |f|
+%w(valid_commit valid_commit_with_alt_email big_commits select2_helper test_env).each do |f|
   require Rails.root.join('spec', 'support', f)
 end
 

spec/factories.rb

@@ -220,6 +220,19 @@ FactoryGirl.define do
     end
   end
   
+  factory :email do
+    user
+    email do
+      Faker::Internet.email('alias')
+    end
+
+    factory :another_email do
+      email do
+        Faker::Internet.email('another.alias')
+      end
+    end
+  end
+
   factory :milestone do
     title
     project

spec/mailers/notify_spec.rb

@@ -90,6 +90,28 @@ describe Notify do
     end
   end
 
+  describe 'user added email' do
+    let(:email) { create(:email) }
+
+    subject { Notify.new_email_email(email.id) }
+
+    it 'is sent to the new user' do
+      should deliver_to email.user.email
+    end
+
+    it 'has the correct subject' do
+      should have_subject /^gitlab \| Email was added to your account$/i
+    end
+
+    it 'contains the new email address' do
+      should have_body_text /#{email.email}/
+    end
+
+    it 'includes a link to emails page' do
+      should have_body_text /#{profile_emails_path}/
+    end
+  end
+
   context 'for a project' do
     describe 'items that are assignable, the email' do
       let(:assignee) { create(:user, email: 'assignee@example.com') }

spec/observers/email_observer_spec.rb

+require 'spec_helper'
+
+describe EmailObserver do
+  let(:email)      { create(:email) }
+
+  before { subject.stub(notification: double('NotificationService').as_null_object) }
+
+  subject { EmailObserver.instance }
+
+  describe '#after_create' do
+    it 'trigger notification to send emails' do
+      subject.should_receive(:notification)
+
+      subject.after_create(email)
+    end
+  end
+end

spec/routing/routing_spec.rb

@@ -183,6 +183,23 @@ describe Profiles::KeysController, "routing" do
   end
 end
 
+#   emails GET    /emails(.:format)        emails#index
+#          POST   /keys(.:format)          emails#create
+#          DELETE /keys/:id(.:format)      keys#destroy
+describe Profiles::EmailsController, "routing" do
+  it "to #index" do
+    get("/profile/emails").should route_to('profiles/emails#index')
+  end
+
+  it "to #create" do
+    post("/profile/emails").should route_to('profiles/emails#create')
+  end
+
+  it "to #destroy" do
+    delete("/profile/emails/1").should route_to('profiles/emails#destroy', id: '1')
+  end
+end
+
 # profile_avatar DELETE /profile/avatar(.:format) profiles/avatars#destroy
 describe Profiles::AvatarsController, "routing" do
   it "to #destroy" do

spec/services/notification_service_spec.rb

@@ -16,6 +16,19 @@ describe NotificationService do
     end
   end
 
+  describe 'Email' do
+    describe :new_email do
+      let(:email) { create(:email) }
+
+      it { notification.new_email(email).should be_true }
+
+      it 'should send email to email owner' do
+        Notify.should_receive(:new_email_email).with(email.id)
+        notification.new_email(email)
+      end
+    end
+  end
+
   describe 'Notes' do
     context 'issue note' do
       let(:issue) { create(:issue, assignee: create(:user)) }

spec/support/valid_commit.rb

@@ -2,6 +2,7 @@ module ValidCommit
   ID = "8470d70da67355c9c009e4401746b1d5410af2e3"
   MESSAGE = "notes controller refactored"
   AUTHOR_FULL_NAME = "Dmitriy Zaporozhets"
+  AUTHOR_EMAIL = "dmitriy.zaporozhets@gmail.com"
 
   FILES = [".foreman", ".gitignore", ".rails_footnotes", ".rspec", ".travis.yml", "CHANGELOG", "Gemfile", "Gemfile.lock", "LICENSE", "Procfile", "Procfile.production", "README.md", "Rakefile", "VERSION", "app", "config.ru", "config", "db", "doc", "lib", "log", "public", "resque.sh", "script", "spec", "vendor"]
   FILES_COUNT = 26

spec/support/valid_commit_with_alt_email.rb

+module ValidCommitWithAltEmail
+  ID = "1e689bfba39525ead225eaf611948cfbe8ac34cf"
+  MESSAGE = "fixed notes logic"
+  AUTHOR_FULL_NAME = "Dmitriy Zaporozhets"
+  AUTHOR_EMAIL = "dzaporozhets@sphereconsultinginc.com"
+end
\ No newline at end of file