From eda27610324f9ceeeb01d171e3f926096ab9b589 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Wed, 19 Apr 2023 18:33:06 +0200 Subject: [PATCH] refactor(credo): Refactor to appease new credo checks (complexity and logging) Signed-off-by: Thomas Citharel --- config/config.exs | 2 +- lib/federation/activity_pub/actions/update.ex | 2 +- lib/federation/activity_pub/transmogrifier.ex | 50 ++++++------ lib/federation/web_finger/web_finger.ex | 38 +++++----- lib/graphql/resolvers/admin.ex | 56 ++++++++------ lib/graphql/resolvers/comment.ex | 20 +++-- lib/graphql/resolvers/user.ex | 76 +++++++++++-------- lib/mobilizon/actors/actors.ex | 3 +- lib/service/actor_suspension.ex | 8 +- lib/service/auth/applications.ex | 5 +- lib/service/export/participants/csv.ex | 32 ++++---- lib/service/notifier/email.ex | 5 +- lib/service/workers/helper.ex | 4 +- 13 files changed, 169 insertions(+), 132 deletions(-) diff --git a/config/config.exs b/config/config.exs index 67536ba98..59a6a4b6c 100644 --- a/config/config.exs +++ b/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", diff --git a/lib/federation/activity_pub/actions/update.ex b/lib/federation/activity_pub/actions/update.ex index c58032fb2..d87003ce3 100644 --- a/lib/federation/activity_pub/actions/update.ex +++ b/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 diff --git a/lib/federation/activity_pub/transmogrifier.ex b/lib/federation/activity_pub/transmogrifier.ex index dabb5a9a2..9348bb80b 100644 --- a/lib/federation/activity_pub/transmogrifier.ex +++ b/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", - activity: params + Logger.warn( + "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 - # 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 + handle_group_being_gone(actor, actor_url, object_id) 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 diff --git a/lib/federation/web_finger/web_finger.ex b/lib/federation/web_finger/web_finger.ex index 1f97361f4..a1d0b3af7 100644 --- a/lib/federation/web_finger/web_finger.ex +++ b/lib/federation/web_finger/web_finger.ex @@ -259,24 +259,7 @@ defmodule Mobilizon.Federation.WebFinger do subject = Map.get(doc, "subject") if !is_nil(links) && !is_nil(subject) do - data = - Enum.reduce(links, %{"subject" => subject}, fn link, data -> - case {link["type"], link["rel"]} do - {"application/activity+json", "self"} -> - Map.put(data, "url", link["href"]) - - {nil, _rel} -> - Logger.debug("No type declared for the following link #{inspect(link)}") - data - - _ -> - Logger.debug(fn -> - "Unhandled type to finger: #{inspect(link)}" - end) - - data - end - end) + data = Enum.reduce(links, %{"subject" => subject}, &handle_link/2) {:ok, data} else @@ -286,6 +269,25 @@ defmodule Mobilizon.Federation.WebFinger do 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"]) + + {nil, _rel} -> + Logger.debug("No type declared for the following link #{inspect(link)}") + data + + _ -> + Logger.debug(fn -> + "Unhandled type to finger: #{inspect(link)}" + end) + + data + end + end + @spec find_link_from_template(String.t()) :: String.t() | {:error, :link_not_found} defp find_link_from_template(doc) do with res when res in [nil, ""] <- diff --git a/lib/graphql/resolvers/admin.ex b/lib/graphql/resolvers/admin.ex index bdb35c98c..daf6f16fc 100644 --- a/lib/graphql/resolvers/admin.ex +++ b/lib/graphql/resolvers/admin.ex @@ -313,34 +313,46 @@ 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 - if Email.Checker.valid?(new_email) do - case Users.update_user(user, %{email: new_email}) do - {:ok, %User{} = updated_user} -> - if notify do - updated_user - |> Email.Admin.user_email_change_old(old_email) - |> Email.Mailer.send_email() - - updated_user - |> Email.Admin.user_email_change_new(old_email) - |> Email.Mailer.send_email() - end - - {:ok, updated_user} - - {:error, %Ecto.Changeset{} = err} -> - 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 + 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 + updated_user + |> Email.Admin.user_email_change_old(old_email) + |> Email.Mailer.send_email() + + updated_user + |> Email.Admin.user_email_change_new(old_email) + |> Email.Mailer.send_email() + end + + {:ok, updated_user} + + {:error, %Ecto.Changeset{} = err} -> + Logger.debug(inspect(err)) + {:error, dgettext("errors", "Failed to update user email")} + end + end + @spec change_role(User.t(), atom(), boolean()) :: {:ok, User.t()} | {:error, String.t() | Ecto.Changeset.t()} defp change_role(%User{role: old_role} = user, new_role, notify) do diff --git a/lib/graphql/resolvers/comment.ex b/lib/graphql/resolvers/comment.ex index 4344edfd3..92aacea11 100644 --- a/lib/graphql/resolvers/comment.ex +++ b/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 - {:ok, _, %CommentModel{} = comment} -> - {:ok, comment} - - {:error, err} -> - {:error, err} - end + do_create_comment(args) 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( diff --git a/lib/graphql/resolvers/user.ex b/lib/graphql/resolvers/user.ex index 66c664ea1..538c2124d 100644 --- a/lib/graphql/resolvers/user.ex +++ b/lib/graphql/resolvers/user.ex @@ -480,36 +480,7 @@ defmodule Mobilizon.GraphQL.Resolvers.User do context: %{current_user: %User{email: old_email} = user} }) do if Authenticator.can_change_email?(user) do - case Authenticator.login(old_email, password) do - {:ok, %User{}} -> - if new_email != old_email do - if Email.Checker.valid?(new_email) do - case Users.update_user_email(user, new_email) do - {:ok, %User{} = user} -> - user - |> Email.User.send_email_reset_old_email() - |> Email.Mailer.send_email() - - user - |> Email.User.send_email_reset_new_email() - |> Email.Mailer.send_email() - - {:ok, user} - - {:error, %Ecto.Changeset{} = err} -> - 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 + do_change_mail_can_change_mail(user, old_email, new_email, password) else {:error, dgettext("errors", "User cannot change email")} end @@ -519,6 +490,51 @@ defmodule Mobilizon.GraphQL.Resolvers.User 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 + |> Email.User.send_email_reset_old_email() + |> Email.Mailer.send_email() + + user + |> Email.User.send_email_reset_new_email() + |> Email.Mailer.send_email() + + {:ok, user} + + {:error, %Ecto.Changeset{} = err} -> + Logger.debug(inspect(err)) + {:error, dgettext("errors", "Failed to update user email")} + end + end + @spec validate_email(map(), %{token: String.t()}, map()) :: {:ok, User.t()} | {:error, String.t()} def validate_email(_parent, %{token: token}, _resolution) do diff --git a/lib/mobilizon/actors/actors.ex b/lib/mobilizon/actors/actors.ex index ebb914bb3..1436ae6d2 100644 --- a/lib/mobilizon/actors/actors.ex +++ b/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", - attrs: attrs, + Logger.error("Trying to update local relay actor (#{inspect(attrs)})", trace: Process.info(self(), :current_stacktrace) ) end diff --git a/lib/service/actor_suspension.ex b/lib/service/actor_suspension.ex index ca9c2fc80..0d34bb6c6 100644 --- a/lib/service/actor_suspension.ex +++ b/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", - error: inspect(error), - actor: Actor.preferred_username_and_domain(actor), - file_url: url + Logger.error( + "Error while removing an upload file: #{inspect(error)}, actor: #{Actor.preferred_username_and_domain(actor)}, file_url: #{url}" ) - Logger.debug(inspect(error)) - {:ok, actor} end end diff --git a/lib/service/auth/applications.ex b/lib/service/auth/applications.ex index 52c22092a..04b8d3cca 100644 --- a/lib/service/auth/applications.ex +++ b/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", - client_id: client_id, - device_code: device_code + Logger.debug( + "Generating access token for application device with client_id=#{client_id}, device_code=#{device_code}" ) case Applications.get_application_device_activation_by_device_code(client_id, device_code) do diff --git a/lib/service/export/participants/csv.ex b/lib/service/export/participants/csv.ex index 90aa737a6..1a14ff0ea 100644 --- a/lib/service/export/participants/csv.ex +++ b/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 - |> 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 + produce_file(event, filename, full_path, file, options) 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 diff --git a/lib/service/notifier/email.ex b/lib/service/notifier/email.ex index 1d5265d17..86027b631 100644 --- a/lib/service/notifier/email.ex +++ b/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", - activity: inspect(activity), - options: inspect(options) + Logger.warn( + "Can't check if user #{inspect(user)} can be sent an activity (#{inspect(activity)}) (#{inspect(options)})" ) false diff --git a/lib/service/workers/helper.ex b/lib/service/workers/helper.ex index 002ff2cca..5ebfacf9c 100644 --- a/lib/service/workers/helper.ex +++ b/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) - |> apply(:new, [params, worker_args]) - |> Oban.insert() + Oban.insert(unquote(caller_module).new(params, worker_args)) end end end