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..cc43badbb81 100644 --- a/app/controllers/user_registrations_controller.rb +++ b/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 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/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/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! 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/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/controllers/user_registrations_controller_spec.rb b/spec/controllers/user_registrations_controller_spec.rb index 578ff272142..cb9d2787744 100644 --- a/spec/controllers/user_registrations_controller_spec.rb +++ b/spec/controllers/user_registrations_controller_spec.rb @@ -3,24 +3,48 @@ 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 - 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: "test@test.com", 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" => "test@test.com"} + expect(json).to eq({"email" => "test@test.com"}) expect(controller.spree_current_user).to be_nil end end @@ -28,8 +52,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 +61,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 +71,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 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/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 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