Merge branch 'notification-fixes' into 'main'

Notification fixes

Closes #1077

See merge request framasoft/mobilizon!1238

lib/service/date_time/date_time.ex

@@ -205,9 +205,12 @@ defmodule Mobilizon.Service.DateTime do
     is_first_day_of_week(compare_to_day, locale) && is_between_hours?(options)
   end
 
-  @spec is_delay_ok_since_last_notification_sent?(DateTime.t()) :: boolean()
-  def is_delay_ok_since_last_notification_sent?(%DateTime{} = last_notification_sent) do
-    DateTime.compare(DateTime.add(last_notification_sent, 3_600), DateTime.utc_now()) ==
+  @spec is_delay_ok_since_last_notification_sent?(DateTime.t(), pos_integer()) :: boolean()
+  def is_delay_ok_since_last_notification_sent?(
+        %DateTime{} = last_notification_sent,
+        delay \\ 3_600
+      ) do
+    DateTime.compare(DateTime.add(last_notification_sent, delay), DateTime.utc_now()) ==
       :lt
   end
 

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

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()

lib/web/email/group.ex

@@ -17,25 +17,27 @@ defmodule Mobilizon.Web.Email.Group do
     # TODO: When we have events restricted to members, don't send emails to followers
     group
     |> Actors.list_actors_to_notify_from_group_event()
+    |> Enum.reduce([], fn actor, users ->
+      # No emails for remote actors
+      if is_nil(actor.user_id) do
+        users
+      else
+        users ++ [Users.get_user_with_activity_settings!(actor.user_id)]
+      end
+    end)
     |> Enum.each(&notify_follower(event, group, &1))
   end
 
   def notify_of_new_event(%Event{}), do: :ok
 
-  defp notify_follower(%Event{} = _event, %Actor{}, %Actor{user_id: nil}), do: :ok
-
-  defp notify_follower(%Event{} = event, %Actor{type: :Group} = group, %Actor{
-         id: profile_id,
-         user_id: user_id
+  defp notify_follower(%Event{} = event, %Actor{type: :Group} = group, %User{
+         email: email,
+         locale: locale,
+         settings: %Setting{timezone: timezone},
+         activity_settings: activity_settings,
+         default_actor_id: default_actor_id
        }) do
-    %User{
-      email: email,
-      locale: locale,
-      settings: %Setting{timezone: timezone},
-      activity_settings: activity_settings
-    } = Users.get_user_with_activity_settings!(user_id)
-
-    if profile_id != event.organizer_actor_id &&
+    if default_actor_id != event.organizer_actor_id &&
          accepts_new_events_notifications(activity_settings) do
       Gettext.put_locale(locale)
 

lib/web/templates/email/group_membership_approval.html.heex

@@ -48,7 +48,7 @@
               "Your membership request for group %{link_start}<b>%{group}</b>%{link_end} has been approved.",
               group: Mobilizon.Actors.Actor.display_name(@group),
               link_start:
-                "<a href=\"#{Routes.page_url(Mobilizon.Web.Endpoint, :actor, Mobilizon.Actors.Actor.preferred_username_and_domain(@group))}\">",
+                "<a href=\"#{Routes.page_url(Mobilizon.Web.Endpoint, :actor, Mobilizon.Actors.Actor.preferred_username_and_domain(@group)) |> URI.decode()}\">",
               link_end: "</a>"
             )
             |> raw %>
@@ -66,7 +66,7 @@
                     <td align="center" style="border-radius: 3px;" bgcolor="#3C376E">
                       <a
                         href={
-                          "#{Routes.page_url(Mobilizon.Web.Endpoint, :actor, Mobilizon.Actors.Actor.preferred_username_and_domain(@group))}"
+                          "#{Routes.page_url(Mobilizon.Web.Endpoint, :actor, Mobilizon.Actors.Actor.preferred_username_and_domain(@group)) |> URI.decode()}"
                         }
                         target="_blank"
                         style="font-size: 20px; font-family: Helvetica, Arial, sans-serif; color: #ffffff; text-decoration: none; padding: 15px 25px; border-radius: 2px; border: 1px solid #3C376E; display: inline-block;"

lib/web/templates/email/group_membership_approval.text.eex

@@ -2,4 +2,4 @@
 ==
 <%= gettext "Your membership request for group %{group} has been approved.", group: Mobilizon.Actors.Actor.display_name(@group) %>
 
-<%= Routes.page_url(Mobilizon.Web.Endpoint, :actor, Mobilizon.Actors.Actor.preferred_username_and_domain(@group)) %>
+<%= Routes.page_url(Mobilizon.Web.Endpoint, :actor, Mobilizon.Actors.Actor.preferred_username_and_domain(@group)) |> URI.decode() %>

lib/web/templates/email/group_membership_rejection.html.heex

@@ -48,7 +48,7 @@
               "Your membership request for group %{link_start}<b>%{group}</b>%{link_end} has been rejected.",
               group: Mobilizon.Actors.Actor.display_name(@group),
               link_start:
-                "<a href=\"#{Routes.page_url(Mobilizon.Web.Endpoint, :actor, Mobilizon.Actors.Actor.preferred_username_and_domain(@group))}\">",
+                "<a href=\"#{Routes.page_url(Mobilizon.Web.Endpoint, :actor, Mobilizon.Actors.Actor.preferred_username_and_domain(@group)) |> URI.decode()}\">",
               link_end: "</a>"
             )
             |> raw %>

lib/web/templates/email/group_membership_rejection.text.eex

@@ -2,4 +2,4 @@
 ==
 <%= gettext "Your membership request for group %{group} has been rejected.", group: Mobilizon.Actors.Actor.display_name(@group) %>
 
-<%= Routes.page_url(Mobilizon.Web.Endpoint, :actor, Mobilizon.Actors.Actor.preferred_username_and_domain(@group)) %>
+<%= Routes.page_url(Mobilizon.Web.Endpoint, :actor, Mobilizon.Actors.Actor.preferred_username_and_domain(@group)) |> URI.decode() %>

lib/web/templates/email/participation/card/_title.html.heex

@@ -40,7 +40,7 @@
                       :actor,
                       Mobilizon.Actors.Actor.preferred_username_and_domain(@event.attributed_to)
                     )
-                    |> raw
+                    |> URI.decode()
                   }
                   style="color: rgb(254,56,89); font-family: Helvetica,Arial,sans-serif; font-weight: normal; text-align: left; line-height: 1.3; text-decoration: none; vertical-align: baseline; margin: 0; padding: 0; border: 0;"
                   target="_blank"

lib/web/templates/email/participation/card/_title.text.eex

@@ -1,3 +1,3 @@
 <%= gettext("Title: %{title}", title: @event.title) %>
 
-<%= if @event.attributed_to do %><%= gettext("Organizer: %{organizer}", organizer: @event.attributed_to.name || @event.attributed_to.preferred_username) %> <%= Routes.page_url(Mobilizon.Web.Endpoint, :actor, Mobilizon.Actors.Actor.preferred_username_and_domain(@event.attributed_to)) %><% else %><%= gettext("Organizer: %{organizer}", organizer: @event.organizer_actor.name || @event.organizer_actor.preferred_username) %><% end %>
+<%= if @event.attributed_to do %><%= gettext("Organizer: %{organizer}", organizer: @event.attributed_to.name || @event.attributed_to.preferred_username) %> <%= Routes.page_url(Mobilizon.Web.Endpoint, :actor, Mobilizon.Actors.Actor.preferred_username_and_domain(@event.attributed_to)) |> URI.decode() %><% else %><%= gettext("Organizer: %{organizer}", organizer: @event.organizer_actor.name || @event.organizer_actor.preferred_username) %><% end %>

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