From 76df4d41b71f835d0e35c041756f786ae0b577c2 Mon Sep 17 00:00:00 2001 From: Hugo Peixoto Date: Sat, 23 Mar 2024 15:22:04 +0000 Subject: [PATCH] Redo notification system Previously, saucy generated each member's future notification every day (marked as scheduled). And every day saucy would deliver every unsent notification scheduled for that day. This means that each member with expiration date in the future had ~6 notifications scheduled for them in the database. This was troublesome because if the cron job that delivers notification was down or the server was down for more than 24h, notifications were silently skipped and we had no easy way of detecting it: we had to check every member for missing sent notifications. It also had the disadvantage that we were deleting and recreating hundreds of database entries for no good reason. To fix this, the new system no longer stores future scheduled notifications. Instead, it now only stores sent notifications and notifications currently being delivered. Every day, the deliver task checks every member if there's a notification that needs to be sent in that day, by checking if we're past the date of sending a particular notification type and checking if in that window of time no notifications of that type have been sent. Let's suppose we send 6 notifications, one per month starting 60 days before the membership expires until 90 days after: -60d -30d t=0 30d 60d 90d ----|------|------|------|------|------|------ A B C D E F If we're on day t=-60d, we need to deliver a notification of type A. But first we check if no notifications of type A have been sent to that member on that day. From days t=-59d to t=-31d, we check if in the time range t=[-60d, -31d] any notification of type A was sent to that member. If not, we need to deliver it. If we're on days t=[-30d, -1d], we do the same but for notification of type B. --- app/controllers/members_controller.rb | 12 +++ app/controllers/notifications_controller.rb | 3 +- app/mailers/notification_mailer.rb | 18 ++-- app/models/member.rb | 70 +++++++------ app/models/notification.rb | 30 +++--- .../expiration_in_30d.html.erb | 2 +- .../expiration_in_30d.text.erb | 2 +- .../expiration_in_60d.html.erb | 2 +- .../expiration_in_60d.text.erb | 2 +- .../notification_mailer/expired.html.erb | 2 +- .../notification_mailer/expired.text.erb | 2 +- .../expired_30d_ago.html.erb | 2 +- .../expired_30d_ago.text.erb | 2 +- .../expired_60d_ago.html.erb | 2 +- .../expired_60d_ago.text.erb | 2 +- app/views/notifications/index.html.erb | 83 ++++++---------- lib/tasks/saucy.rake | 2 - test/models/member_test.rb | 99 ++++++++++++------- 18 files changed, 175 insertions(+), 162 deletions(-) diff --git a/app/controllers/members_controller.rb b/app/controllers/members_controller.rb index bc1afce..eefedd7 100644 --- a/app/controllers/members_controller.rb +++ b/app/controllers/members_controller.rb @@ -31,6 +31,12 @@ class MembersController < ApplicationController @member.generate_missing_ifthenpay_links! @member.reset_status! NotificationMailer.with(member: @member).registration.deliver_now! + @member.notifications.create!( + template: "registration", + to_be_sent_on: Date.today, + sent_at: Time.current, + status: "sent", + ) redirect_to @member, notice: "Member was successfully created." else @@ -40,6 +46,12 @@ class MembersController < ApplicationController def resend_registration NotificationMailer.with(member: @member).registration.deliver_now! + @member.notifications.create!( + template: "registration", + to_be_sent_on: Date.today, + sent_at: Time.current, + status: "sent", + ) redirect_to @member, notice: "Payment reminder sent." end diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index c3a16f8..d5acfc8 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -4,8 +4,7 @@ class NotificationsController < ApplicationController # GET /notifications def index - @scheduled = Notification.where(status: 'scheduled').order(to_be_sent_on: :asc) - @sent = Notification.where(status: 'sent').order(sent_at: :desc) + @notifications = Notification.order(to_be_sent_on: :asc) end # POST /notifications/1/deliver diff --git a/app/mailers/notification_mailer.rb b/app/mailers/notification_mailer.rb index 8353e81..c4b0391 100644 --- a/app/mailers/notification_mailer.rb +++ b/app/mailers/notification_mailer.rb @@ -2,32 +2,32 @@ class NotificationMailer < ApplicationMailer default to: ->() { @member.email } def expiration_in_60d - set_notification + set_member mail end def expiration_in_30d - set_notification + set_member mail end def expired - set_notification + set_member mail end def expired_30d_ago - set_notification + set_member mail end def expired_60d_ago - set_notification + set_member mail end def cancelled - set_notification + set_member mail end @@ -47,12 +47,6 @@ class NotificationMailer < ApplicationMailer end private - def set_notification - @notification = params[:notification] - @member = @notification.member - @payment = @notification.member.create_payment - end - def set_contribution @contribution = params[:contribution] @member = @contribution.member diff --git a/app/models/member.rb b/app/models/member.rb index b29a9bf..c3ff65f 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -65,7 +65,6 @@ class Member < ApplicationRecord if contribution.save self.handle_new_contribution(contribution, overriden_expires_on) self.reset_status! - self.regenerate_notifications if should_send_notification if is_first_contribution @@ -73,11 +72,23 @@ class Member < ApplicationRecord .with(contribution: contribution) .first_payment_confirmation .deliver_now! + self.notifications.create!( + template: "first_payment_confirmation", + to_be_sent_on: Date.today, + sent_at: Time.current, + status: "sent", + ) else NotificationMailer .with(contribution: contribution) .payment_renewal_confirmation .deliver_now! + self.notifications.create!( + template: "payment_renewal_confirmation", + to_be_sent_on: Date.today, + sent_at: Time.current, + status: "sent", + ) end end @@ -99,24 +110,29 @@ class Member < ApplicationRecord save! end - def regenerate_notifications(from=Date.today) - notifications.where(status: 'scheduled').delete_all + def todays_notification + candidate = Notification + .templates + .filter { |t| self.expires_on + t[:to_be_sent_on] <= Date.today } + .last - dates = notifications.pluck(:to_be_sent_on) + if candidate + last_sent = self + .notifications + .where(status: 'sent', template: candidate[:template]) + .where.not(sent_at: nil) + .order(:sent_at) + .last - return if expires_on.nil? - - [ - { to_be_sent_on: expires_on - 60.days, template: "expiration_in_60d" }, - { to_be_sent_on: expires_on - 30.days, template: "expiration_in_30d" }, - { to_be_sent_on: expires_on + 0.days, template: "expired" }, - { to_be_sent_on: expires_on + 30.days, template: "expired_30d_ago" }, - { to_be_sent_on: expires_on + 60.days, template: "expired_60d_ago" }, - { to_be_sent_on: expires_on + Config.payment_pending_grace_period, template: "cancelled" }, - ].reject do |n| - n[:to_be_sent_on].before?(from) || dates.include?(n[:to_be_sent_on]) - end.each do |n| - notifications.create(n.merge(status: "scheduled")) + if last_sent && self.expires_on + candidate[:to_be_sent_on] <= last_sent.sent_at + nil + else + self.notifications.new( + template: candidate[:template], + status: 'scheduled', + to_be_sent_on: Date.today, + ) + end end end @@ -137,24 +153,6 @@ class Member < ApplicationRecord end def self.reset_all_status! - Member.all.each do |member| - member.reset_status! - end - end - - def self.generate_all_missing_ifthenpay_links! - Member.all.each do |member| - member.generate_missing_ifthenpay_links! - end - end - - def self.regenerate_all_notifications - ActiveRecord::Base.transaction do - ActiveRecord::Base.connection.execute('LOCK members IN ACCESS EXCLUSIVE MODE') - - Member.all.each do |member| - member.regenerate_notifications - end - end + Member.all.each(&:reset_status!) end end diff --git a/app/models/notification.rb b/app/models/notification.rb index 63587eb..e0af780 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -1,23 +1,29 @@ class Notification < ApplicationRecord belongs_to :member - scope :scheduled_for_today, ->() { where(status: 'scheduled', to_be_sent_on: Date.today) } - def self.send_scheduled_for_today - ActiveRecord::Base.transaction do - ActiveRecord::Base.connection.execute('LOCK members IN ACCESS EXCLUSIVE MODE') + Member + .where('expires_on < ?', Date.today - self.templates.first[:to_be_sent_on]) + .filter_map(&:todays_notification) + .each(&:deliver!) + end - scheduled_for_today.each do |n| - n.deliver! - end - end + def self.templates + [ + { to_be_sent_on: -60.days, template: "expiration_in_60d" }, + { to_be_sent_on: -30.days, template: "expiration_in_30d" }, + { to_be_sent_on: 0.days, template: "expired" }, + { to_be_sent_on: 30.days, template: "expired_30d_ago" }, + { to_be_sent_on: 60.days, template: "expired_60d_ago" }, + { to_be_sent_on: Config.payment_pending_grace_period, template: "cancelled" }, + ] end def deliver! - # actually send the email. - NotificationMailer.with(notification: self).send(template).deliver_now! - - update(status: 'sent', sent_at: Time.current) # TODO: do something about failures + + save! + NotificationMailer.with(member: self.member).send(template).deliver_now! + update(status: 'sent', sent_at: Time.current) end end diff --git a/app/views/notification_mailer/expiration_in_30d.html.erb b/app/views/notification_mailer/expiration_in_30d.html.erb index bd817c8..507898c 100644 --- a/app/views/notification_mailer/expiration_in_30d.html.erb +++ b/app/views/notification_mailer/expiration_in_30d.html.erb @@ -13,7 +13,7 @@

Para estender a tua inscrição por mais um ano, pedimos que faças o pagamento - das quotas até <%= @notification.member.expires_on %>. + das quotas até <%= @member.expires_on %>.

<%= render partial: "payment", locals: { payment: @payment } %> diff --git a/app/views/notification_mailer/expiration_in_30d.text.erb b/app/views/notification_mailer/expiration_in_30d.text.erb index 69a08f2..b5ed5e5 100644 --- a/app/views/notification_mailer/expiration_in_30d.text.erb +++ b/app/views/notification_mailer/expiration_in_30d.text.erb @@ -8,7 +8,7 @@ nossas actividades, e gostaríamos de continuar a contar com a tua participação. Para estender a tua inscrição por mais um ano, pedimos que faças o pagamento -das quotas até <%= @notification.member.expires_on %>. +das quotas até <%= @member.expires_on %>. <%= render partial: "payment", locals: { payment: @payment } %> diff --git a/app/views/notification_mailer/expiration_in_60d.html.erb b/app/views/notification_mailer/expiration_in_60d.html.erb index c92d920..5d082d8 100644 --- a/app/views/notification_mailer/expiration_in_60d.html.erb +++ b/app/views/notification_mailer/expiration_in_60d.html.erb @@ -13,7 +13,7 @@

Para estender a tua inscrição por mais um ano, pedimos que faças o pagamento - das quotas até <%= @notification.member.expires_on %>. + das quotas até <%= @member.expires_on %>.

<%= render partial: "payment", locals: { payment: @payment } %> diff --git a/app/views/notification_mailer/expiration_in_60d.text.erb b/app/views/notification_mailer/expiration_in_60d.text.erb index 13ba728..e40f637 100644 --- a/app/views/notification_mailer/expiration_in_60d.text.erb +++ b/app/views/notification_mailer/expiration_in_60d.text.erb @@ -8,7 +8,7 @@ nossas actividades, e gostaríamos de continuar a contar com a tua participação. Para estender a tua inscrição por mais um ano, pedimos que faças o pagamento -das quotas até <%= @notification.member.expires_on %>. +das quotas até <%= @member.expires_on %>. <%= render partial: "payment", locals: { payment: @payment } %> diff --git a/app/views/notification_mailer/expired.html.erb b/app/views/notification_mailer/expired.html.erb index 4400eab..4450688 100644 --- a/app/views/notification_mailer/expired.html.erb +++ b/app/views/notification_mailer/expired.html.erb @@ -13,7 +13,7 @@ <%= render partial: "payment", locals: { payment: @payment } %>

- Caso não recebamos o pagamento até dia <%= @notification.member.cancelled_on + Caso não recebamos o pagamento até dia <%= @member.cancelled_on %>, cancelaremos permanentemente a tua inscrição. Estamos disponíveis para esclarecer qualquer dúvida através do endereço direccao@ansol.org.

diff --git a/app/views/notification_mailer/expired.text.erb b/app/views/notification_mailer/expired.text.erb index 441c1dc..4698901 100644 --- a/app/views/notification_mailer/expired.text.erb +++ b/app/views/notification_mailer/expired.text.erb @@ -8,7 +8,7 @@ nossas actividades. Gostaríamos de continuar a contar com a tua participação. <%= render partial: "payment", locals: { payment: @payment } %> -Caso não recebamos o pagamento até dia <%= @notification.member.cancelled_on +Caso não recebamos o pagamento até dia <%= @member.cancelled_on %>, cancelaremos permanentemente a tua inscrição. Estamos disponíveis para esclarecer qualquer dúvida através do endereço direccao@ansol.org. diff --git a/app/views/notification_mailer/expired_30d_ago.html.erb b/app/views/notification_mailer/expired_30d_ago.html.erb index 4b05df8..941b866 100644 --- a/app/views/notification_mailer/expired_30d_ago.html.erb +++ b/app/views/notification_mailer/expired_30d_ago.html.erb @@ -13,7 +13,7 @@ <%= render partial: "payment", locals: { payment: @payment } %>

- Caso não recebamos o pagamento até dia <%= @notification.member.cancelled_on + Caso não recebamos o pagamento até dia <%= @member.cancelled_on %>, cancelaremos permanentemente a tua inscrição. Estamos disponíveis para esclarecer qualquer dúvida através do endereço direccao@ansol.org.

diff --git a/app/views/notification_mailer/expired_30d_ago.text.erb b/app/views/notification_mailer/expired_30d_ago.text.erb index 628701c..6f8fd8f 100644 --- a/app/views/notification_mailer/expired_30d_ago.text.erb +++ b/app/views/notification_mailer/expired_30d_ago.text.erb @@ -8,7 +8,7 @@ nossas actividades. Gostaríamos de continuar a contar com a tua participação. <%= render partial: "payment", locals: { payment: @payment } %> -Caso não recebamos o pagamento até dia <%= @notification.member.cancelled_on +Caso não recebamos o pagamento até dia <%= @member.cancelled_on %>, cancelaremos permanentemente a tua inscrição. Estamos disponíveis para esclarecer qualquer dúvida através do endereço direccao@ansol.org. diff --git a/app/views/notification_mailer/expired_60d_ago.html.erb b/app/views/notification_mailer/expired_60d_ago.html.erb index 4400eab..4450688 100644 --- a/app/views/notification_mailer/expired_60d_ago.html.erb +++ b/app/views/notification_mailer/expired_60d_ago.html.erb @@ -13,7 +13,7 @@ <%= render partial: "payment", locals: { payment: @payment } %>

- Caso não recebamos o pagamento até dia <%= @notification.member.cancelled_on + Caso não recebamos o pagamento até dia <%= @member.cancelled_on %>, cancelaremos permanentemente a tua inscrição. Estamos disponíveis para esclarecer qualquer dúvida através do endereço direccao@ansol.org.

diff --git a/app/views/notification_mailer/expired_60d_ago.text.erb b/app/views/notification_mailer/expired_60d_ago.text.erb index f42d761..f89d984 100644 --- a/app/views/notification_mailer/expired_60d_ago.text.erb +++ b/app/views/notification_mailer/expired_60d_ago.text.erb @@ -8,7 +8,7 @@ nossas actividades. Gostaríamos de continuar a contar com a tua participação. <%= render partial: "payment", locals: { payment: @payment } %> -Caso não recebamos o pagamento até dia <%= @notification.member.cancelled_on +Caso não recebamos o pagamento até dia <%= @member.cancelled_on %>, cancelaremos permanentemente a tua inscrição. Estamos disponíveis para esclarecer qualquer dúvida através do endereço direccao@ansol.org. diff --git a/app/views/notifications/index.html.erb b/app/views/notifications/index.html.erb index 95755e8..b8c41a4 100644 --- a/app/views/notifications/index.html.erb +++ b/app/views/notifications/index.html.erb @@ -1,59 +1,32 @@

<%= t('notifications.index.title') %>

-
-

<%= t('notifications.index.scheduled') %>

 - +
+ + + + + + + + <% @notifications.each do |notification| %> - - - - + + + + + - <% @scheduled.each do |notification| %> - - - - - - - <% end %> -
<%= t('notifications.attributes.to_be_sent_on') %><%= t('notifications.attributes.status') %><%= t('notifications.attributes.template') %><%= t('notifications.attributes.member') %><%= t('members.show.contribution_actions') %>
<%= t('notifications.attributes.to_be_sent_on') %><%= t('notifications.attributes.template') %><%= t('notifications.attributes.member') %><%= t('members.show.contribution_actions') %><%= notification.to_be_sent_on %><%= notification.status %><%= notification.template %> + <% if notification.member %> + <%= link_to notification.member do %><%= notification.member.display_name %> <<%= notification.member.email %>><% end %> + <% else %> + --- + <% end %> + + <% if notification.status == 'scheduled' %> + <%= form_with url: deliver_notification_path(notification) do |form| %> + <%= form.submit t('members.show.actions.deliver_notification') %> + <% end %> + <% end %> +
<%= notification.to_be_sent_on %><%= notification.template %> - <% if notification.member %> - <%= link_to notification.member do %><%= notification.member.display_name %> <<%= notification.member.email %>><% end %> - <% else %> - --- - <% end %> -
-
- -
-

<%= t('notifications.index.sent') %>

 - - - - - - - - <% @sent.each do |notification| %> - - - - - - - <% end %> -
<%= t('notifications.attributes.to_be_sent_on') %><%= t('notifications.attributes.template') %><%= t('notifications.attributes.member') %><%= t('members.show.contribution_actions') %>
<%= notification.to_be_sent_on %><%= notification.template %> - <% if notification.member %> - <%= link_to notification.member do %><%= notification.member.display_name %> <<%= notification.member.email %>><% end %> - <% else %> - --- - <% end %> - - <% if notification.status == 'scheduled' %> - <%= form_with url: deliver_notification_path(notification) do |form| %> - <%= form.submit t('members.show.actions.deliver_notification') %> - <% end %> - <% end %> -
-
+ <% end %> + diff --git a/lib/tasks/saucy.rake b/lib/tasks/saucy.rake index ac40257..de84d94 100644 --- a/lib/tasks/saucy.rake +++ b/lib/tasks/saucy.rake @@ -2,9 +2,7 @@ desc "Application specific tasks" namespace :saucy do desc "Background sync operations" task sync: :environment do - Member.generate_all_missing_ifthenpay_links! Member.reset_all_status! - Member.regenerate_all_notifications end desc "Send daily email notifications" diff --git a/test/models/member_test.rb b/test/models/member_test.rb index 51f8bf1..0f8ce2e 100644 --- a/test/models/member_test.rb +++ b/test/models/member_test.rb @@ -18,44 +18,77 @@ class MemberTest < ActiveSupport::TestCase end end - test "expired after 1 year and 90 days" do - Timecop.freeze(Date.today + 1.year + 90.days) do - assert_equal :cancelled, @member.expected_status - end - end - - test "regenerate notifications creates all 6 notifications" do - @member.regenerate_notifications - - assert_equal 6, @member.notifications.count - end - - test "regenerate_notifications does not create notifications in the past" do - Timecop.freeze(Date.today + 1.year) do - @member.regenerate_notifications - assert_equal 4, @member.notifications.count - end - - Timecop.freeze(Date.today + 2.year) do - @member.regenerate_notifications - assert_equal 0, @member.notifications.count + test "expired after 1 year and 90 days" do + Timecop.freeze(Date.today + 1.year + 90.days) do + assert_equal :cancelled, @member.expected_status end end - test "regenerate_notifications does not create manually sent notifications" do - @member.regenerate_notifications - - @member.notifications.first.deliver! - assert_equal 5, @member.notifications.where(status: 'scheduled').count - - @member.regenerate_notifications - assert_equal 1, @member.notifications.where(status: 'sent').count - assert_equal 5, @member.notifications.where(status: 'scheduled').count + test "no notifications in the first ~10 months" do + (Date.today .. (@member.expires_on + Notification.templates.first[:to_be_sent_on] - 1.day)).each do |day| + Timecop.freeze(day) do + assert_nil @member.todays_notification + end + end end - test "regenerate_notifications when given a date generates only notifications after that date" do - @member.regenerate_notifications(Date.today + 1.year) + test "first warning notification 60~30 days before expiration" do + (@member.expires_on - 60.days .. (@member.expires_on - 31.days)).each do |day| + Timecop.freeze(day) do + assert_equal "expiration_in_60d", @member.todays_notification[:template] + end + end + end - assert_equal 4, @member.notifications.count + test "no duplicate notification" do + @member + .notifications + .create(template: "expiration_in_60d", to_be_sent_on: @member.expires_on - 60.days, status: "sent", sent_at: @member.expires_on - 60.days) + + Timecop.freeze(@member.expires_on - 60.days) do + assert_nil @member.todays_notification + end + end + + test "don't consider old notifications as duplicate" do + @member + .notifications + .create(template: "expiration_in_60d", to_be_sent_on: @member.expires_on - 60.days - 1.year, status: "sent", sent_at: @member.expires_on - 60.days - 1.year) + + Timecop.freeze(@member.expires_on - 60.days) do + assert_equal "expiration_in_60d", @member.todays_notification[:template] + end + end + + test "don't consider unsent notifications as duplicate" do + @member + .notifications + .create(template: "expiration_in_60d", to_be_sent_on: @member.expires_on - 60.days, status: "sent", sent_at: nil) + + Timecop.freeze(@member.expires_on - 60.days) do + assert_equal "expiration_in_60d", @member.todays_notification[:template] + end + end + + test "last notification ~90 days after expiration" do + Timecop.freeze(@member.expires_on + 90.days) do + assert_equal "cancelled", @member.todays_notification[:template] + end + end + + test "last notification ~95 days after expiration" do + Timecop.freeze(@member.expires_on + 95.days) do + assert_equal "cancelled", @member.todays_notification[:template] + end + end + + test "no duplicate last notification" do + @member + .notifications + .create(template: "cancelled", to_be_sent_on: @member.expires_on + 90.days, status: "sent", sent_at: @member.expires_on + 90.days) + + Timecop.freeze(@member.expires_on + 95.days) do + assert_nil @member.todays_notification + end end end