From 600c8fcd4c077cb4c4979f0dde52b18875249b0b Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 14 Sep 2018 15:16:09 +1000 Subject: [PATCH] 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 cf375e7e65..dec0309d2b 100644 --- a/app/models/spree/user_decorator.rb +++ b/app/models/spree/user_decorator.rb @@ -24,8 +24,6 @@ Spree.user_class.class_eval do # 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 c5e6bf35f8..6a2623220f 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 678e7ccd86..f8f38883c8 100644 --- a/spec/controllers/user_confirmations_controller_spec.rb +++ b/spec/controllers/user_confirmations_controller_spec.rb @@ -57,6 +57,8 @@ describe UserConfirmationsController, type: :controller do 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 @@ describe UserConfirmationsController, type: :controller do 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 fd4e4e1bf5..c440b54048 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -513,6 +513,16 @@ FactoryBot.modify do 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 56db2cb889..a0d075741b 100644 --- a/spec/features/admin/enterprise_roles_spec.rb +++ b/spec/features/admin/enterprise_roles_spec.rb @@ -137,6 +137,7 @@ feature %q{ 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 9af920577a..6b2cbc2177 100644 --- a/spec/features/admin/users_spec.rb +++ b/spec/features/admin/users_spec.rb @@ -4,7 +4,10 @@ feature "Managing users" do 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 @@ feature "Managing users" do 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 3806356375..383d03a4be 100644 --- a/spec/features/consumer/account/settings_spec.rb +++ b/spec/features/consumer/account/settings_spec.rb @@ -7,6 +7,7 @@ feature "Account Settings", js: true do 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 b1f8b1be1f..24a03e4d7e 100644 --- a/spec/features/consumer/authentication_spec.rb +++ b/spec/features/consumer/authentication_spec.rb @@ -75,6 +75,7 @@ feature "Authentication", js: true, retry: 3 do 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 @@ feature "Authentication", js: true, retry: 3 do 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 9fea517638..c42c2ff8f0 100644 --- a/spec/features/consumer/confirm_invitation_spec.rb +++ b/spec/features/consumer/confirm_invitation_spec.rb @@ -8,6 +8,7 @@ feature "Confirm invitation as manager" do 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 3cd4da93af..db0af97d15 100644 --- a/spec/models/spree/user_spec.rb +++ b/spec/models/spree/user_spec.rb @@ -72,10 +72,11 @@ describe Spree.user_class do 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 @@ describe Spree.user_class do 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 0000000000..8599f820e9 --- /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