From dee7c5844920715ce5b565a0f68db468c8ad86d0 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Tue, 5 Oct 2021 17:43:45 +0200 Subject: [PATCH] Spec fixes Signed-off-by: Thomas Citharel --- lib/federation/activity_pub/actions/reject.ex | 14 +--- lib/federation/activity_pub/transmogrifier.ex | 1 + lib/federation/activity_pub/utils.ex | 3 +- .../activity_stream/converter/discussion.ex | 2 + lib/graphql/api/comments.ex | 11 ++- lib/graphql/error.ex | 2 +- lib/graphql/resolvers/comment.ex | 3 +- lib/graphql/resolvers/resource.ex | 41 +++++----- lib/graphql/resolvers/tag.ex | 2 +- lib/graphql/resolvers/user.ex | 74 ++++++++++--------- lib/mix/tasks/mobilizon/actors/refresh.ex | 5 +- lib/mix/tasks/mobilizon/instance.ex | 30 +++----- lib/mix/tasks/mobilizon/media/clean_orphan.ex | 24 +++--- lib/mix/tasks/mobilizon/relay/accept.ex | 2 +- lib/mix/tasks/mobilizon/users/clean.ex | 25 +++---- lib/mobilizon/admin/admin.ex | 24 +++--- lib/mobilizon/discussions/comment.ex | 8 +- lib/mobilizon/events/events.ex | 4 +- lib/mobilizon/posts/posts.ex | 2 +- lib/mobilizon/reports/reports.ex | 3 +- lib/mobilizon/storage/repo.ex | 2 +- lib/mobilizon/users/user.ex | 4 +- lib/service/activity/activity.ex | 4 +- lib/service/activity/member.ex | 2 +- lib/service/auth/ldap_authenticator.ex | 10 +-- lib/service/clean_orphan_media.ex | 2 +- lib/service/clean_unconfirmed_users.ex | 2 +- lib/service/export/feed.ex | 36 ++++----- lib/service/geospatial/addok.ex | 6 +- lib/service/geospatial/google_maps.ex | 39 ++++++---- lib/service/geospatial/provider.ex | 18 ++--- test/tasks/media/clean_orphan_test.exs | 11 --- .../users/clean_unconfirmed_users_test.exs | 11 --- 33 files changed, 193 insertions(+), 234 deletions(-) diff --git a/lib/federation/activity_pub/actions/reject.ex b/lib/federation/activity_pub/actions/reject.ex index cdc0ae50e..00320ad72 100644 --- a/lib/federation/activity_pub/actions/reject.ex +++ b/lib/federation/activity_pub/actions/reject.ex @@ -30,16 +30,10 @@ defmodule Mobilizon.Federation.ActivityPub.Actions.Reject do :invite -> reject_invite(entity, additional) end - with {:ok, activity} <- create_activity(update_data, local), - :ok <- maybe_federate(activity), - :ok <- maybe_relay_if_group_activity(activity) do - {:ok, activity, entity} - else - err -> - Logger.error("Something went wrong while creating an activity") - Logger.debug(inspect(err)) - err - end + {:ok, activity} = create_activity(update_data, local) + maybe_federate(activity) + maybe_relay_if_group_activity(activity) + {:ok, activity, entity} end @spec reject_join(Participant.t(), map()) :: {:ok, Participant.t(), Activity.t()} | any() diff --git a/lib/federation/activity_pub/transmogrifier.ex b/lib/federation/activity_pub/transmogrifier.ex index 128bf4d73..ca14b42d0 100644 --- a/lib/federation/activity_pub/transmogrifier.ex +++ b/lib/federation/activity_pub/transmogrifier.ex @@ -1016,6 +1016,7 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier do end # Comment and conversations have different attributes for actor and groups + @spec transform_object_data_for_discussion(map()) :: map() defp transform_object_data_for_discussion(object_data) do # Basic comment if is_data_a_discussion_initialization?(object_data) do diff --git a/lib/federation/activity_pub/utils.ex b/lib/federation/activity_pub/utils.ex index 77a3e5f78..9c6f28443 100644 --- a/lib/federation/activity_pub/utils.ex +++ b/lib/federation/activity_pub/utils.ex @@ -176,7 +176,8 @@ defmodule Mobilizon.Federation.ActivityPub.Utils do :ok end - @spec do_maybe_relay_if_group_activity(map(), list(String.t()) | String.t()) :: :ok + # TODO: Is this a map or a String? + @spec do_maybe_relay_if_group_activity(map() | String.t(), list(String.t()) | String.t()) :: :ok defp do_maybe_relay_if_group_activity(object, attributed_to) when is_list(attributed_to), do: do_maybe_relay_if_group_activity(object, hd(attributed_to)) diff --git a/lib/federation/activity_stream/converter/discussion.ex b/lib/federation/activity_stream/converter/discussion.ex index 3d611d1db..4989871a2 100644 --- a/lib/federation/activity_stream/converter/discussion.ex +++ b/lib/federation/activity_stream/converter/discussion.ex @@ -68,6 +68,8 @@ defmodule Mobilizon.Federation.ActivityStream.Converter.Discussion do %{actor_id: actor_id, creator_id: creator_id} else {:error, error} -> {:error, error} + {:ok, %Actor{url: ^creator_url}} -> {:error, :creator_suspended} + {:ok, %Actor{url: ^actor_url}} -> {:error, :actor_suspended} end end end diff --git a/lib/graphql/api/comments.ex b/lib/graphql/api/comments.ex index f8dd17b5b..0c7fabf1a 100644 --- a/lib/graphql/api/comments.ex +++ b/lib/graphql/api/comments.ex @@ -11,7 +11,9 @@ defmodule Mobilizon.GraphQL.API.Comments do @doc """ Create a comment """ - @spec create_comment(map) :: {:ok, Activity.t(), Comment.t()} | any + @spec create_comment(map) :: + {:ok, Activity.t(), Comment.t()} + | {:error, :entity_tombstoned | atom() | Ecto.Changeset.t()} def create_comment(args) do args = extract_pictures_from_comment_body(args) Actions.Create.create(:comment, args, true) @@ -20,7 +22,8 @@ defmodule Mobilizon.GraphQL.API.Comments do @doc """ Updates a comment """ - @spec update_comment(Comment.t(), map()) :: {:ok, Activity.t(), Comment.t()} | any + @spec update_comment(Comment.t(), map()) :: + {:ok, Activity.t(), Comment.t()} | {:error, atom() | Ecto.Changeset.t()} def update_comment(%Comment{} = comment, args) do args = extract_pictures_from_comment_body(args) Actions.Update.update(comment, args, true) @@ -37,7 +40,9 @@ defmodule Mobilizon.GraphQL.API.Comments do @doc """ Creates a discussion (or reply to a discussion) """ - @spec create_discussion(map()) :: map() + @spec create_discussion(map()) :: + {:ok, Activity.t(), Discussion.t()} + | {:error, :entity_tombstoned | atom | Ecto.Changeset.t()} def create_discussion(args) do args = extract_pictures_from_comment_body(args) diff --git a/lib/graphql/error.ex b/lib/graphql/error.ex index 561567c5b..e412d6465 100644 --- a/lib/graphql/error.ex +++ b/lib/graphql/error.ex @@ -20,7 +20,7 @@ defmodule Mobilizon.GraphQL.Error do # Error Tuples # ------------ # Regular errors - @spec normalize(error | list(error) | String.t() | any()) :: t() + @spec normalize(any()) :: t() | list(t()) def normalize({:error, reason}) do handle(reason) end diff --git a/lib/graphql/resolvers/comment.ex b/lib/graphql/resolvers/comment.ex index 247138f83..766749f93 100644 --- a/lib/graphql/resolvers/comment.ex +++ b/lib/graphql/resolvers/comment.ex @@ -106,7 +106,8 @@ defmodule Mobilizon.GraphQL.Resolvers.Comment do case Discussions.get_comment_with_preload(comment_id) do %CommentModel{deleted_at: nil} = comment -> cond do - {:comment_can_be_managed, true} == CommentModel.can_be_managed_by(comment, actor_id) -> + {:comment_can_be_managed, true} == + {:comment_can_be_managed, CommentModel.can_be_managed_by?(comment, actor_id)} -> do_delete_comment(comment, actor) role in [:moderator, :administrator] -> diff --git a/lib/graphql/resolvers/resource.ex b/lib/graphql/resolvers/resource.ex index f10f35c84..386eb470e 100644 --- a/lib/graphql/resolvers/resource.ex +++ b/lib/graphql/resolvers/resource.ex @@ -107,28 +107,29 @@ defmodule Mobilizon.GraphQL.Resolvers.Resource do } } = _resolution ) do - with {:member, true} <- {:member, Actors.is_member?(actor_id, group_id)}, - parent <- get_eventual_parent(args), - {:own_check, true} <- {:own_check, check_resource_owned_by_group(parent, group_id)}, - {:ok, _, %Resource{} = resource} <- - Actions.Create.create( - :resource, - args - |> Map.put(:actor_id, group_id) - |> Map.put(:creator_id, actor_id), - true, - %{} - ) do - {:ok, resource} - else - {:error, _} -> - {:error, dgettext("errors", "Error while creating resource")} + if Actors.is_member?(actor_id, group_id) do + parent = get_eventual_parent(args) - {:own_check, _} -> + if check_resource_owned_by_group(parent, group_id) do + case Actions.Create.create( + :resource, + args + |> Map.put(:actor_id, group_id) + |> Map.put(:creator_id, actor_id), + true, + %{} + ) do + {:ok, _, %Resource{} = resource} -> + {:ok, resource} + + {:error, _err} -> + {:error, dgettext("errors", "Error while creating resource")} + end + else {:error, dgettext("errors", "Parent resource doesn't belong to this group")} - - {:member, _} -> - {:error, dgettext("errors", "Profile is not member of group")} + end + else + {:error, dgettext("errors", "Profile is not member of group")} end end diff --git a/lib/graphql/resolvers/tag.ex b/lib/graphql/resolvers/tag.ex index 8257187e1..2fd96608e 100644 --- a/lib/graphql/resolvers/tag.ex +++ b/lib/graphql/resolvers/tag.ex @@ -54,7 +54,7 @@ defmodule Mobilizon.GraphQL.Resolvers.Tag do @doc """ Retrieve the list of related tags for a parent tag """ - @spec list_tags_for_post(Tag.t(), map(), Absinthe.Resolution.t()) :: {:ok, list(Tag.t())} + @spec get_related_tags(Tag.t(), map(), Absinthe.Resolution.t()) :: {:ok, list(Tag.t())} def get_related_tags(%Tag{} = tag, _args, _resolution) do {:ok, Events.list_tag_neighbors(tag)} end diff --git a/lib/graphql/resolvers/user.ex b/lib/graphql/resolvers/user.ex index d24cfdab3..6300028ca 100644 --- a/lib/graphql/resolvers/user.ex +++ b/lib/graphql/resolvers/user.ex @@ -425,35 +425,39 @@ defmodule Mobilizon.GraphQL.Resolvers.User do def change_email(_parent, %{email: new_email, password: password}, %{ context: %{current_user: %User{email: old_email} = user} }) do - with {:can_change_password, true} <- - {:can_change_password, Authenticator.can_change_email?(user)}, - {:current_password, {:ok, %User{}}} <- - {:current_password, Authenticator.login(user.email, password)}, - {:same_email, false} <- {:same_email, new_email == old_email}, - {:email_valid, true} <- {:email_valid, Email.Checker.valid?(new_email)}, - {:ok, %User{} = user} <- Users.update_user_email(user, new_email) do - user - |> Email.User.send_email_reset_old_email() - |> Email.Mailer.send_email_later() + 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_later() - user - |> Email.User.send_email_reset_new_email() - |> Email.Mailer.send_email_later() + user + |> Email.User.send_email_reset_new_email() + |> Email.Mailer.send_email_later() - {:ok, user} + {: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 else - {:current_password, {:error, _}} -> - {:error, dgettext("errors", "The password provided is invalid")} - - {:same_email, true} -> - {:error, dgettext("errors", "The new email must be different")} - - {:email_valid, false} -> - {:error, dgettext("errors", "The new email doesn't seem to be valid")} - - {:error, %Ecto.Changeset{} = err} -> - Logger.debug(inspect(err)) - {:error, dgettext("errors", "Failed to update user email")} + {:error, dgettext("errors", "User cannot change email")} end end @@ -635,14 +639,14 @@ defmodule Mobilizon.GraphQL.Resolvers.User do user, context ) do - with current_ip <- Map.get(context, :ip), - now <- DateTime.utc_now() do - Users.update_user(user, %{ - last_sign_in_at: current_sign_in_at || now, - last_sign_in_ip: current_sign_in_ip || current_ip, - current_sign_in_ip: current_ip, - current_sign_in_at: now - }) - end + current_ip = Map.get(context, :ip) + now = DateTime.utc_now() + + Users.update_user(user, %{ + last_sign_in_at: current_sign_in_at || now, + last_sign_in_ip: current_sign_in_ip || current_ip, + current_sign_in_ip: current_ip, + current_sign_in_at: now + }) end end diff --git a/lib/mix/tasks/mobilizon/actors/refresh.ex b/lib/mix/tasks/mobilizon/actors/refresh.ex index 9c1b20d06..aafbfe22d 100644 --- a/lib/mix/tasks/mobilizon/actors/refresh.ex +++ b/lib/mix/tasks/mobilizon/actors/refresh.ex @@ -71,10 +71,7 @@ defmodule Mix.Tasks.Mobilizon.Actors.Refresh do Actor #{preferred_username} refreshed """) - {:error, err} when is_binary(err) -> - shell_error(err) - - _err -> + {:error, _err} -> shell_error("Error while refreshing actor #{preferred_username}") end end diff --git a/lib/mix/tasks/mobilizon/instance.ex b/lib/mix/tasks/mobilizon/instance.ex index c2529b117..0bbbd0c69 100644 --- a/lib/mix/tasks/mobilizon/instance.ex +++ b/lib/mix/tasks/mobilizon/instance.ex @@ -187,35 +187,29 @@ defmodule Mix.Tasks.Mobilizon.Instance do end end + @spec write_config(String.t(), String.t()) :: :ok | {:error, atom()} defp write_config(config_path, result_config) do shell_info("Writing config to #{config_path}.") - case File.write(config_path, result_config) do - :ok -> - :ok + with {:error, err} <- File.write(config_path, result_config) do + shell_error( + "\nERROR: Unable to write config file to #{config_path}. Make sure you have permissions on the destination and that the parent path exists.\n" + ) - {:error, err} -> - shell_error( - "\nERROR: Unable to write config file to #{config_path}. Make sure you have permissions on the destination and that the parent path exists.\n" - ) - - {:error, err} + {:error, err} end end + @spec write_psql(String.t(), String.t()) :: :ok | {:error, atom()} defp write_psql(psql_path, result_psql) do shell_info("Writing #{psql_path}.") - case File.write(psql_path, result_psql) do - :ok -> - :ok + with {:error, err} <- File.write(psql_path, result_psql) do + shell_error( + "\nERROR: Unable to write psql file to #{psql_path}. Make sure you have permissions on the destination and that the parent path exists.\n" + ) - {:error, err} -> - shell_error( - "\nERROR: Unable to write psql file to #{psql_path}. Make sure you have permissions on the destination and that the parent path exists.\n" - ) - - {:error, err} + {:error, err} end end end diff --git a/lib/mix/tasks/mobilizon/media/clean_orphan.ex b/lib/mix/tasks/mobilizon/media/clean_orphan.ex index 09ba6a37f..42f954ab9 100644 --- a/lib/mix/tasks/mobilizon/media/clean_orphan.ex +++ b/lib/mix/tasks/mobilizon/media/clean_orphan.ex @@ -34,22 +34,16 @@ defmodule Mix.Tasks.Mobilizon.Media.CleanOrphan do start_mobilizon() - case CleanOrphanMedia.clean(dry_run: dry_run, grace_period: grace_period) do - {:ok, medias} -> - if length(medias) > 0 do - if dry_run or verbose do - details(medias, dry_run, verbose) - end + {:ok, medias} = CleanOrphanMedia.clean(dry_run: dry_run, grace_period: grace_period) - result(dry_run, length(medias)) - else - empty_result(dry_run) - end + if length(medias) > 0 do + if dry_run or verbose do + details(medias, dry_run, verbose) + end - :ok - - _err -> - shell_error("Error while cleaning orphan media files") + result(dry_run, length(medias)) + else + empty_result(dry_run) end end @@ -70,7 +64,7 @@ defmodule Mix.Tasks.Mobilizon.Media.CleanOrphan do end) end - @spec result(boolean(), boolean()) :: :ok + @spec result(boolean(), non_neg_integer()) :: :ok defp result(dry_run, nb_medias) do if dry_run do shell_info("#{nb_medias} files would have been deleted") diff --git a/lib/mix/tasks/mobilizon/relay/accept.ex b/lib/mix/tasks/mobilizon/relay/accept.ex index 88d594ff8..e2401324e 100644 --- a/lib/mix/tasks/mobilizon/relay/accept.ex +++ b/lib/mix/tasks/mobilizon/relay/accept.ex @@ -13,7 +13,7 @@ defmodule Mix.Tasks.Mobilizon.Relay.Accept do start_mobilizon() case Relay.accept(target) do - {:ok, _activity} -> + {:ok, _activity, _follow} -> # put this task to sleep to allow the genserver to push out the messages :timer.sleep(500) diff --git a/lib/mix/tasks/mobilizon/users/clean.ex b/lib/mix/tasks/mobilizon/users/clean.ex index 336af5ab6..43af9116e 100644 --- a/lib/mix/tasks/mobilizon/users/clean.ex +++ b/lib/mix/tasks/mobilizon/users/clean.ex @@ -35,23 +35,20 @@ defmodule Mix.Tasks.Mobilizon.Users.Clean do start_mobilizon() - case CleanUnconfirmedUsers.clean(dry_run: dry_run, grace_period: grace_period) do - {:ok, deleted_users} -> - if length(deleted_users) > 0 do - if dry_run or verbose do - details(deleted_users, dry_run, verbose) - end + {:ok, deleted_users} = + CleanUnconfirmedUsers.clean(dry_run: dry_run, grace_period: grace_period) - result(dry_run, length(deleted_users)) - else - empty_result(dry_run) - end + if length(deleted_users) > 0 do + if dry_run or verbose do + details(deleted_users, dry_run, verbose) + end - :ok - - _err -> - shell_error("Error while cleaning unconfirmed users") + result(dry_run, length(deleted_users)) + else + empty_result(dry_run) end + + :ok end @spec details(list(Media.t()), boolean(), boolean()) :: :ok diff --git a/lib/mobilizon/admin/admin.ex b/lib/mobilizon/admin/admin.ex index 24924e481..de267042d 100644 --- a/lib/mobilizon/admin/admin.ex +++ b/lib/mobilizon/admin/admin.ex @@ -48,20 +48,18 @@ defmodule Mobilizon.Admin do @spec log_action(Actor.t(), String.t(), struct()) :: {:ok, ActionLog.t()} | {:error, Ecto.Changeset.t() | :user_not_moderator} def log_action(%Actor{user_id: user_id, id: actor_id}, action, target) do - with %User{role: role} <- Users.get_user!(user_id), - {:role, true} <- {:role, role in [:administrator, :moderator]}, - {:ok, %ActionLog{} = create_action_log} <- - Admin.create_action_log(%{ - "actor_id" => actor_id, - "target_type" => to_string(target.__struct__), - "target_id" => target.id, - "action" => action, - "changes" => stringify_struct(target) - }) do - {:ok, create_action_log} + %User{role: role} = Users.get_user!(user_id) + + if role in [:administrator, :moderator] do + Admin.create_action_log(%{ + "actor_id" => actor_id, + "target_type" => to_string(target.__struct__), + "target_id" => target.id, + "action" => action, + "changes" => stringify_struct(target) + }) else - {:role, false} -> - {:error, :user_not_moderator} + {:error, :user_not_moderator} end end diff --git a/lib/mobilizon/discussions/comment.ex b/lib/mobilizon/discussions/comment.ex index 6ae10d27e..42915189f 100644 --- a/lib/mobilizon/discussions/comment.ex +++ b/lib/mobilizon/discussions/comment.ex @@ -115,13 +115,13 @@ defmodule Mobilizon.Discussions.Comment do @doc """ Checks whether an comment can be managed. """ - @spec can_be_managed_by(t, integer | String.t()) :: boolean - def can_be_managed_by(%__MODULE__{actor_id: creator_actor_id}, actor_id) + @spec can_be_managed_by?(t, integer | String.t()) :: boolean() + def can_be_managed_by?(%__MODULE__{actor_id: creator_actor_id}, actor_id) when creator_actor_id == actor_id do - {:comment_can_be_managed, true} + creator_actor_id == actor_id end - def can_be_managed_by(_comment, _actor), do: {:comment_can_be_managed, false} + def can_be_managed_by?(_comment, _actor), do: false defp common_changeset(%__MODULE__{} = comment, attrs) do comment diff --git a/lib/mobilizon/events/events.ex b/lib/mobilizon/events/events.ex index 25982f68a..a1f9aeef2 100644 --- a/lib/mobilizon/events/events.ex +++ b/lib/mobilizon/events/events.ex @@ -572,7 +572,7 @@ defmodule Mobilizon.Events do @doc """ Returns the list of tags. """ - @spec list_tags(String.t() | nil, integer | nil, integer | nil) :: [Tag.t()] + @spec list_tags(String.t() | nil, integer | nil, integer | nil) :: Page.t(Tag.t()) def list_tags(filter \\ nil, page \\ nil, limit \\ nil) do Tag |> tag_filter(filter) @@ -608,7 +608,7 @@ defmodule Mobilizon.Events do Creates a relation between two tags. """ @spec create_tag_relation(map) :: {:ok, TagRelation.t()} | {:error, Changeset.t()} - def create_tag_relation(attrs \\ {}) do + def create_tag_relation(attrs) do %TagRelation{} |> TagRelation.changeset(attrs) |> Repo.insert( diff --git a/lib/mobilizon/posts/posts.ex b/lib/mobilizon/posts/posts.ex index 2f4d7897b..716379bf9 100644 --- a/lib/mobilizon/posts/posts.ex +++ b/lib/mobilizon/posts/posts.ex @@ -131,7 +131,7 @@ defmodule Mobilizon.Posts do |> Repo.all() end - @spec tags_for_post_query(integer) :: Ecto.Query.t() + @spec tags_for_post_query(String.t()) :: Ecto.Query.t() defp tags_for_post_query(post_id) do from( t in Tag, diff --git a/lib/mobilizon/reports/reports.ex b/lib/mobilizon/reports/reports.ex index 419dd3b9d..c9b59f9cc 100644 --- a/lib/mobilizon/reports/reports.ex +++ b/lib/mobilizon/reports/reports.ex @@ -49,7 +49,8 @@ defmodule Mobilizon.Reports do @doc """ Returns the list of reports. """ - @spec list_reports(integer | nil, integer | nil, atom, atom, ReportStatus) :: Page.t() + @spec list_reports(integer | nil, integer | nil, atom, atom, ReportStatus.t()) :: + Page.t(Report.t()) def list_reports( page \\ nil, limit \\ nil, diff --git a/lib/mobilizon/storage/repo.ex b/lib/mobilizon/storage/repo.ex index 67796d3d4..545b9e6c0 100644 --- a/lib/mobilizon/storage/repo.ex +++ b/lib/mobilizon/storage/repo.ex @@ -10,7 +10,7 @@ defmodule Mobilizon.Storage.Repo do @doc """ Dynamically loads the repository url from the DATABASE_URL environment variable. """ - @spec init(any(), any()) :: any() + @spec init(any(), any()) :: {:ok, Keyword.t()} def init(_, opts) do {:ok, opts} end diff --git a/lib/mobilizon/users/user.ex b/lib/mobilizon/users/user.ex index c0e50b6a9..cd3a951d7 100644 --- a/lib/mobilizon/users/user.ex +++ b/lib/mobilizon/users/user.ex @@ -31,8 +31,8 @@ defmodule Mobilizon.Users.User do feed_tokens: [FeedToken.t()], last_sign_in_at: DateTime.t(), last_sign_in_ip: String.t(), - current_sign_in_ip: String.t(), - current_sign_in_at: DateTime.t(), + current_sign_in_ip: String.t() | nil, + current_sign_in_at: DateTime.t() | nil, activity_settings: [ActivitySetting.t()], settings: Setting.t(), unconfirmed_email: String.t() | nil diff --git a/lib/service/activity/activity.ex b/lib/service/activity/activity.ex index 6287f86b4..9c436fecf 100644 --- a/lib/service/activity/activity.ex +++ b/lib/service/activity/activity.ex @@ -6,8 +6,8 @@ defmodule Mobilizon.Service.Activity do alias Mobilizon.Activities.Activity alias Mobilizon.Service.Activity.{Comment, Discussion, Event, Group, Member, Post, Resource} - @callback insert_activity(entity :: struct(), options :: map()) :: - {:ok, Oban.Job.t()} | {:ok, any()} + @callback insert_activity(entity :: struct(), options :: Keyword.t()) :: + {:ok, Oban.Job.t()} | {:ok, any()} | {:error, Ecto.Changeset.t()} @callback get_object(object_id :: String.t() | integer()) :: struct() diff --git a/lib/service/activity/member.ex b/lib/service/activity/member.ex index 704a55e6e..72d0f55f8 100644 --- a/lib/service/activity/member.ex +++ b/lib/service/activity/member.ex @@ -40,7 +40,7 @@ defmodule Mobilizon.Service.Activity.Member do Actors.get_member(member_id) end - @spec get_author(Member.t(), Member.t() | nil) :: integer() + @spec get_author(Member.t(), Keyword.t()) :: integer() defp get_author(%Member{actor_id: actor_id}, options) do moderator = Keyword.get(options, :moderator) diff --git a/lib/service/auth/ldap_authenticator.ex b/lib/service/auth/ldap_authenticator.ex index d4816a30b..6dba56edd 100644 --- a/lib/service/auth/ldap_authenticator.ex +++ b/lib/service/auth/ldap_authenticator.ex @@ -191,14 +191,10 @@ defmodule Mobilizon.Service.Auth.LDAPAuthenticator do ]) end - @spec register_user(String.t()) :: User.t() | any() + @spec register_user(String.t()) :: User.t() | {:error, Ecto.Changeset.t()} defp register_user(email) do - case Users.create_external(email, "ldap") do - {:ok, %User{} = user} -> - user - - error -> - error + with {:ok, %User{} = user} <- Users.create_external(email, "ldap") do + user end end diff --git a/lib/service/clean_orphan_media.ex b/lib/service/clean_orphan_media.ex index 27c674aa5..b02d2953f 100644 --- a/lib/service/clean_orphan_media.ex +++ b/lib/service/clean_orphan_media.ex @@ -19,7 +19,7 @@ defmodule Mobilizon.Service.CleanOrphanMedia do * `grace_period` how old in hours can the media be before it's taken into account for deletion * `dry_run` just return the media that would have been deleted, don't actually delete it """ - @spec clean(Keyword.t()) :: {:ok, list(Media.t())} | {:error, String.t()} + @spec clean(Keyword.t()) :: {:ok, list(Media.t())} def clean(opts \\ []) do medias = find_media(opts) diff --git a/lib/service/clean_unconfirmed_users.ex b/lib/service/clean_unconfirmed_users.ex index 28c77d39e..c78db8fc9 100644 --- a/lib/service/clean_unconfirmed_users.ex +++ b/lib/service/clean_unconfirmed_users.ex @@ -15,7 +15,7 @@ defmodule Mobilizon.Service.CleanUnconfirmedUsers do Remove media that is not attached to an entity, such as media uploads that were never used in entities. """ - @spec clean(Keyword.t()) :: {:ok, list(Media.t())} | {:error, String.t()} + @spec clean(Keyword.t()) :: {:ok, list(Media.t())} def clean(opts \\ []) do users_to_delete = find_unconfirmed_users_to_clean(opts) diff --git a/lib/service/export/feed.ex b/lib/service/export/feed.ex index 6af40bada..106d0a320 100644 --- a/lib/service/export/feed.ex +++ b/lib/service/export/feed.ex @@ -21,49 +21,41 @@ defmodule Mobilizon.Service.Export.Feed do @item_limit 500 - def version, do: Config.instance_version() + @spec version :: String.t() + defp version, do: Config.instance_version() - @spec create_cache(String.t()) :: {:commit, String.t()} | {:ignore, any()} + @spec create_cache(String.t()) :: + {:commit, String.t()} + | {:ignore, :actor_not_found | :actor_not_public | :bad_token | :token_not_found} def create_cache("actor_" <> name) do case fetch_actor_event_feed(name) do {:ok, res} -> {:commit, res} - err -> + {:error, err} -> {:ignore, err} end end - @spec create_cache(String.t()) :: {:commit, String.t()} | {:ignore, any()} def create_cache("token_" <> token) do case fetch_events_from_token(token) do {:ok, res} -> {:commit, res} - err -> + {:error, err} -> {:ignore, err} end end def create_cache("instance") do - case fetch_instance_feed() do - {:ok, res} -> - {:commit, res} - - err -> - {:ignore, err} - end + {:ok, res} = fetch_instance_feed() + {:commit, res} end @spec fetch_instance_feed :: {:ok, String.t()} defp fetch_instance_feed do - case Common.fetch_instance_public_content(@item_limit) do - {:ok, events, posts} -> - {:ok, build_instance_feed(events, posts)} - - err -> - {:error, err} - end + {:ok, events, posts} = Common.fetch_instance_public_content(@item_limit) + {:ok, build_instance_feed(events, posts)} end # Build an atom feed from the whole instance and its public events and posts @@ -90,7 +82,8 @@ defmodule Mobilizon.Service.Export.Feed do |> Atomex.generate_document() end - @spec fetch_actor_event_feed(String.t(), integer()) :: String.t() + @spec fetch_actor_event_feed(String.t(), integer()) :: + {:ok, String.t()} | {:error, :actor_not_found | :actor_not_public} defp fetch_actor_event_feed(name, limit \\ @item_limit) do case Common.fetch_actor_event_feed(name, limit) do {:ok, actor, events, posts} -> @@ -199,7 +192,8 @@ defmodule Mobilizon.Service.Export.Feed do end # Only events, not posts - @spec fetch_events_from_token(String.t(), integer()) :: {:ok, String.t()} | {:error, atom()} + @spec fetch_events_from_token(String.t(), integer()) :: + {:ok, String.t()} | {:error, :bad_token | :token_not_found} defp fetch_events_from_token(token, limit \\ @item_limit) do case Common.fetch_events_from_token(token, limit) do %{events: events, token: token, user: user, actor: actor, type: type} -> diff --git a/lib/service/geospatial/addok.ex b/lib/service/geospatial/addok.ex index bf3d1f253..d9c196dbf 100644 --- a/lib/service/geospatial/addok.ex +++ b/lib/service/geospatial/addok.ex @@ -93,11 +93,7 @@ defmodule Mobilizon.Service.Geospatial.Addok do if is_nil(value), do: url, else: do_add_parameter(url, key, value) end - @spec do_add_parameter(String.t(), :coords | :type, %{lat: float, lon: float} | :administrative) :: - String.t() - defp do_add_parameter(url, :coords, coords), - do: "#{url}&lat=#{coords.lat}&lon=#{coords.lon}" - + @spec do_add_parameter(String.t(), :type, :administrative | atom()) :: String.t() defp do_add_parameter(url, :type, :administrative), do: "#{url}&type=municipality" diff --git a/lib/service/geospatial/google_maps.ex b/lib/service/geospatial/google_maps.ex index 0b50c5991..936e000f5 100644 --- a/lib/service/geospatial/google_maps.ex +++ b/lib/service/geospatial/google_maps.ex @@ -31,16 +31,18 @@ defmodule Mobilizon.Service.Geospatial.GoogleMaps do @doc """ Google Maps implementation for `c:Mobilizon.Service.Geospatial.Provider.geocode/3`. """ - @spec geocode(String.t(), keyword()) :: list(Address.t()) | no_return + @spec geocode(float(), float(), keyword()) :: list(Address.t()) def geocode(lon, lat, options \\ []) do url = build_url(:geocode, %{lon: lon, lat: lat}, options) Logger.debug("Asking Google Maps for reverse geocode with #{url}") - with {:ok, %{status: 200, body: body}} <- GeospatialClient.get(url), - %{"results" => results, "status" => "OK"} <- body do - Enum.map(results, fn entry -> process_data(entry, options) end) - else + %Tesla.Env{status: 200, body: body} = GeospatialClient.get!(url) + + case body do + %{"results" => results, "status" => "OK"} -> + Enum.map(results, &process_data(&1, options)) + %{"status" => "REQUEST_DENIED", "error_message" => error_message} -> raise ArgumentError, message: to_string(error_message) end @@ -50,16 +52,18 @@ defmodule Mobilizon.Service.Geospatial.GoogleMaps do @doc """ Google Maps implementation for `c:Mobilizon.Service.Geospatial.Provider.search/2`. """ - @spec search(String.t(), keyword()) :: list(Address.t()) | no_return + @spec search(String.t(), keyword()) :: list(Address.t()) def search(q, options \\ []) do url = build_url(:search, %{q: q}, options) Logger.debug("Asking Google Maps for addresses with #{url}") - with {:ok, %{status: 200, body: body}} <- GeospatialClient.get(url), - %{"results" => results, "status" => "OK"} <- body do - results |> Enum.map(fn entry -> process_data(entry, options) end) - else + %Tesla.Env{status: 200, body: body} = GeospatialClient.get!(url) + + case body do + %{"results" => results, "status" => "OK"} -> + results |> Enum.map(fn entry -> process_data(entry, options) end) + %{"status" => "REQUEST_DENIED", "error_message" => error_message} -> raise ArgumentError, message: to_string(error_message) @@ -68,7 +72,7 @@ defmodule Mobilizon.Service.Geospatial.GoogleMaps do end end - @spec build_url(atom(), map(), list()) :: String.t() | no_return + @spec build_url(:search | :geocode, map(), list()) :: String.t() | no_return defp build_url(method, args, options) do limit = Keyword.get(options, :limit, 10) lang = Keyword.get(options, :lang, "en") @@ -97,6 +101,7 @@ defmodule Mobilizon.Service.Geospatial.GoogleMaps do URI.encode(uri) end + @spec process_data(map(), Keyword.t()) :: Address.t() defp process_data( %{ "formatted_address" => description, @@ -148,16 +153,18 @@ defmodule Mobilizon.Service.Geospatial.GoogleMaps do end end - @spec do_fetch_place_details(String.t() | nil, Keyword.t()) :: String.t() | no_return + @spec do_fetch_place_details(String.t() | nil, Keyword.t()) :: String.t() | nil defp do_fetch_place_details(place_id, options) do url = build_url(:place_details, %{place_id: place_id}, options) Logger.debug("Asking Google Maps for details with #{url}") - with {:ok, %{status: 200, body: body}} <- GeospatialClient.get(url), - %{"result" => %{"name" => name}, "status" => "OK"} <- body do - name - else + %Tesla.Env{status: 200, body: body} = GeospatialClient.get!(url) + + case body do + %{"result" => %{"name" => name}, "status" => "OK"} -> + name + %{"status" => "REQUEST_DENIED", "error_message" => error_message} -> raise ArgumentError, message: to_string(error_message) diff --git a/lib/service/geospatial/provider.ex b/lib/service/geospatial/provider.ex index 47d92f7ba..8c20712c6 100644 --- a/lib/service/geospatial/provider.ex +++ b/lib/service/geospatial/provider.ex @@ -43,7 +43,7 @@ defmodule Mobilizon.Service.Geospatial.Provider do %Address{} """ @callback geocode(longitude :: number, latitude :: number, options :: keyword) :: - [Address.t()] | {:error, atom()} + [Address.t()] @doc """ Search for an address @@ -63,23 +63,21 @@ defmodule Mobilizon.Service.Geospatial.Provider do iex> search("10 rue Jangot") %Address{} """ - @callback search(address :: String.t(), options :: keyword) :: [Address.t()] | {:error, atom()} + @callback search(address :: String.t(), options :: keyword) :: [Address.t()] @doc """ Returns a `Geo.Point` for given coordinates """ - @spec coordinates([number | String.t()], number) :: Geo.Point.t() | nil - def coordinates(coords, srid \\ 4326) - - def coordinates([x, y], srid) when is_number(x) and is_number(y) do - %Geo.Point{coordinates: {x, y}, srid: srid} + @spec coordinates([number | String.t()]) :: Geo.Point.t() | nil + def coordinates([x, y]) when is_number(x) and is_number(y) do + %Geo.Point{coordinates: {x, y}, srid: 4326} end - def coordinates([x, y], srid) when is_binary(x) and is_binary(y) do - %Geo.Point{coordinates: {String.to_float(x), String.to_float(y)}, srid: srid} + def coordinates([x, y]) when is_binary(x) and is_binary(y) do + %Geo.Point{coordinates: {String.to_float(x), String.to_float(y)}, srid: 4326} end - def coordinates(_, _), do: nil + def coordinates(_), do: nil @spec endpoint(atom()) :: String.t() def endpoint(provider) do diff --git a/test/tasks/media/clean_orphan_test.exs b/test/tasks/media/clean_orphan_test.exs index 83440dcf2..073dc1ded 100644 --- a/test/tasks/media/clean_orphan_test.exs +++ b/test/tasks/media/clean_orphan_test.exs @@ -113,17 +113,6 @@ defmodule Mix.Tasks.Mobilizon.Media.CleanOrphanTest do end end - describe "returns an error" do - test "for some reason" do - with_mock CleanOrphanMedia, - clean: fn [dry_run: false, grace_period: 48] -> {:error, "Some error"} end do - CleanOrphan.run([]) - assert_received {:mix_shell, :error, [output_received]} - assert output_received == "Error while cleaning orphan media files" - end - end - end - defp create_file do File.cp!("test/fixtures/picture.png", "test/fixtures/picture_tmp.png") diff --git a/test/tasks/users/clean_unconfirmed_users_test.exs b/test/tasks/users/clean_unconfirmed_users_test.exs index 10d56c573..165ac8709 100644 --- a/test/tasks/users/clean_unconfirmed_users_test.exs +++ b/test/tasks/users/clean_unconfirmed_users_test.exs @@ -117,15 +117,4 @@ defmodule Mix.Tasks.Mobilizon.Media.CleanUnconfirmedUsersTest do end end end - - describe "returns an error" do - test "for some reason" do - with_mock CleanUnconfirmedUsers, - clean: fn [dry_run: false, grace_period: 48] -> {:error, "Some error"} end do - Clean.run([]) - assert_received {:mix_shell, :error, [output_received]} - assert output_received == "Error while cleaning unconfirmed users" - end - end - end end