refactor(credo): Refactor to appease new credo checks (complexity and logging)

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

config/config.exs

@@ -138,7 +138,7 @@ config :vite_phx,
 config :logger, :console,
   backends: [:console],
   format: "$time $metadata[$level] $message\n",
-  metadata: [:request_id, :graphql_operation_name, :user_id, :actor_name]
+  metadata: [:request_id, :graphql_operation_name, :user_id, :actor_name, :trace]
 
 config :mobilizon, Mobilizon.Web.Auth.Guardian,
   issuer: "mobilizon",

lib/federation/activity_pub/actions/update.ex

@@ -36,7 +36,7 @@ defmodule Mobilizon.Federation.ActivityPub.Actions.Update do
         {:ok, activity, entity}
 
       {:error, err} ->
-        Logger.error("Something went wrong while creating an activity", err: inspect(err))
+        Logger.error("Something went wrong while creating an activity: #{inspect(err)}")
         {:error, err}
     end
   end

lib/federation/activity_pub/transmogrifier.ex

@@ -203,7 +203,7 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier do
         Actions.Delete.delete(entity, Relay.get_actor(), false)
 
       {:error, err} ->
-        Logger.warn("Error while fetching object from URL", error: inspect(err))
+        Logger.warn("Error while fetching object from URL: #{inspect(err)}")
         :error
     end
   end
@@ -405,8 +405,8 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier do
       {:ok, activity, new_actor}
     else
       {:error, :update_not_allowed} ->
-        Logger.warn("Activity tried to update an actor that's local or not a group",
+        Logger.warn(
-          activity: params
+          "Activity tried to update an actor that's local or not a group: #{inspect(params)}"
         )
 
         :error
@@ -623,25 +623,7 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier do
         if remote_actor_is_being_deleted(data) do
           Actions.Delete.delete(actor, actor, false)
         else
-          case is_group_object_gone(object_id) do
+          handle_group_being_gone(actor, actor_url, object_id)
-            # The group object is no longer there, we can remove the element
-            {:ok, entity} ->
-              if Utils.origin_check_from_id?(actor_url, object_id) ||
-                   Permission.can_delete_group_object?(actor, entity) do
-                Actions.Delete.delete(entity, actor, false)
-              else
-                Logger.warn("Object origin check failed")
-                :error
-              end
-
-            {:error, err} ->
-              Logger.debug(inspect(err))
-              {:error, err}
-
-            {:error, :http_not_found, err} ->
-              Logger.debug(inspect(err))
-              {:error, err}
-          end
         end
     end
   end
@@ -1247,8 +1229,30 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier do
         :error
 
       {:error, err} ->
-        Logger.debug("Generic error when saving external comment", err: inspect(err))
+        Logger.debug("Generic error when saving external comment: #{inspect(err)}")
         :error
     end
   end
+
+  defp handle_group_being_gone(actor, actor_url, object_id) do
+    case is_group_object_gone(object_id) do
+      # The group object is no longer there, we can remove the element
+      {:ok, entity} ->
+        if Utils.origin_check_from_id?(actor_url, object_id) ||
+             Permission.can_delete_group_object?(actor, entity) do
+          Actions.Delete.delete(entity, actor, false)
+        else
+          Logger.warn("Object origin check failed")
+          :error
+        end
+
+      {:error, err} ->
+        Logger.debug(inspect(err))
+        {:error, err}
+
+      {:error, :http_not_found, err} ->
+        Logger.debug(inspect(err))
+        {:error, err}
+    end
+  end
 end

lib/federation/web_finger/web_finger.ex

@@ -259,8 +259,18 @@ defmodule Mobilizon.Federation.WebFinger do
     subject = Map.get(doc, "subject")
 
     if !is_nil(links) && !is_nil(subject) do
-      data =
+      data = Enum.reduce(links, %{"subject" => subject}, &handle_link/2)
-        Enum.reduce(links, %{"subject" => subject}, fn link, data ->
+
+      {:ok, data}
+    else
+      {:error, :webfinger_information_not_valid}
+    end
+  end
+
+  defp webfinger_from_json(_doc), do: {:error, :webfinger_information_not_json}
+
+  @spec handle_link(map(), map()) :: map()
+  defp handle_link(link, data) do
     case {link["type"], link["rel"]} do
       {"application/activity+json", "self"} ->
         Map.put(data, "url", link["href"])
@@ -276,15 +286,7 @@ defmodule Mobilizon.Federation.WebFinger do
 
         data
     end
-        end)
-
-      {:ok, data}
-    else
-      {:error, :webfinger_information_not_valid}
   end
-  end
-
-  defp webfinger_from_json(_doc), do: {:error, :webfinger_information_not_json}
 
   @spec find_link_from_template(String.t()) :: String.t() | {:error, :link_not_found}
   defp find_link_from_template(doc) do

lib/graphql/resolvers/admin.ex

@@ -313,7 +313,26 @@ defmodule Mobilizon.GraphQL.Resolvers.Admin do
   defp change_email(%User{email: old_email} = user, new_email, notify) do
     if Authenticator.can_change_email?(user) do
       if new_email != old_email do
+        do_change_email_different(user, old_email, new_email, notify)
+      else
+        {:error, dgettext("errors", "The new email must be different")}
+      end
+    end
+  end
+
+  @spec do_change_email_different(User.t(), String.t(), String.t(), boolean()) ::
+          {:ok, User.t()} | {:error, String.t()}
+  defp do_change_email_different(user, old_email, new_email, notify) do
     if Email.Checker.valid?(new_email) do
+      do_change_email(user, old_email, new_email, notify)
+    else
+      {:error, dgettext("errors", "The new email doesn't seem to be valid")}
+    end
+  end
+
+  @spec do_change_email(User.t(), String.t(), String.t(), boolean()) ::
+          {:ok, User.t()} | {:error, String.t()}
+  defp do_change_email(user, old_email, new_email, notify) do
     case Users.update_user(user, %{email: new_email}) do
       {:ok, %User{} = updated_user} ->
         if notify do
@@ -332,13 +351,6 @@ defmodule Mobilizon.GraphQL.Resolvers.Admin do
         Logger.debug(inspect(err))
         {:error, dgettext("errors", "Failed to update user email")}
     end
-        else
-          {:error, dgettext("errors", "The new email doesn't seem to be valid")}
-        end
-      else
-        {:error, dgettext("errors", "The new email must be different")}
-      end
-    end
   end
 
   @spec change_role(User.t(), atom(), boolean()) ::

lib/graphql/resolvers/comment.ex

@@ -53,13 +53,7 @@ defmodule Mobilizon.GraphQL.Resolvers.Comment do
                current_ip,
                user_agent
              ) do
-            case Comments.create_comment(args) do
+            do_create_comment(args)
-              {:ok, _, %CommentModel{} = comment} ->
-                {:ok, comment}
-
-              {:error, err} ->
-                {:error, err}
-            end
           else
             {:error,
              dgettext(
@@ -80,6 +74,18 @@ defmodule Mobilizon.GraphQL.Resolvers.Comment do
     {:error, dgettext("errors", "You are not allowed to create a comment if not connected")}
   end
 
+  @spec do_create_comment(map()) ::
+          {:ok, CommentModel.t()} | {:error, :entity_tombstoned | atom() | Ecto.Changeset.t()}
+  defp do_create_comment(args) do
+    case Comments.create_comment(args) do
+      {:ok, _, %CommentModel{} = comment} ->
+        {:ok, comment}
+
+      {:error, err} ->
+        {:error, err}
+    end
+  end
+
   @spec update_comment(any(), map(), Absinthe.Resolution.t()) ::
           {:ok, CommentModel.t()} | {:error, :unauthorized | :not_found | any() | String.t()}
   def update_comment(

lib/graphql/resolvers/user.ex

@@ -480,10 +480,43 @@ defmodule Mobilizon.GraphQL.Resolvers.User do
         context: %{current_user: %User{email: old_email} = user}
       }) do
     if Authenticator.can_change_email?(user) do
+      do_change_mail_can_change_mail(user, old_email, new_email, password)
+    else
+      {:error, dgettext("errors", "User cannot change email")}
+    end
+  end
+
+  def change_email(_parent, _args, _resolution) do
+    {:error, dgettext("errors", "You need to be logged-in to change your email")}
+  end
+
+  @spec do_change_mail_can_change_mail(User.t(), String.t(), String.t(), String.t()) ::
+          {:ok, User.t()} | {:error, String.t()}
+  defp do_change_mail_can_change_mail(user, old_email, new_email, password) do
     case Authenticator.login(old_email, password) do
       {:ok, %User{}} ->
         if new_email != old_email do
+          do_change_mail_new_email(user, new_email)
+        else
+          {:error, dgettext("errors", "The new email must be different")}
+        end
+
+      {:error, _} ->
+        {:error, dgettext("errors", "The password provided is invalid")}
+    end
+  end
+
+  @spec do_change_mail_new_email(User.t(), String.t()) :: {:ok, User.t()} | {:error, String.t()}
+  defp do_change_mail_new_email(user, new_email) do
     if Email.Checker.valid?(new_email) do
+      do_change_mail(user, new_email)
+    else
+      {:error, dgettext("errors", "The new email doesn't seem to be valid")}
+    end
+  end
+
+  @spec do_change_mail(User.t(), String.t()) :: {:ok, User.t()} | {:error, String.t()}
+  defp do_change_mail(user, new_email) do
     case Users.update_user_email(user, new_email) do
       {:ok, %User{} = user} ->
         user
@@ -500,23 +533,6 @@ defmodule Mobilizon.GraphQL.Resolvers.User do
         Logger.debug(inspect(err))
         {:error, dgettext("errors", "Failed to update user email")}
     end
-            else
-              {:error, dgettext("errors", "The new email doesn't seem to be valid")}
-            end
-          else
-            {:error, dgettext("errors", "The new email must be different")}
-          end
-
-        {:error, _} ->
-          {:error, dgettext("errors", "The password provided is invalid")}
-      end
-    else
-      {:error, dgettext("errors", "User cannot change email")}
-    end
-  end
-
-  def change_email(_parent, _args, _resolution) do
-    {:error, dgettext("errors", "You need to be logged-in to change your email")}
   end
 
   @spec validate_email(map(), %{token: String.t()}, map()) ::

lib/mobilizon/actors/actors.ex

@@ -248,8 +248,7 @@ defmodule Mobilizon.Actors do
   @spec update_actor(Actor.t(), map) :: {:ok, Actor.t()} | {:error, Ecto.Changeset.t()}
   def update_actor(%Actor{preferred_username: preferred_username, domain: domain} = actor, attrs) do
     if is_nil(domain) and preferred_username == "relay" do
-      Logger.error("Trying to update local relay actor",
+      Logger.error("Trying to update local relay actor (#{inspect(attrs)})",
-        attrs: attrs,
         trace: Process.info(self(), :current_stacktrace)
       )
     end

lib/service/actor_suspension.ex

@@ -283,14 +283,10 @@ defmodule Mobilizon.Service.ActorSuspension do
         {:ok, actor}
 
       {:error, error} ->
-        Logger.error("Error while removing an upload file",
+        Logger.error(
-          error: inspect(error),
+          "Error while removing an upload file: #{inspect(error)}, actor: #{Actor.preferred_username_and_domain(actor)}, file_url: #{url}"
-          actor: Actor.preferred_username_and_domain(actor),
-          file_url: url
         )
 
-        Logger.debug(inspect(error))
-
         {:ok, actor}
     end
   end

lib/service/auth/applications.ex

@@ -185,9 +185,8 @@ defmodule Mobilizon.Service.Auth.Applications do
   end
 
   def generate_access_token_for_device_flow(client_id, device_code) do
-    Logger.debug("Generating access token for application device with",
+    Logger.debug(
-      client_id: client_id,
+      "Generating access token for application device with client_id=#{client_id}, device_code=#{device_code}"
-      device_code: device_code
     )
 
     case Applications.get_application_device_activation_by_device_code(client_id, device_code) do

lib/service/export/participants/csv.ex

@@ -27,7 +27,7 @@ defmodule Mobilizon.Service.Export.Participants.CSV do
 
   @spec export(Event.t(), Keyword.t()) ::
           {:ok, String.t()} | {:error, :failed_to_save_upload | :export_dependency_not_installed}
-  def export(%Event{id: event_id} = event, options \\ []) do
+  def export(%Event{} = event, options \\ []) do
     if ready?() do
       filename = "#{ShortUUID.encode!(Ecto.UUID.generate())}.csv"
       full_path = Path.join([export_path(@extension), filename])
@@ -36,18 +36,7 @@ defmodule Mobilizon.Service.Export.Participants.CSV do
 
       case Repo.transaction(
              fn ->
-               event_id
+               produce_file(event, filename, full_path, file, options)
-               |> Events.participant_for_event_export_query(Keyword.get(options, :roles, []))
-               |> Repo.stream()
-               |> Stream.map(&to_list/1)
-               |> NimbleCSV.RFC4180.dump_to_iodata()
-               |> add_header_column()
-               |> Stream.each(fn line -> IO.write(file, line) end)
-               |> Stream.run()
-
-               with {:error, err} <- save_csv_upload(full_path, filename, event) do
-                 Repo.rollback(err)
-               end
              end,
              timeout: :infinity
            ) do
@@ -63,6 +52,23 @@ defmodule Mobilizon.Service.Export.Participants.CSV do
     end
   end
 
+  @spec produce_file(Event.t(), String.t(), String.t(), File.io_device(), Keyword.t()) ::
+          no_return()
+  defp produce_file(%Event{id: event_id} = event, filename, full_path, file, options) do
+    event_id
+    |> Events.participant_for_event_export_query(Keyword.get(options, :roles, []))
+    |> Repo.stream()
+    |> Stream.map(&to_list/1)
+    |> NimbleCSV.RFC4180.dump_to_iodata()
+    |> add_header_column()
+    |> Stream.each(fn line -> IO.write(file, line) end)
+    |> Stream.run()
+
+    with {:error, err} <- save_csv_upload(full_path, filename, event) do
+      Repo.rollback(err)
+    end
+  end
+
   defp add_header_column(stream) do
     Stream.concat([Enum.join(columns(), ","), "\n"], stream)
   end

lib/service/notifier/email.ex

@@ -95,9 +95,8 @@ defmodule Mobilizon.Service.Notifier.Email do
   end
 
   defp can_send_activity?(activity, user, options) do
-    Logger.warn("Can't check if user #{inspect(user)} can be sent an activity",
+    Logger.warn(
-      activity: inspect(activity),
+      "Can't check if user #{inspect(user)} can be sent an activity (#{inspect(activity)}) (#{inspect(options)})"
-      options: inspect(options)
     )
 
     false

lib/service/workers/helper.ex

@@ -48,9 +48,7 @@ defmodule Mobilizon.Service.Workers.Helper do
         queue_atom = String.to_existing_atom(unquote(queue))
         worker_args = worker_args ++ Helper.worker_args(queue_atom)
 
-        unquote(caller_module)
+        Oban.insert(unquote(caller_module).new(params, worker_args))
-        |> apply(:new, [params, worker_args])
-        |> Oban.insert()
       end
     end
   end