From 7a6667bd3b1a865a5cec1b99956030f3878f89ca Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Wed, 23 Jun 2021 19:49:10 +0200 Subject: [PATCH] Make mentions send notifications Signed-off-by: Thomas Citharel --- lib/service/activity/comment.ex | 11 +- lib/service/activity/discussion.ex | 70 +++++++-- lib/service/activity/renderer/comment.ex | 50 ------ lib/service/activity/renderer/discussion.ex | 38 +++-- lib/service/activity/utils.ex | 9 ++ .../workers/legacy_notifier_builder.ex | 21 ++- test/service/activity/discussion_test.exs | 119 ++++++++++++++ .../workers/legacy_notifier_builder_test.exs | 147 +++++++++++++++++- 8 files changed, 372 insertions(+), 93 deletions(-) create mode 100644 test/service/activity/discussion_test.exs diff --git a/lib/service/activity/comment.ex b/lib/service/activity/comment.ex index dc08b2fb3..6d3993483 100644 --- a/lib/service/activity/comment.ex +++ b/lib/service/activity/comment.ex @@ -9,6 +9,8 @@ defmodule Mobilizon.Service.Activity.Comment do alias Mobilizon.Service.Activity alias Mobilizon.Service.Workers.{ActivityBuilder, LegacyNotifierBuilder} + import Mobilizon.Service.Activity.Utils, only: [maybe_inserted_at: 0] + @behaviour Activity @impl Activity @@ -64,15 +66,6 @@ defmodule Mobilizon.Service.Activity.Comment do ) end - @spec maybe_inserted_at :: map() - defp maybe_inserted_at do - if Application.fetch_env!(:mobilizon, :env) == :test do - %{} - else - %{"inserted_at" => DateTime.utc_now()} - end - end - @type notification_type :: atom() # An actor is mentionned diff --git a/lib/service/activity/discussion.ex b/lib/service/activity/discussion.ex index 12de6bbd2..519d73d3e 100644 --- a/lib/service/activity/discussion.ex +++ b/lib/service/activity/discussion.ex @@ -3,9 +3,12 @@ defmodule Mobilizon.Service.Activity.Discussion do Insert a discussion activity """ alias Mobilizon.{Actors, Discussions} - alias Mobilizon.Discussions.Discussion + alias Mobilizon.Actors.Actor + alias Mobilizon.Discussions.{Comment, Discussion} alias Mobilizon.Service.Activity - alias Mobilizon.Service.Workers.ActivityBuilder + alias Mobilizon.Service.Workers.{ActivityBuilder, LegacyNotifierBuilder} + + import Mobilizon.Service.Activity.Utils, only: [maybe_inserted_at: 0] @behaviour Activity @@ -24,16 +27,22 @@ defmodule Mobilizon.Service.Activity.Discussion do author_id = Keyword.get(options, :actor_id, author.id) old_discussion = Keyword.get(options, :old_discussion) - ActivityBuilder.enqueue(:build_activity, %{ - "type" => "discussion", - "subject" => subject, - "subject_params" => subject_params(discussion, subject, old_discussion), - "group_id" => group.id, - "author_id" => author_id, - "object_type" => "discussion", - "object_id" => if(subject != "discussion_deleted", do: to_string(discussion.id), else: nil), - "inserted_at" => DateTime.utc_now() - }) + send_mention_notifications(subject, discussion, discussion.last_comment, options) + + ActivityBuilder.enqueue( + :build_activity, + %{ + "type" => "discussion", + "subject" => subject, + "subject_params" => subject_params(discussion, subject, old_discussion), + "group_id" => group.id, + "author_id" => author_id, + "object_type" => "discussion", + "object_id" => + if(subject != "discussion_deleted", do: to_string(discussion.id), else: nil) + } + |> Map.merge(maybe_inserted_at()) + ) end def insert_activity(_, _), do: {:ok, nil} @@ -53,4 +62,41 @@ defmodule Mobilizon.Service.Activity.Discussion do defp subject_params(%Discussion{} = discussion, _, _) do %{discussion_slug: discussion.slug, discussion_title: discussion.title} end + + # An actor is mentionned + @spec send_mention_notifications(String.t(), Discussion.t(), Comment.t(), Keyword.t()) :: + {:ok, Oban.Job.t()} | {:ok, :skipped} + defp send_mention_notifications( + subject, + %Discussion{ + id: discussion_id, + title: title, + slug: slug, + actor: %Actor{id: group_id, type: :Group} + }, + %Comment{actor_id: actor_id, mentions: mentions}, + _options + ) + when subject in ["discussion_created", "discussion_replied"] and length(mentions) > 0 do + LegacyNotifierBuilder.enqueue( + :legacy_notify, + %{ + "subject" => :discussion_mention, + "subject_params" => %{ + discussion_slug: slug, + discussion_title: title + }, + "type" => :discussion, + "object_type" => :discussion, + "author_id" => actor_id, + "group_id" => group_id, + "object_id" => to_string(discussion_id), + "mentions" => Enum.map(mentions, & &1.actor_id) + } + ) + + {:ok, :enqueued} + end + + defp send_mention_notifications(_, _, _, _), do: {:ok, :skipped} end diff --git a/lib/service/activity/renderer/comment.ex b/lib/service/activity/renderer/comment.ex index 31febd367..6689a7d8d 100644 --- a/lib/service/activity/renderer/comment.ex +++ b/lib/service/activity/renderer/comment.ex @@ -45,58 +45,9 @@ defmodule Mobilizon.Service.Activity.Renderer.Comment do ), url: event_url(activity) } - - :discussion_mention -> - %{ - body: - dgettext("activity", "%{profile} mentionned you in the discussion %{discussion}.", %{ - profile: profile, - discussion: title(activity) - }), - url: discussion_url(activity) - } - - :discussion_renamed -> - %{ - body: - dgettext("activity", "%{profile} renamed the discussion %{discussion}.", %{ - profile: profile, - discussion: title(activity) - }), - url: discussion_url(activity) - } - - :discussion_archived -> - %{ - body: - dgettext("activity", "%{profile} archived the discussion %{discussion}.", %{ - profile: profile, - discussion: title(activity) - }), - url: discussion_url(activity) - } - - :discussion_deleted -> - %{ - body: - dgettext("activity", "%{profile} deleted the discussion %{discussion}.", %{ - profile: profile, - discussion: title(activity) - }), - url: nil - } end end - defp discussion_url(activity) do - Routes.page_url( - Endpoint, - :discussion, - Actor.preferred_username_and_domain(activity.group), - activity.subject_params["discussion_slug"] - ) - end - defp event_url(activity) do Routes.page_url( Endpoint, @@ -107,5 +58,4 @@ defmodule Mobilizon.Service.Activity.Renderer.Comment do defp profile(activity), do: Actor.display_name_and_username(activity.author) defp event_title(activity), do: activity.subject_params["event_title"] - defp title(activity), do: activity.subject_params["discussion_title"] end diff --git a/lib/service/activity/renderer/discussion.ex b/lib/service/activity/renderer/discussion.ex index 99f201b91..11328ed85 100644 --- a/lib/service/activity/renderer/discussion.ex +++ b/lib/service/activity/renderer/discussion.ex @@ -15,14 +15,16 @@ defmodule Mobilizon.Service.Activity.Renderer.Discussion do def render(%Activity{} = activity, options) do locale = Keyword.get(options, :locale, "en") Gettext.put_locale(locale) + profile = profile(activity) + title = title(activity) case activity.subject do :discussion_created -> %{ body: dgettext("activity", "%{profile} created the discussion %{discussion}.", %{ - profile: profile(activity), - discussion: title(activity) + profile: profile, + discussion: title }), url: discussion_url(activity) } @@ -31,8 +33,18 @@ defmodule Mobilizon.Service.Activity.Renderer.Discussion do %{ body: dgettext("activity", "%{profile} replied to the discussion %{discussion}.", %{ - profile: profile(activity), - discussion: title(activity) + profile: profile, + discussion: title + }), + url: discussion_url(activity) + } + + :discussion_mention -> + %{ + body: + dgettext("activity", "%{profile} mentionned you in the discussion %{discussion}.", %{ + profile: profile, + discussion: title }), url: discussion_url(activity) } @@ -41,8 +53,8 @@ defmodule Mobilizon.Service.Activity.Renderer.Discussion do %{ body: dgettext("activity", "%{profile} renamed the discussion %{discussion}.", %{ - profile: profile(activity), - discussion: title(activity) + profile: profile, + discussion: title }), url: discussion_url(activity) } @@ -51,8 +63,8 @@ defmodule Mobilizon.Service.Activity.Renderer.Discussion do %{ body: dgettext("activity", "%{profile} archived the discussion %{discussion}.", %{ - profile: profile(activity), - discussion: title(activity) + profile: profile, + discussion: title }), url: discussion_url(activity) } @@ -61,8 +73,8 @@ defmodule Mobilizon.Service.Activity.Renderer.Discussion do %{ body: dgettext("activity", "%{profile} deleted the discussion %{discussion}.", %{ - profile: profile(activity), - discussion: title(activity) + profile: profile, + discussion: title }), url: nil } @@ -79,6 +91,8 @@ defmodule Mobilizon.Service.Activity.Renderer.Discussion do |> URI.decode() end - defp profile(activity), do: Actor.display_name_and_username(activity.author) - defp title(activity), do: activity.subject_params["discussion_title"] + defp profile(%Activity{author: author}), do: Actor.display_name_and_username(author) + + defp title(%Activity{subject_params: %{"discussion_title" => discussion_title}}), + do: discussion_title end diff --git a/lib/service/activity/utils.ex b/lib/service/activity/utils.ex index dcb26e2da..7a0f15754 100644 --- a/lib/service/activity/utils.ex +++ b/lib/service/activity/utils.ex @@ -27,4 +27,13 @@ defmodule Mobilizon.Service.Activity.Utils do end defp transform_value(value), do: value + + @spec maybe_inserted_at :: map() + def maybe_inserted_at do + if Application.fetch_env!(:mobilizon, :env) == :test do + %{} + else + %{"inserted_at" => DateTime.utc_now()} + end + end end diff --git a/lib/service/workers/legacy_notifier_builder.ex b/lib/service/workers/legacy_notifier_builder.ex index 80f4c25c7..25018b92f 100644 --- a/lib/service/workers/legacy_notifier_builder.ex +++ b/lib/service/workers/legacy_notifier_builder.ex @@ -15,7 +15,7 @@ defmodule Mobilizon.Service.Workers.LegacyNotifierBuilder do activity = build_activity(args) args - |> users_to_notify(args["author_id"]) + |> users_to_notify(author_id: args["author_id"], group_id: Map.get(args, "group_id")) |> Enum.each(&Notifier.notify(&1, activity, single_activity: true)) end end @@ -35,12 +35,21 @@ defmodule Mobilizon.Service.Workers.LegacyNotifierBuilder do } end - @spec users_to_notify(map(), integer() | String.t()) :: list(Users.t()) + @spec users_to_notify(map(), Keyword.t()) :: list(Users.t()) defp users_to_notify( %{"subject" => "event_comment_mention", "mentions" => mentionned_actor_ids}, - author_id + options ) do - users_from_actor_ids(mentionned_actor_ids, author_id) + users_from_actor_ids(mentionned_actor_ids, Keyword.fetch!(options, :author_id)) + end + + defp users_to_notify( + %{"subject" => "discussion_mention", "mentions" => mentionned_actor_ids}, + options + ) do + mentionned_actor_ids + |> Enum.filter(&Actors.is_member?(&1, Keyword.fetch!(options, :group_id))) + |> users_from_actor_ids(Keyword.fetch!(options, :author_id)) end defp users_to_notify( @@ -48,13 +57,13 @@ defmodule Mobilizon.Service.Workers.LegacyNotifierBuilder do "subject" => "participation_event_comment", "subject_params" => subject_params }, - author_id + options ) do subject_params |> Map.get("event_id") |> Events.list_actors_participants_for_event() |> Enum.map(& &1.id) - |> users_from_actor_ids(author_id) + |> users_from_actor_ids(Keyword.fetch!(options, :author_id)) end @spec users_from_actor_ids(list(), integer() | String.t()) :: list(Users.t()) diff --git a/test/service/activity/discussion_test.exs b/test/service/activity/discussion_test.exs new file mode 100644 index 000000000..e36ead866 --- /dev/null +++ b/test/service/activity/discussion_test.exs @@ -0,0 +1,119 @@ +defmodule Mobilizon.Service.Activity.DiscussionTest do + @moduledoc """ + Test the Discussion activity provider module + """ + + alias Mobilizon.Actors.Actor + alias Mobilizon.Discussions.{Comment, Discussion} + alias Mobilizon.Mention + alias Mobilizon.Service.Activity.Discussion, as: DiscussionActivity + alias Mobilizon.Service.Workers.{ActivityBuilder, LegacyNotifierBuilder} + alias Mobilizon.Users.User + + use Mobilizon.DataCase, async: true + use Oban.Testing, repo: Mobilizon.Storage.Repo + import Mobilizon.Factory + + describe "handle discussion with mentions" do + test "with no mentions" do + %Comment{} = comment = insert(:comment) + + %Discussion{ + id: discussion_id, + actor_id: group_id, + creator_id: author_id, + title: discussion_title, + slug: discussion_slug + } = discussion = insert(:discussion) + + assert {:ok, _} = + DiscussionActivity.insert_activity(%Discussion{discussion | last_comment: comment}, + subject: "discussion_created" + ) + + refute_enqueued( + worker: LegacyNotifierBuilder, + args: %{op: :discussion_mention} + ) + + assert_enqueued( + worker: ActivityBuilder, + args: %{ + "group_id" => group_id, + "author_id" => author_id, + "object_id" => to_string(discussion_id), + "object_type" => "discussion", + "op" => "build_activity", + "subject" => "discussion_created", + "subject_params" => %{ + "discussion_slug" => discussion_slug, + "discussion_title" => discussion_title + }, + "type" => "discussion" + } + ) + end + + test "with some mentions" do + %User{} = user = insert(:user) + %Actor{id: actor_id} = actor = insert(:actor, user: user) + + %Comment{actor_id: author_id} = comment = insert(:comment, text: "Hey @you") + + comment = %Comment{ + comment + | mentions: [ + %Mention{actor: actor, comment: comment, actor_id: actor_id} + ] + } + + %Discussion{ + id: discussion_id, + actor_id: group_id, + creator_id: discussion_author_id, + title: discussion_title, + slug: discussion_slug + } = discussion = insert(:discussion) + + assert {:ok, _} = + DiscussionActivity.insert_activity(%Discussion{discussion | last_comment: comment}, + subject: "discussion_created" + ) + + assert_enqueued( + worker: LegacyNotifierBuilder, + args: %{ + "author_id" => author_id, + "group_id" => group_id, + "mentions" => [actor_id], + "object_id" => to_string(discussion_id), + "object_type" => "discussion", + "op" => "legacy_notify", + "subject" => "discussion_mention", + "subject_params" => %{ + "discussion_slug" => discussion_slug, + "discussion_title" => discussion_title + }, + "type" => "discussion" + } + ) + + assert_enqueued( + worker: ActivityBuilder, + args: %{ + "group_id" => group_id, + "author_id" => discussion_author_id, + "object_id" => to_string(discussion_id), + "object_type" => "discussion", + "op" => "build_activity", + "subject" => "discussion_created", + "subject_params" => %{ + "discussion_slug" => discussion_slug, + "discussion_title" => discussion_title + }, + "type" => "discussion" + } + ) + end + end +end diff --git a/test/service/workers/legacy_notifier_builder_test.exs b/test/service/workers/legacy_notifier_builder_test.exs index 754657a01..2a079ab07 100644 --- a/test/service/workers/legacy_notifier_builder_test.exs +++ b/test/service/workers/legacy_notifier_builder_test.exs @@ -5,7 +5,7 @@ defmodule Mobilizon.Service.Workers.LegacyNotifierBuilderTest do alias Mobilizon.Activities.Activity alias Mobilizon.Actors.Actor - alias Mobilizon.Discussions.Comment + alias Mobilizon.Discussions.{Comment, Discussion} alias Mobilizon.Events.Event alias Mobilizon.Service.Notifier.Mock, as: NotifierMock alias Mobilizon.Service.Workers.LegacyNotifierBuilder @@ -26,7 +26,7 @@ defmodule Mobilizon.Service.Workers.LegacyNotifierBuilderTest do :ok end - @mentionned %{ + @comment_mentionned %{ "type" => "comment", "subject" => "event_comment_mention", "object_type" => "comment", @@ -34,6 +34,14 @@ defmodule Mobilizon.Service.Workers.LegacyNotifierBuilderTest do "op" => "legacy_notify" } + @discussion_mentionned %{ + "type" => "discussion", + "subject" => "discussion_mention", + "object_type" => "discussion", + "inserted_at" => DateTime.utc_now(), + "op" => "legacy_notify" + } + @announcement %{ "type" => "comment", "subject" => "participation_event_comment", @@ -55,7 +63,7 @@ defmodule Mobilizon.Service.Workers.LegacyNotifierBuilderTest do %Comment{id: comment_id} = insert(:comment, event: event, actor: actor) args = - Map.merge(@mentionned, %{ + Map.merge(@comment_mentionned, %{ "subject_params" => %{ "event_uuid" => uuid, "event_title" => title @@ -93,7 +101,7 @@ defmodule Mobilizon.Service.Workers.LegacyNotifierBuilderTest do %Comment{id: comment_id} = insert(:comment, event: event, actor: actor) args = - Map.merge(@mentionned, %{ + Map.merge(@comment_mentionned, %{ "subject_params" => %{ "event_uuid" => uuid, "event_title" => title @@ -194,4 +202,135 @@ defmodule Mobilizon.Service.Workers.LegacyNotifierBuilderTest do assert :ok == LegacyNotifierBuilder.perform(%Oban.Job{args: args}) end end + + describe "Generates a discussion mention notification " do + test "not if the actor is remote" do + %User{} = user1 = insert(:user) + + %Actor{id: actor_id} = actor = insert(:actor, user: user1) + %Actor{id: actor_id_2} = insert(:actor, domain: "remote.tld", user: nil) + + %Comment{id: comment_id} = insert(:comment, actor: actor) + + %Discussion{ + actor_id: group_id, + title: discussion_title, + slug: discussion_slug + } = insert(:discussion) + + args = + Map.merge(@discussion_mentionned, %{ + "subject_params" => %{ + "discussion_slug" => discussion_slug, + "discussion_title" => discussion_title + }, + "author_id" => actor_id, + "group_id" => group_id, + "object_id" => to_string(comment_id), + "mentions" => [actor_id_2] + }) + + NotifierMock + |> expect(:ready?, 0, fn -> true end) + |> expect(:send, 0, fn %User{}, + %Activity{ + type: :discussion, + subject: :discussion_mention, + object_type: :discussion + }, + [single_activity: true] -> + {:ok, :sent} + end) + + assert :ok == LegacyNotifierBuilder.perform(%Oban.Job{args: args}) + end + + test "not if the actor is not a member" do + %User{} = user1 = insert(:user) + + %Actor{id: actor_id} = actor = insert(:actor, user: user1) + %Actor{id: actor_id_2} = insert(:actor) + + %Comment{id: comment_id} = insert(:comment, actor: actor) + + %Discussion{ + actor_id: group_id, + title: discussion_title, + slug: discussion_slug + } = insert(:discussion) + + args = + Map.merge(@discussion_mentionned, %{ + "subject_params" => %{ + "discussion_slug" => discussion_slug, + "discussion_title" => discussion_title + }, + "author_id" => actor_id, + "group_id" => group_id, + "object_id" => to_string(comment_id), + "mentions" => [actor_id_2] + }) + + NotifierMock + |> expect(:ready?, 0, fn -> true end) + |> expect(:send, 0, fn %User{}, + %Activity{ + type: :discussion, + subject: :discussion_mention, + object_type: :discussion + }, + [single_activity: true] -> + {:ok, :sent} + end) + + assert :ok == LegacyNotifierBuilder.perform(%Oban.Job{args: args}) + end + + test "if the actor mentionned is local and a member" do + %User{} = user1 = insert(:user) + %User{} = user2 = insert(:user) + %Setting{} = settings2 = insert(:settings, user: user2, user_id: user2.id) + user2 = %User{user2 | settings: settings2} + + %Actor{id: actor_id} = actor = insert(:actor, user: user1) + %Actor{id: actor_id_2} = actor2 = insert(:actor, user: user2) + %Actor{} = group = insert(:group) + + insert(:member, actor: actor2, parent: group, role: :member) + + %Comment{id: comment_id} = insert(:comment, actor: actor) + + %Discussion{ + actor_id: group_id, + title: discussion_title, + slug: discussion_slug + } = insert(:discussion, actor: group) + + args = + Map.merge(@discussion_mentionned, %{ + "subject_params" => %{ + "discussion_slug" => discussion_slug, + "discussion_title" => discussion_title + }, + "author_id" => actor_id, + "group_id" => group_id, + "object_id" => to_string(comment_id), + "mentions" => [actor_id_2] + }) + + NotifierMock + |> expect(:ready?, fn -> true end) + |> expect(:send, fn %User{}, + %Activity{ + type: :discussion, + subject: :discussion_mention, + object_type: :discussion + }, + [single_activity: true] -> + {:ok, :sent} + end) + + assert :ok == LegacyNotifierBuilder.perform(%Oban.Job{args: args}) + end + end end