Improve searching for group actors

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

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

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,
+      {:ok, followers}
-       Actors.list_paginated_followers_for_actor(group, Map.get(args, :approved), page, limit)}
     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}, %{

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
 

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"

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),
+    {lon, lat} = Geohax.decode(location)
-         point <- Geo.WKT.decode!("SRID=4326;POINT(#{lon} #{lat})") do
+    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))
     )
-    else
-      _ -> query
-    end
   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
 

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
         )

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", %{

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",
+            Actors.search_actors("tcit",
-              actor_type: [:Person],
+              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
+    test "test search_actors/4 returns actors with similar names" do
-      %{total: 0, elements: actors} =
+      %{total: 0, elements: actors} = Actors.search_actors("ohno", actor_type: :Person)
-        Actors.build_actors_by_username_or_name_page("ohno", actor_type: [:Person])
 
       assert actors == []
     end