diff --git a/app/controllers/manager/application_controller.rb b/app/controllers/manager/application_controller.rb index badd07636..f529fa7cf 100644 --- a/app/controllers/manager/application_controller.rb +++ b/app/controllers/manager/application_controller.rb @@ -4,7 +4,7 @@ module Manager before_action :default_params def default_params - params[resource_name] ||= { + request.query_parameters[resource_name] ||= { order: "created_at", direction: "desc" } diff --git a/app/controllers/manager/email_events_controller.rb b/app/controllers/manager/email_events_controller.rb new file mode 100644 index 000000000..bd7b59ff7 --- /dev/null +++ b/app/controllers/manager/email_events_controller.rb @@ -0,0 +1,4 @@ +module Manager + class EmailEventsController < Manager::ApplicationController + end +end diff --git a/app/dashboards/email_event_dashboard.rb b/app/dashboards/email_event_dashboard.rb new file mode 100644 index 000000000..156e0f595 --- /dev/null +++ b/app/dashboards/email_event_dashboard.rb @@ -0,0 +1,24 @@ +require "administrate/base_dashboard" + +class EmailEventDashboard < Administrate::BaseDashboard + ATTRIBUTE_TYPES = { + id: Field::Number, + to: Field::String, + subject: Field::String, + method: Field::Enum, + status: Field::Enum, + processed_at: Field::DateTime.with_options(format: "%F %T") + } + COLLECTION_ATTRIBUTES = [:id, :to, :subject, :method, :status, :processed_at].freeze + SHOW_PAGE_ATTRIBUTES = [:id, :to, :subject, :method, :status, :processed_at].freeze + + METHODS_FILTERS = + ActionMailer::Base.delivery_methods.keys.index_with do |method| + -> (resources) { resources.where(method: method) } + end + + COLLECTION_FILTERS = { + dispatched: -> (resources) { resources.dispatched }, + dispatch_error: -> (resources) { resources.dispatch_error } + }.merge(METHODS_FILTERS).freeze +end diff --git a/app/lib/mailjet/api.rb b/app/lib/mailjet/api.rb index e97845cdf..92e135160 100644 --- a/app/lib/mailjet/api.rb +++ b/app/lib/mailjet/api.rb @@ -1,6 +1,6 @@ class Mailjet::API def properly_configured? - [Mailjet.config.api_key, Mailjet.config.secret_key].all?(&:present?) + Mailjet.respond_to?(:config) && [Mailjet.config.api_key, Mailjet.config.secret_key].all?(&:present?) end # Get messages sent to a user through SendInBlue. diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index a09385116..7fc719874 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -1,21 +1,12 @@ class ApplicationMailer < ActionMailer::Base + include MailerMonitoringConcern + helper :application # gives access to all helpers defined within `application_helper`. default from: "#{APPLICATION_NAME} <#{CONTACT_EMAIL}>" layout 'mailer' before_action :add_dolist_header - # Don’t retry to send a message if the server rejects the recipient address - rescue_from Net::SMTPSyntaxError do |_error| - message.perform_deliveries = false - end - - rescue_from Net::SMTPServerBusy do |error| - if /unexpected recipients/.match?(error.message) - message.perform_deliveries = false - end - end - # Attach the procedure logo to the email (if any). # Returns the attachment url. def attach_logo(procedure) @@ -24,18 +15,9 @@ class ApplicationMailer < ActionMailer::Base attachments.inline[logo_filename] = procedure.logo.download attachments[logo_filename].url end - rescue StandardError => e # A problem occured when reading logo, maybe the logo is missing and we should clean the procedure to remove logo reference ? Sentry.capture_exception(e, extra: { procedure_id: procedure.id }) nil end - - # mandatory for dolist - # used for tracking in Dolist UI - # the delivery_method is yet unknown (:balancer) - # so we add the dolist header for everyone - def add_dolist_header - headers['X-Dolist-Message-Name'] = action_name - end end diff --git a/app/mailers/concerns/mailer_monitoring_concern.rb b/app/mailers/concerns/mailer_monitoring_concern.rb new file mode 100644 index 000000000..8dfd84179 --- /dev/null +++ b/app/mailers/concerns/mailer_monitoring_concern.rb @@ -0,0 +1,37 @@ +module MailerMonitoringConcern + extend ActiveSupport::Concern + + included do + # Don’t retry to send a message if the server rejects the recipient address + rescue_from Net::SMTPSyntaxError do |_exception| + message.perform_deliveries = false + end + + rescue_from Net::SMTPServerBusy do |exception| + if /unexpected recipients/.match?(exception.message) + message.perform_deliveries = false + else + log_delivery_error(exception) + end + end + + rescue_from StandardError, with: :log_delivery_error + + # mandatory for dolist + # used for tracking in Dolist UI + # the delivery_method is yet unknown (:balancer) + # so we add the dolist header for everyone + def add_dolist_header + headers['X-Dolist-Message-Name'] = action_name + end + + protected + + def log_delivery_error(exception) + EmailEvent.create_from_message!(message, status: "dispatch_error") + Sentry.capture_exception(exception, extra: { to: message.to, subject: message.subject }) + + # TODO find a way to re attempt the job + end + end +end diff --git a/app/mailers/devise_user_mailer.rb b/app/mailers/devise_user_mailer.rb index 964fa2c5e..77986e421 100644 --- a/app/mailers/devise_user_mailer.rb +++ b/app/mailers/devise_user_mailer.rb @@ -3,19 +3,9 @@ class DeviseUserMailer < Devise::Mailer helper :application # gives access to all helpers defined within `application_helper`. helper MailerHelper include Devise::Controllers::UrlHelpers # Optional. eg. `confirmation_url` + include MailerMonitoringConcern layout 'mailers/layout' - # Don’t retry to send a message if the server rejects the recipient address - rescue_from Net::SMTPSyntaxError do |_error| - message.perform_deliveries = false - end - - rescue_from Net::SMTPServerBusy do |error| - if /unexpected recipients/.match?(error.message) - message.perform_deliveries = false - end - end - def template_paths ['devise_mailer'] end diff --git a/app/models/email_event.rb b/app/models/email_event.rb new file mode 100644 index 000000000..fca749ff7 --- /dev/null +++ b/app/models/email_event.rb @@ -0,0 +1,37 @@ +# == Schema Information +# +# Table name: email_events +# +# id :bigint not null, primary key +# method :string not null +# processed_at :datetime +# status :string not null +# subject :string not null +# to :string not null +# created_at :datetime not null +# updated_at :datetime not null +# +class EmailEvent < ApplicationRecord + enum status: { + dispatched: 'dispatched', + dispatch_error: 'dispatch_error' + } + + class << self + def create_from_message!(message, status:) + to = message.to || ["unset"] # no recipients when error occurs *before* setting to: in the mailer + + to.each do |recipient| + EmailEvent.create!( + to: recipient, + subject: message.subject, + processed_at: message.date, + method: ActionMailer::Base.delivery_methods.key(message.delivery_method.class), + status: + ) + rescue StandardError => error + Sentry.capture_exception(error, extra: { subject: message.subject, status: }) + end + end + end +end diff --git a/app/services/email_delivery_observer.rb b/app/services/email_delivery_observer.rb new file mode 100644 index 000000000..c297dcf85 --- /dev/null +++ b/app/services/email_delivery_observer.rb @@ -0,0 +1,5 @@ +class EmailDeliveryObserver + def self.delivered_email(message) + EmailEvent.create_from_message!(message, status: "dispatched") + end +end diff --git a/app/views/manager/users/emails.html.erb b/app/views/manager/users/emails.html.erb index 1b9991d72..44b9cdb3e 100644 --- a/app/views/manager/users/emails.html.erb +++ b/app/views/manager/users/emails.html.erb @@ -77,6 +77,8 @@

<% end %> +<%= link_to "Voir aussi les événements d'email", manager_email_events_path("search" => @user.email) %> +

Problèmes potentiel

<% if @user.confirmed? %> diff --git a/config/initializers/mail_observers.rb b/config/initializers/mail_observers.rb new file mode 100644 index 000000000..c441735c8 --- /dev/null +++ b/config/initializers/mail_observers.rb @@ -0,0 +1,3 @@ +Rails.application.configure do + config.action_mailer.observers = ['EmailDeliveryObserver'] +end diff --git a/config/routes.rb b/config/routes.rb index a6a4b6b9e..d741ad658 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -65,6 +65,8 @@ Rails.application.routes.draw do resources :team_accounts, only: [:index, :show] + resources :email_events, only: [:index, :show] + resources :dubious_procedures, only: [:index] resources :outdated_procedures, only: [:index] do patch :bulk_update, on: :collection diff --git a/db/migrate/20230109140138_create_email_events.rb b/db/migrate/20230109140138_create_email_events.rb new file mode 100644 index 000000000..90d5e7b73 --- /dev/null +++ b/db/migrate/20230109140138_create_email_events.rb @@ -0,0 +1,13 @@ +class CreateEmailEvents < ActiveRecord::Migration[6.1] + def change + create_table :email_events do |t| + t.string :to, null: false + t.string :method, null: false + t.string :status, null: false + t.string :subject, null: false + t.datetime :processed_at + + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 262138d5a..6fa8d9a5c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2022_12_27_084442) do +ActiveRecord::Schema.define(version: 2023_01_09_140138) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" @@ -407,6 +407,16 @@ ActiveRecord::Schema.define(version: 2022_12_27_084442) do t.index ["type_de_champ_id"], name: "index_drop_down_lists_on_type_de_champ_id" end + create_table "email_events", force: :cascade do |t| + t.datetime "created_at", precision: 6, null: false + t.string "method", null: false + t.datetime "processed_at" + t.string "status", null: false + t.string "subject", null: false + t.string "to", null: false + t.datetime "updated_at", precision: 6, null: false + end + create_table "etablissements", id: :serial, force: :cascade do |t| t.string "adresse" t.date "association_date_creation" diff --git a/spec/mailers/application_mailer_spec.rb b/spec/mailers/application_mailer_spec.rb index 8cde8e6fc..d81fee7e2 100644 --- a/spec/mailers/application_mailer_spec.rb +++ b/spec/mailers/application_mailer_spec.rb @@ -25,4 +25,48 @@ RSpec.describe ApplicationMailer, type: :mailer do it { expect(subject.message).not_to be_an_instance_of(ActionMailer::Base::NullMail) } end end + + describe 'EmailDeliveryObserver is invoked' do + let(:user1) { create(:user) } + let(:user2) { create(:user, email: "your@email.com") } + + it 'creates a new EmailEvent record with the correct information' do + expect { UserMailer.ask_for_merge(user1, user2.email).deliver_now }.to change { EmailEvent.count }.by(1) + event = EmailEvent.last + + expect(event.to).to eq("your@email.com") + expect(event.method).to eq("test") + expect(event.subject).to eq('Fusion de compte') + expect(event.processed_at).to be_within(1.second).of(Time.zone.now) + expect(event.status).to eq('dispatched') + end + + context "when email is not sent" do + subject(:send_email) { UserMailer.ask_for_merge(user1, user2.email).deliver_now } + + before do + allow_any_instance_of(Mail::Message) + .to receive(:deliver) + .and_raise(smtp_error) + end + + context "smtp server busy" do + let(:smtp_error) { Net::SMTPServerBusy.new } + + it "creates an event" do + expect { send_email }.to change { EmailEvent.count }.by(1) + expect(EmailEvent.last.status).to eq('dispatch_error') + end + end + + context "generic unknown error" do + let(:smtp_error) { Net::OpenTimeout.new } + + it "creates an event" do + expect { send_email }.to change { EmailEvent.count }.by(1) + expect(EmailEvent.last.status).to eq('dispatch_error') + end + end + end + end end diff --git a/spec/models/email_event_spec.rb b/spec/models/email_event_spec.rb new file mode 100644 index 000000000..cf5ad1460 --- /dev/null +++ b/spec/models/email_event_spec.rb @@ -0,0 +1,2 @@ +RSpec.describe EmailEvent, type: :model do +end