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

@@ -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

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)
+    @notifications = Notification.order(to_be_sent_on: :asc)
-    @sent = Notification.where(status: 'sent').order(sent_at: :desc)
   end
 
   # POST /notifications/1/deliver

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

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)
+  def todays_notification
-    notifications.where(status: 'scheduled').delete_all
+    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?
+      if last_sent && self.expires_on + candidate[:to_be_sent_on] <= last_sent.sent_at
-
+        nil
-    [
+      else
-      { to_be_sent_on: expires_on - 60.days, template: "expiration_in_60d" },
+        self.notifications.new(
-      { to_be_sent_on: expires_on - 30.days, template: "expiration_in_30d" },
+          template: candidate[:template],
-      { to_be_sent_on: expires_on +  0.days, template: "expired" },
+          status: 'scheduled',
-      { to_be_sent_on: expires_on + 30.days, template: "expired_30d_ago" },
+          to_be_sent_on: Date.today,
-      { 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" },
+      end
-    ].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"))
     end
   end
 
@@ -137,24 +153,6 @@ class Member < ApplicationRecord
   end
 
   def self.reset_all_status!
-    Member.all.each do |member|
+    Member.all.each(&:reset_status!)
-      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
   end
 end

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
+    Member
-      ActiveRecord::Base.connection.execute('LOCK members IN ACCESS EXCLUSIVE MODE')
+      .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|
+  def self.templates
-        n.deliver!
+    [
-      end
+      { to_be_sent_on: -60.days, template: "expiration_in_60d" },
-    end
+      { 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

app/views/notification_mailer/expiration_in_30d.html.erb

@@ -13,7 +13,7 @@
 
 <p>
   Para estender a tua inscrição por mais um ano, pedimos que faças o pagamento
-  das quotas até <strong><%= @notification.member.expires_on %></strong>.
+  das quotas até <strong><%= @member.expires_on %></strong>.
 </p>
 
 <%= render partial: "payment", locals: { payment: @payment } %>

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

app/views/notification_mailer/expiration_in_60d.html.erb

@@ -13,7 +13,7 @@
 
 <p>
   Para estender a tua inscrição por mais um ano, pedimos que faças o pagamento
-  das quotas até <strong><%= @notification.member.expires_on %></strong>.
+  das quotas até <strong><%= @member.expires_on %></strong>.
 </p>
 
 <%= render partial: "payment", locals: { payment: @payment } %>

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

app/views/notification_mailer/expired.html.erb

@@ -13,7 +13,7 @@
 <%= render partial: "payment", locals: { payment: @payment } %>
 
 <p>
-  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.
 </p>

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.
 

app/views/notification_mailer/expired_30d_ago.html.erb

@@ -13,7 +13,7 @@
 <%= render partial: "payment", locals: { payment: @payment } %>
 
 <p>
-  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.
 </p>

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.
 

app/views/notification_mailer/expired_60d_ago.html.erb

@@ -13,7 +13,7 @@
 <%= render partial: "payment", locals: { payment: @payment } %>
 
 <p>
-  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.
 </p>

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.
 

app/views/notifications/index.html.erb

@@ -1,59 +1,32 @@
 <h1><%= t('notifications.index.title') %></h1>
 
-<details>
+<table class='zebra'>
-  <summary><h2 style="display: inline"><%= t('notifications.index.scheduled') %></h2></summary>
+  <tr>
-  <table class='zebra'>
+    <th><%= t('notifications.attributes.to_be_sent_on') %></th>
+    <th><%= t('notifications.attributes.status') %></th>
+    <th><%= t('notifications.attributes.template') %></th>
+    <th><%= t('notifications.attributes.member') %></th>
+    <th><%= t('members.show.contribution_actions') %></th>
+  </tr>
+  <% @notifications.each do |notification| %>
     <tr>
-      <th><%= t('notifications.attributes.to_be_sent_on') %></th>
+      <td><%= notification.to_be_sent_on %></td>
-      <th><%= t('notifications.attributes.template') %></th>
+      <td><%= notification.status %></td>
-      <th><%= t('notifications.attributes.member') %></th>
+      <td><code><%= notification.template %></code></td>
-      <th><%= t('members.show.contribution_actions') %></th>
+      <td>
+        <% if notification.member %>
+          <%= link_to notification.member do %><%= notification.member.display_name %> &lt;<%= notification.member.email %>&gt;<% end %>
+        <% else %>
+          ---
+        <% end %>
+      </td>
+      <td>
+        <% if notification.status == 'scheduled' %>
+          <%= form_with url: deliver_notification_path(notification) do |form| %>
+            <%= form.submit t('members.show.actions.deliver_notification') %>
+          <% end %>
+        <% end %>
+      </td>
     </tr>
-    <% @scheduled.each do |notification| %>
+  <% end %>
-      <tr>
+</table>
-        <td><%= notification.to_be_sent_on %></td>
-        <td><code><%= notification.template %></code></td>
-        <td>
-          <% if notification.member %>
-            <%= link_to notification.member do %><%= notification.member.display_name %> &lt;<%= notification.member.email %>&gt;<% end %>
-          <% else %>
-            ---
-          <% end %>
-        </td>
-        <td></td>
-      </tr>
-    <% end %>
-  </table>
-</details>
-
-<details>
-  <summary><h2 style="display: inline"><%= t('notifications.index.sent') %></h2></summary>
-  <table class='zebra'>
-    <tr>
-      <th><%= t('notifications.attributes.to_be_sent_on') %></th>
-      <th><%= t('notifications.attributes.template') %></th>
-      <th><%= t('notifications.attributes.member') %></th>
-      <th><%= t('members.show.contribution_actions') %></th>
-    </tr>
-    <% @sent.each do |notification| %>
-      <tr>
-        <td><%= notification.to_be_sent_on %></td>
-        <td><code><%= notification.template %></code></td>
-        <td>
-          <% if notification.member %>
-            <%= link_to notification.member do %><%= notification.member.display_name %> &lt;<%= notification.member.email %>&gt;<% end %>
-          <% else %>
-            ---
-          <% end %>
-        </td>
-        <td>
-          <% if notification.status == 'scheduled' %>
-            <%= form_with url: deliver_notification_path(notification) do |form| %>
-              <%= form.submit t('members.show.actions.deliver_notification') %>
-            <% end %>
-          <% end %>
-        </td>
-      </tr>
-    <% end %>
-  </table>
-</details>

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"

test/models/member_test.rb

@@ -18,44 +18,77 @@ class MemberTest < ActiveSupport::TestCase
     end
   end
 
-   test "expired after 1 year and 90 days" do
+  test "expired after 1 year and 90 days" do
-     Timecop.freeze(Date.today + 1.year + 90.days) do
+    Timecop.freeze(Date.today + 1.year + 90.days) do
-        assert_equal :cancelled, @member.expected_status
+      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
     end
   end
 
-  test "regenerate_notifications does not create manually sent notifications" do
+  test "no notifications in the first ~10 months" do
-    @member.regenerate_notifications
+    (Date.today .. (@member.expires_on + Notification.templates.first[:to_be_sent_on] - 1.day)).each do |day|
-
+      Timecop.freeze(day) do
-    @member.notifications.first.deliver!
+        assert_nil @member.todays_notification
-    assert_equal 5, @member.notifications.where(status: 'scheduled').count
+      end
-
+    end
-    @member.regenerate_notifications
-    assert_equal 1, @member.notifications.where(status: 'sent').count
-    assert_equal 5, @member.notifications.where(status: 'scheduled').count
   end
 
-  test "regenerate_notifications when given a date generates only notifications after that date" do
+  test "first warning notification 60~30 days before expiration" do
-    @member.regenerate_notifications(Date.today + 1.year)
+    (@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