From 3a4a006c4454b1caf29e0f5faea1d8310e7ebf5d Mon Sep 17 00:00:00 2001 From: miffy Date: Sun, 8 Sep 2019 03:05:30 +0200 Subject: [PATCH] Refactoring of Adresses context --- lib/mobilizon/addresses/address.ex | 43 +++- lib/mobilizon/addresses/addresses.ex | 257 +++++++------------- test/mobilizon/addresses/addresses_test.exs | 18 -- 3 files changed, 119 insertions(+), 199 deletions(-) diff --git a/lib/mobilizon/addresses/address.ex b/lib/mobilizon/addresses/address.ex index 97f635668..7fd4b3af7 100644 --- a/lib/mobilizon/addresses/address.ex +++ b/lib/mobilizon/addresses/address.ex @@ -1,12 +1,31 @@ defmodule Mobilizon.Addresses.Address do - @moduledoc "An address for an event or a group" + @moduledoc """ + Represents an address for an event or a group. + """ use Ecto.Schema + import Ecto.Changeset + alias Mobilizon.Addresses.Address alias Mobilizon.Events.Event - # alias Mobilizon.Actors.Actor - @attrs [ + + @type t :: %__MODULE__{ + country: String.t(), + locality: String.t(), + region: String.t(), + description: String.t(), + floor: String.t(), + geom: Geo.PostGIS.Geometry.t(), + postal_code: String.t(), + street: String.t(), + url: String.t(), + origin_id: String.t(), + events: [Event.t()] + } + + @required_attrs [:url] + @optional_attrs [ :description, :floor, :geom, @@ -15,12 +34,9 @@ defmodule Mobilizon.Addresses.Address do :region, :postal_code, :street, - :url, :origin_id ] - @required [ - :url - ] + @attrs @required_attrs ++ @optional_attrs schema "addresses" do field(:country, :string) @@ -33,22 +49,29 @@ defmodule Mobilizon.Addresses.Address do field(:street, :string) field(:url, :string) field(:origin_id, :string) - has_many(:event, Event, foreign_key: :physical_address_id) + + has_many(:events, Event, foreign_key: :physical_address_id) timestamps() end @doc false + @spec changeset(t | Ecto.Changeset.t(), map) :: Ecto.Changeset.t() def changeset(%Address{} = address, attrs) do address |> cast(attrs, @attrs) |> set_url() - |> validate_required(@required) + |> validate_required(@required_attrs) end + @spec set_url(Ecto.Changeset.t()) :: Ecto.Changeset.t() defp set_url(%Ecto.Changeset{changes: changes} = changeset) do url = - Map.get(changes, :url, MobilizonWeb.Endpoint.url() <> "/address/#{Ecto.UUID.generate()}") + Map.get( + changes, + :url, + "#{MobilizonWeb.Endpoint.url()}/address/#{Ecto.UUID.generate()}" + ) put_change(changeset, :url, url) end diff --git a/lib/mobilizon/addresses/addresses.ex b/lib/mobilizon/addresses/addresses.ex index 19f6325ad..d1dc3b99c 100644 --- a/lib/mobilizon/addresses/addresses.ex +++ b/lib/mobilizon/addresses/addresses.ex @@ -3,83 +3,50 @@ defmodule Mobilizon.Addresses do The Addresses context. """ - import Ecto.Query, warn: false + import Ecto.Query alias Mobilizon.Addresses.Address alias Mobilizon.Storage.Repo require Logger - @geom_types [:point] + @doc false + @spec data :: Dataloader.Ecto.t() + def data, do: Dataloader.Ecto.new(Repo, query: &query/2) @doc false - def data() do - Dataloader.Ecto.new(Repo, query: &query/2) - end - - @doc false - def query(queryable, _params) do - queryable - end + @spec query(Ecto.Query.t(), map) :: Ecto.Query.t() + def query(queryable, _params), do: queryable @doc """ Returns the list of addresses. - - ## Examples - - iex> list_addresses() - [%Address{}, ...] - """ - def list_addresses do - Repo.all(Address) - end + @spec list_addresses :: [Address.t()] + def list_addresses, do: Repo.all(Address) @doc """ Gets a single address. - - Raises `Ecto.NoResultsError` if the Address does not exist. - - ## Examples - - iex> get_address!(123) - %Address{} - - iex> get_address!(456) - ** (Ecto.NoResultsError) - """ - def get_address!(id), do: Repo.get!(Address, id) - + @spec get_address(integer | String.t()) :: Address.t() | nil def get_address(id), do: Repo.get(Address, id) @doc """ - Gets a single address by it's url - - ## Examples - - iex> get_address_by_url("https://mobilizon.social/addresses/4572") - %Address{} - - iex> get_address_by_url("https://mobilizon.social/addresses/099") - nil + Gets a single address. + Raises `Ecto.NoResultsError` if the address does not exist. """ - def get_address_by_url(url) do - Repo.get_by(Address, url: url) - end + @spec get_address!(integer | String.t()) :: Address.t() + def get_address!(id), do: Repo.get!(Address, id) @doc """ - Creates a address. - - ## Examples - - iex> create_address(%{field: value}) - {:ok, %Address{}} - - iex> create_address(%{field: bad_value}) - {:error, %Ecto.Changeset{}} - + Gets a single address by its url. """ + @spec get_address_by_url(String.t()) :: Address.t() | nil + def get_address_by_url(url), do: Repo.get_by(Address, url: url) + + @doc """ + Creates an address. + """ + @spec create_address(map) :: {:ok, Address.t()} | {:error, Ecto.Changeset.t()} def create_address(attrs \\ %{}) do %Address{} |> Address.changeset(attrs) @@ -90,17 +57,9 @@ defmodule Mobilizon.Addresses do end @doc """ - Updates a address. - - ## Examples - - iex> update_address(address, %{field: new_value}) - {:ok, %Address{}} - - iex> update_address(address, %{field: bad_value}) - {:error, %Ecto.Changeset{}} - + Updates an address. """ + @spec update_address(Address.t(), map) :: {:ok, Address.t()} | {:error, Ecto.Changeset.t()} def update_address(%Address{} = address, attrs) do address |> Address.changeset(attrs) @@ -108,131 +67,87 @@ defmodule Mobilizon.Addresses do end @doc """ - Deletes a Address. - - ## Examples - - iex> delete_address(address) - {:ok, %Address{}} - - iex> delete_address(address) - {:error, %Ecto.Changeset{}} - + Deletes an address. """ - def delete_address(%Address{} = address) do - Repo.delete(address) - end + @spec delete_address(Address.t()) :: {:ok, Address.t()} | {:error, Ecto.Changeset.t()} + def delete_address(%Address{} = address), do: Repo.delete(address) @doc """ - Returns an `%Ecto.Changeset{}` for tracking address changes. - - ## Examples - - iex> change_address(address) - %Ecto.Changeset{source: %Address{}} + Searches addresses. + We only look at the description for now, and eventually order by object distance. """ - def change_address(%Address{} = address) do - Address.changeset(address, %{}) - end + @spec search_addresses(String.t(), keyword) :: [Address.t()] + def search_addresses(search, options \\ []) do + query = + search + |> search_addresses_query(Keyword.get(options, :limit, 5)) + |> order_by_coords(Keyword.get(options, :coords)) + |> filter_by_contry(Keyword.get(options, :country)) - @doc """ - Processes raw geo data informations and return a `Geo` geometry which can be one of `Geo.Point`. - """ - # TODO: Unused, remove me - def process_geom(%{"type" => type_input, "data" => data}) do - type = - if !is_atom(type_input) && type_input != nil do - try do - String.to_existing_atom(type_input) - rescue - e in ArgumentError -> - Logger.error("#{type_input} is not an existing atom : #{inspect(e)}") - :invalid_type - end - else - type_input - end - - if Enum.member?(@geom_types, type) do - case type do - :point -> - process_point(data["latitude"], data["longitude"]) - end - else - {:error, :invalid_type} + case Keyword.get(options, :single, false) do + true -> Repo.one(query) + false -> Repo.all(query) end end - @doc false - def process_geom(nil) do - {:error, nil} - end - - @spec process_point(number(), number()) :: tuple() - defp process_point(latitude, longitude) when is_number(latitude) and is_number(longitude) do - {:ok, %Geo.Point{coordinates: {latitude, longitude}, srid: 4326}} - end - - defp process_point(_, _) do - {:error, "Latitude and longitude must be numbers"} - end - @doc """ - Search addresses in our database + Reverse geocode from coordinates. - We only look at the description for now, and eventually order by object distance + We only take addresses 50km around and sort them by distance. """ - @spec search_addresses(String.t(), list()) :: list(Address.t()) - def search_addresses(search, options \\ []) do - limit = Keyword.get(options, :limit, 5) - - query = from(a in Address, where: ilike(a.description, ^"%#{search}%"), limit: ^limit) - - query = - if coords = Keyword.get(options, :coords, false), - do: - from(a in query, - order_by: [fragment("? <-> ?", a.geom, ^"POINT(#{coords.lon} #{coords.lat})'")] - ), - else: query - - query = - if country = Keyword.get(options, :country, nil), - do: from(a in query, where: ilike(a.country, ^"%#{country}%")), - else: query - - if Keyword.get(options, :single, false) == true, do: Repo.one(query), else: Repo.all(query) - end - - @doc """ - Reverse geocode from coordinates in our database - - We only take addresses 50km around and sort them by distance - """ - @spec reverse_geocode(number(), number(), list()) :: list(Address.t()) + @spec reverse_geocode(number, number, keyword) :: [Address.t()] def reverse_geocode(lon, lat, options) do limit = Keyword.get(options, :limit, 5) radius = Keyword.get(options, :radius, 50_000) - country = Keyword.get(options, :country, nil) + country = Keyword.get(options, :country) srid = Keyword.get(options, :srid, 4326) - import Geo.PostGIS - with {:ok, point} <- Geo.WKT.decode("SRID=#{srid};POINT(#{lon} #{lat})") do - query = - from(a in Address, - order_by: [fragment("? <-> ?", a.geom, ^point)], - limit: ^limit, - where: st_dwithin_in_meters(^point, a.geom, ^radius) - ) - - query = - if country, - do: from(a in query, where: ilike(a.country, ^"%#{country}%")), - else: query - - Repo.all(query) + point + |> addresses_around_query(radius, limit) + |> filter_by_contry(country) + |> Repo.all() end end + + @spec search_addresses_query(String.t(), integer) :: Ecto.Query.t() + defp search_addresses_query(search, limit) do + from( + a in Address, + where: ilike(a.description, ^"%#{search}%"), + limit: ^limit + ) + end + + @spec order_by_coords(Ecto.Query.t(), map | nil) :: Ecto.Query.t() + defp order_by_coords(query, nil), do: query + + defp order_by_coords(query, coords) do + from( + a in query, + order_by: [fragment("? <-> ?", a.geom, ^"POINT(#{coords.lon} #{coords.lat})'")] + ) + end + + @spec filter_by_contry(Ecto.Query.t(), String.t() | nil) :: Ecto.Query.t() + defp filter_by_contry(query, nil), do: query + + defp filter_by_contry(query, country) do + from( + a in query, + where: ilike(a.country, ^"%#{country}%") + ) + end + + @spec addresses_around_query(Geo.geometry(), integer, integer) :: Ecto.Query.t() + defp addresses_around_query(point, radius, limit) do + import Geo.PostGIS + + from(a in Address, + where: st_dwithin_in_meters(^point, a.geom, ^radius), + order_by: [fragment("? <-> ?", a.geom, ^point)], + limit: ^limit + ) + end end diff --git a/test/mobilizon/addresses/addresses_test.exs b/test/mobilizon/addresses/addresses_test.exs index 8f9e267fe..8be79b2f1 100644 --- a/test/mobilizon/addresses/addresses_test.exs +++ b/test/mobilizon/addresses/addresses_test.exs @@ -76,23 +76,5 @@ defmodule Mobilizon.AddressesTest do assert {:ok, %Address{}} = Addresses.delete_address(address) assert_raise Ecto.NoResultsError, fn -> Addresses.get_address!(address.id) end end - - test "change_address/1 returns a address changeset" do - address = insert(:address) - assert %Ecto.Changeset{} = Addresses.change_address(address) - end - - test "process_geom/2 with valid data returns a Point element" do - attrs = %{"type" => "point", "data" => %{"latitude" => 10, "longitude" => -10}} - assert {:ok, %Geo.Point{}} = Addresses.process_geom(attrs) - end - - test "process_geom/2 with invalid data returns nil" do - attrs = %{"type" => :point, "data" => %{"latitude" => nil, "longitude" => nil}} - assert {:error, "Latitude and longitude must be numbers"} = Addresses.process_geom(attrs) - - attrs = %{"type" => :not_valid, "data" => %{"latitude" => nil, "longitude" => nil}} - assert {:error, :invalid_type} == Addresses.process_geom(attrs) - end end end