Merge branch 'fixes' into 'main'

Various fixes

Closes #1068

See merge request framasoft/mobilizon!1233

js/src/graphql/admin.ts

@@ -74,6 +74,7 @@ export const INSTANCE_FRAGMENT = gql`
   fragment InstanceFragment on Instance {
     domain
     hasRelay
+    relayAddress
     followerStatus
     followedStatus
     eventCount

js/src/types/instance.model.ts

@@ -3,6 +3,7 @@ import { InstanceFollowStatus } from "./enums";
 export interface IInstance {
   domain: string;
   hasRelay: boolean;
+  relayAddress: string | null;
   followerStatus: InstanceFollowStatus;
   followedStatus: InstanceFollowStatus;
   personCount: number;

js/src/views/Admin/Instance.vue

@@ -66,8 +66,11 @@
         <span class="text-sm block">{{ $t("Uploaded media size") }}</span>
       </div>
     </div>
-    <div class="mt-3 grid xl:grid-cols-2 gap-4" v-if="instance.hasRelay">
+    <div class="mt-3 grid xl:grid-cols-2 gap-4">
-      <div class="border bg-white p-6 shadow-md rounded-md">
+      <div
+        class="border bg-white p-6 shadow-md rounded-md"
+        v-if="instance.hasRelay"
+      >
         <button
           @click="removeInstanceFollow"
           v-if="instance.followedStatus == InstanceFollowStatus.APPROVED"
@@ -90,6 +93,9 @@
           {{ $t("Follow instance") }}
         </button>
       </div>
+      <div v-else class="md:h-48 py-16 text-center opacity-50">
+        {{ $t("Only Mobilizon instances can be followed") }}
+      </div>
       <div class="border bg-white p-6 shadow-md rounded-md flex flex-col gap-2">
         <button
           @click="acceptInstance"
@@ -110,9 +116,6 @@
         </p>
       </div>
     </div>
-    <div v-else class="md:h-48 py-16 text-center opacity-50">
-      {{ $t("Only Mobilizon instances can be followed") }}
-    </div>
   </div>
 </template>
 <script lang="ts">
@@ -159,7 +162,7 @@ export default class Instance extends Vue {
       await this.$apollo.mutate({
         mutation: ACCEPT_RELAY,
         variables: {
-          address: `relay@${this.domain}`,
+          address: this.instance.relayAddress,
         },
         update(cache: ApolloCache<any>) {
           cache.writeFragment({
@@ -191,7 +194,7 @@ export default class Instance extends Vue {
       await this.$apollo.mutate({
         mutation: REJECT_RELAY,
         variables: {
-          address: `relay@${this.domain}`,
+          address: this.instance.relayAddress,
         },
         update(cache: ApolloCache<any>) {
           cache.writeFragment({
@@ -239,7 +242,7 @@ export default class Instance extends Vue {
       await this.$apollo.mutate({
         mutation: REMOVE_RELAY,
         variables: {
-          address: `relay@${this.domain}`,
+          address: this.instance.relayAddress,
         },
         update(cache: ApolloCache<any>) {
           cache.writeFragment({

lib/federation/activity_pub/publisher.ex

@@ -110,13 +110,17 @@ defmodule Mobilizon.Federation.ActivityPub.Publisher do
   @spec convert_followers_in_recipients(list(String.t())) :: {list(String.t()), list(String.t())}
   defp convert_followers_in_recipients(recipients) do
     Enum.reduce(recipients, {recipients, []}, fn recipient, {recipients, follower_actors} = acc ->
-      case Actors.get_actor_by_followers_url(recipient) do
+      if is_nil(recipient) do
-        %Actor{} = group ->
+        acc
-          {Enum.filter(recipients, fn recipient -> recipient != group.followers_url end),
+      else
-           follower_actors ++ Actors.list_external_followers_for_actor(group)}
+        case Actors.get_actor_by_followers_url(recipient) do
+          %Actor{} = group ->
+            {Enum.filter(recipients, fn recipient -> recipient != group.followers_url end),
+             follower_actors ++ Actors.list_external_followers_for_actor(group)}
 
-        nil ->
+          nil ->
-          acc
+            acc
+        end
       end
     end)
   end
@@ -128,19 +132,23 @@ defmodule Mobilizon.Federation.ActivityPub.Publisher do
   @spec convert_members_in_recipients(list(String.t())) :: {list(String.t()), list(Actor.t())}
   defp convert_members_in_recipients(recipients) do
     Enum.reduce(recipients, {recipients, []}, fn recipient, {recipients, member_actors} = acc ->
-      case Actors.get_group_by_members_url(recipient) do
+      if is_nil(recipient) do
-        # If the group is local just add external members
+        acc
-        %Actor{domain: domain} = group when is_nil(domain) ->
+      else
-          {Enum.filter(recipients, fn recipient -> recipient != group.members_url end),
+        case Actors.get_group_by_members_url(recipient) do
-           member_actors ++ Actors.list_external_actors_members_for_group(group)}
+          # If the group is local just add external members
+          %Actor{domain: domain} = group when is_nil(domain) ->
+            {Enum.filter(recipients, fn recipient -> recipient != group.members_url end),
+             member_actors ++ Actors.list_external_actors_members_for_group(group)}
 
-        # If it's remote add the remote group actor as well
+          # If it's remote add the remote group actor as well
-        %Actor{} = group ->
+          %Actor{} = group ->
-          {Enum.filter(recipients, fn recipient -> recipient != group.members_url end),
+            {Enum.filter(recipients, fn recipient -> recipient != group.members_url end),
-           member_actors ++ Actors.list_external_actors_members_for_group(group) ++ [group]}
+             member_actors ++ Actors.list_external_actors_members_for_group(group) ++ [group]}
 
-        _ ->
+          _ ->
-          acc
+            acc
+        end
       end
     end)
   end

lib/graphql/resolvers/admin.ex

@@ -468,12 +468,16 @@ defmodule Mobilizon.GraphQL.Resolvers.Admin do
         context: %{current_user: %User{role: role}}
       })
       when is_admin(role) do
-    has_relay = Actors.has_relay?(domain)
+    remote_relay = Actors.get_relay(domain)
-    remote_relay = Actors.get_actor_by_name("relay@#{domain}")
     local_relay = Relay.get_actor()
 
     result = %{
-      has_relay: has_relay,
+      has_relay: !is_nil(remote_relay),
+      relay_address:
+        if(is_nil(remote_relay),
+          do: nil,
+          else: "#{remote_relay.preferred_username}@#{remote_relay.domain}"
+        ),
       follower_status: follow_status(remote_relay, local_relay),
       followed_status: follow_status(local_relay, remote_relay)
     }

lib/graphql/schema/admin.ex

@@ -216,6 +216,10 @@ defmodule Mobilizon.GraphQL.Schema.AdminType do
       description:
         "Whether this instance has a relay, meaning that it's a Mobilizon instance that we can follow"
     )
+
+    field(:relay_address, :string,
+      description: "If this instance has a relay, it's federated username"
+    )
   end
 
   @desc """

lib/mobilizon/actors/actor.ex

@@ -19,6 +19,7 @@ defmodule Mobilizon.Actors.Actor do
   alias Mobilizon.Web.Endpoint
   alias Mobilizon.Web.Router.Helpers, as: Routes
   import Mobilizon.Web.Gettext, only: [dgettext: 2]
+  import Mobilizon.Service.Guards, only: [is_valid_string: 1]
 
   require Logger
 
@@ -224,10 +225,23 @@ defmodule Mobilizon.Actors.Actor do
     preferred_username_and_domain(actor)
   end
 
-  def display_name_and_username(%__MODULE__{name: name} = actor) do
+  def display_name_and_username(%__MODULE__{
+        type: :Application,
+        name: name,
+        preferred_username: "relay",
+        domain: domain
+      })
+      when domain not in [nil, ""] and name not in [nil, ""] do
+    "#{name} (#{domain})"
+  end
+
+  def display_name_and_username(%__MODULE__{name: name, preferred_username: username} = actor)
+      when username not in [nil, ""] do
     "#{name} (@#{preferred_username_and_domain(actor)})"
   end
 
+  def display_name_and_username(_), do: nil
+
   @doc """
   Returns the preferred username with the eventual @domain suffix if it's
   a distant actor.
@@ -235,8 +249,18 @@ defmodule Mobilizon.Actors.Actor do
   @spec preferred_username_and_domain(t) :: String.t()
   def preferred_username_and_domain(%__MODULE__{
         preferred_username: preferred_username,
-        domain: nil
+        domain: domain
-      }) do
+      })
+      when domain in [nil, ""] do
+    preferred_username
+  end
+
+  def preferred_username_and_domain(%__MODULE__{
+        type: :Application,
+        preferred_username: preferred_username,
+        domain: domain
+      })
+      when not is_nil(domain) and preferred_username == domain do
     preferred_username
   end
 
@@ -290,6 +314,7 @@ defmodule Mobilizon.Actors.Actor do
     |> build_urls()
     |> common_changeset(attrs)
     |> unique_username_validator()
+    |> username_validator()
     |> validate_required(@registration_required_attrs)
   end
 
@@ -333,6 +358,7 @@ defmodule Mobilizon.Actors.Actor do
     |> put_change(:keys, Crypto.generate_rsa_2048_private_key())
     |> put_change(:type, :Group)
     |> unique_username_validator()
+    |> username_validator()
     |> validate_required(@group_creation_required_attrs)
     |> validate_length(:summary, max: 5000)
     |> validate_length(:preferred_username, max: 100)
@@ -358,6 +384,23 @@ defmodule Mobilizon.Actors.Actor do
   # When we don't even have any preferred_username, don't even try validating preferred_username
   defp unique_username_validator(changeset), do: changeset
 
+  defp username_validator(%Ecto.Changeset{} = changeset) do
+    username = Ecto.Changeset.fetch_field!(changeset, :preferred_username)
+
+    if is_valid_string(username) and Regex.match?(~r/^[a-z0-9_]+$/, username) do
+      changeset
+    else
+      add_error(
+        changeset,
+        :preferred_username,
+        dgettext(
+          "errors",
+          "Username must only contain alphanumeric lowercased characters and underscores."
+        )
+      )
+    end
+  end
+
   @spec build_urls(Ecto.Changeset.t(), atom()) :: Ecto.Changeset.t()
   defp build_urls(changeset, type \\ :Person)
 

lib/mobilizon/actors/actors.ex

@@ -1297,14 +1297,12 @@ defmodule Mobilizon.Actors do
     :ok
   end
 
-  @spec has_relay?(String.t()) :: boolean()
+  @doc """
-  def has_relay?(domain) do
+  Returns a relay actor, either `relay@domain` (Mobilizon) or `domain@domain` (Mastodon)
-    Actor
+  """
-    |> where(
+  @spec get_relay(String.t()) :: Actor.t() | nil
-      [a],
+  def get_relay(domain) do
-      a.preferred_username == "relay" and a.domain == ^domain and a.type == :Application
+    get_actor_by_name("relay@#{domain}") || get_actor_by_name("#{domain}@#{domain}")
-    )
-    |> Repo.exists?()
   end
 
   @spec delete_files_if_media_changed(Ecto.Changeset.t()) :: Ecto.Changeset.t()

lib/web/email/follow.ex

@@ -44,11 +44,19 @@ defmodule Mobilizon.Web.Email.Follow do
 
     subject =
       if follower_type == :Application do
-        gettext(
+        # Mastodon instance actor has no name and an username equal to the domain
-          "Instance %{name} (%{domain}) requests to follow your instance",
+        if is_nil(follower.name) and follower.preferred_username == follower.domain do
-          name: follower.name,
+          gettext(
-          domain: follower.domain
+            "Instance %{domain} requests to follow your instance",
-        )
+            domain: follower.domain
+          )
+        else
+          gettext(
+            "Instance %{name} (%{domain}) requests to follow your instance",
+            name: follower.name || follower.preferred_username,
+            domain: follower.domain
+          )
+        end
       else
         gettext(
           "%{name} requests to follow your instance",

lib/web/templates/email/instance_follow.html.heex

@@ -44,18 +44,10 @@
           style="padding: 20px 30px 0px 30px; color: #474467; font-family: 'Roboto', Helvetica, Arial, sans-serif; font-size: 18px; font-weight: 400; line-height: 25px;"
         >
           <p style="margin: 0;">
-            <%= if @follower.type == :Application do %>
+            <%= gettext("<b>%{name}</b> just requested to follow your instance.",
-              <%= gettext("<b>%{name} (%{domain})</b> just requested to follow your instance.",
+              name: Mobilizon.Actors.Actor.display_name_and_username(@follower)
-                name: @follower.name,
+            )
-                domain: @follower.domain
+            |> raw %>
-              )
-              |> raw %>
-            <% else %>
-              <%= gettext("<b>%{name}</b> just requested to follow your instance.",
-                name: Mobilizon.Actors.Actor.display_name_and_username(@follower)
-              )
-              |> raw %>
-            <% end %>
             <br />
             <%= if @follower.type == :Application do %>
               <%= gettext("If you accept, this instance will receive all of your public events.") %>
@@ -74,9 +66,8 @@
           >
             <p style="margin: 0;">
               <%= gettext(
-                "Note: %{name} (%{domain}) following you doesn't necessarily imply that you follow this instance, but you can ask to follow them too.",
+                "Note: %{name} following you doesn't necessarily imply that you follow this instance, but you can ask to follow them too.",
-                name: @follower.name,
+                name: Mobilizon.Actors.Actor.display_name_and_username(@follower)
-                domain: @follower.domain
               ) %>
             </p>
           </td>

lib/web/templates/email/instance_follow.text.eex

@@ -2,9 +2,9 @@
 
 ==
 
-<%= if @follower.type == :Application do %><%= gettext "%{name} (%{domain}) just requested to follow your instance.", name: @follower.name, domain: @follower.domain %><% else %><%= gettext "%{name} just requested to follow your instance.", name: Mobilizon.Actors.Actor.display_name_and_username(@follower) %><% end %>
+<%= if @follower.type == :Application do %><%= gettext "%{name} just requested to follow your instance.", name: Mobilizon.Actors.Actor.display_name_and_username(@follower) %><% end %>
 <%= if @follower.type == :Application do %><%= gettext "If you accept, this instance will receive all of your public events." %><% else %><%= gettext "If you accept, this profile will receive all of your public events." %><% end %>
-<%= if @follower.type == :Application do %><%= gettext "Note: %{name} (%{domain}) following you doesn't necessarily imply that you follow this instance, but you can ask to follow them too.", name: @follower.name, domain: @follower.domain %><% end %>
+<%= if @follower.type == :Application do %><%= gettext "Note: %{name} following you doesn't necessarily imply that you follow this instance, but you can ask to follow them too.", name: Mobilizon.Actors.Actor.display_name_and_username(@follower) %><% end %>
 
 <%= if @follower.type == :Application do %><%= gettext "To accept this invitation, head over to the instance's admin settings." %><% else %><%= gettext "To accept this invitation, head over to the profile's admin page." %><% end %>
 <%= if @follower.type == :Application do %><%= "#{Mobilizon.Web.Endpoint.url()}/settings/admin/relays/followers" %><% else %><%= "#{Mobilizon.Web.Endpoint.url()}/settings/admin/profiles/#{@follower.id}" %><% end %>

test/graphql/resolvers/group_test.exs

@@ -8,7 +8,7 @@ defmodule Mobilizon.Web.Resolvers.GroupTest do
   alias Mobilizon.GraphQL.AbsintheHelpers
 
   @non_existent_username "nonexistent"
-  @new_group_params %{groupname: "new group"}
+  @new_group_params %{name: "new group", preferredUsername: "new_group"}
 
   setup %{conn: conn} do
     user = insert(:user)
@@ -17,49 +17,75 @@ defmodule Mobilizon.Web.Resolvers.GroupTest do
     {:ok, conn: conn, actor: actor, user: user}
   end
 
-  describe "create a group" do
+  describe "create_group/3" do
-    test "create_group/3 creates a group and check a group with this name does not already exist",
+    @create_group_mutation """
+    mutation CreateGroup(
+    $preferredUsername: String!
+    $name: String!
+    $summary: String
+    $avatar: MediaInput
+    $banner: MediaInput
+    ) {
+      createGroup(
+        preferredUsername: $preferredUsername
+        name: $name
+        summary: $summary
+        banner: $banner
+        avatar: $avatar
+      ) {
+        preferredUsername
+        type
+        banner {
+          id
+          url
+        }
+      }
+    }
+    """
+
+    test "creates a group and check a group with this name does not already exist",
          %{conn: conn, user: user} do
-      mutation = """
+      res =
-          mutation {
+        conn
-            createGroup(
+        |> auth_conn(user)
-              preferred_username: "#{@new_group_params.groupname}"
+        |> AbsintheHelpers.graphql_query(
-            ) {
+          query: @create_group_mutation,
-                preferred_username,
+          variables: @new_group_params
-                type
+        )
-              }
+
-            }
+      assert res["errors"] == nil
-      """
+
+      assert res["data"]["createGroup"]["preferredUsername"] ==
+               @new_group_params.preferredUsername
+
+      assert res["data"]["createGroup"]["type"] == "GROUP"
 
       res =
         conn
         |> auth_conn(user)
-        |> post("/api", AbsintheHelpers.mutation_skeleton(mutation))
+        |> AbsintheHelpers.graphql_query(
+          query: @create_group_mutation,
+          variables: @new_group_params
+        )
 
-      assert json_response(res, 200)["data"]["createGroup"]["preferred_username"] ==
+      assert hd(res["errors"])["message"] ==
-               @new_group_params.groupname
-
-      assert json_response(res, 200)["data"]["createGroup"]["type"] == "GROUP"
-
-      mutation = """
-          mutation {
-            createGroup(
-              preferred_username: "#{@new_group_params.groupname}"
-            ) {
-                preferred_username,
-                type
-              }
-            }
-      """
-
-      res =
-        conn
-        |> auth_conn(user)
-        |> post("/api", AbsintheHelpers.mutation_skeleton(mutation))
-
-      assert hd(json_response(res, 200)["errors"])["message"] ==
                "A profile or group with that name already exists"
     end
+
+    test "doesn't creates a group if the username doesn't match the requirements",
+         %{conn: conn, user: user} do
+      res =
+        conn
+        |> auth_conn(user)
+        |> AbsintheHelpers.graphql_query(
+          query: @create_group_mutation,
+          variables: Map.put(@new_group_params, :preferredUsername, "no@way")
+        )
+
+      assert hd(res["errors"])["message"] == [
+               "Username must only contain alphanumeric lowercased characters and underscores."
+             ]
+    end
   end
 
   describe "list groups" do

test/graphql/resolvers/person_test.exs

@@ -13,23 +13,34 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
 
   @non_existent_username "nonexistent"
 
-  describe "Person Resolver" do
+  @get_person_query """
-    @get_person_query """
+  query Person($id: ID!) {
-    query Person($id: ID!) {
+      person(id: $id) {
-        person(id: $id) {
+          preferredUsername,
-            preferredUsername,
-        }
       }
-    """
+    }
+  """
 
-    @fetch_person_query """
+  @fetch_person_query """
-    query FetchPerson($preferredUsername: String!) {
+  query FetchPerson($preferredUsername: String!) {
-        fetchPerson(preferredUsername: $preferredUsername) {
+      fetchPerson(preferredUsername: $preferredUsername) {
-            preferredUsername,
+          preferredUsername,
-        }
       }
-    """
+    }
+  """
 
+  @fetch_identities_query """
+  {
+    identities {
+      avatar {
+        url
+      },
+      preferredUsername,
+    }
+  }
+  """
+
+  describe "Getting a person" do
     test "get_person/3 returns a person by its username", %{conn: conn} do
       user = insert(:user)
       actor = insert(:actor, user: user)
@@ -128,107 +139,114 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
 
       assert json_response(res, 200)["data"]["loggedPerson"]["avatar"]["url"] =~ Endpoint.url()
     end
+  end
 
-    test "create_person/3 creates a new identity", context do
+  describe "create_person/3" do
+    @create_person_mutation """
+    mutation CreatePerson(
+      $preferredUsername: String!
+      $name: String!
+      $summary: String
+      $avatar: MediaInput
+      $banner: MediaInput
+      ) {
+      createPerson(
+        preferredUsername: $preferredUsername
+        name: $name
+        summary: $summary
+        avatar: $avatar
+        banner: $banner
+      ) {
+        id
+        preferredUsername
+        avatar {
+          id,
+          url
+        },
+        banner {
+          id,
+          name,
+          url
+        }
+      }
+    }
+    """
+
+    test "creates a new identity", %{conn: conn} do
       user = insert(:user)
       actor = insert(:actor, user: user)
 
-      mutation = """
-          mutation {
-            createPerson(
-              preferredUsername: "new_identity",
-              name: "secret person",
-              summary: "no-one will know who I am"
-            ) {
-              id,
-              preferredUsername
-            }
-          }
-      """
-
       res =
-        context.conn
+        conn
-        |> post("/api", AbsintheHelpers.mutation_skeleton(mutation))
+        |> AbsintheHelpers.graphql_query(
+          query: @create_person_mutation,
+          variables: %{
+            preferredUsername: "new_identity",
+            name: "secret person",
+            summary: "no-one will know who I am"
+          }
+        )
 
-      assert json_response(res, 200)["data"]["createPerson"] == nil
+      assert res["data"]["createPerson"] == nil
 
-      assert hd(json_response(res, 200)["errors"])["message"] ==
+      assert hd(res["errors"])["message"] ==
                "You need to be logged in"
 
       res =
-        context.conn
+        conn
         |> auth_conn(user)
-        |> post("/api", AbsintheHelpers.mutation_skeleton(mutation))
+        |> AbsintheHelpers.graphql_query(
+          query: @create_person_mutation,
+          variables: %{
+            preferredUsername: "new_identity",
+            name: "secret person",
+            summary: "no-one will know who I am"
+          }
+        )
 
-      assert json_response(res, 200)["data"]["createPerson"]["preferredUsername"] ==
+      assert res["data"]["createPerson"]["preferredUsername"] ==
                "new_identity"
 
-      query = """
-      {
-          identities {
-            avatar {
-              url
-            },
-            preferredUsername,
-          }
-        }
-      """
-
       res =
-        context.conn
+        conn
-        |> get("/api", AbsintheHelpers.query_skeleton(query, "identities"))
+        |> AbsintheHelpers.graphql_query(query: @fetch_identities_query)
 
-      assert json_response(res, 200)["data"]["identities"] == nil
+      assert res["data"]["identities"] == nil
 
-      assert hd(json_response(res, 200)["errors"])["message"] ==
+      assert hd(res["errors"])["message"] ==
                "You need to be logged in"
 
       res =
-        context.conn
+        conn
         |> auth_conn(user)
-        |> get("/api", AbsintheHelpers.query_skeleton(query, "identities"))
+        |> AbsintheHelpers.graphql_query(query: @fetch_identities_query)
 
-      assert json_response(res, 200)["data"]["identities"]
+      assert res["data"]["identities"]
              |> Enum.map(fn identity -> Map.get(identity, "preferredUsername") end)
              |> MapSet.new() ==
                MapSet.new([actor.preferred_username, "new_identity"])
     end
 
-    test "create_person/3 with an avatar and an banner creates a new identity", context do
+    test "with an avatar and an banner creates a new identity", %{conn: conn} do
       user = insert(:user)
       insert(:actor, user: user)
 
-      mutation = """
+      variables = %{
-          mutation {
+        preferredUsername: "new_identity",
-            createPerson(
+        name: "secret person",
-              preferredUsername: "new_identity",
+        summary: "no-one will know who I am",
-              name: "secret person",
+        banner: %{
-              summary: "no-one will know who I am",
+          media: %{
-              banner: {
+            file: "landscape.jpg",
-                media: {
+            name: "irish landscape",
-                  file: "landscape.jpg",
+            alt: "The beautiful atlantic way"
-                  name: "irish landscape",
-                  alt: "The beautiful atlantic way"
-                }
-              }
-            ) {
-              id,
-              preferredUsername
-              avatar {
-                id,
-                url
-              },
-              banner {
-                id,
-                name,
-                url
-              }
-            }
           }
-      """
+        }
+      }
 
       map = %{
-        "query" => mutation,
+        "query" => @create_person_mutation,
+        "variables" => variables,
         "landscape.jpg" => %Plug.Upload{
           path: "test/fixtures/picture.png",
           filename: "landscape.jpg"
@@ -236,34 +254,63 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
       }
 
       res =
-        context.conn
+        conn
         |> put_req_header("content-type", "multipart/form-data")
-        |> post("/api", map)
+        |> post(
+          "/api",
+          map
+        )
+        |> json_response(200)
 
-      assert json_response(res, 200)["data"]["createPerson"] == nil
+      assert res["data"]["createPerson"] == nil
 
-      assert hd(json_response(res, 200)["errors"])["message"] ==
+      assert hd(res["errors"])["message"] ==
                "You need to be logged in"
 
       res =
-        context.conn
+        conn
         |> auth_conn(user)
         |> put_req_header("content-type", "multipart/form-data")
-        |> post("/api", map)
+        |> post(
+          "/api",
+          map
+        )
+        |> json_response(200)
 
-      assert json_response(res, 200)["data"]["createPerson"]["preferredUsername"] ==
+      assert res["data"]["createPerson"]["preferredUsername"] ==
                "new_identity"
 
-      assert json_response(res, 200)["data"]["createPerson"]["banner"]["id"]
+      assert res["data"]["createPerson"]["banner"]["id"]
 
-      assert json_response(res, 200)["data"]["createPerson"]["banner"]["name"] ==
+      assert res["data"]["createPerson"]["banner"]["name"] ==
                "The beautiful atlantic way"
 
-      assert json_response(res, 200)["data"]["createPerson"]["banner"]["url"] =~
+      assert res["data"]["createPerson"]["banner"]["url"] =~
                Endpoint.url() <> "/media/"
     end
 
-    test "update_person/3 updates an existing identity", context do
+    test "with an username that is not acceptable", %{conn: conn} do
+      user = insert(:user)
+      _actor = insert(:actor, user: user)
+
+      res =
+        conn
+        |> auth_conn(user)
+        |> AbsintheHelpers.graphql_query(
+          query: @create_person_mutation,
+          variables: %{
+            preferredUsername: "me@no",
+            name: "wrong person"
+          }
+        )
+
+      assert hd(res["errors"])["message"] ==
+               ["Username must only contain alphanumeric lowercased characters and underscores."]
+    end
+  end
+
+  describe "update_person/3" do
+    test "updates an existing identity", context do
       user = insert(:user)
       %Actor{id: person_id} = insert(:actor, user: user, preferred_username: "riri")
 
@@ -333,7 +380,7 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
       assert res_person["banner"]["url"] =~ Endpoint.url() <> "/media/"
     end
 
-    test "update_person/3 should fail to update a not owned identity", context do
+    test "should fail to update a not owned identity", context do
       user1 = insert(:user)
       user2 = insert(:user)
       %Actor{id: person_id} = insert(:actor, user: user2, preferred_username: "riri")
@@ -360,7 +407,7 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
                "Profile is not owned by authenticated user"
     end
 
-    test "update_person/3 should fail to update a not existing identity", context do
+    test "should fail to update a not existing identity", context do
       user = insert(:user)
       insert(:actor, user: user, preferred_username: "riri")
 
@@ -385,8 +432,10 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
       assert hd(json_response(res, 200)["errors"])["message"] ==
                "Profile not found"
     end
+  end
 
-    test "delete_person/3 should fail to update a not owned identity", context do
+  describe "delete_person/3" do
+    test "should fail to update a not owned identity", context do
       user1 = insert(:user)
       user2 = insert(:user)
       %Actor{id: person_id} = insert(:actor, user: user2, preferred_username: "riri")
@@ -410,7 +459,7 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
                "Profile is not owned by authenticated user"
     end
 
-    test "delete_person/3 should fail to delete a not existing identity", context do
+    test "should fail to delete a not existing identity", context do
       user = insert(:user)
       insert(:actor, user: user, preferred_username: "riri")
 
@@ -433,7 +482,7 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
                "Profile not found"
     end
 
-    test "delete_person/3 should fail to delete the last user identity", context do
+    test "should fail to delete the last user identity", context do
       user = insert(:user)
       %Actor{id: person_id} = insert(:actor, user: user, preferred_username: "riri")
 
@@ -456,7 +505,7 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
                "Cannot remove the last identity of a user"
     end
 
-    test "delete_person/3 should fail to delete an identity that is the last admin of a group",
+    test "should fail to delete an identity that is the last admin of a group",
          context do
       group = insert(:group)
       classic_user = insert(:user)
@@ -488,7 +537,7 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
                "Cannot remove the last administrator of a group"
     end
 
-    test "delete_person/3 should delete an actor identity", context do
+    test "should delete an actor identity", context do
       user = insert(:user)
       %Actor{id: person_id} = insert(:actor, user: user, preferred_username: "riri")
       insert(:actor, user: user, preferred_username: "fifi")

test/mobilizon/actors/actor_test.exs

@@ -0,0 +1,70 @@
+defmodule Mobilizon.ActorTest do
+  use Mobilizon.DataCase
+  alias Mobilizon.Actors.Actor
+
+  describe "display_name_and_username/1" do
+    test "returns correctly if everything is given" do
+      assert "hello (@someone@remote.tld)" ==
+               Actor.display_name_and_username(%Actor{
+                 name: "hello",
+                 domain: "remote.tld",
+                 preferred_username: "someone"
+               })
+    end
+
+    test "returns for a local actor" do
+      assert "hello (@someone)" ==
+               Actor.display_name_and_username(%Actor{
+                 name: "hello",
+                 domain: nil,
+                 preferred_username: "someone"
+               })
+
+      assert "hello (@someone)" ==
+               Actor.display_name_and_username(%Actor{
+                 name: "hello",
+                 domain: "",
+                 preferred_username: "someone"
+               })
+    end
+
+    test "returns nil if the name is all that's given" do
+      assert nil == Actor.display_name_and_username(%Actor{name: "hello"})
+    end
+
+    test "returns with just the username if that's all that's given" do
+      assert "someone" ==
+               Actor.display_name_and_username(%Actor{preferred_username: "someone"})
+    end
+
+    test "returns an appropriate name for a Mobilizon instance actor" do
+      assert "My Mobilizon Instance (remote.tld)" ==
+               Actor.display_name_and_username(%Actor{
+                 name: "My Mobilizon Instance",
+                 domain: "remote.tld",
+                 preferred_username: "relay",
+                 type: :Application
+               })
+    end
+
+    test "returns an appropriate name for a Mastodon instance actor" do
+      assert "remote.tld" ==
+               Actor.display_name_and_username(%Actor{
+                 name: nil,
+                 domain: "remote.tld",
+                 preferred_username: "remote.tld",
+                 type: :Application
+               })
+    end
+  end
+
+  describe "display_name/1" do
+    test "with name" do
+      assert "hello" == Actor.display_name(%Actor{preferred_username: "someone", name: "hello"})
+    end
+
+    test "without name" do
+      assert "someone" == Actor.display_name(%Actor{preferred_username: "someone"})
+    end
+  end
+end

test/mobilizon/actors/actors_test.exs

@@ -25,7 +25,7 @@ defmodule Mobilizon.ActorsTest do
       suspended: true,
       uri: "some uri",
       url: "some url",
-      preferred_username: "some username"
+      preferred_username: "some_username"
     }
     @update_attrs %{
       summary: "some updated description",
@@ -35,7 +35,7 @@ defmodule Mobilizon.ActorsTest do
       suspended: false,
       uri: "some updated uri",
       url: "some updated url",
-      preferred_username: "some updated username"
+      preferred_username: "some_updated_username"
     }
     @invalid_attrs %{
       summary: nil,
@@ -234,7 +234,7 @@ defmodule Mobilizon.ActorsTest do
       assert actor.domain == "some domain"
       assert actor.keys == "some keypair"
       assert actor.suspended
-      assert actor.preferred_username == "some username"
+      assert actor.preferred_username == "some_username"
     end
 
     test "create_actor/1 with empty data returns error changeset" do
@@ -381,13 +381,13 @@ defmodule Mobilizon.ActorsTest do
     @valid_attrs %{
       summary: "some description",
       suspended: true,
-      preferred_username: "some-title",
+      preferred_username: "some_title",
       name: "Some Title"
     }
     @update_attrs %{
       summary: "some updated description",
       suspended: false,
-      preferred_username: "some-updated-title",
+      preferred_username: "some_updated_title",
       name: "Some Updated Title"
     }
     @invalid_attrs %{summary: nil, suspended: nil, preferred_username: nil, name: nil}
@@ -400,7 +400,7 @@ defmodule Mobilizon.ActorsTest do
 
       assert group.summary == "some description"
       refute group.suspended
-      assert group.preferred_username == "some-title"
+      assert group.preferred_username == "some_title"
     end
 
     test "create_group/1 with an existing profile username fails" do