From 600c8fcd4c077cb4c4979f0dde52b18875249b0b Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 14 Sep 2018 15:16:09 +1000 Subject: [PATCH 1/6] Send confirmation emails immediately Using deferred methods on the user model breaks delayed jobs when the user is deleted while the job still exists. We could create a proper job referencing a user id for sending these emails instead. But since the user has to wait for the confirmation email anyway, we can send it within the current request. This should be revised if performance becomes an issue. Sending the email directly also has the advantage that we can tell the user if emailing failed. See the following commits. This change impacts a bunch of specs as we now need a working email setup to create unconfirmed users. This commit introduces a custom matcher to unify testing for confirmation emails. --- app/models/spree/user_decorator.rb | 2 -- .../admin/manager_invitations_controller_spec.rb | 4 +++- .../user_confirmations_controller_spec.rb | 5 +++-- spec/factories.rb | 10 ++++++++++ spec/features/admin/enterprise_roles_spec.rb | 1 + spec/features/admin/users_spec.rb | 16 +++++++++------- spec/features/consumer/account/settings_spec.rb | 1 + spec/features/consumer/authentication_spec.rb | 5 ++--- .../features/consumer/confirm_invitation_spec.rb | 1 + spec/models/spree/user_spec.rb | 9 ++++++--- .../matchers/email_confirmation_matchers.rb | 12 ++++++++++++ 11 files changed, 48 insertions(+), 18 deletions(-) create mode 100644 spec/support/matchers/email_confirmation_matchers.rb diff --git a/app/models/spree/user_decorator.rb b/app/models/spree/user_decorator.rb index cf375e7e65d..dec0309d2b7 100644 --- a/app/models/spree/user_decorator.rb +++ b/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? } diff --git a/spec/controllers/admin/manager_invitations_controller_spec.rb b/spec/controllers/admin/manager_invitations_controller_spec.rb index c5e6bf35f83..6a2623220f8 100644 --- a/spec/controllers/admin/manager_invitations_controller_spec.rb +++ b/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: 'un.registered@email.com', enterprise_id: enterprise.id} - end.to enqueue_job Delayed::PerformableMethod + end.to send_confirmation_instructions new_user = Spree::User.find_by_email('un.registered@email.com') @@ -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 diff --git a/spec/controllers/user_confirmations_controller_spec.rb b/spec/controllers/user_confirmations_controller_spec.rb index 678e7ccd86f..f8f38883c8b 100644 --- a/spec/controllers/user_confirmations_controller_spec.rb +++ b/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 diff --git a/spec/factories.rb b/spec/factories.rb index fd4e4e1bf54..c440b54048c 100644 --- a/spec/factories.rb +++ b/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 diff --git a/spec/features/admin/enterprise_roles_spec.rb b/spec/features/admin/enterprise_roles_spec.rb index 56db2cb8896..a0d075741b3 100644 --- a/spec/features/admin/enterprise_roles_spec.rb +++ b/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' diff --git a/spec/features/admin/users_spec.rb b/spec/features/admin/users_spec.rb index 9af920577ac..6b2cbc21777 100644 --- a/spec/features/admin/users_spec.rb +++ b/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 diff --git a/spec/features/consumer/account/settings_spec.rb b/spec/features/consumer/account/settings_spec.rb index 38063563754..383d03a4bed 100644 --- a/spec/features/consumer/account/settings_spec.rb +++ b/spec/features/consumer/account/settings_spec.rb @@ -7,6 +7,7 @@ let(:user) { create(:user, email: 'old@email.com') } before do + create(:mail_method) quick_login_as user end diff --git a/spec/features/consumer/authentication_spec.rb b/spec/features/consumer/authentication_spec.rb index b1f8b1be1fa..24a03e4d7e4 100644 --- a/spec/features/consumer/authentication_spec.rb +++ b/spec/features/consumer/authentication_spec.rb @@ -75,6 +75,7 @@ end scenario "Signing up successfully" do + create(:mail_method) fill_in "Email", with: "test@foo.com" fill_in "Choose a password", with: "test12345" fill_in "Confirm password", with: "test12345" @@ -82,9 +83,7 @@ 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 diff --git a/spec/features/consumer/confirm_invitation_spec.rb b/spec/features/consumer/confirm_invitation_spec.rb index 9fea517638a..c42c2ff8f04 100644 --- a/spec/features/consumer/confirm_invitation_spec.rb +++ b/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! diff --git a/spec/models/spree/user_spec.rb b/spec/models/spree/user_spec.rb index 3cd4da93af4..db0af97d15e 100644 --- a/spec/models/spree/user_spec.rb +++ b/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 diff --git a/spec/support/matchers/email_confirmation_matchers.rb b/spec/support/matchers/email_confirmation_matchers.rb new file mode 100644 index 00000000000..8599f820e92 --- /dev/null +++ b/spec/support/matchers/email_confirmation_matchers.rb @@ -0,0 +1,12 @@ +RSpec::Matchers.define :send_confirmation_instructions do + match do |event_proc| + expect(&event_proc).to change { ActionMailer::Base.deliveries.count }.by 1 + + message = ActionMailer::Base.deliveries.last + expect(message.subject).to eq "Please confirm your OFN account" + end + + def supports_block_expectations? + true + end +end From 3ae073dce598e9ff5c3fcd9d8ef240ca2ef4538e Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 14 Sep 2018 15:29:18 +1000 Subject: [PATCH 2/6] Convert specs to RSpec 3.7.1 syntax with Transpec This conversion is done by Transpec 3.3.0 with the following command: transpec spec/controllers/user_registrations_controller_spec.rb * 10 conversions from: obj.should to: expect(obj).to * 7 conversions from: == expected to: eq(expected) For more details: https://github.com/yujinakayama/transpec#supported-conversions --- .../user_registrations_controller_spec.rb | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/spec/controllers/user_registrations_controller_spec.rb b/spec/controllers/user_registrations_controller_spec.rb index 578ff272142..165a35d8549 100644 --- a/spec/controllers/user_registrations_controller_spec.rb +++ b/spec/controllers/user_registrations_controller_spec.rb @@ -11,16 +11,16 @@ render_views it "returns errors when registration fails" do xhr :post, :create, spree_user: {}, :use_route => :spree - response.status.should == 401 + 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({"email" => ["can't be blank"], "password" => ["can't be blank"]}) end it "returns 200 when registration succeeds" do xhr :post, :create, spree_user: {email: "test@test.com", password: "testy123", password_confirmation: "testy123"}, :use_route => :spree - response.status.should == 200 + expect(response.status).to eq(200) json = JSON.parse(response.body) - json.should == {"email" => "test@test.com"} + expect(json).to eq({"email" => "test@test.com"}) expect(controller.spree_current_user).to be_nil end end @@ -28,8 +28,8 @@ 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 @@ -37,8 +37,8 @@ context "when referer is not '/checkout'" do it "redirects to root" do spree_post :create, spree_user: {email: "test@test.com", password: "testy123", password_confirmation: "testy123"}, :use_route => :spree - response.should redirect_to root_path - assigns[:user].email.should == "test@test.com" + expect(response).to redirect_to root_path + expect(assigns[:user].email).to eq("test@test.com") end end @@ -47,8 +47,8 @@ it "redirects to checkout" do spree_post :create, spree_user: {email: "test@test.com", password: "testy123", password_confirmation: "testy123"}, :use_route => :spree - response.should redirect_to checkout_path - assigns[:user].email.should == "test@test.com" + expect(response).to redirect_to checkout_path + expect(assigns[:user].email).to eq("test@test.com") end end end From 17d951f99d1e49ca70c66d9d1157611f6947d875 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 14 Sep 2018 17:48:40 +1000 Subject: [PATCH 3/6] Rescue from any sign-up errors The most common failure would happen when sending the confirmation email triggered by `user.save`. We rescue any errors here and give feedback to the user. This allows for immediate feedback when the user types an email address that is not accepted by our mail server or the email setup is not configured properly. --- .../javascripts/templates/signup.html.haml | 3 +++ .../user_registrations_controller.rb | 24 +++++++++++------ config/locales/en.yml | 1 + .../user_registrations_controller_spec.rb | 27 +++++++++++++++++-- 4 files changed, 45 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/templates/signup.html.haml b/app/assets/javascripts/templates/signup.html.haml index f2d480c8b9c..51aa9d7fbca 100644 --- a/app/assets/javascripts/templates/signup.html.haml +++ b/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}} diff --git a/app/controllers/user_registrations_controller.rb b/app/controllers/user_registrations_controller.rb index b7001ab78b1..4827a9a0b07 100644 --- a/app/controllers/user_registrations_controller.rb +++ b/app/controllers/user_registrations_controller.rb @@ -19,14 +19,22 @@ 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 + render_error(message: I18n.t('devise.user_registrations.spree_user.unknown_error')) + 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 diff --git a/config/locales/en.yml b/config/locales/en.yml index 6f6550befae..a289a1e4b68 100644 --- a/config/locales/en.yml +++ b/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. diff --git a/spec/controllers/user_registrations_controller_spec.rb b/spec/controllers/user_registrations_controller_spec.rb index 165a35d8549..505a1846e2d 100644 --- a/spec/controllers/user_registrations_controller_spec.rb +++ b/spec/controllers/user_registrations_controller_spec.rb @@ -3,21 +3,44 @@ 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: "test@test.com", + password: "testy123", + password_confirmation: "testy123" + } + end + + it "returns validation errors" do xhr :post, :create, spree_user: {}, :use_route => :spree 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") + + xhr :post, :create, spree_user: user_params, use_route: :spree + + expect(response.status).to eq(401) + json = JSON.parse(response.body) + 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: "test@test.com", password: "testy123", password_confirmation: "testy123"}, :use_route => :spree + xhr :post, :create, spree_user: user_params, use_route: :spree expect(response.status).to eq(200) json = JSON.parse(response.body) expect(json).to eq({"email" => "test@test.com"}) From 9dcc683dc03428a03c68cc63fd6c9994c10c77f7 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 17 Sep 2018 10:50:24 +1000 Subject: [PATCH 4/6] Notify Bugsnag on sign-up errors This may lead to more error reports than we want to see. A not existing email address may cause Bugsnag to be notified. If this happens, we can rescue form these specific errors and only report the rest. --- app/controllers/user_registrations_controller.rb | 5 ++++- lib/open_food_network/error_logger.rb | 13 +++++++++++++ .../user_registrations_controller_spec.rb | 1 + spec/lib/open_food_network/error_logger_spec.rb | 14 ++++++++++++++ 4 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 lib/open_food_network/error_logger.rb create mode 100644 spec/lib/open_food_network/error_logger_spec.rb diff --git a/app/controllers/user_registrations_controller.rb b/app/controllers/user_registrations_controller.rb index 4827a9a0b07..654fe1791ef 100644 --- a/app/controllers/user_registrations_controller.rb +++ b/app/controllers/user_registrations_controller.rb @@ -1,3 +1,5 @@ +require 'open_food_network/error_logger' + class UserRegistrationsController < Spree::UserRegistrationsController before_filter :set_checkout_redirect, only: :create @@ -21,7 +23,8 @@ def create else render_error(@user.errors) end - rescue StandardError + rescue StandardError => error + OpenFoodNetwork::ErrorLogger.notify(error) render_error(message: I18n.t('devise.user_registrations.spree_user.unknown_error')) end diff --git a/lib/open_food_network/error_logger.rb b/lib/open_food_network/error_logger.rb new file mode 100644 index 00000000000..a1199c9e68f --- /dev/null +++ b/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 diff --git a/spec/controllers/user_registrations_controller_spec.rb b/spec/controllers/user_registrations_controller_spec.rb index 505a1846e2d..cb9d2787744 100644 --- a/spec/controllers/user_registrations_controller_spec.rb +++ b/spec/controllers/user_registrations_controller_spec.rb @@ -31,6 +31,7 @@ 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 diff --git a/spec/lib/open_food_network/error_logger_spec.rb b/spec/lib/open_food_network/error_logger_spec.rb new file mode 100644 index 00000000000..f1cd326fa28 --- /dev/null +++ b/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 From af1ac333df533960288dd8f4f54b7eff44e31759 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 17 Sep 2018 15:31:42 +1000 Subject: [PATCH 5/6] Create MailMethod before User when seeding --- db/seeds.rb | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/db/seeds.rb b/db/seeds.rb index d285cbda014..ddc3d76374d 100644 --- a/db/seeds.rb +++ b/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! From f0021be53c18cfc1193942e2750c4c095beb2313 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 27 Sep 2018 11:34:09 +1000 Subject: [PATCH 6/6] Style I18n call --- app/controllers/user_registrations_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/user_registrations_controller.rb b/app/controllers/user_registrations_controller.rb index 654fe1791ef..cc43badbb81 100644 --- a/app/controllers/user_registrations_controller.rb +++ b/app/controllers/user_registrations_controller.rb @@ -1,6 +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 @@ -25,7 +27,7 @@ def create end rescue StandardError => error OpenFoodNetwork::ErrorLogger.notify(error) - render_error(message: I18n.t('devise.user_registrations.spree_user.unknown_error')) + render_error(message: I18n.t('unknown_error', scope: I18N_SCOPE)) end private