From 38a3ffc19fc0b7293d53a318d40280397d4ce296 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Wed, 10 Nov 2021 20:31:29 +0100 Subject: [PATCH] Send event creation and event update notifications in a background task The event update notification is made unique so that repeated changes only trigger one notificate every 30 minutes Signed-off-by: Thomas Citharel --- lib/federation/activity_pub/types/events.ex | 14 ++-- lib/mobilizon/events/events.ex | 67 ++++++++++++++++--- lib/mobilizon/events/utils.ex | 18 +++++ .../event_delayed_notification_worker.ex | 51 ++++++++++++++ lib/web/email/event.ex | 11 +-- .../templates/email/event_updated.text.eex | 26 +++---- test/graphql/resolvers/event_test.exs | 48 +++++++++++-- test/mobilizon/events/utils_test.exs | 19 ++++++ ...event_delayed_notification_worker_test.exs | 43 ++++++++++++ 9 files changed, 254 insertions(+), 43 deletions(-) create mode 100644 lib/mobilizon/events/utils.ex create mode 100644 lib/service/workers/event_delayed_notification_worker.ex create mode 100644 test/mobilizon/events/utils_test.exs create mode 100644 test/service/workers/event_delayed_notification_worker_test.exs diff --git a/lib/federation/activity_pub/types/events.ex b/lib/federation/activity_pub/types/events.ex index 33bd03668..ed817af80 100644 --- a/lib/federation/activity_pub/types/events.ex +++ b/lib/federation/activity_pub/types/events.ex @@ -14,9 +14,10 @@ defmodule Mobilizon.Federation.ActivityPub.Types.Events do alias Mobilizon.Service.Formatter.HTML alias Mobilizon.Service.LanguageDetection alias Mobilizon.Service.Notifications.Scheduler + alias Mobilizon.Service.Workers.EventDelayedNotificationWorker alias Mobilizon.Share alias Mobilizon.Tombstone - alias Mobilizon.Web.Email.Group + import Mobilizon.Events.Utils, only: [calculate_notification_time: 1] import Mobilizon.Federation.ActivityPub.Utils, only: [make_create_data: 2, make_update_data: 2] require Logger @@ -29,10 +30,15 @@ defmodule Mobilizon.Federation.ActivityPub.Types.Events do args = prepare_args_for_event(args) case EventsManager.create_event(args) do - {:ok, %Event{} = event} -> + {:ok, %Event{uuid: event_uuid, begins_on: begins_on} = event} -> EventActivity.insert_activity(event, subject: "event_created") - # TODO make this async - Group.notify_of_new_event(event) + + %{action: :notify_of_new_event, event_uuid: event_uuid} + |> EventDelayedNotificationWorker.new( + scheduled_at: calculate_notification_time(begins_on) + ) + |> Oban.insert() + event_as_data = Convertible.model_to_as(event) audience = Audience.get_audience(event) create_data = make_create_data(event_as_data, Map.merge(audience, additional)) diff --git a/lib/mobilizon/events/events.ex b/lib/mobilizon/events/events.ex index 52e89a275..f349ff42f 100644 --- a/lib/mobilizon/events/events.ex +++ b/lib/mobilizon/events/events.ex @@ -10,6 +10,7 @@ defmodule Mobilizon.Events do import Mobilizon.Service.Guards import Mobilizon.Storage.Ecto + import Mobilizon.Events.Utils, only: [calculate_notification_time: 1] alias Ecto.{Changeset, Multi} @@ -18,6 +19,7 @@ defmodule Mobilizon.Events do alias Mobilizon.Events.{ Event, + EventOptions, EventParticipantStats, FeedToken, Participant, @@ -27,13 +29,12 @@ defmodule Mobilizon.Events do Track } - alias Mobilizon.Service.Workers + alias Mobilizon.Service.Workers.BuildSearch + alias Mobilizon.Service.Workers.EventDelayedNotificationWorker alias Mobilizon.Share alias Mobilizon.Storage.{Page, Repo} alias Mobilizon.Users.{Setting, User} - alias Mobilizon.Web.Email - defenum(EventVisibility, :event_visibility, [ :public, :unlisted, @@ -229,7 +230,7 @@ defmodule Mobilizon.Events do with {:ok, %{insert: %Event{} = event}} <- do_create_event(attrs), %Event{} = event <- Repo.preload(event, @event_preloads) do unless event.draft do - Workers.BuildSearch.enqueue(:insert_search_event, %{"event_id" => event.id}) + BuildSearch.enqueue(:insert_search_event, %{"event_id" => event.id}) end {:ok, event} @@ -303,19 +304,63 @@ defmodule Mobilizon.Events do %Event{} = new_event <- Repo.preload(new_event, @event_preloads, force: true) do Cachex.del(:ics, "event_#{new_event.uuid}") - Email.Event.calculate_event_diff_and_send_notifications( - old_event, - new_event, - changes - ) + unless new_event.draft do + %{ + action: :notify_of_event_update, + event_uuid: new_event.uuid, + old_event: old_event |> Map.from_struct() |> Map.take(Event.__schema__(:fields)), + changes: build_changes(changes) + } + |> EventDelayedNotificationWorker.new( + scheduled_at: calculate_notification_time(new_event.begins_on), + replace: [:scheduled_at, :args], + unique: [period: 1, keys: [:event_uuid, :action]] + ) + |> Oban.insert() - unless new_event.draft, - do: Workers.BuildSearch.enqueue(:update_search_event, %{"event_id" => new_event.id}) + BuildSearch.enqueue(:update_search_event, %{"event_id" => new_event.id}) + end {:ok, new_event} end end + @spec build_changes(map()) :: map() + defp build_changes(changes) do + changes + |> Map.take(Event.__schema__(:fields)) + |> maybe_add_address(changes) + |> maybe_add_options(changes) + end + + @spec maybe_add_address(map(), map()) :: map() + defp maybe_add_address(changes, %{physical_address: %Ecto.Changeset{} = changeset}), + do: + Map.put( + changes, + :physical_address, + changeset + |> Ecto.Changeset.apply_changes() + |> Map.from_struct() + |> Map.take(Address.__schema__(:fields)) + ) + + defp maybe_add_address(changes, _), do: Map.drop(changes, [:physical_address]) + + @spec maybe_add_options(map(), map()) :: map() + defp maybe_add_options(changes, %{options: %Ecto.Changeset{} = changeset}), + do: + Map.put( + changes, + :options, + changeset + |> Ecto.Changeset.apply_changes() + |> Map.from_struct() + |> Map.take(EventOptions.__schema__(:fields)) + ) + + defp maybe_add_options(changes, _), do: Map.drop(changes, [:options]) + @doc """ Deletes an event. """ diff --git a/lib/mobilizon/events/utils.ex b/lib/mobilizon/events/utils.ex new file mode 100644 index 000000000..7ac37cfae --- /dev/null +++ b/lib/mobilizon/events/utils.ex @@ -0,0 +1,18 @@ +defmodule Mobilizon.Events.Utils do + @moduledoc """ + Utils related to events + """ + + @spec calculate_notification_time(DateTime.t()) :: DateTime.t() + def calculate_notification_time(begins_on, options \\ []) do + now = Keyword.get(options, :now, DateTime.utc_now()) + notify_at = DateTime.add(now, 1800) + + # If the event begins in less than half an hour, send the notification right now + if DateTime.compare(notify_at, begins_on) == :lt do + notify_at + else + now + end + end +end diff --git a/lib/service/workers/event_delayed_notification_worker.ex b/lib/service/workers/event_delayed_notification_worker.ex new file mode 100644 index 000000000..a58a91af2 --- /dev/null +++ b/lib/service/workers/event_delayed_notification_worker.ex @@ -0,0 +1,51 @@ +defmodule Mobilizon.Service.Workers.EventDelayedNotificationWorker do + @moduledoc """ + Worker to send notifications about an event changes a while after they're performed + """ + + use Oban.Worker, unique: [period: :infinity, keys: [:event_uuid, :action]] + + alias Mobilizon.Events + alias Mobilizon.Events.Event + alias Mobilizon.Web.Email.Event, as: EventEmail + alias Mobilizon.Web.Email.Group + alias Oban.Job + + @impl Oban.Worker + def perform(%Job{args: %{"action" => "notify_of_new_event", "event_uuid" => event_uuid}}) do + case Events.get_event_by_uuid_with_preload(event_uuid) do + %Event{} = event -> + Group.notify_of_new_event(event) + + nil -> + # Event deleted inbetween, no worries, just ignore + :ok + end + end + + @impl Oban.Worker + def perform(%Job{ + args: %{ + "action" => "notify_of_event_update", + "event_uuid" => event_uuid, + "old_event" => old_event, + "changes" => changes + } + }) do + old_event = for {key, val} <- old_event, into: %{}, do: {String.to_existing_atom(key), val} + old_event = struct(Event, old_event) + + case Events.get_event_by_uuid_with_preload(event_uuid) do + %Event{draft: false} = new_event -> + EventEmail.calculate_event_diff_and_send_notifications( + old_event, + new_event, + changes + ) + + _ -> + # Event deleted inbetween, no worries, just ignore + :ok + end + end +end diff --git a/lib/web/email/event.ex b/lib/web/email/event.ex index 13a627d7c..551479299 100644 --- a/lib/web/email/event.ex +++ b/lib/web/email/event.ex @@ -26,6 +26,7 @@ defmodule Mobilizon.Web.Email.Event do Event.t(), Event.t(), MapSet.t(), + String.t(), String.t() ) :: Bamboo.Email.t() @@ -36,8 +37,8 @@ defmodule Mobilizon.Web.Email.Event do %Event{} = old_event, %Event{} = event, changes, - timezone \\ "Etc/UTC", - locale \\ "en" + timezone, + locale ) do Gettext.put_locale(locale) @@ -70,13 +71,15 @@ defmodule Mobilizon.Web.Email.Event do %Event{id: event_id} = event, changes ) do - important = MapSet.new(@important_changes) + important = @important_changes |> Enum.map(&to_string/1) |> MapSet.new() diff = changes |> Map.keys() |> MapSet.new() |> MapSet.intersection(important) + |> Enum.map(&String.to_existing_atom/1) + |> MapSet.new() if MapSet.size(diff) > 0 do Repo.transaction(fn -> @@ -178,7 +181,7 @@ defmodule Mobilizon.Web.Email.Event do locale ) do email - |> Email.Event.event_updated(participant, actor, old_event, event, diff, timezone, locale) + |> event_updated(participant, actor, old_event, event, diff, timezone, locale) |> Email.Mailer.send_email_later() end end diff --git a/lib/web/templates/email/event_updated.text.eex b/lib/web/templates/email/event_updated.text.eex index cf0733f35..8ac662667 100644 --- a/lib/web/templates/email/event_updated.text.eex +++ b/lib/web/templates/email/event_updated.text.eex @@ -1,22 +1,14 @@ <%= gettext "Event update!" %> == <%= gettext "There have been changes for %{title} so we'd thought we'd let you know.", title: @old_event.title %> -<%= if MapSet.member?(@changes, :status) do %> - <%= case @event.status do %> - <% :confirmed -> %> - <%= gettext "This event has been confirmed" %> - <% :tentative -> %> - <%= gettext "This event has yet to be confirmed: organizers will let you know if they do confirm it." %> - <% :cancelled -> %> - <%= gettext "This event has been cancelled by its organizers. Sorry!" %> - <% end %> -<% end %> -<%= if MapSet.member?(@changes, :title) do %> - <%= gettext "New title: %{title}", title: @event.title %> -<% end %> -<%= if MapSet.member?(@changes, :begins_on) do %><%= render("date/event_tz_date.text", event: @event, date: @event.begins_on, timezone: @timezone, locale: @locale) %> -<% end %> -<%= if MapSet.member?(@changes, :ends_on) && !is_nil(@event.ends_on) do %><%= render("date/event_tz_date.text", event: @event, date: @event.ends_on, timezone: @timezone, locale: @locale) %> -<% end %> +<%= if MapSet.member?(@changes, :status) do %><%= case @event.status do %><% :confirmed -> %><%= gettext "This event has been confirmed" %><% :tentative -> %> +<%= gettext "This event has yet to be confirmed: organizers will let you know if they do confirm it." %><% :cancelled -> %> +<%= gettext "This event has been cancelled by its organizers. Sorry!" %><% end %><% end %><%= if MapSet.member?(@changes, :title) do %> +<%= gettext "New title: %{title}", title: @event.title %><% end %><%= if MapSet.member?(@changes, :begins_on) do %> +<%= gettext "New start date:" %> <%= render("date/event_tz_date.text", event: @event, date: @event.begins_on, timezone: @timezone, locale: @locale) %><% end %><%= if MapSet.member?(@changes, :ends_on) && !is_nil(@event.ends_on) do %> +<%= gettext "New end date:" %> <%= render("date/event_tz_date.text", event: @event, date: @event.ends_on, timezone: @timezone, locale: @locale) %><% end %><%= if MapSet.member?(@changes, :physical_address) do %> +<%= gettext "New location:" %> <%= Mobilizon.Addresses.Address.representation(@event.physical_address) %><% end %> + <%= gettext "Visit the updated event page: %{link}", link: Routes.page_url(Mobilizon.Web.Endpoint, :event, @event.uuid) %> + <%= ngettext "Would you wish to cancel your attendance, visit the event page through the link above and click the « Attending » button.", "Would you wish to cancel your attendance to one or several events, visit the event pages through the links above and click the « Attending » button.", 1 %> diff --git a/test/graphql/resolvers/event_test.exs b/test/graphql/resolvers/event_test.exs index 9badb50e8..e56b32ded 100644 --- a/test/graphql/resolvers/event_test.exs +++ b/test/graphql/resolvers/event_test.exs @@ -1,6 +1,6 @@ defmodule Mobilizon.Web.Resolvers.EventTest do use Mobilizon.Web.ConnCase - use Bamboo.Test + use Bamboo.Test, shared: true use Oban.Testing, repo: Mobilizon.Storage.Repo import Mobilizon.Factory @@ -180,6 +180,8 @@ defmodule Mobilizon.Web.Resolvers.EventTest do end test "create_event/3 creates an event", %{conn: conn, actor: actor, user: user} do + begins_on = DateTime.utc_now() + res = conn |> auth_conn(user) @@ -188,18 +190,25 @@ defmodule Mobilizon.Web.Resolvers.EventTest do variables: %{ title: "come to my event", description: "it will be fine", - begins_on: "#{DateTime.utc_now()}", + begins_on: "#{DateTime.add(begins_on, 3600 * 24)}", organizer_actor_id: "#{actor.id}" } ) assert res["data"]["createEvent"]["title"] == "come to my event" - {id, ""} = res["data"]["createEvent"]["id"] |> Integer.parse() + id = String.to_integer(res["data"]["createEvent"]["id"]) + uuid = res["data"]["createEvent"]["uuid"] assert_enqueued( worker: Workers.BuildSearch, args: %{event_id: id, op: :insert_search_event} ) + + assert_enqueued( + worker: Workers.EventDelayedNotificationWorker, + args: %{event_uuid: uuid, action: :notify_of_new_event}, + scheduled_at: {DateTime.add(begins_on, 1800), delta: 5} + ) end test "create_event/3 creates an event and escapes title", %{ @@ -930,8 +939,10 @@ defmodule Mobilizon.Web.Resolvers.EventTest do ["ends_on cannot be set before begins_on"] end - test "update_event/3 updates an event", %{conn: conn, actor: actor, user: user} do - event = insert(:event, organizer_actor: actor) + test "updates an event", %{conn: conn, actor: actor, user: user} do + %Event{uuid: event_uuid, title: event_title} = + event = insert(:event, organizer_actor: actor) + creator = insert(:participant, event: event, actor: actor, role: :creator) participant_user = insert(:user) participant_actor = insert(:actor, user: participant_user) @@ -1005,6 +1016,25 @@ defmodule Mobilizon.Web.Resolvers.EventTest do args: %{event_id: event_id_int, op: :update_search_event} ) + assert [ + %{ + args: %{ + "event_uuid" => ^event_uuid, + "action" => "notify_of_event_update", + "old_event" => %{ + "title" => ^event_title + }, + "changes" => %{ + "description" => "description updated", + "status" => "tentative", + "title" => "my event updated" + } + } + } + ] = all_enqueued(worker: Workers.EventDelayedNotificationWorker) + + Oban.drain_queue(queue: :default, with_scheduled: true) + {:ok, new_event} = Mobilizon.Events.get_event_with_preload(event.id) assert_delivered_email( @@ -1014,7 +1044,9 @@ defmodule Mobilizon.Web.Resolvers.EventTest do actor, event, new_event, - MapSet.new([:title, :begins_on, :ends_on, :status, :physical_address]) + MapSet.new([:title, :begins_on, :ends_on, :status, :physical_address]), + "Etc/UTC", + "en" ) ) @@ -1025,7 +1057,9 @@ defmodule Mobilizon.Web.Resolvers.EventTest do participant_actor, event, new_event, - MapSet.new([:title, :begins_on, :ends_on, :status, :physical_address]) + MapSet.new([:title, :begins_on, :ends_on, :status, :physical_address]), + "Etc/UTC", + "en" ) ) end diff --git a/test/mobilizon/events/utils_test.exs b/test/mobilizon/events/utils_test.exs new file mode 100644 index 000000000..a992994c5 --- /dev/null +++ b/test/mobilizon/events/utils_test.exs @@ -0,0 +1,19 @@ +defmodule Mobilizon.Events.UtilsTest do + use Mobilizon.DataCase, async: true + + alias Mobilizon.Events.Utils + + @now ~U[2021-11-19T18:17:00Z] + + describe "calculate_notification_time" do + test "when the event begins in less than 30 minutes" do + begins_on = ~U[2021-11-19T18:27:00Z] + assert @now == Utils.calculate_notification_time(begins_on, now: @now) + end + + test "when the event begins in more than 30 minutes" do + begins_on = ~U[2021-11-19T18:17:00Z] + assert begins_on == Utils.calculate_notification_time(begins_on, now: @now) + end + end +end diff --git a/test/service/workers/event_delayed_notification_worker_test.exs b/test/service/workers/event_delayed_notification_worker_test.exs new file mode 100644 index 000000000..b1bb1e2a0 --- /dev/null +++ b/test/service/workers/event_delayed_notification_worker_test.exs @@ -0,0 +1,43 @@ +defmodule Mobilizon.Service.Workers.EventDelayedNotificationWorkerTest do + @moduledoc """ + Test the event delayed notification worker + """ + + alias Mobilizon.Events.Event + alias Mobilizon.Service.Workers.EventDelayedNotificationWorker + alias Oban.Job + + use Mobilizon.DataCase + import Mobilizon.Factory + + test "Run notify of new event" do + group = insert(:group) + event = insert(:event, attributed_to: group) + + assert :ok == + EventDelayedNotificationWorker.perform(%Job{ + args: %{"action" => "notify_of_new_event", "event_uuid" => event.uuid} + }) + end + + test "Run notify of updates to event" do + group = insert(:group) + event = insert(:event, attributed_to: group) + old_event = %Event{event | title: "Previous title"} + + old_event = + for {key, val} <- Map.from_struct(old_event), into: %{}, do: {Atom.to_string(key), val} + + changes = %{"title" => "New title"} + + assert {:ok, :ok} == + EventDelayedNotificationWorker.perform(%Job{ + args: %{ + "action" => "notify_of_event_update", + "event_uuid" => event.uuid, + "old_event" => old_event, + "changes" => changes + } + }) + end +end