From ed3c25e4294bfc75f7167ed63e30561e39ac3a97 Mon Sep 17 00:00:00 2001 From: Alexandra Date: Sun, 1 Oct 2017 17:07:26 +0200 Subject: [PATCH] spacing and small optimisations --- app/controllers/confirmations_controller.rb | 2 +- app/controllers/profiles/emails_controller.rb | 3 ++- app/models/user.rb | 14 +++++++------- app/services/emails/base_service.rb | 5 ++--- app/services/emails/confirm_service.rb | 4 ++-- app/services/emails/create_service.rb | 4 ++-- app/services/emails/destroy_service.rb | 4 ++-- app/views/profiles/emails/index.html.haml | 6 +++--- .../controllers/profiles/emails_controller_spec.rb | 7 ++++--- spec/models/email_spec.rb | 3 +++ 10 files changed, 28 insertions(+), 24 deletions(-) diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb index 51c26b9c17e..62ee9470704 100644 --- a/app/controllers/confirmations_controller.rb +++ b/app/controllers/confirmations_controller.rb @@ -10,7 +10,7 @@ class ConfirmationsController < Devise::ConfirmationsController users_almost_there_path end - def after_confirmation_path_for(resource_name, resource) + def after_confirmation_path_for(_resource_name, resource) # incoming resource can either be a :user or an :email if signed_in?(:user) after_sign_in_path_for(resource) diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb index 2c85606c271..3ffe5729c2c 100644 --- a/app/controllers/profiles/emails_controller.rb +++ b/app/controllers/profiles/emails_controller.rb @@ -2,7 +2,7 @@ class Profiles::EmailsController < Profiles::ApplicationController before_action :find_email, only: [:destroy, :resend_confirmation_instructions] def index - @primary = current_user.email + @primary_email = current_user.email @emails = current_user.emails.order_id_desc end @@ -30,6 +30,7 @@ class Profiles::EmailsController < Profiles::ApplicationController else flash[:alert] = "There was a problem sending the confirmation email" end + redirect_to profile_emails_url end diff --git a/app/models/user.rb b/app/models/user.rb index c6b2baea8e8..7a9561a04ff 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -164,7 +164,7 @@ class User < ActiveRecord::Base before_save :ensure_authentication_token, :ensure_incoming_email_token before_save :ensure_user_rights_and_limits, if: :external_changed? before_save :skip_reconfirmation!, if: ->(user) { user.email_changed? && user.read_only_attribute?(:email) } - before_save :check_for_verified_email, if: ->(user) { user.email_changed? && !user.new_record?} + before_save :check_for_verified_email, if: ->(user) { user.email_changed? && !user.new_record? } after_save :ensure_namespace_correct after_destroy :post_destroy_hook after_commit :update_emails_with_primary_email, on: :update, if: -> { previous_changes.key?('email') } @@ -223,11 +223,6 @@ class User < ActiveRecord::Base end end - # see if the new email is already a verified secondary email - def check_for_verified_email - skip_reconfirmation! if emails.find_by(email: self.email).try(:confirmed?) - end - mount_uploader :avatar, AvatarUploader has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent @@ -529,6 +524,11 @@ class User < ActiveRecord::Base errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email) end + # see if the new email is already a verified secondary email + def check_for_verified_email + skip_reconfirmation! if emails.confirmed.where(email: self.email).any? + end + # Note: the use of the Emails services will cause `saves` on the user object, running # through the callbacks again and can have side effects, such as the `previous_changes` # hash and `_was` variables getting munged. @@ -843,7 +843,7 @@ class User < ActiveRecord::Base def verified_email?(check_email) downcased = check_email.downcase - (email == downcased && primary_email_verified?) || emails.confirmed.where(email: downcased).exists? + email == downcased ? primary_email_verified? : emails.confirmed.where(email: downcased).exists? end def hook_attrs diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb index 227c87602fc..caddbf505fe 100644 --- a/app/services/emails/base_service.rb +++ b/app/services/emails/base_service.rb @@ -1,8 +1,7 @@ module Emails class BaseService - def initialize(user, opts = {}) - @user = user - @email = opts[:email] + def initialize(user, params = {}) + @user, @params = user, params.dup end end end diff --git a/app/services/emails/confirm_service.rb b/app/services/emails/confirm_service.rb index e764f18ddd0..b5301bf2b82 100644 --- a/app/services/emails/confirm_service.rb +++ b/app/services/emails/confirm_service.rb @@ -1,7 +1,7 @@ module Emails class ConfirmService < ::Emails::BaseService - def execute(email_record) - email_record.resend_confirmation_instructions + def execute(email) + email.resend_confirmation_instructions end end end diff --git a/app/services/emails/create_service.rb b/app/services/emails/create_service.rb index 0f6a1e24f02..94a841af7c3 100644 --- a/app/services/emails/create_service.rb +++ b/app/services/emails/create_service.rb @@ -1,7 +1,7 @@ module Emails class CreateService < ::Emails::BaseService - def execute(options = {}) - @user.emails.create({ email: @email }.merge(options)) + def execute(extra_params = {}) + @user.emails.create(@params.merge(extra_params)) end end end diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb index d29d7e69bde..aef863bd9d1 100644 --- a/app/services/emails/destroy_service.rb +++ b/app/services/emails/destroy_service.rb @@ -1,7 +1,7 @@ module Emails class DestroyService < ::Emails::BaseService - def execute(email_record) - email_record.destroy && update_secondary_emails! + def execute(email) + email.destroy && update_secondary_emails! end private diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml index b0dc8c526e1..df1df4f5d72 100644 --- a/app/views/profiles/emails/index.html.haml +++ b/app/views/profiles/emails/index.html.haml @@ -32,12 +32,12 @@ All email addresses will be used to identify your commits. %ul.well-list %li - = render partial: 'shared/email_with_badge', locals: { email: @primary, verified: current_user.confirmed? } + = render partial: 'shared/email_with_badge', locals: { email: @primary_email, verified: current_user.confirmed? } %span.pull-right %span.label.label-success Primary email - - if @primary === current_user.public_email + - if @primary_email === current_user.public_email %span.label.label-info Public email - - if @primary === current_user.notification_email + - if @primary_email === current_user.notification_email %span.label.label-info Notification email - @emails.each do |email| %li diff --git a/spec/controllers/profiles/emails_controller_spec.rb b/spec/controllers/profiles/emails_controller_spec.rb index 69b34db0c2e..ecf14aad54f 100644 --- a/spec/controllers/profiles/emails_controller_spec.rb +++ b/spec/controllers/profiles/emails_controller_spec.rb @@ -11,7 +11,7 @@ describe Profiles::EmailsController do let(:email_params) { { email: "add_email@example.com" } } it 'sends an email confirmation' do - expect {post(:create, { email: email_params })}.to change { ActionMailer::Base.deliveries.size } + expect { post(:create, { email: email_params }) }.to change { ActionMailer::Base.deliveries.size } expect(ActionMailer::Base.deliveries.last.to).to eq [email_params[:email]] expect(ActionMailer::Base.deliveries.last.subject).to match "Confirmation instructions" end @@ -22,13 +22,14 @@ describe Profiles::EmailsController do it 'resends an email confirmation' do email = user.emails.create(email: 'add_email@example.com') - expect {put(:resend_confirmation_instructions, { id: email })}.to change { ActionMailer::Base.deliveries.size } + + expect { put(:resend_confirmation_instructions, { id: email }) }.to change { ActionMailer::Base.deliveries.size } expect(ActionMailer::Base.deliveries.last.to).to eq [email_params[:email]] expect(ActionMailer::Base.deliveries.last.subject).to match "Confirmation instructions" end it 'unable to resend an email confirmation' do - expect {put(:resend_confirmation_instructions, { id: 1 })}.not_to change { ActionMailer::Base.deliveries.size } + expect { put(:resend_confirmation_instructions, { id: 1 }) }.not_to change { ActionMailer::Base.deliveries.size } end end end diff --git a/spec/models/email_spec.rb b/spec/models/email_spec.rb index 3fd3fe03be5..b32dd31ae6d 100644 --- a/spec/models/email_spec.rb +++ b/spec/models/email_spec.rb @@ -22,7 +22,9 @@ describe Email do it 'synchronizes the gpg keys when the email is updated' do email = user.emails.create(email: 'new@email.com') + expect(user).to receive(:update_invalid_gpg_signatures) + email.confirm end end @@ -33,6 +35,7 @@ describe Email do it 'scopes confirmed emails' do create(:email, :confirmed, user: user) create(:email, user: user) + expect(user.emails.count).to eq 2 expect(user.emails.confirmed.count).to eq 1 end -- GitLab