Merge pull request #8389 from colinux/email-events

Email events: premières briques pour un monitoring — emails dispatchés par ActionMailer

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"
       }

app/controllers/manager/email_events_controller.rb

@@ -0,0 +1,4 @@
+module Manager
+  class EmailEventsController < Manager::ApplicationController
+  end
+end

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

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.

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

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

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

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

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

app/views/manager/users/emails.html.erb

@@ -77,6 +77,8 @@
   </p>
 <% end %>
 
+<%= link_to "Voir aussi les événements d'email", manager_email_events_path("search" => @user.email) %>
+
   <h2 style="font-size: 1.3em; margin: 24px 0 8px 0">Problèmes potentiel</h2>
 
   <% if @user.confirmed? %>

config/initializers/mail_observers.rb

@@ -0,0 +1,3 @@
+Rails.application.configure do
+  config.action_mailer.observers = ['EmailDeliveryObserver']
+end

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

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

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"

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

spec/models/email_event_spec.rb

@@ -0,0 +1,2 @@
+RSpec.describe EmailEvent, type: :model do
+end