Merge pull request #2725 from mkllnk/2699-email-confirm-jobs

2699 email confirm jobs

app/assets/javascripts/templates/signup.html.haml

@@ -4,6 +4,9 @@
       .large-12.columns
         .alert-box.success{ng: {show: 'messages != null'}}
           {{ messages }}
+      .large-12.columns
+        .alert-box.alert{ng: {show: 'errors.message != null'}}
+          {{ errors.message }}
     .row
       .large-12.columns
         %label{for: "email"} {{'signup_email' | t}}

app/controllers/user_registrations_controller.rb

@@ -1,4 +1,8 @@
+require 'open_food_network/error_logger'
+
 class UserRegistrationsController < Spree::UserRegistrationsController
+  I18N_SCOPE = 'devise.user_registrations.spree_user'.freeze
+
   before_filter :set_checkout_redirect, only: :create
 
   # POST /resource/sign_up
@@ -19,14 +23,23 @@ def create
         end
       end
     else
-      clean_up_passwords(resource)
-      respond_to do |format|
-        format.html do
-          render :new
-        end
-        format.js do
-          render json: @user.errors, status: :unauthorized
-        end
+      render_error(@user.errors)
+    end
+  rescue StandardError => error
+    OpenFoodNetwork::ErrorLogger.notify(error)
+    render_error(message: I18n.t('unknown_error', scope: I18N_SCOPE))
+  end
+
+  private
+
+  def render_error(errors = {})
+    clean_up_passwords(resource)
+    respond_to do |format|
+      format.html do
+        render :new
+      end
+      format.js do
+        render json: errors, status: :unauthorized
       end
     end
   end

app/models/spree/user_decorator.rb

@@ -24,8 +24,6 @@
 
   # We use the same options as Spree and add :confirmable
   devise :confirmable, reconfirmable: true
-  handle_asynchronously :send_confirmation_instructions
-  handle_asynchronously :send_on_create_confirmation_instructions
   # TODO: Later versions of devise have a dedicated after_confirmation callback, so use that
   after_update :welcome_after_confirm, if: lambda { confirmation_token_changed? && confirmation_token.nil? }
 

config/locales/en.yml

@@ -79,6 +79,7 @@ en:
     user_registrations:
       spree_user:
        signed_up_but_unconfirmed: "A message with a confirmation link has been sent to your email address. Please open the link to activate your account."
+       unknown_error: "Something went wrong while creating your account. Check your email address and try again."
     failure:
       invalid: |
         Invalid email or password.

db/seeds.rb

@@ -2,6 +2,29 @@
 # The data can then be loaded with the rake db:seed (or created alongside the db with db:setup).
 require 'yaml'
 
+def create_mail_method
+  Spree::MailMethod.destroy_all
+
+  CreateMailMethod.new(
+    environment: Rails.env,
+    preferred_enable_mail_delivery: true,
+    preferred_mail_host: ENV.fetch('MAIL_HOST'),
+    preferred_mail_domain: ENV.fetch('MAIL_DOMAIN'),
+    preferred_mail_port: ENV.fetch('MAIL_PORT'),
+    preferred_mail_auth_type: 'login',
+    preferred_smtp_username: ENV.fetch('SMTP_USERNAME'),
+    preferred_smtp_password: ENV.fetch('SMTP_PASSWORD'),
+    preferred_secure_connection_type: ENV.fetch('MAIL_SECURE_CONNECTION', 'None'),
+    preferred_mails_from: ENV.fetch('MAILS_FROM', "no-reply@#{ENV.fetch('MAIL_DOMAIN')}"),
+    preferred_mail_bcc: ENV.fetch('MAIL_BCC', ''),
+    preferred_intercept_email: ''
+  ).call
+end
+
+# We need a mail method to create a user account, because it sends a
+# confirmation email.
+create_mail_method
+
 # -- Spree
 unless Spree::Country.find_by_iso(ENV['DEFAULT_COUNTRY_CODE'])
   puts "[db:seed] Seeding Spree"
@@ -30,26 +53,5 @@
   end
 end
 
-def create_mail_method
-  Spree::MailMethod.destroy_all
-
-  CreateMailMethod.new(
-    environment: Rails.env,
-    preferred_enable_mail_delivery: true,
-    preferred_mail_host: ENV.fetch('MAIL_HOST'),
-    preferred_mail_domain: ENV.fetch('MAIL_DOMAIN'),
-    preferred_mail_port: ENV.fetch('MAIL_PORT'),
-    preferred_mail_auth_type: 'login',
-    preferred_smtp_username: ENV.fetch('SMTP_USERNAME'),
-    preferred_smtp_password: ENV.fetch('SMTP_PASSWORD'),
-    preferred_secure_connection_type: ENV.fetch('MAIL_SECURE_CONNECTION', 'None'),
-    preferred_mails_from: ENV.fetch('MAILS_FROM', "no-reply@#{ENV.fetch('MAIL_DOMAIN')}"),
-    preferred_mail_bcc: ENV.fetch('MAIL_BCC', ''),
-    preferred_intercept_email: ''
-  ).call
-end
-
-create_mail_method
-
 spree_user = Spree::User.first
 spree_user && spree_user.confirm!

lib/open_food_network/error_logger.rb

@@ -0,0 +1,13 @@
+# Our error logging API currently wraps Bugsnag.
+# It makes us more flexible if we wanted to replace Bugsnag or change logging
+# behaviour.
+module OpenFoodNetwork
+  module ErrorLogger
+    # Tries to escalate the error to a developer.
+    # If Bugsnag is configured, it will notify it. It would be nice to implement
+    # some kind of fallback.
+    def self.notify(error)
+      Bugsnag.notify(error)
+    end
+  end
+end

spec/controllers/admin/manager_invitations_controller_spec.rb

@@ -25,13 +25,14 @@ module Admin
 
       context "signing up a new user" do
         before do
+          create(:mail_method)
           controller.stub spree_current_user: admin
         end
 
         it "creates a new user, sends an invitation email, and returns the user id" do
           expect do
             spree_post :create, {email: '[email protected]', enterprise_id: enterprise.id}
-          end.to enqueue_job Delayed::PerformableMethod
+          end.to send_confirmation_instructions
 
           new_user = Spree::User.find_by_email('[email protected]')
 
@@ -45,6 +46,7 @@ module Admin
     describe "with enterprise permissions" do
       context "as user with proper enterprise permissions" do
         before do
+          create(:mail_method)
           controller.stub spree_current_user: enterprise_owner
         end
 

spec/controllers/user_confirmations_controller_spec.rb

@@ -57,6 +57,8 @@
   end
 
   context "requesting confirmation instructions to be resent" do
+    before { create(:mail_method) }
+
     it "redirects the user to login" do
       spree_post :create, { spree_user: { email: unconfirmed_user.email } }
       expect(response).to redirect_to login_path
@@ -66,8 +68,7 @@
     it "sends the confirmation email" do
       expect do
         spree_post :create, { spree_user: { email: unconfirmed_user.email } }
-      end.to enqueue_job Delayed::PerformableMethod
-      expect(Delayed::Job.last.payload_object.method_name).to eq(:send_confirmation_instructions_without_delay)
+      end.to send_confirmation_instructions
     end
   end
 end

spec/controllers/user_registrations_controller_spec.rb

@@ -3,42 +3,66 @@
 
 describe UserRegistrationsController, type: :controller do
 
+  before(:all) do
+    create(:mail_method)
+  end
+
   before do
     @request.env["devise.mapping"] = Devise.mappings[:spree_user]
   end
 
   describe "via ajax" do
     render_views
-    it "returns errors when registration fails" do
+
+    let(:user_params) do
+      {
+        email: "[email protected]",
+        password: "testy123",
+        password_confirmation: "testy123"
+      }
+    end
+
+    it "returns validation errors" do
       xhr :post, :create, spree_user: {}, :use_route => :spree
-      response.status.should == 401
+      expect(response.status).to eq(401)
+      json = JSON.parse(response.body)
+      expect(json).to eq({"email" => ["can't be blank"], "password" => ["can't be blank"]})
+    end
+
+    it "returns error when emailing fails" do
+      allow(Spree::UserMailer).to receive(:confirmation_instructions).and_raise("Some error")
+      expect(OpenFoodNetwork::ErrorLogger).to receive(:notify)
+
+      xhr :post, :create, spree_user: user_params, use_route: :spree
+
+      expect(response.status).to eq(401)
       json = JSON.parse(response.body)
-      json.should == {"email" => ["can't be blank"], "password" => ["can't be blank"]}
+      expect(json).to eq({"message" => I18n.t('devise.user_registrations.spree_user.unknown_error')})
     end
 
     it "returns 200 when registration succeeds" do
-      xhr :post, :create, spree_user: {email: "[email protected]", password: "testy123", password_confirmation: "testy123"}, :use_route => :spree
-      response.status.should == 200
+      xhr :post, :create, spree_user: user_params, use_route: :spree
+      expect(response.status).to eq(200)
       json = JSON.parse(response.body)
-      json.should == {"email" => "[email protected]"}
+      expect(json).to eq({"email" => "[email protected]"})
       expect(controller.spree_current_user).to be_nil
     end
   end
 
   context "when registration fails" do
     it "renders new" do
       spree_post :create, spree_user: {}
-      response.status.should == 200
-      response.should render_template "spree/user_registrations/new"
+      expect(response.status).to eq(200)
+      expect(response).to render_template "spree/user_registrations/new"
     end
   end
 
   context "when registration succeeds" do
     context "when referer is not '/checkout'" do
       it "redirects to root" do
         spree_post :create, spree_user: {email: "[email protected]", password: "testy123", password_confirmation: "testy123"}, :use_route => :spree
-        response.should redirect_to root_path
-        assigns[:user].email.should == "[email protected]"
+        expect(response).to redirect_to root_path
+        expect(assigns[:user].email).to eq("[email protected]")
       end
     end
 
@@ -47,8 +71,8 @@
 
       it "redirects to checkout" do
         spree_post :create, spree_user: {email: "[email protected]", password: "testy123", password_confirmation: "testy123"}, :use_route => :spree
-        response.should redirect_to checkout_path
-        assigns[:user].email.should == "[email protected]"
+        expect(response).to redirect_to checkout_path
+        expect(assigns[:user].email).to eq("[email protected]")
       end
     end
   end

spec/factories.rb

@@ -513,6 +513,16 @@
     confirmation_sent_at '1970-01-01 00:00:00'
     confirmed_at '1970-01-01 00:00:01'
 
+    before(:create) do |user, evaluator|
+      if evaluator.confirmation_sent_at
+        if evaluator.confirmed_at
+          user.skip_confirmation!
+        else
+          user.skip_confirmation_notification!
+        end
+      end
+    end
+
     after(:create) do |user|
       user.spree_roles.clear # Remove admin role
     end

spec/features/admin/enterprise_roles_spec.rb

@@ -137,6 +137,7 @@
       end
 
       it "can invite unregistered users to be managers" do
+        create(:mail_method)
         find('a.button.help-modal').click
         expect(page).to have_css '#invite-manager-modal'
 

spec/features/admin/users_spec.rb

@@ -4,7 +4,10 @@
   include AuthenticationWorkflow
 
   context "as super-admin" do
-    before { quick_login_as_admin }
+    before do
+      create(:mail_method)
+      quick_login_as_admin
+    end
 
     describe "creating a user" do
       it "shows no confirmation message to start with" do
@@ -31,12 +34,11 @@
       it "displays success" do
         visit spree.edit_admin_user_path user
 
-        # The `a` element doesn't have an href, so we can't use click_link.
-        find("a", text: "Resend").click
-        expect(page).to have_text "Resend done"
-
-        # And it's successful. (testing it here for reduced test time)
-        expect(Delayed::Job.last.payload_object.method_name).to eq :send_confirmation_instructions_without_delay
+        expect do
+          # The `a` element doesn't have an href, so we can't use click_link.
+          find("a", text: "Resend").click
+          expect(page).to have_text "Resend done"
+        end.to send_confirmation_instructions
       end
     end
   end

spec/features/consumer/account/settings_spec.rb

@@ -7,6 +7,7 @@
     let(:user) { create(:user, email: '[email protected]') }
 
     before do
+      create(:mail_method)
       quick_login_as user
     end
 

spec/features/consumer/authentication_spec.rb

@@ -75,16 +75,15 @@
           end
 
           scenario "Signing up successfully" do
+            create(:mail_method)
             fill_in "Email", with: "[email protected]"
             fill_in "Choose a password", with: "test12345"
             fill_in "Confirm password", with: "test12345"
 
             expect do
               click_signup_button
               expect(page).to have_content I18n.t('devise.user_registrations.spree_user.signed_up_but_unconfirmed')
-            end.to enqueue_job Delayed::PerformableMethod
-
-            expect(Delayed::Job.last.payload_object.method_name).to eq(:send_on_create_confirmation_instructions_without_delay)
+            end.to send_confirmation_instructions
           end
         end
 

spec/features/consumer/confirm_invitation_spec.rb

@@ -8,6 +8,7 @@
     let(:user) { Spree::User.create(email: email, unconfirmed_email: email, password: "secret") }
 
     before do
+      create(:mail_method)
       user.reset_password_token = Devise.friendly_token
       user.reset_password_sent_at = Time.now.utc
       user.save!

spec/lib/open_food_network/error_logger_spec.rb

@@ -0,0 +1,14 @@
+require 'spec_helper'
+require 'open_food_network/error_logger'
+
+module OpenFoodNetwork
+  describe ErrorLogger do
+    let(:error) { StandardError.new("Test") }
+
+    it "notifies Bugsnag" do
+      expect(Bugsnag).to receive(:notify).with(error)
+
+      ErrorLogger.notify(error)
+    end
+  end
+end

spec/models/spree/user_spec.rb

@@ -72,10 +72,11 @@
 
   context "#create" do
     it "should send a confirmation email" do
+      create(:mail_method)
+
       expect do
-        create(:user, confirmed_at: nil)
-      end.to enqueue_job Delayed::PerformableMethod
-      expect(Delayed::Job.last.payload_object.method_name).to eq(:send_on_create_confirmation_instructions_without_delay)
+        create(:user, confirmation_sent_at: nil, confirmed_at: nil)
+      end.to send_confirmation_instructions
     end
 
     context "with the the same email as existing customers" do
@@ -96,6 +97,8 @@
 
   context "confirming email" do
     it "should send a welcome email" do
+      create(:mail_method)
+
       expect do
         create(:user, confirmed_at: nil).confirm!
       end.to enqueue_job ConfirmSignupJob