From 1eb111f52f2d832fb0bb03c2d2921c13534c2b91 Mon Sep 17 00:00:00 2001
From: Thomas Citharel <tcit@tcit.fr>
Date: Tue, 10 May 2022 13:13:48 +0200
Subject: [PATCH] Make sure activity notification recaps can't be sent multiple
 times

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
---
 lib/service/notifier/email.ex                 | 32 ++++++++++++++++---
 .../workers/send_activity_recap_worker.ex     |  3 ++
 test/service/notifier/email_test.exs          | 20 ++++++++++--
 3 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/lib/service/notifier/email.ex b/lib/service/notifier/email.ex
index fb2b2113..6fa37cc7 100644
--- a/lib/service/notifier/email.ex
+++ b/lib/service/notifier/email.ex
@@ -12,7 +12,8 @@ defmodule Mobilizon.Service.Notifier.Email do
 
   import Mobilizon.Service.DateTime,
     only: [
-      is_delay_ok_since_last_notification_sent?: 1
+      is_delay_ok_since_last_notification_sent?: 1,
+      is_delay_ok_since_last_notification_sent?: 2
     ]
 
   require Logger
@@ -35,9 +36,10 @@ defmodule Mobilizon.Service.Notifier.Email do
   def send(%User{email: email, locale: locale} = user, activities, options)
       when is_list(activities) do
     activities = Enum.filter(activities, &can_send_activity?(&1, user, options))
+    nb_activities = length(activities)
 
-    if length(activities) > 0 do
-      Logger.debug("Found some activities to send by email")
+    if nb_activities > 0 do
+      Logger.info("Sending email containing #{nb_activities} activities to #{email}")
 
       email
       |> EmailActivity.direct_activity(activities, Keyword.put(options, :locale, locale))
@@ -119,6 +121,27 @@ defmodule Mobilizon.Service.Notifier.Email do
     is_delay_ok_since_last_notification_sent?(last_notification_sent)
   end
 
+  # Delay ok since last notification
+  defp match_group_notifications_setting(
+         :one_day,
+         _,
+         %DateTime{} = last_notification_sent,
+         options
+       ) do
+    is_delay_ok_since_last_notification_sent?(last_notification_sent, 3_600 * 23) and
+      Keyword.get(options, :recap, false) != false
+  end
+
+  defp match_group_notifications_setting(
+         :one_week,
+         _,
+         %DateTime{} = last_notification_sent,
+         options
+       ) do
+    is_delay_ok_since_last_notification_sent?(last_notification_sent, 3_600 * 24 * 6) and
+      Keyword.get(options, :recap, false) != false
+  end
+
   # This is a recap
   defp match_group_notifications_setting(
          _group_notifications,
@@ -154,7 +177,8 @@ defmodule Mobilizon.Service.Notifier.Email do
   end
 
   @spec save_last_notification_time(User.t()) :: {:ok, Setting.t()} | {:error, Ecto.Changeset.t()}
-  defp save_last_notification_time(%User{id: user_id}) do
+  defp save_last_notification_time(%User{id: user_id, email: email}) do
+    Logger.debug("Saving last notification time for user #{email}")
     attrs = %{user_id: user_id, last_notification_sent: DateTime.utc_now()}
 
     case Users.get_setting(user_id) do
diff --git a/lib/service/workers/send_activity_recap_worker.ex b/lib/service/workers/send_activity_recap_worker.ex
index ec8a7b82..62d95788 100644
--- a/lib/service/workers/send_activity_recap_worker.ex
+++ b/lib/service/workers/send_activity_recap_worker.ex
@@ -10,6 +10,7 @@ defmodule Mobilizon.Service.Workers.SendActivityRecapWorker do
   alias Mobilizon.Service.Notifier.Email
   alias Mobilizon.Storage.Repo
   alias Mobilizon.Users.{Setting, User}
+  require Logger
 
   import Mobilizon.Service.DateTime,
     only: [
@@ -20,6 +21,8 @@ defmodule Mobilizon.Service.Workers.SendActivityRecapWorker do
 
   @impl Oban.Worker
   def perform(%Job{}) do
+    Logger.info("Sending scheduled activity recap")
+
     Repo.transaction(
       fn ->
         Users.stream_users_for_recap()
diff --git a/test/service/notifier/email_test.exs b/test/service/notifier/email_test.exs
index 49a2e0a3..20c99d83 100644
--- a/test/service/notifier/email_test.exs
+++ b/test/service/notifier/email_test.exs
@@ -4,7 +4,7 @@ defmodule Mobilizon.Service.Notifier.EmailTest do
   """
 
   alias Mobilizon.Activities.Activity
-  alias Mobilizon.Config
+  alias Mobilizon.{Config, Users}
   alias Mobilizon.Service.Notifier.Email
   alias Mobilizon.Users.{ActivitySetting, Setting, User}
 
@@ -101,12 +101,14 @@ defmodule Mobilizon.Service.Notifier.EmailTest do
       %Activity{} = activity = insert(:mobilizon_activity, inserted_at: DateTime.utc_now())
       %User{} = user = insert(:user)
 
+      old = DateTime.add(DateTime.utc_now(), -3600 * 24 * 3)
+
       %Setting{} =
         user_settings =
         insert(:settings,
           user_id: user.id,
           group_notifications: :one_day,
-          last_notification_sent: DateTime.add(DateTime.utc_now(), 3600)
+          last_notification_sent: old
         )
 
       %ActivitySetting{} =
@@ -114,7 +116,19 @@ defmodule Mobilizon.Service.Notifier.EmailTest do
 
       user = %User{user | settings: user_settings, activity_settings: [activity_setting]}
 
-      assert {:ok, :skipped} == Email.send(user, activity)
+      assert {:ok, :sent} == Email.send(user, activity, recap: :one_day)
+
+      assert_email_sent(to: user.email)
+
+      assert %{last_notification_sent: updated_last_notification_sent} =
+               user_settings = Users.get_setting(user.id)
+
+      assert old != updated_last_notification_sent
+      assert DateTime.diff(DateTime.utc_now(), updated_last_notification_sent) < 5
+
+      user = %User{user | settings: user_settings, activity_settings: [activity_setting]}
+
+      assert {:ok, :skipped} == Email.send(user, activity, recap: :one_day)
 
       refute_email_sent()
     end