-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor Smoove GBFS controller #1867
Changes from all commits
df75f24
d7d083c
8a2d13e
ea25adf
77fe355
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,127 +4,143 @@ defmodule GBFS.SmooveController do | |
require Logger | ||
|
||
plug(:put_view, GBFS.FeedView) | ||
@gbfs_version "1.1" | ||
|
||
@spec index(Plug.Conn.t(), map()) :: Plug.Conn.t() | ||
def index(conn, _params) do | ||
contract_id = conn.assigns.smoove_params.contract_id | ||
|
||
conn | ||
|> assign( | ||
:data, | ||
%{ | ||
"fr" => %{ | ||
"feeds" => | ||
Enum.map( | ||
[:system_information, :station_information, :station_status], | ||
fn a -> | ||
%{ | ||
"name" => Atom.to_string(a), | ||
"url" => apply(Routes, String.to_atom("#{contract_id}_url"), [conn, a]) | ||
} | ||
end | ||
) | ||
} | ||
} | ||
) | ||
|> render("gbfs.json") | ||
{:ok, | ||
%{ | ||
"fr" => %{ | ||
"feeds" => | ||
Enum.map( | ||
[:system_information, :station_information, :station_status], | ||
fn a -> | ||
%{ | ||
"name" => Atom.to_string(a), | ||
"url" => apply(Routes, String.to_atom("#{contract_id}_url"), [conn, a]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pour info on peut avoir à faire attention avec les conversions vers les atoms, si la donnée d'input est arbitraire / extérieure (https://til.hashrocket.com/posts/gkwwfy9xvw-converting-strings-to-atoms-safely). Là dans ce cas ma compréhension est que la liste des contract_id est fixe et contrôlée par nous, donc pas de problème. |
||
} | ||
end | ||
) | ||
} | ||
}} | ||
|> render_response(conn) | ||
end | ||
|
||
@spec system_information(Plug.Conn.t(), map()) :: Plug.Conn.t() | ||
def system_information(conn, _params) do | ||
smoove_params = conn.assigns.smoove_params | ||
|
||
conn | ||
|> assign( | ||
:data, | ||
%{ | ||
"system_id" => smoove_params.contract_id, | ||
"language" => "fr", | ||
"name" => smoove_params.nom, | ||
"timezone" => "Europe/Paris" | ||
} | ||
) | ||
|> render("gbfs.json") | ||
{:ok, | ||
%{ | ||
"system_id" => smoove_params.contract_id, | ||
"language" => "fr", | ||
"name" => smoove_params.nom, | ||
"timezone" => "Europe/Paris" | ||
}} | ||
|> render_response(conn) | ||
end | ||
|
||
@spec station_information(Plug.Conn.t(), map()) :: Plug.Conn.t() | ||
def station_information(conn, _params) do | ||
url = conn.assigns.smoove_params.url | ||
|
||
conn | ||
|> assign(:data, get_station_information(url)) | ||
|> render("gbfs.json") | ||
conn.assigns.smoove_params.url |> get_station_information() |> render_response(conn) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tiens curieux que le formateur Elixir laisse ça sous cette forme (et credo) derrière, je me serais attendu à ce qu'il impose un passage à la ligne. |
||
end | ||
|
||
@spec station_status(Plug.Conn.t(), map()) :: Plug.Conn.t() | ||
def station_status(conn, _params) do | ||
url = conn.assigns.smoove_params.url | ||
|
||
conn | ||
|> assign(:data, get_station_status(url)) | ||
|> render("gbfs.json") | ||
conn.assigns.smoove_params.url |> get_station_status() |> render_response(conn) | ||
end | ||
|
||
@spec get_station_status(binary()) :: map() | ||
@spec get_station_status(binary()) :: {:ok, map()} | {:error, binary()} | ||
defp get_station_status(url) do | ||
%{ | ||
"stations" => | ||
url | ||
|> get_stations() | ||
|> Enum.map(&Map.take(&1, [:station_id, :capacity, :num_bikes_available, :num_docks_available, :credit_card])) | ||
|> Enum.map(&Map.put(&1, :is_installed, 1)) | ||
|> Enum.map(&Map.put(&1, :is_returning, 1)) | ||
|> Enum.map(&Map.put(&1, :last_reported, DateTime.utc_now() |> DateTime.to_unix())) | ||
|> Enum.map( | ||
&Map.put( | ||
&1, | ||
:is_renting, | ||
if(Map.has_key?(&1, :credit_card), do: 1, else: 0) | ||
) | ||
) | ||
|> Enum.map(&Map.delete(&1, :credit_card)) | ||
} | ||
case get_stations(url) do | ||
{:ok, data} -> | ||
{:ok, | ||
%{ | ||
"stations" => | ||
data | ||
|> Enum.map( | ||
&Map.take(&1, [:station_id, :capacity, :num_bikes_available, :num_docks_available, :credit_card]) | ||
) | ||
|> Enum.map(&Map.put(&1, :is_installed, 1)) | ||
|> Enum.map(&Map.put(&1, :is_returning, 1)) | ||
|> Enum.map(&Map.put(&1, :last_reported, DateTime.utc_now() |> DateTime.to_unix())) | ||
|> Enum.map( | ||
&Map.put( | ||
&1, | ||
:is_renting, | ||
if(Map.has_key?(&1, :credit_card), do: 1, else: 0) | ||
) | ||
) | ||
|> Enum.map(&Map.delete(&1, :credit_card)) | ||
}} | ||
|
||
{:error, e} -> | ||
{:error, e} | ||
end | ||
end | ||
|
||
@spec get_station_information(binary()) :: map() | ||
@spec get_station_information(binary()) :: {:ok, map()} | {:error, binary()} | ||
defp get_station_information(url) do | ||
%{ | ||
"stations" => | ||
url | ||
|> get_stations() | ||
|> Enum.map(&Map.take(&1, [:name, :station_id, :lat, :lon, :capacity, :credit_card])) | ||
|> Enum.map(&set_rental_method/1) | ||
|> Enum.map(&Map.delete(&1, :credit_card)) | ||
} | ||
case get_stations(url) do | ||
{:ok, data} -> | ||
{:ok, | ||
%{ | ||
"stations" => | ||
data | ||
|> Enum.map(&Map.take(&1, [:name, :station_id, :lat, :lon, :capacity, :credit_card])) | ||
|> Enum.map(&set_rental_method/1) | ||
|> Enum.map(&Map.delete(&1, :credit_card)) | ||
}} | ||
|
||
{:error, e} -> | ||
{:error, e} | ||
end | ||
end | ||
|
||
@spec set_rental_method(map()) :: map() | ||
defp set_rental_method(%{credit_card: 1} = station), do: Map.put(station, :rental_method, "CREDIT_CARD") | ||
defp set_rental_method(station), do: station | ||
|
||
@spec get_stations(binary()) :: map() | ||
@spec get_stations(binary()) :: {:ok, map()} | {:error, binary()} | ||
defp get_stations(url) do | ||
with {:ok, %{status_code: 200, body: body}} <- HTTPoison.get(url), | ||
body when not is_nil(body) <- :iconv.convert("iso8859-1", "latin1", body) do | ||
body | ||
|> xpath(~x"//si"l, | ||
name: ~x"@na"S, | ||
station_id: ~x"@id"s, | ||
lat: ~x"@la"f, | ||
lon: ~x"@lg"f, | ||
capacity: ~x"@to"i, | ||
credit_card: ~x"@cb"I, | ||
num_bikes_available: ~x"@av"i, | ||
num_docks_available: ~x"@fr"i | ||
) | ||
else | ||
nil -> | ||
Logger.error("Unable to decode body") | ||
nil | ||
|
||
error -> | ||
Logger.error(error) | ||
nil | ||
http_client = Transport.Shared.Wrapper.HTTPoison.impl() | ||
|
||
case http_client.get(url) do | ||
{:ok, %{status_code: 200, body: body}} -> | ||
{:ok, | ||
body | ||
|> xpath(~x"//si"l, | ||
name: ~x"@na"S, | ||
station_id: ~x"@id"s, | ||
lat: ~x"@la"f, | ||
lon: ~x"@lg"f, | ||
capacity: ~x"@to"i, | ||
credit_card: ~x"@cb"I, | ||
num_bikes_available: ~x"@av"i, | ||
num_docks_available: ~x"@fr"i | ||
)} | ||
|
||
e -> | ||
Logger.error("impossible to query smoove: #{inspect(e)}") | ||
{:error, "smoove service unavailable"} | ||
end | ||
end | ||
|
||
defp render_response({:ok, data}, conn), | ||
do: | ||
conn | ||
|> assign(:data, data) | ||
|> assign(:version, @gbfs_version) | ||
|> render("gbfs.json") | ||
|
||
defp render_response({:error, msg}, conn), | ||
do: | ||
conn | ||
|> assign(:error, msg) | ||
# for the moment we always return a BAD_GATEWAY in case of error | ||
|> put_status(502) | ||
|> put_view(GBFS.ErrorView) | ||
|> render("error.json") | ||
end |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,61 @@ | ||||||||
defmodule GBFS.SmooveControllerTest do | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Un copier/coller quasi complet de https://github.com/etalab/transport-site/blob/master/apps/gbfs/test/gbfs/controllers/jcdecaux_test.exs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Idéalement il faudrait essayer d'adapter les tests pour éviter d'ajouter d'usage de Si tu en as l'énergie! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @thbar Tu aurais un exemple de PR où on a abandonné There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Je n'ai pas trouvé facilement d'exemple précisément pour ça, mais on a quelque chose d'approchant dans le travail réalisé par @fchabouis dans #1863. Je vais essayer de décrire rapidement la guideline générale pour enlever autant
transport-site/apps/gbfs/lib/gbfs/controllers/smoove_controller.ex Lines 107 to 109 in 8a2d13e
Attention, il est recommandé de faire un "verify" des mocks (ce qui n'est pas fait dans #1863 je le remarque) avec Voilà pour l'idée générale ; ça paraît un peu compliqué initialement, toutefois au final ça a le mérite de définir des boundaries claires et de rendre le code très testable et substituable. À ta dispo si besoin de clarifications! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Merci beaucoup, fait dans le commit suivant ! Je pourrai faire de même pour JC Decaux dans une autre PR. |
||||||||
use GBFS.ConnCase, async: true | ||||||||
alias GBFS.Router.Helpers, as: Routes | ||||||||
import Mox | ||||||||
import GBFS.Checker | ||||||||
|
||||||||
setup :verify_on_exit! | ||||||||
|
||||||||
describe "Smoove GBFS conversion" do | ||||||||
test "on gbfs.json", %{conn: conn} do | ||||||||
conn = conn |> get(Routes.montpellier_path(conn, :index)) | ||||||||
body = json_response(conn, 200) | ||||||||
check_entrypoint(body) | ||||||||
end | ||||||||
|
||||||||
test "on system_information.json", %{conn: conn} do | ||||||||
conn = conn |> get(Routes.montpellier_path(conn, :system_information)) | ||||||||
body = json_response(conn, 200) | ||||||||
check_system_information(body) | ||||||||
end | ||||||||
|
||||||||
test "on station_information.json", %{conn: conn} do | ||||||||
setup_stations_response() | ||||||||
|
||||||||
conn = conn |> get(Routes.montpellier_path(conn, :station_information)) | ||||||||
body = json_response(conn, 200) | ||||||||
check_station_information(body) | ||||||||
end | ||||||||
|
||||||||
test "on station_status.json", %{conn: conn} do | ||||||||
setup_stations_response() | ||||||||
|
||||||||
conn = conn |> get(Routes.montpellier_path(conn, :station_status)) | ||||||||
body = json_response(conn, 200) | ||||||||
check_station_status(body) | ||||||||
end | ||||||||
|
||||||||
test "on invalid response", %{conn: conn} do | ||||||||
Transport.HTTPoison.Mock |> expect(:get, fn _url -> {:ok, %HTTPoison.Response{status_code: 500}} end) | ||||||||
|
||||||||
conn = conn |> get(Routes.montpellier_path(conn, :station_status)) | ||||||||
assert %{"error" => "smoove service unavailable"} == json_response(conn, 502) | ||||||||
end | ||||||||
|
||||||||
defp setup_stations_response do | ||||||||
Transport.HTTPoison.Mock | ||||||||
|> expect(:get, fn url -> | ||||||||
assert url == "https://data.montpellier3m.fr/sites/default/files/ressources/TAM_MMM_VELOMAG.xml" | ||||||||
|
||||||||
{:ok, | ||||||||
%HTTPoison.Response{ | ||||||||
status_code: 200, | ||||||||
body: """ | ||||||||
<vcs ver="1"><sl><si na="001 Rue Jules Ferry - Gare Saint-Roch" id="001" la="43.605366" lg="3.881346" av="5" fr="7" to="12"></si></sl></vcs> | ||||||||
""", | ||||||||
headers: [{"Content-Type", "application/xml"}] | ||||||||
}} | ||||||||
end) | ||||||||
end | ||||||||
end | ||||||||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Il y a beaucoup de lignes de code modifiées mais c'est en réalité beaucoup de refactor en