From 4de39d5850a57ae7e050bed6af441733a8df1688 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Sat, 6 Nov 2021 10:09:54 +0100 Subject: [PATCH] Improve searching for group actors Signed-off-by: Thomas Citharel --- lib/graphql/api/search.ex | 8 ++-- lib/graphql/resolvers/followers.ex | 18 ++++++-- lib/graphql/resolvers/search.ex | 9 +++- lib/graphql/schema/search.ex | 8 ++++ lib/mobilizon/actors/actors.ex | 59 ++++++++++++++++-------- test/graphql/api/search_test.exs | 13 ++++-- test/graphql/resolvers/follower_test.exs | 12 +++-- test/mobilizon/actors/actors_test.exs | 11 ++--- 8 files changed, 97 insertions(+), 41 deletions(-) diff --git a/lib/graphql/api/search.ex b/lib/graphql/api/search.ex index 6252cb04e..902e43a35 100644 --- a/lib/graphql/api/search.ex +++ b/lib/graphql/api/search.ex @@ -40,13 +40,15 @@ defmodule Mobilizon.GraphQL.API.Search do true -> page = - Actors.build_actors_by_username_or_name_page( + Actors.search_actors( term, [ - actor_type: [result_type], + actor_type: result_type, radius: Map.get(args, :radius), location: Map.get(args, :location), - minimum_visibility: Map.get(args, :minimum_visibility, :public) + minimum_visibility: Map.get(args, :minimum_visibility, :public), + current_actor_id: Map.get(args, :current_actor_id), + exclude_my_groups: Map.get(args, :exclude_my_groups, false) ], page, limit diff --git a/lib/graphql/resolvers/followers.ex b/lib/graphql/resolvers/followers.ex index b75520542..e293fd751 100644 --- a/lib/graphql/resolvers/followers.ex +++ b/lib/graphql/resolvers/followers.ex @@ -13,7 +13,7 @@ defmodule Mobilizon.GraphQL.Resolvers.Followers do @spec find_followers_for_group(Actor.t(), map(), map()) :: {:ok, Page.t()} def find_followers_for_group( %Actor{id: group_id} = group, - %{page: page, limit: limit} = args, + args, %{ context: %{ current_user: %User{role: user_role}, @@ -21,15 +21,23 @@ defmodule Mobilizon.GraphQL.Resolvers.Followers do } } ) do + followers = group_followers(group, args) + if Actors.is_moderator?(actor_id, group_id) or is_moderator(user_role) do - {:ok, - Actors.list_paginated_followers_for_actor(group, Map.get(args, :approved), page, limit)} + {:ok, followers} else - {:error, :unauthorized} + {:ok, %Page{followers | elements: []}} end end - def find_followers_for_group(_, _, _), do: {:error, :unauthenticated} + def find_followers_for_group(%Actor{} = group, args, _) do + followers = group_followers(group, args) + {:ok, %Page{followers | elements: []}} + end + + defp group_followers(group, %{page: page, limit: limit} = args) do + Actors.list_paginated_followers_for_actor(group, Map.get(args, :approved), page, limit) + end @spec update_follower(any(), map(), map()) :: {:ok, Follower.t()} | {:error, any()} def update_follower(_, %{id: follower_id, approved: approved}, %{ diff --git a/lib/graphql/resolvers/search.ex b/lib/graphql/resolvers/search.ex index 6daf445b0..c9b32ec13 100644 --- a/lib/graphql/resolvers/search.ex +++ b/lib/graphql/resolvers/search.ex @@ -21,7 +21,14 @@ defmodule Mobilizon.GraphQL.Resolvers.Search do """ @spec search_groups(any(), map(), Absinthe.Resolution.t()) :: {:ok, Page.t(Actor.t())} | {:error, String.t()} - def search_groups(_parent, %{page: page, limit: limit} = args, _resolution) do + def search_groups( + _parent, + %{page: page, limit: limit} = args, + %{context: context} = _resolution + ) do + current_actor = Map.get(context, :current_actor, nil) + current_actor_id = if current_actor, do: current_actor.id, else: nil + args = Map.put(args, :current_actor_id, current_actor_id) Search.search_actors(args, page, limit, :Group) end diff --git a/lib/graphql/schema/search.ex b/lib/graphql/schema/search.ex index 916ab05b2..c11856144 100644 --- a/lib/graphql/schema/search.ex +++ b/lib/graphql/schema/search.ex @@ -59,6 +59,14 @@ defmodule Mobilizon.GraphQL.Schema.SearchType do arg(:term, :string, default_value: "", description: "Search term") arg(:location, :string, description: "A geohash for coordinates") + arg(:exclude_my_groups, :boolean, + description: "Whether to include the groups the current actor is member or follower" + ) + + arg(:minimum_visibility, :group_visibility, + description: "The minimum visibility the group must have" + ) + arg(:radius, :float, default_value: 50, description: "Radius around the location to search in" diff --git a/lib/mobilizon/actors/actors.ex b/lib/mobilizon/actors/actors.ex index da6b0e848..ea88f996e 100644 --- a/lib/mobilizon/actors/actors.ex +++ b/lib/mobilizon/actors/actors.ex @@ -450,29 +450,55 @@ defmodule Mobilizon.Actors do @doc """ Builds a page struct for actors by their name or displayed name. """ - @spec build_actors_by_username_or_name_page( + @spec search_actors( String.t(), Keyword.t(), integer | nil, integer | nil ) :: Page.t() - def build_actors_by_username_or_name_page( + def search_actors( term, options \\ [], page \\ nil, limit \\ nil ) do + term + |> build_actors_by_username_or_name_page_query(options) + |> maybe_exclude_my_groups( + Keyword.get(options, :exclude_my_groups, false), + Keyword.get(options, :current_actor_id) + ) + |> Page.build_page(page, limit) + end + + defp maybe_exclude_my_groups(query, true, current_actor_id) when current_actor_id != nil do + query + |> join(:left, [a], m in Member, on: a.id == m.parent_id) + |> join(:left, [a], f in Follower, on: a.id == f.target_actor_id) + |> where([_a, ..., m, f], m.actor_id != ^current_actor_id and f.actor_id != ^current_actor_id) + end + + defp maybe_exclude_my_groups(query, _, _), do: query + + @spec build_actors_by_username_or_name_page_query( + String.t(), + Keyword.t() + ) :: Ecto.Query.t() + defp build_actors_by_username_or_name_page_query( + term, + options + ) do anonymous_actor_id = Mobilizon.Config.anonymous_actor_id() query = from(a in Actor) query + |> distinct([q], q.id) |> actor_by_username_or_name_query(term) |> actors_for_location(Keyword.get(options, :location), Keyword.get(options, :radius)) - |> filter_by_types(Keyword.get(options, :actor_type, :Group)) + |> filter_by_type(Keyword.get(options, :actor_type, :Group)) |> filter_by_minimum_visibility(Keyword.get(options, :minimum_visibility, :public)) |> filter_suspended(false) |> filter_out_anonymous_actor_id(anonymous_actor_id) - |> Page.build_page(page, limit) end @doc """ @@ -1313,17 +1339,15 @@ defmodule Mobilizon.Actors do @spec actors_for_location(Ecto.Queryable.t(), String.t(), integer()) :: Ecto.Query.t() defp actors_for_location(query, location, radius) when is_valid_string(location) and not is_nil(radius) do - with {lon, lat} <- Geohax.decode(location), - point <- Geo.WKT.decode!("SRID=4326;POINT(#{lon} #{lat})") do - query - |> join(:inner, [q], a in Address, on: a.id == q.physical_address_id, as: :address) - |> where( - [q], - st_dwithin_in_meters(^point, as(:address).geom, ^(radius * 1000)) - ) - else - _ -> query - end + {lon, lat} = Geohax.decode(location) + point = Geo.WKT.decode!("SRID=4326;POINT(#{lon} #{lat})") + + query + |> join(:inner, [q], a in Address, on: a.id == q.physical_address_id, as: :address) + |> where( + [q], + st_dwithin_in_meters(^point, as(:address).geom, ^(radius * 1000)) + ) end defp actors_for_location(query, _location, _radius), do: query @@ -1564,11 +1588,6 @@ defmodule Mobilizon.Actors do defp filter_by_type(query, _type), do: query - @spec filter_by_types(Ecto.Queryable.t(), [ActorType.t()]) :: Ecto.Query.t() - defp filter_by_types(query, types) do - from(a in query, where: a.type in ^types) - end - @spec filter_by_minimum_visibility(Ecto.Queryable.t(), atom()) :: Ecto.Query.t() defp filter_by_minimum_visibility(query, :private), do: query diff --git a/test/graphql/api/search_test.exs b/test/graphql/api/search_test.exs index bfd1f4b36..62016294c 100644 --- a/test/graphql/api/search_test.exs +++ b/test/graphql/api/search_test.exs @@ -38,16 +38,23 @@ defmodule Mobilizon.GraphQL.API.SearchTest do test "search actors" do with_mock Actors, - build_actors_by_username_or_name_page: fn "toto", _options, 1, 10 -> + search_actors: fn "toto", _options, 1, 10 -> %Page{total: 1, elements: [%Actor{id: 42}]} end do assert {:ok, %{total: 1, elements: [%Actor{id: 42}]}} = Search.search_actors(%{term: "toto"}, 1, 10, :Person) assert_called( - Actors.build_actors_by_username_or_name_page( + Actors.search_actors( "toto", - [actor_type: [:Person], radius: nil, location: nil, minimum_visibility: :public], + [ + actor_type: :Person, + radius: nil, + location: nil, + minimum_visibility: :public, + current_actor_id: nil, + exclude_my_groups: false + ], 1, 10 ) diff --git a/test/graphql/resolvers/follower_test.exs b/test/graphql/resolvers/follower_test.exs index 15af8bffc..27b400c67 100644 --- a/test/graphql/resolvers/follower_test.exs +++ b/test/graphql/resolvers/follower_test.exs @@ -70,7 +70,9 @@ defmodule Mobilizon.Web.Resolvers.FollowerTest do variables: %{name: preferred_username} ) - assert hd(res["errors"])["message"] == "unauthenticated" + assert res["errors"] == nil + assert res["data"]["group"]["followers"]["total"] == 1 + assert res["data"]["group"]["followers"]["elements"] == [] end test "without being a member", %{ @@ -88,7 +90,9 @@ defmodule Mobilizon.Web.Resolvers.FollowerTest do variables: %{name: preferred_username} ) - assert hd(res["errors"])["message"] == "unauthorized" + assert res["errors"] == nil + assert res["data"]["group"]["followers"]["total"] == 1 + assert res["data"]["group"]["followers"]["elements"] == [] end test "without being a moderator", %{ @@ -107,7 +111,9 @@ defmodule Mobilizon.Web.Resolvers.FollowerTest do variables: %{name: preferred_username} ) - assert hd(res["errors"])["message"] == "unauthorized" + assert res["errors"] == nil + assert res["data"]["group"]["followers"]["total"] == 1 + assert res["data"]["group"]["followers"]["elements"] == [] end test "while being a moderator", %{ diff --git a/test/mobilizon/actors/actors_test.exs b/test/mobilizon/actors/actors_test.exs index 0e79e680a..1766de4a7 100644 --- a/test/mobilizon/actors/actors_test.exs +++ b/test/mobilizon/actors/actors_test.exs @@ -183,14 +183,14 @@ defmodule Mobilizon.ActorsTest do assert MapSet.new([actor_found_id, actor2_found_id]) == MapSet.new([actor.id, actor2.id]) end - test "test build_actors_by_username_or_name_page/4 returns actors with similar usernames", + test "test search_actors/4 returns actors with similar usernames", %{actor: %Actor{id: actor_id}} do use_cassette "actors/remote_actor_mastodon_tcit" do with {:ok, %Actor{id: actor2_id}} <- ActivityPubActor.get_or_fetch_actor_by_url(@remote_account_url) do %Page{total: 2, elements: actors} = - Actors.build_actors_by_username_or_name_page("tcit", - actor_type: [:Person], + Actors.search_actors("tcit", + actor_type: :Person, minimum_visibility: :private ) @@ -201,9 +201,8 @@ defmodule Mobilizon.ActorsTest do end end - test "test build_actors_by_username_or_name_page/4 returns actors with similar names" do - %{total: 0, elements: actors} = - Actors.build_actors_by_username_or_name_page("ohno", actor_type: [:Person]) + test "test search_actors/4 returns actors with similar names" do + %{total: 0, elements: actors} = Actors.search_actors("ohno", actor_type: :Person) assert actors == [] end