From a01cb6af46338936ce7b87b5dc4de725ef9521c9 Mon Sep 17 00:00:00 2001 From: Diego Steiner Date: Tue, 25 Jul 2023 12:27:40 +0200 Subject: [PATCH] improvement: track email devivery more accurately --- app/jobs/mail_delivery_job.rb | 2 +- app/mailers/organisation_mailer.rb | 24 +++++++++++++++++++++++- app/models/booking_question_response.rb | 12 ++++++++---- app/models/data_digest_template.rb | 2 +- app/models/notification.rb | 4 +--- spec/models/notification_spec.rb | 6 ++++-- 6 files changed, 38 insertions(+), 12 deletions(-) diff --git a/app/jobs/mail_delivery_job.rb b/app/jobs/mail_delivery_job.rb index ecea5a69..46333184 100644 --- a/app/jobs/mail_delivery_job.rb +++ b/app/jobs/mail_delivery_job.rb @@ -2,6 +2,6 @@ class MailDeliveryJob < ActionMailer::MailDeliveryJob retry_on Net::SMTPFatalError, Net::SMTPAuthenticationError, Net::ReadTimeout, wait: 30.seconds - discard_on ActiveJob::DeserializationError + # discard_on ActiveJob::DeserializationError sidekiq_options retry: 5 end diff --git a/app/mailers/organisation_mailer.rb b/app/mailers/organisation_mailer.rb index c31c2919..e258edc3 100644 --- a/app/mailers/organisation_mailer.rb +++ b/app/mailers/organisation_mailer.rb @@ -1,6 +1,9 @@ # frozen_string_literal: true class OrganisationMailer < ApplicationMailer + BOOKING_HEADER_NAME = 'X-Heimv-Booking-Id' + NOTIFICATION_HEADER_NAME = 'X-Heimv-Notification-Id' + default from: -> { (@organisation.mail_from || @organisation.email) }, reply_to: -> { @organisation.email }, charset: 'UTF-8' @@ -11,8 +14,10 @@ class OrganisationMailer < ApplicationMailer def booking_email(notification) @organisation = notification.organisation @notification = notification - attach_active_storage_attachments(notification.attachments) + return if @notification.blank? + set_headers + attach_active_storage_attachments(notification.attachments) mail(to: notification.to, cc: notification.cc, bcc: notification.bcc, subject: notification.subject) do |format| format.text { render plain: @notification.text } # rubocop:disable Rails/OutputSafety @@ -35,6 +40,11 @@ def attach_active_storage_attachments(values) end end + def set_headers + headers[NOTIFICATION_HEADER_NAME] = @notification.to_param + headers[BOOKING_HEADER_NAME] = @notification.booking.to_param + end + def set_delivery_options return unless Rails.env.production? && @organisation.smtp_settings.present? @@ -46,4 +56,16 @@ def set_organisation @organisation ||= params end # rubocop:enable Naming/MemoizedInstanceVariableName + + class DeliveryObserver + def self.delivered_email(mail) + notification = Notification.find_by(id: mail.header[NOTIFICATION_HEADER_NAME]&.value) + + # rubocop:disable Rails/SkipsModelValidations + notification&.touch(:sent_at) + # rubocop:enable Rails/SkipsModelValidations + end + end + + register_observer DeliveryObserver end diff --git a/app/models/booking_question_response.rb b/app/models/booking_question_response.rb index e8489df3..d7ed5acf 100644 --- a/app/models/booking_question_response.rb +++ b/app/models/booking_question_response.rb @@ -31,18 +31,22 @@ class BookingQuestionResponse < ApplicationRecord end def self.process_nested_attributes(booking, attributes) - existing_responses = booking.booking_question_responses.index_by(&:booking_question_id) + existing_responses = indexed_by_booking_question_id(booking) questions = BookingQuestion.applying_to_booking(booking).index_by(&:id) attributes&.values&.map do |attribute_set| question = questions[attribute_set[:booking_question_id]&.to_i] - next unless question.present? + next if question.blank? - response = existing_responses.fetch(question.id, nil) || booking.booking_question_responses.build - response&.assign_attributes(booking_question: question, value: question.cast(attribute_set[:value])) + response = existing_responses.fetch(question.id, booking.booking_question_responses.build) + response.assign_attributes(booking_question: question, value: question.cast(attribute_set[:value])) response end end + def self.indexed_by_booking_question_id(booking) + (booking&.booking_question_responses.presence || []).index_by(&:booking_question_id) + end + def self.prepare_booking(booking) booking.booking_questions = BookingQuestion.applying_to_booking(booking) end diff --git a/app/models/data_digest_template.rb b/app/models/data_digest_template.rb index 6021e15d..a9d62a26 100644 --- a/app/models/data_digest_template.rb +++ b/app/models/data_digest_template.rb @@ -81,7 +81,7 @@ def digest(period = nil) end def columns - @columns ||= (columns_config || self.class::DEFAULT_COLUMN_CONFIG).map do |config| + @columns ||= (columns_config.presence || self.class::DEFAULT_COLUMN_CONFIG).map do |config| config.symbolize_keys! column_type = config.fetch(:type, :default)&.to_sym self.class.column_types.fetch(column_type, ColumnType.new).column_from_config(config) diff --git a/app/models/notification.rb b/app/models/notification.rb index 463eece8..707a5e02 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -57,9 +57,7 @@ def deliverable? end def deliver - return false unless deliverable? && update!(sent_at: Time.zone.now) - - message_delivery.tap(&:deliver_later) + deliverable? && save && message_delivery.tap(&:deliver_later) end def attach(*files_or_documents_to_attach) diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index d74fd20c..3878e169 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -78,11 +78,13 @@ subject(:message) { notification.deliver } it do - is_expected.to be_truthy + expect(notification.sent_at).to be_nil + expect { message }.to change { ActionMailer::Base.deliveries.count }.by(1) expect(message.to).to eq(notification.to) expect(message.cc).to eq(notification.cc) expect(message.subject).to eq(notification.subject) - expect(notification.sent_at).not_to be nil + notification.reload + expect(notification.sent_at).not_to be_nil end end