Send activity recap emails outside of the transaction

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

lib/service/workers/send_activity_recap_worker.ex

@@ -23,20 +23,11 @@ defmodule Mobilizon.Service.Workers.SendActivityRecapWorker do
   def perform(%Job{}) do
     Logger.info("Sending scheduled activity recap")
 
-    Repo.transaction(
-      fn ->
-        Users.stream_users_for_recap()
-        |> Enum.to_list()
-        |> Repo.preload([:settings, :activity_settings])
-        |> Enum.filter(&filter_elegible_users/1)
-        |> Enum.map(fn %User{} = user ->
-          %{
-            activities: activities_for_user(user),
-            user: user
-          }
-        end)
-        |> Enum.filter(fn %{activities: activities, user: _user} -> length(activities) > 0 end)
-        |> Enum.map(fn %{
+    case Repo.transaction(&produce_notifications/0, timeout: :infinity) do
+      {:ok, res} ->
+        Logger.info("Processed #{length(res)} notifications to send")
+
+        Enum.each(res, fn %{
                             activities: activities,
                             user:
                               %User{settings: %Setting{group_notifications: group_notifications}} =
@@ -48,9 +39,25 @@ defmodule Mobilizon.Service.Workers.SendActivityRecapWorker do
 
           Email.send(user, activities, recap: group_notifications)
         end)
-      end,
-      timeout: :infinity
-    )
+
+      {:error, err} ->
+        Logger.error("Error producing notifications #{inspect(err)}")
+        {:error, err}
+    end
+  end
+
+  defp produce_notifications do
+    Users.stream_users_for_recap()
+    |> Enum.to_list()
+    |> Repo.preload([:settings, :activity_settings])
+    |> Enum.filter(&filter_elegible_users/1)
+    |> Enum.map(fn %User{} = user ->
+      %{
+        activities: activities_for_user(user),
+        user: user
+      }
+    end)
+    |> Enum.filter(fn %{activities: activities, user: _user} -> length(activities) > 0 end)
   end
 
   defp activities_for_user(
@@ -87,6 +94,7 @@ defmodule Mobilizon.Service.Workers.SendActivityRecapWorker do
   defp filter_elegible_users(%User{
          settings: %Setting{last_notification_sent: nil, group_notifications: :one_hour}
        }) do
+    Logger.debug("Sending because never sent before, and we must do it at most once an hour")
     true
   end
 
@@ -96,6 +104,10 @@ defmodule Mobilizon.Service.Workers.SendActivityRecapWorker do
            group_notifications: :one_hour
          }
        }) do
+    Logger.debug(
+      "Testing if it's less than an hour since the last time we sent an activity recap"
+    )
+
     is_delay_ok_since_last_notification_sent?(last_notification_sent)
   end
 
@@ -106,6 +118,7 @@ defmodule Mobilizon.Service.Workers.SendActivityRecapWorker do
            timezone: timezone
          }
        }) do
+    Logger.debug("Testing if we're between daily sending hours")
     is_between_hours?(timezone: timezone || "Etc/UTC")
   end
 
@@ -117,6 +130,7 @@ defmodule Mobilizon.Service.Workers.SendActivityRecapWorker do
            timezone: timezone
          }
        }) do
+    Logger.debug("Testing if we're between weekly sending day and hours")
     is_between_hours_on_first_day?(timezone: timezone || "Etc/UTC", locale: locale)
   end
 end

test/service/workers/send_activity_recap_worker_test.exs

@@ -0,0 +1,73 @@
+defmodule Mobilizon.Service.Workers.SendActivityRecapWorkerTest do
+  @moduledoc """
+  Test the SendActivityRecapWorker module
+  """
+
+  alias Mobilizon.{Activities, Users}
+  alias Mobilizon.Activities.Activity
+  alias Mobilizon.Actors.Actor
+  alias Mobilizon.Service.Workers.SendActivityRecapWorker
+  alias Mobilizon.Storage.Page
+  alias Mobilizon.Users.{ActivitySetting, Setting, User}
+
+  use Mobilizon.DataCase
+  import Swoosh.TestAssertions
+  import Mobilizon.Factory
+
+  describe "Send activity recap" do
+    # Skipped because this depends on the test being run between @start_time and @end_time
+    @tag :skip
+    test "not if we already have sent notifications" do
+      %User{} = user = insert(:user)
+      %Actor{} = actor = insert(:actor, user: user)
+      %Actor{} = group = insert(:group)
+
+      insert(:member,
+        parent: group,
+        actor: actor,
+        role: :administrator,
+        member_since: DateTime.add(DateTime.utc_now(), -3600)
+      )
+
+      %Activity{id: activity_id} =
+        insert(:mobilizon_activity, inserted_at: DateTime.utc_now(), group: group)
+
+      assert %Page{elements: [%Activity{id: ^activity_id}], total: 1} =
+               Activities.list_group_activities(group.id)
+
+      assert [%Activity{id: ^activity_id}] =
+               Activities.list_group_activities_for_recap(group.id, actor.id)
+
+      old = DateTime.utc_now() |> DateTime.add(-3600 * 24 * 3) |> DateTime.truncate(:second)
+
+      %Setting{} =
+        user_settings =
+        insert(:settings,
+          user: user,
+          user_id: user.id,
+          group_notifications: :one_day,
+          last_notification_sent: old
+        )
+
+      %ActivitySetting{} =
+        activity_setting = insert(:mobilizon_activity_setting, user_id: user.id, user: user)
+
+      Users.update_user(user, %{settings: user_settings, activity_settings: [activity_setting]})
+      assert old == Users.get_user_with_settings!(user.id).settings.last_notification_sent
+
+      assert :ok == SendActivityRecapWorker.perform(%Oban.Job{})
+
+      assert_email_sent(to: user.email)
+
+      assert %{last_notification_sent: updated_last_notification_sent} =
+               Users.get_setting(user.id)
+
+      assert old != updated_last_notification_sent
+      assert DateTime.diff(DateTime.utc_now(), updated_last_notification_sent) < 5
+
+      assert :ok == SendActivityRecapWorker.perform(%Oban.Job{})
+
+      refute_email_sent()
+    end
+  end
+end

test/support/factory.ex

@@ -263,6 +263,7 @@ defmodule Mobilizon.Factory do
       parent: build(:actor),
       actor: build(:actor),
       role: :not_approved,
+      member_since: nil,
       id: uuid,
       url: "#{Endpoint.url()}/member/#{uuid}"
     }