From 7c8003c3fcdcab075b9722ab236bf2d1d0e0e8cd Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Sun, 15 Mar 2020 21:00:12 +0300 Subject: [PATCH 1/4] [#1364] Improved control over generation / sending of notifications. Fixed blocking / muting users notifications issue. Added tests. --- lib/pleroma/activity.ex | 10 ++ lib/pleroma/notification.ex | 129 +++++++++++++----- lib/pleroma/thread_mute.ex | 37 ++++- lib/pleroma/user.ex | 50 +++++-- lib/pleroma/user_relationship.ex | 9 +- .../web/activity_pub/transmogrifier.ex | 8 +- test/notification_test.exs | 112 ++++++++++++++- 7 files changed, 292 insertions(+), 63 deletions(-) diff --git a/lib/pleroma/activity.ex b/lib/pleroma/activity.ex index 6ca05f74e..bbaa561a7 100644 --- a/lib/pleroma/activity.ex +++ b/lib/pleroma/activity.ex @@ -95,6 +95,16 @@ def with_preloaded_object(query, join_type \\ :inner) do |> preload([activity, object: object], object: object) end + def user_actor(%Activity{actor: nil}), do: nil + + def user_actor(%Activity{} = activity) do + with %User{} <- activity.user_actor do + activity.user_actor + else + _ -> User.get_cached_by_ap_id(activity.actor) + end + end + def with_joined_user_actor(query, join_type \\ :inner) do join(query, join_type, [activity], u in User, on: u.ap_id == activity.actor, diff --git a/lib/pleroma/notification.ex b/lib/pleroma/notification.ex index 60dba3434..0d7a6610a 100644 --- a/lib/pleroma/notification.ex +++ b/lib/pleroma/notification.ex @@ -10,6 +10,7 @@ defmodule Pleroma.Notification do alias Pleroma.Object alias Pleroma.Pagination alias Pleroma.Repo + alias Pleroma.ThreadMute alias Pleroma.User alias Pleroma.Web.CommonAPI.Utils alias Pleroma.Web.Push @@ -17,6 +18,7 @@ defmodule Pleroma.Notification do import Ecto.Query import Ecto.Changeset + require Logger @type t :: %__MODULE__{} @@ -101,7 +103,7 @@ defp exclude_notification_muted(query, user, opts) do query |> where([n, a], a.actor not in ^notification_muted_ap_ids) - |> join(:left, [n, a], tm in Pleroma.ThreadMute, + |> join(:left, [n, a], tm in ThreadMute, on: tm.user_id == ^user.id and tm.context == fragment("?->>'context'", a.data) ) |> where([n, a, o, tm], is_nil(tm.user_id)) @@ -284,58 +286,108 @@ def dismiss(%{id: user_id} = _user, id) do def create_notifications(%Activity{data: %{"to" => _, "type" => "Create"}} = activity) do object = Object.normalize(activity) - unless object && object.data["type"] == "Answer" do - users = get_notified_from_activity(activity) - notifications = Enum.map(users, fn user -> create_notification(activity, user) end) - {:ok, notifications} - else + if object && object.data["type"] == "Answer" do {:ok, []} + else + do_create_notifications(activity) end end def create_notifications(%Activity{data: %{"type" => type}} = activity) when type in ["Like", "Announce", "Follow", "Move", "EmojiReact"] do - notifications = - activity - |> get_notified_from_activity() - |> Enum.map(&create_notification(activity, &1)) - - {:ok, notifications} + do_create_notifications(activity) end def create_notifications(_), do: {:ok, []} + defp do_create_notifications(%Activity{} = activity) do + {enabled_receivers, disabled_receivers} = get_notified_from_activity(activity) + potential_receivers = enabled_receivers ++ disabled_receivers + + notifications = + Enum.map(potential_receivers, fn user -> + do_send = user in enabled_receivers + create_notification(activity, user, do_send) + end) + + {:ok, notifications} + end + # TODO move to sql, too. - def create_notification(%Activity{} = activity, %User{} = user) do + def create_notification(%Activity{} = activity, %User{} = user, do_send \\ true) do unless skip?(activity, user) do notification = %Notification{user_id: user.id, activity: activity} {:ok, notification} = Repo.insert(notification) - ["user", "user:notification"] - |> Streamer.stream(notification) + if do_send do + Streamer.stream(["user", "user:notification"], notification) + Push.send(notification) + end - Push.send(notification) notification end end + @doc """ + Returns a tuple with 2 elements: + {enabled notification receivers, currently disabled receivers (blocking / [thread] muting)} + """ def get_notified_from_activity(activity, local_only \\ true) def get_notified_from_activity(%Activity{data: %{"type" => type}} = activity, local_only) when type in ["Create", "Like", "Announce", "Follow", "Move", "EmojiReact"] do - [] - |> Utils.maybe_notify_to_recipients(activity) - |> Utils.maybe_notify_mentioned_recipients(activity) - |> Utils.maybe_notify_subscribers(activity) - |> Utils.maybe_notify_followers(activity) - |> Enum.uniq() - |> User.get_users_from_set(local_only) + potential_receiver_ap_ids = + [] + |> Utils.maybe_notify_to_recipients(activity) + |> Utils.maybe_notify_mentioned_recipients(activity) + |> Utils.maybe_notify_subscribers(activity) + |> Utils.maybe_notify_followers(activity) + |> Enum.uniq() + + notification_enabled_ap_ids = + potential_receiver_ap_ids + |> exclude_relation_restricting_ap_ids(activity) + |> exclude_thread_muter_ap_ids(activity) + + potential_receivers = + potential_receiver_ap_ids + |> Enum.uniq() + |> User.get_users_from_set(local_only) + + notification_enabled_users = + Enum.filter(potential_receivers, fn u -> u.ap_id in notification_enabled_ap_ids end) + + {notification_enabled_users, potential_receivers -- notification_enabled_users} end - def get_notified_from_activity(_, _local_only), do: [] + def get_notified_from_activity(_, _local_only), do: {[], []} + + @doc "Filters out AP IDs of users basing on their relationships with activity actor user" + def exclude_relation_restricting_ap_ids([], _activity), do: [] + + def exclude_relation_restricting_ap_ids(ap_ids, %Activity{} = activity) do + relation_restricted_ap_ids = + activity + |> Activity.user_actor() + |> User.incoming_relations_ungrouped_ap_ids([ + :block, + :notification_mute + ]) + + Enum.uniq(ap_ids) -- relation_restricted_ap_ids + end + + @doc "Filters out AP IDs of users who mute activity thread" + def exclude_thread_muter_ap_ids([], _activity), do: [] + + def exclude_thread_muter_ap_ids(ap_ids, %Activity{} = activity) do + thread_muter_ap_ids = ThreadMute.muter_ap_ids(activity.data["context"]) + + Enum.uniq(ap_ids) -- thread_muter_ap_ids + end @spec skip?(Activity.t(), User.t()) :: boolean() - def skip?(activity, user) do + def skip?(%Activity{} = activity, %User{} = user) do [ :self, :followers, @@ -344,18 +396,20 @@ def skip?(activity, user) do :non_follows, :recently_followed ] - |> Enum.any?(&skip?(&1, activity, user)) + |> Enum.find(&skip?(&1, activity, user)) end + def skip?(_, _), do: false + @spec skip?(atom(), Activity.t(), User.t()) :: boolean() - def skip?(:self, activity, user) do + def skip?(:self, %Activity{} = activity, %User{} = user) do activity.data["actor"] == user.ap_id end def skip?( :followers, - activity, - %{notification_settings: %{followers: false}} = user + %Activity{} = activity, + %User{notification_settings: %{followers: false}} = user ) do actor = activity.data["actor"] follower = User.get_cached_by_ap_id(actor) @@ -364,15 +418,19 @@ def skip?( def skip?( :non_followers, - activity, - %{notification_settings: %{non_followers: false}} = user + %Activity{} = activity, + %User{notification_settings: %{non_followers: false}} = user ) do actor = activity.data["actor"] follower = User.get_cached_by_ap_id(actor) !User.following?(follower, user) end - def skip?(:follows, activity, %{notification_settings: %{follows: false}} = user) do + def skip?( + :follows, + %Activity{} = activity, + %User{notification_settings: %{follows: false}} = user + ) do actor = activity.data["actor"] followed = User.get_cached_by_ap_id(actor) User.following?(user, followed) @@ -380,15 +438,16 @@ def skip?(:follows, activity, %{notification_settings: %{follows: false}} = user def skip?( :non_follows, - activity, - %{notification_settings: %{non_follows: false}} = user + %Activity{} = activity, + %User{notification_settings: %{non_follows: false}} = user ) do actor = activity.data["actor"] followed = User.get_cached_by_ap_id(actor) !User.following?(user, followed) end - def skip?(:recently_followed, %{data: %{"type" => "Follow"}} = activity, user) do + # To do: consider defining recency in hours and checking FollowingRelationship with a single SQL + def skip?(:recently_followed, %Activity{data: %{"type" => "Follow"}} = activity, %User{} = user) do actor = activity.data["actor"] Notification.for_user(user) diff --git a/lib/pleroma/thread_mute.ex b/lib/pleroma/thread_mute.ex index cc815430a..2b4cf02cf 100644 --- a/lib/pleroma/thread_mute.ex +++ b/lib/pleroma/thread_mute.ex @@ -9,7 +9,8 @@ defmodule Pleroma.ThreadMute do alias Pleroma.ThreadMute alias Pleroma.User - require Ecto.Query + import Ecto.Changeset + import Ecto.Query schema "thread_mutes" do belongs_to(:user, User, type: FlakeId.Ecto.CompatType) @@ -18,19 +19,43 @@ defmodule Pleroma.ThreadMute do def changeset(mute, params \\ %{}) do mute - |> Ecto.Changeset.cast(params, [:user_id, :context]) - |> Ecto.Changeset.foreign_key_constraint(:user_id) - |> Ecto.Changeset.unique_constraint(:user_id, name: :unique_index) + |> cast(params, [:user_id, :context]) + |> foreign_key_constraint(:user_id) + |> unique_constraint(:user_id, name: :unique_index) end def query(user_id, context) do {:ok, user_id} = FlakeId.Ecto.CompatType.dump(user_id) ThreadMute - |> Ecto.Query.where(user_id: ^user_id) - |> Ecto.Query.where(context: ^context) + |> where(user_id: ^user_id) + |> where(context: ^context) end + def muters_query(context) do + ThreadMute + |> join(:inner, [tm], u in assoc(tm, :user)) + |> where([tm], tm.context == ^context) + |> select([tm, u], u.ap_id) + end + + def muter_ap_ids(context, ap_ids \\ nil) + + def muter_ap_ids(context, ap_ids) when context not in [nil, ""] do + context + |> muters_query() + |> maybe_filter_on_ap_id(ap_ids) + |> Repo.all() + end + + def muter_ap_ids(_context, _ap_ids), do: [] + + defp maybe_filter_on_ap_id(query, ap_ids) when is_list(ap_ids) do + where(query, [tm, u], u.ap_id in ^ap_ids) + end + + defp maybe_filter_on_ap_id(query, _ap_ids), do: query + def add_mute(user_id, context) do %ThreadMute{} |> changeset(%{user_id: user_id, context: context}) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index db510d957..8c8ecfe35 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -149,22 +149,26 @@ defmodule Pleroma.User do {outgoing_relation, outgoing_relation_target}, {incoming_relation, incoming_relation_source} ]} <- @user_relationships_config do - # Definitions of `has_many :blocker_blocks`, `has_many :muter_mutes` etc. + # Definitions of `has_many` relations: :blocker_blocks, :muter_mutes, :reblog_muter_mutes, + # :notification_muter_mutes, :subscribee_subscriptions has_many(outgoing_relation, UserRelationship, foreign_key: :source_id, where: [relationship_type: relationship_type] ) - # Definitions of `has_many :blockee_blocks`, `has_many :mutee_mutes` etc. + # Definitions of `has_many` relations: :blockee_blocks, :mutee_mutes, :reblog_mutee_mutes, + # :notification_mutee_mutes, :subscriber_subscriptions has_many(incoming_relation, UserRelationship, foreign_key: :target_id, where: [relationship_type: relationship_type] ) - # Definitions of `has_many :blocked_users`, `has_many :muted_users` etc. + # Definitions of `has_many` relations: :blocked_users, :muted_users, :reblog_muted_users, + # :notification_muted_users, :subscriber_users has_many(outgoing_relation_target, through: [outgoing_relation, :target]) - # Definitions of `has_many :blocker_users`, `has_many :muter_users` etc. + # Definitions of `has_many` relations: :blocker_users, :muter_users, :reblog_muter_users, + # :notification_muter_users, :subscribee_users has_many(incoming_relation_source, through: [incoming_relation, :source]) end @@ -184,7 +188,9 @@ defmodule Pleroma.User do for {_relationship_type, [{_outgoing_relation, outgoing_relation_target}, _]} <- @user_relationships_config do - # Definitions of `blocked_users_relation/1`, `muted_users_relation/1`, etc. + # `def blocked_users_relation/2`, `def muted_users_relation/2`, + # `def reblog_muted_users_relation/2`, `def notification_muted_users/2`, + # `def subscriber_users/2` def unquote(:"#{outgoing_relation_target}_relation")(user, restrict_deactivated? \\ false) do target_users_query = assoc(user, unquote(outgoing_relation_target)) @@ -195,7 +201,8 @@ def unquote(:"#{outgoing_relation_target}_relation")(user, restrict_deactivated? end end - # Definitions of `blocked_users/1`, `muted_users/1`, etc. + # `def blocked_users/2`, `def muted_users/2`, `def reblog_muted_users/2`, + # `def notification_muted_users/2`, `def subscriber_users/2` def unquote(outgoing_relation_target)(user, restrict_deactivated? \\ false) do __MODULE__ |> apply(unquote(:"#{outgoing_relation_target}_relation"), [ @@ -205,7 +212,8 @@ def unquote(outgoing_relation_target)(user, restrict_deactivated? \\ false) do |> Repo.all() end - # Definitions of `blocked_users_ap_ids/1`, `muted_users_ap_ids/1`, etc. + # `def blocked_users_ap_ids/2`, `def muted_users_ap_ids/2`, `def reblog_muted_users_ap_ids/2`, + # `def notification_muted_users_ap_ids/2`, `def subscriber_users_ap_ids/2` def unquote(:"#{outgoing_relation_target}_ap_ids")(user, restrict_deactivated? \\ false) do __MODULE__ |> apply(unquote(:"#{outgoing_relation_target}_relation"), [ @@ -1217,7 +1225,9 @@ def subscribed_to?(%User{} = user, %{ap_id: ap_id}) do E.g. `outgoing_relations_ap_ids(user, [:block])` -> `%{block: ["https://some.site/users/userapid"]}` """ @spec outgoing_relations_ap_ids(User.t(), list(atom())) :: %{atom() => list(String.t())} - def outgoing_relations_ap_ids(_, []), do: %{} + def outgoing_relations_ap_ids(_user, []), do: %{} + + def outgoing_relations_ap_ids(nil, _relationship_types), do: %{} def outgoing_relations_ap_ids(%User{} = user, relationship_types) when is_list(relationship_types) do @@ -1238,6 +1248,30 @@ def outgoing_relations_ap_ids(%User{} = user, relationship_types) ) end + def incoming_relations_ungrouped_ap_ids(user, relationship_types, ap_ids \\ nil) + + def incoming_relations_ungrouped_ap_ids(_user, [], _ap_ids), do: [] + + def incoming_relations_ungrouped_ap_ids(nil, _relationship_types, _ap_ids), do: [] + + def incoming_relations_ungrouped_ap_ids(%User{} = user, relationship_types, ap_ids) + when is_list(relationship_types) do + user + |> assoc(:incoming_relationships) + |> join(:inner, [user_rel], u in assoc(user_rel, :source)) + |> where([user_rel, u], user_rel.relationship_type in ^relationship_types) + |> maybe_filter_on_ap_id(ap_ids) + |> select([user_rel, u], u.ap_id) + |> distinct(true) + |> Repo.all() + end + + defp maybe_filter_on_ap_id(query, ap_ids) when is_list(ap_ids) do + where(query, [user_rel, u], u.ap_id in ^ap_ids) + end + + defp maybe_filter_on_ap_id(query, _ap_ids), do: query + def deactivate_async(user, status \\ true) do BackgroundWorker.enqueue("deactivate_user", %{"user_id" => user.id, "status" => status}) end diff --git a/lib/pleroma/user_relationship.ex b/lib/pleroma/user_relationship.ex index 393947942..01b6ace9d 100644 --- a/lib/pleroma/user_relationship.ex +++ b/lib/pleroma/user_relationship.ex @@ -21,15 +21,18 @@ defmodule Pleroma.UserRelationship do end for relationship_type <- Keyword.keys(UserRelationshipTypeEnum.__enum_map__()) do - # Definitions of `create_block/2`, `create_mute/2` etc. + # `def create_block/2`, `def create_mute/2`, `def create_reblog_mute/2`, + # `def create_notification_mute/2`, `def create_inverse_subscription/2` def unquote(:"create_#{relationship_type}")(source, target), do: create(unquote(relationship_type), source, target) - # Definitions of `delete_block/2`, `delete_mute/2` etc. + # `def delete_block/2`, `def delete_mute/2`, `def delete_reblog_mute/2`, + # `def delete_notification_mute/2`, `def delete_inverse_subscription/2` def unquote(:"delete_#{relationship_type}")(source, target), do: delete(unquote(relationship_type), source, target) - # Definitions of `block_exists?/2`, `mute_exists?/2` etc. + # `def block_exists?/2`, `def mute_exists?/2`, `def reblog_mute_exists?/2`, + # `def notification_mute_exists?/2`, `def inverse_subscription_exists?/2` def unquote(:"#{relationship_type}_exists?")(source, target), do: exists?(unquote(relationship_type), source, target) end diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index 9cd3de705..d6549a932 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -1108,13 +1108,11 @@ def add_hashtags(object) do end def add_mention_tags(object) do - mentions = - object - |> Utils.get_notified_from_object() - |> Enum.map(&build_mention_tag/1) + {enabled_receivers, disabled_receivers} = Utils.get_notified_from_object(object) + potential_receivers = enabled_receivers ++ disabled_receivers + mentions = Enum.map(potential_receivers, &build_mention_tag/1) tags = object["tag"] || [] - Map.put(object, "tag", tags ++ mentions) end diff --git a/test/notification_test.exs b/test/notification_test.exs index 56a581810..bc2d80f05 100644 --- a/test/notification_test.exs +++ b/test/notification_test.exs @@ -6,12 +6,14 @@ defmodule Pleroma.NotificationTest do use Pleroma.DataCase import Pleroma.Factory + import Mock alias Pleroma.Notification alias Pleroma.Tests.ObanHelpers alias Pleroma.User alias Pleroma.Web.ActivityPub.Transmogrifier alias Pleroma.Web.CommonAPI + alias Pleroma.Web.Push alias Pleroma.Web.Streamer describe "create_notifications" do @@ -382,7 +384,7 @@ test "Returns recent notifications" do end end - describe "notification target determination" do + describe "notification target determination / get_notified_from_activity/2" do test "it sends notifications to addressed users in new messages" do user = insert(:user) other_user = insert(:user) @@ -392,7 +394,9 @@ test "it sends notifications to addressed users in new messages" do "status" => "hey @#{other_user.nickname}!" }) - assert other_user in Notification.get_notified_from_activity(activity) + {enabled_receivers, _disabled_receivers} = Notification.get_notified_from_activity(activity) + + assert other_user in enabled_receivers end test "it sends notifications to mentioned users in new messages" do @@ -420,7 +424,9 @@ test "it sends notifications to mentioned users in new messages" do {:ok, activity} = Transmogrifier.handle_incoming(create_activity) - assert other_user in Notification.get_notified_from_activity(activity) + {enabled_receivers, _disabled_receivers} = Notification.get_notified_from_activity(activity) + + assert other_user in enabled_receivers end test "it does not send notifications to users who are only cc in new messages" do @@ -442,7 +448,9 @@ test "it does not send notifications to users who are only cc in new messages" d {:ok, activity} = Transmogrifier.handle_incoming(create_activity) - assert other_user not in Notification.get_notified_from_activity(activity) + {enabled_receivers, _disabled_receivers} = Notification.get_notified_from_activity(activity) + + assert other_user not in enabled_receivers end test "it does not send notification to mentioned users in likes" do @@ -457,7 +465,10 @@ test "it does not send notification to mentioned users in likes" do {:ok, activity_two, _} = CommonAPI.favorite(activity_one.id, third_user) - assert other_user not in Notification.get_notified_from_activity(activity_two) + {enabled_receivers, _disabled_receivers} = + Notification.get_notified_from_activity(activity_two) + + assert other_user not in enabled_receivers end test "it does not send notification to mentioned users in announces" do @@ -472,7 +483,96 @@ test "it does not send notification to mentioned users in announces" do {:ok, activity_two, _} = CommonAPI.repeat(activity_one.id, third_user) - assert other_user not in Notification.get_notified_from_activity(activity_two) + {enabled_receivers, _disabled_receivers} = + Notification.get_notified_from_activity(activity_two) + + assert other_user not in enabled_receivers + end + + test_with_mock "it returns blocking recipient in disabled recipients list", + Push, + [:passthrough], + [] do + user = insert(:user) + other_user = insert(:user) + {:ok, _user_relationship} = User.block(other_user, user) + + {:ok, activity} = CommonAPI.post(user, %{"status" => "hey @#{other_user.nickname}!"}) + + {enabled_receivers, disabled_receivers} = Notification.get_notified_from_activity(activity) + + assert [] == enabled_receivers + assert [other_user] == disabled_receivers + + assert 1 == length(Repo.all(Notification)) + refute called(Push.send(:_)) + end + + test_with_mock "it returns notification-muting recipient in disabled recipients list", + Push, + [:passthrough], + [] do + user = insert(:user) + other_user = insert(:user) + {:ok, _user_relationships} = User.mute(other_user, user) + + {:ok, activity} = CommonAPI.post(user, %{"status" => "hey @#{other_user.nickname}!"}) + + {enabled_receivers, disabled_receivers} = Notification.get_notified_from_activity(activity) + + assert [] == enabled_receivers + assert [other_user] == disabled_receivers + + assert 1 == length(Repo.all(Notification)) + refute called(Push.send(:_)) + end + + test_with_mock "it returns thread-muting recipient in disabled recipients list", + Push, + [:passthrough], + [] do + user = insert(:user) + other_user = insert(:user) + + {:ok, activity} = CommonAPI.post(user, %{"status" => "hey @#{other_user.nickname}!"}) + + {:ok, _} = CommonAPI.add_mute(other_user, activity) + + {:ok, same_context_activity} = + CommonAPI.post(user, %{ + "status" => "hey-hey-hey @#{other_user.nickname}!", + "in_reply_to_status_id" => activity.id + }) + + {enabled_receivers, disabled_receivers} = + Notification.get_notified_from_activity(same_context_activity) + + assert [other_user] == disabled_receivers + refute other_user in enabled_receivers + + [pre_mute_notification, post_mute_notification] = + Repo.all(from(n in Notification, where: n.user_id == ^other_user.id, order_by: n.id)) + + pre_mute_notification_id = pre_mute_notification.id + post_mute_notification_id = post_mute_notification.id + + assert called( + Push.send( + :meck.is(fn + %Notification{id: ^pre_mute_notification_id} -> true + _ -> false + end) + ) + ) + + refute called( + Push.send( + :meck.is(fn + %Notification{id: ^post_mute_notification_id} -> true + _ -> false + end) + ) + ) end end From 74388336852b18d5d5f108a8305f1a038301f7a1 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Mon, 16 Mar 2020 21:58:10 +0300 Subject: [PATCH 2/4] [#1364] Improved notification-related tests. --- lib/pleroma/notification.ex | 1 + test/notification_test.exs | 121 +++++++++++++++++++++++------------- 2 files changed, 79 insertions(+), 43 deletions(-) diff --git a/lib/pleroma/notification.ex b/lib/pleroma/notification.ex index 0d7a6610a..104368fd1 100644 --- a/lib/pleroma/notification.ex +++ b/lib/pleroma/notification.ex @@ -344,6 +344,7 @@ def get_notified_from_activity(%Activity{data: %{"type" => type}} = activity, lo |> Utils.maybe_notify_followers(activity) |> Enum.uniq() + # Since even subscribers and followers can mute / thread-mute, filtering all above AP IDs notification_enabled_ap_ids = potential_receiver_ap_ids |> exclude_relation_restricting_ap_ids(activity) diff --git a/test/notification_test.exs b/test/notification_test.exs index bc2d80f05..a7282c929 100644 --- a/test/notification_test.exs +++ b/test/notification_test.exs @@ -82,6 +82,80 @@ test "does not create a notification for subscribed users if status is a reply" end end + describe "CommonApi.post/2 notification-related functionality" do + test_with_mock "creates but does NOT send notification to blocker user", + Push, + [:passthrough], + [] do + user = insert(:user) + blocker = insert(:user) + {:ok, _user_relationship} = User.block(blocker, user) + + {:ok, _activity} = CommonAPI.post(user, %{"status" => "hey @#{blocker.nickname}!"}) + + blocker_id = blocker.id + assert [%Notification{user_id: ^blocker_id}] = Repo.all(Notification) + refute called(Push.send(:_)) + end + + test_with_mock "creates but does NOT send notification to notification-muter user", + Push, + [:passthrough], + [] do + user = insert(:user) + muter = insert(:user) + {:ok, _user_relationships} = User.mute(muter, user) + + {:ok, _activity} = CommonAPI.post(user, %{"status" => "hey @#{muter.nickname}!"}) + + muter_id = muter.id + assert [%Notification{user_id: ^muter_id}] = Repo.all(Notification) + refute called(Push.send(:_)) + end + + test_with_mock "creates but does NOT send notification to thread-muter user", + Push, + [:passthrough], + [] do + user = insert(:user) + thread_muter = insert(:user) + + {:ok, activity} = CommonAPI.post(user, %{"status" => "hey @#{thread_muter.nickname}!"}) + + {:ok, _} = CommonAPI.add_mute(thread_muter, activity) + + {:ok, _same_context_activity} = + CommonAPI.post(user, %{ + "status" => "hey-hey-hey @#{thread_muter.nickname}!", + "in_reply_to_status_id" => activity.id + }) + + [pre_mute_notification, post_mute_notification] = + Repo.all(from(n in Notification, where: n.user_id == ^thread_muter.id, order_by: n.id)) + + pre_mute_notification_id = pre_mute_notification.id + post_mute_notification_id = post_mute_notification.id + + assert called( + Push.send( + :meck.is(fn + %Notification{id: ^pre_mute_notification_id} -> true + _ -> false + end) + ) + ) + + refute called( + Push.send( + :meck.is(fn + %Notification{id: ^post_mute_notification_id} -> true + _ -> false + end) + ) + ) + end + end + describe "create_notification" do @tag needs_streamer: true test "it creates a notification for user and send to the 'user' and the 'user:notification' stream" do @@ -489,10 +563,7 @@ test "it does not send notification to mentioned users in announces" do assert other_user not in enabled_receivers end - test_with_mock "it returns blocking recipient in disabled recipients list", - Push, - [:passthrough], - [] do + test "it returns blocking recipient in disabled recipients list" do user = insert(:user) other_user = insert(:user) {:ok, _user_relationship} = User.block(other_user, user) @@ -503,15 +574,9 @@ test "it does not send notification to mentioned users in announces" do assert [] == enabled_receivers assert [other_user] == disabled_receivers - - assert 1 == length(Repo.all(Notification)) - refute called(Push.send(:_)) end - test_with_mock "it returns notification-muting recipient in disabled recipients list", - Push, - [:passthrough], - [] do + test "it returns notification-muting recipient in disabled recipients list" do user = insert(:user) other_user = insert(:user) {:ok, _user_relationships} = User.mute(other_user, user) @@ -522,15 +587,9 @@ test "it does not send notification to mentioned users in announces" do assert [] == enabled_receivers assert [other_user] == disabled_receivers - - assert 1 == length(Repo.all(Notification)) - refute called(Push.send(:_)) end - test_with_mock "it returns thread-muting recipient in disabled recipients list", - Push, - [:passthrough], - [] do + test "it returns thread-muting recipient in disabled recipients list" do user = insert(:user) other_user = insert(:user) @@ -549,30 +608,6 @@ test "it does not send notification to mentioned users in announces" do assert [other_user] == disabled_receivers refute other_user in enabled_receivers - - [pre_mute_notification, post_mute_notification] = - Repo.all(from(n in Notification, where: n.user_id == ^other_user.id, order_by: n.id)) - - pre_mute_notification_id = pre_mute_notification.id - post_mute_notification_id = post_mute_notification.id - - assert called( - Push.send( - :meck.is(fn - %Notification{id: ^pre_mute_notification_id} -> true - _ -> false - end) - ) - ) - - refute called( - Push.send( - :meck.is(fn - %Notification{id: ^post_mute_notification_id} -> true - _ -> false - end) - ) - ) end end @@ -820,7 +855,7 @@ test "it doesn't return notifications for blocked user" do assert Notification.for_user(user) == [] end - test "it doesn't return notificatitons for blocked domain" do + test "it doesn't return notifications for blocked domain" do user = insert(:user) blocked = insert(:user, ap_id: "http://some-domain.com") {:ok, user} = User.block_domain(user, "some-domain.com") From e743c2232970e321c833604b232520587ad8e402 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Wed, 25 Mar 2020 09:04:00 +0300 Subject: [PATCH 3/4] Fixed incorrect usage of "relations" as a short form of "relationships". --- config/description.exs | 2 +- lib/pleroma/notification.ex | 6 +++--- lib/pleroma/user.ex | 20 +++++++++---------- lib/pleroma/web/activity_pub/activity_pub.ex | 8 ++++---- .../controllers/account_controller.ex | 10 +++++++--- lib/pleroma/web/streamer/worker.ex | 2 +- test/user_test.exs | 6 +++--- 7 files changed, 29 insertions(+), 25 deletions(-) diff --git a/config/description.exs b/config/description.exs index 732c76734..68fa8b03b 100644 --- a/config/description.exs +++ b/config/description.exs @@ -2442,7 +2442,7 @@ %{ key: :relations_actions, type: [:tuple, {:list, :tuple}], - description: "For actions on relations with all users (follow, unfollow)", + description: "For actions on relationships with all users (follow, unfollow)", suggestions: [{1000, 10}, [{10_000, 10}, {10_000, 50}]] }, %{ diff --git a/lib/pleroma/notification.ex b/lib/pleroma/notification.ex index 104368fd1..bc691dce3 100644 --- a/lib/pleroma/notification.ex +++ b/lib/pleroma/notification.ex @@ -39,11 +39,11 @@ def changeset(%Notification{} = notification, attrs) do end defp for_user_query_ap_id_opts(user, opts) do - ap_id_relations = + ap_id_relationships = [:block] ++ if opts[@include_muted_option], do: [], else: [:notification_mute] - preloaded_ap_ids = User.outgoing_relations_ap_ids(user, ap_id_relations) + preloaded_ap_ids = User.outgoing_relationships_ap_ids(user, ap_id_relationships) exclude_blocked_opts = Map.merge(%{blocked_users_ap_ids: preloaded_ap_ids[:block]}, opts) @@ -370,7 +370,7 @@ def exclude_relation_restricting_ap_ids(ap_ids, %Activity{} = activity) do relation_restricted_ap_ids = activity |> Activity.user_actor() - |> User.incoming_relations_ungrouped_ap_ids([ + |> User.incoming_relationships_ungrouped_ap_ids([ :block, :notification_mute ]) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 05efc74d4..4919c8e58 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -1222,15 +1222,15 @@ def subscribed_to?(%User{} = user, %{ap_id: ap_id}) do end @doc """ - Returns map of outgoing (blocked, muted etc.) relations' user AP IDs by relation type. - E.g. `outgoing_relations_ap_ids(user, [:block])` -> `%{block: ["https://some.site/users/userapid"]}` + Returns map of outgoing (blocked, muted etc.) relationships' user AP IDs by relation type. + E.g. `outgoing_relationships_ap_ids(user, [:block])` -> `%{block: ["https://some.site/users/userapid"]}` """ - @spec outgoing_relations_ap_ids(User.t(), list(atom())) :: %{atom() => list(String.t())} - def outgoing_relations_ap_ids(_user, []), do: %{} + @spec outgoing_relationships_ap_ids(User.t(), list(atom())) :: %{atom() => list(String.t())} + def outgoing_relationships_ap_ids(_user, []), do: %{} - def outgoing_relations_ap_ids(nil, _relationship_types), do: %{} + def outgoing_relationships_ap_ids(nil, _relationship_types), do: %{} - def outgoing_relations_ap_ids(%User{} = user, relationship_types) + def outgoing_relationships_ap_ids(%User{} = user, relationship_types) when is_list(relationship_types) do db_result = user @@ -1249,13 +1249,13 @@ def outgoing_relations_ap_ids(%User{} = user, relationship_types) ) end - def incoming_relations_ungrouped_ap_ids(user, relationship_types, ap_ids \\ nil) + def incoming_relationships_ungrouped_ap_ids(user, relationship_types, ap_ids \\ nil) - def incoming_relations_ungrouped_ap_ids(_user, [], _ap_ids), do: [] + def incoming_relationships_ungrouped_ap_ids(_user, [], _ap_ids), do: [] - def incoming_relations_ungrouped_ap_ids(nil, _relationship_types, _ap_ids), do: [] + def incoming_relationships_ungrouped_ap_ids(nil, _relationship_types, _ap_ids), do: [] - def incoming_relations_ungrouped_ap_ids(%User{} = user, relationship_types, ap_ids) + def incoming_relationships_ungrouped_ap_ids(%User{} = user, relationship_types, ap_ids) when is_list(relationship_types) do user |> assoc(:incoming_relationships) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index d9f74b6a4..60e74758f 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -1230,17 +1230,17 @@ defp maybe_order(query, _), do: query defp fetch_activities_query_ap_ids_ops(opts) do source_user = opts["muting_user"] - ap_id_relations = if source_user, do: [:mute, :reblog_mute], else: [] + ap_id_relationships = if source_user, do: [:mute, :reblog_mute], else: [] - ap_id_relations = - ap_id_relations ++ + ap_id_relationships = + ap_id_relationships ++ if opts["blocking_user"] && opts["blocking_user"] == source_user do [:block] else [] end - preloaded_ap_ids = User.outgoing_relations_ap_ids(source_user, ap_id_relations) + preloaded_ap_ids = User.outgoing_relationships_ap_ids(source_user, ap_id_relationships) restrict_blocked_opts = Map.merge(%{"blocked_users_ap_ids" => preloaded_ap_ids[:block]}, opts) restrict_muted_opts = Map.merge(%{"muted_users_ap_ids" => preloaded_ap_ids[:mute]}, opts) diff --git a/lib/pleroma/web/mastodon_api/controllers/account_controller.ex b/lib/pleroma/web/mastodon_api/controllers/account_controller.ex index 88c997b9f..9d83a9fc1 100644 --- a/lib/pleroma/web/mastodon_api/controllers/account_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/account_controller.ex @@ -63,11 +63,15 @@ defmodule Pleroma.Web.MastodonAPI.AccountController do when action != :create ) - @relations [:follow, :unfollow] + @relationship_actions [:follow, :unfollow] @needs_account ~W(followers following lists follow unfollow mute unmute block unblock)a - plug(RateLimiter, [name: :relations_id_action, params: ["id", "uri"]] when action in @relations) - plug(RateLimiter, [name: :relations_actions] when action in @relations) + plug( + RateLimiter, + [name: :relation_id_action, params: ["id", "uri"]] when action in @relationship_actions + ) + + plug(RateLimiter, [name: :relations_actions] when action in @relationship_actions) plug(RateLimiter, [name: :app_account_creation] when action == :create) plug(:assign_account_by_id when action in @needs_account) diff --git a/lib/pleroma/web/streamer/worker.ex b/lib/pleroma/web/streamer/worker.ex index 29f992a67..abfed21c8 100644 --- a/lib/pleroma/web/streamer/worker.ex +++ b/lib/pleroma/web/streamer/worker.ex @@ -130,7 +130,7 @@ defp do_stream(%{topic: topic, item: item}) do defp should_send?(%User{} = user, %Activity{} = item) do %{block: blocked_ap_ids, mute: muted_ap_ids, reblog_mute: reblog_muted_ap_ids} = - User.outgoing_relations_ap_ids(user, [:block, :mute, :reblog_mute]) + User.outgoing_relationships_ap_ids(user, [:block, :mute, :reblog_mute]) recipient_blocks = MapSet.new(blocked_ap_ids ++ muted_ap_ids) recipients = MapSet.new(item.recipients) diff --git a/test/user_test.exs b/test/user_test.exs index b07fed42b..f3d044a80 100644 --- a/test/user_test.exs +++ b/test/user_test.exs @@ -86,7 +86,7 @@ test "returns invisible actor" do {:ok, user: insert(:user)} end - test "outgoing_relations_ap_ids/1", %{user: user} do + test "outgoing_relationships_ap_ids/1", %{user: user} do rel_types = [:block, :mute, :notification_mute, :reblog_mute, :inverse_subscription] ap_ids_by_rel = @@ -124,10 +124,10 @@ test "outgoing_relations_ap_ids/1", %{user: user} do assert ap_ids_by_rel[:inverse_subscription] == Enum.sort(Enum.map(User.subscriber_users(user), & &1.ap_id)) - outgoing_relations_ap_ids = User.outgoing_relations_ap_ids(user, rel_types) + outgoing_relationships_ap_ids = User.outgoing_relationships_ap_ids(user, rel_types) assert ap_ids_by_rel == - Enum.into(outgoing_relations_ap_ids, %{}, fn {k, v} -> {k, Enum.sort(v)} end) + Enum.into(outgoing_relationships_ap_ids, %{}, fn {k, v} -> {k, Enum.sort(v)} end) end end From 3fa3d45dbecafb06fb7eb4f0260f610d4225e0a7 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Wed, 25 Mar 2020 13:05:00 +0300 Subject: [PATCH 4/4] [#1364] Minor improvements / comments. Further fixes of incorrect usage of "relations" as a short form of "relationships". --- lib/pleroma/activity.ex | 1 + lib/pleroma/notification.ex | 12 +++++++----- lib/pleroma/thread_mute.ex | 7 ++++--- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/pleroma/activity.ex b/lib/pleroma/activity.ex index bbaa561a7..5a8329e69 100644 --- a/lib/pleroma/activity.ex +++ b/lib/pleroma/activity.ex @@ -95,6 +95,7 @@ def with_preloaded_object(query, join_type \\ :inner) do |> preload([activity, object: object], object: object) end + # Note: applies to fake activities (ActivityPub.Utils.get_notified_from_object/1 etc.) def user_actor(%Activity{actor: nil}), do: nil def user_actor(%Activity{} = activity) do diff --git a/lib/pleroma/notification.ex b/lib/pleroma/notification.ex index 63e3e9be9..04ee510b9 100644 --- a/lib/pleroma/notification.ex +++ b/lib/pleroma/notification.ex @@ -322,6 +322,8 @@ def create_notification(%Activity{} = activity, %User{} = user, do_send \\ true) @doc """ Returns a tuple with 2 elements: {enabled notification receivers, currently disabled receivers (blocking / [thread] muting)} + + NOTE: might be called for FAKE Activities, see ActivityPub.Utils.get_notified_from_object/1 """ def get_notified_from_activity(activity, local_only \\ true) @@ -338,7 +340,7 @@ def get_notified_from_activity(%Activity{data: %{"type" => type}} = activity, lo # Since even subscribers and followers can mute / thread-mute, filtering all above AP IDs notification_enabled_ap_ids = potential_receiver_ap_ids - |> exclude_relation_restricting_ap_ids(activity) + |> exclude_relationship_restricted_ap_ids(activity) |> exclude_thread_muter_ap_ids(activity) potential_receivers = @@ -355,10 +357,10 @@ def get_notified_from_activity(%Activity{data: %{"type" => type}} = activity, lo def get_notified_from_activity(_, _local_only), do: {[], []} @doc "Filters out AP IDs of users basing on their relationships with activity actor user" - def exclude_relation_restricting_ap_ids([], _activity), do: [] + def exclude_relationship_restricted_ap_ids([], _activity), do: [] - def exclude_relation_restricting_ap_ids(ap_ids, %Activity{} = activity) do - relation_restricted_ap_ids = + def exclude_relationship_restricted_ap_ids(ap_ids, %Activity{} = activity) do + relationship_restricted_ap_ids = activity |> Activity.user_actor() |> User.incoming_relationships_ungrouped_ap_ids([ @@ -366,7 +368,7 @@ def exclude_relation_restricting_ap_ids(ap_ids, %Activity{} = activity) do :notification_mute ]) - Enum.uniq(ap_ids) -- relation_restricted_ap_ids + Enum.uniq(ap_ids) -- relationship_restricted_ap_ids end @doc "Filters out AP IDs of users who mute activity thread" diff --git a/lib/pleroma/thread_mute.ex b/lib/pleroma/thread_mute.ex index 2b4cf02cf..a7ea13891 100644 --- a/lib/pleroma/thread_mute.ex +++ b/lib/pleroma/thread_mute.ex @@ -41,15 +41,16 @@ def muters_query(context) do def muter_ap_ids(context, ap_ids \\ nil) - def muter_ap_ids(context, ap_ids) when context not in [nil, ""] do + # Note: applies to fake activities (ActivityPub.Utils.get_notified_from_object/1 etc.) + def muter_ap_ids(context, _ap_ids) when is_nil(context), do: [] + + def muter_ap_ids(context, ap_ids) do context |> muters_query() |> maybe_filter_on_ap_id(ap_ids) |> Repo.all() end - def muter_ap_ids(_context, _ap_ids), do: [] - defp maybe_filter_on_ap_id(query, ap_ids) when is_list(ap_ids) do where(query, [tm, u], u.ap_id in ^ap_ids) end