Refactoring of Adresses context

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 [
+  @attrs @required_attrs ++ @optional_attrs
-    :url
-  ]
 
   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

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
+  @spec query(Ecto.Query.t(), map) :: Ecto.Query.t()
-    Dataloader.Ecto.new(Repo, query: &query/2)
+  def query(queryable, _params), do: queryable
-  end
-
-  @doc false
-  def query(queryable, _params) do
-    queryable
-  end
 
   @doc """
   Returns the list of addresses.
-
-  ## Examples
-
-      iex> list_addresses()
-      [%Address{}, ...]
-
   """
-  def list_addresses do
+  @spec list_addresses :: [Address.t()]
-    Repo.all(Address)
+  def list_addresses, do: Repo.all(Address)
-  end
 
   @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
+  Gets a single address.
-
+  Raises `Ecto.NoResultsError` if the address does not exist.
-  ## Examples
-
-      iex> get_address_by_url("https://mobilizon.social/addresses/4572")
-      %Address{}
-
-      iex> get_address_by_url("https://mobilizon.social/addresses/099")
-      nil
   """
-  def get_address_by_url(url) do
+  @spec get_address!(integer | String.t()) :: Address.t()
-    Repo.get_by(Address, url: url)
+  def get_address!(id), do: Repo.get!(Address, id)
-  end
 
   @doc """
-  Creates a address.
+  Gets a single address by its url.
-
-  ## Examples
-
-      iex> create_address(%{field: value})
-      {:ok, %Address{}}
-
-      iex> create_address(%{field: bad_value})
-      {:error, %Ecto.Changeset{}}
-
   """
+  @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.
+  Updates an address.
-
-  ## Examples
-
-      iex> update_address(address, %{field: new_value})
-      {:ok, %Address{}}
-
-      iex> update_address(address, %{field: bad_value})
-      {:error, %Ecto.Changeset{}}
-
   """
+  @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.
+  Deletes an address.
-
-  ## Examples
-
-      iex> delete_address(address)
-      {:ok, %Address{}}
-
-      iex> delete_address(address)
-      {:error, %Ecto.Changeset{}}
-
   """
-  def delete_address(%Address{} = address) do
+  @spec delete_address(Address.t()) :: {:ok, Address.t()} | {:error, Ecto.Changeset.t()}
-    Repo.delete(address)
+  def delete_address(%Address{} = address), do: Repo.delete(address)
-  end
 
   @doc """
-  Returns an `%Ecto.Changeset{}` for tracking address changes.
+  Searches addresses.
-
-  ## Examples
-
-      iex> change_address(address)
-      %Ecto.Changeset{source: %Address{}}
 
+  We only look at the description for now, and eventually order by object distance.
   """
-  def change_address(%Address{} = address) do
+  @spec search_addresses(String.t(), keyword) :: [Address.t()]
-    Address.changeset(address, %{})
-  end
-
-  @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}
-    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
-
-  We only look at the description for now, and eventually order by object 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),
+      search
-        do:
+      |> search_addresses_query(Keyword.get(options, :limit, 5))
-          from(a in query,
+      |> order_by_coords(Keyword.get(options, :coords))
-            order_by: [fragment("? <-> ?", a.geom, ^"POINT(#{coords.lon} #{coords.lat})'")]
+      |> filter_by_contry(Keyword.get(options, :country))
-          ),
-        else: query
 
-    query =
+    case Keyword.get(options, :single, false) do
-      if country = Keyword.get(options, :country, nil),
+      true -> Repo.one(query)
-        do: from(a in query, where: ilike(a.country, ^"%#{country}%")),
+      false -> Repo.all(query)
-        else: query
+    end
-
-    if Keyword.get(options, :single, false) == true, do: Repo.one(query), else: Repo.all(query)
   end
 
   @doc """
-  Reverse geocode from coordinates in our database
+  Reverse geocode from coordinates.
 
-  We only take addresses 50km around and sort them by distance
+  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)
 
+    with {:ok, point} <- Geo.WKT.decode("SRID=#{srid};POINT(#{lon} #{lat})") do
+      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
 
-    with {:ok, point} <- Geo.WKT.decode("SRID=#{srid};POINT(#{lon} #{lat})") do
-      query =
     from(a in Address,
+      where: st_dwithin_in_meters(^point, a.geom, ^radius),
       order_by: [fragment("? <-> ?", a.geom, ^point)],
-          limit: ^limit,
+      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)
-    end
   end
 end

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