Skip to content
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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Oct 20, 2021
Merged

Refactor Smoove GBFS controller #1867

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
df75f24
Refactor Smoove GBFS controller
AntoineAugusti Oct 20, 2021
d7d083c
Ajout test pour erreur HTTP 500
AntoineAugusti Oct 20, 2021
8a2d13e
Merge branch 'master' into smoove-gbfs-version
AntoineAugusti Oct 20, 2021
ea25adf
Refactor des tests
AntoineAugusti Oct 20, 2021
77fe355
Suppression iconv
AntoineAugusti Oct 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
186 changes: 100 additions & 86 deletions apps/gbfs/lib/gbfs/controllers/smoove_controller.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,127 +4,141 @@ 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,
Copy link
Member Author

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

{:ok, } |> render_response(conn)

%{
"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])
Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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
thbar marked this conversation as resolved.
Show resolved Hide resolved
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
)
{: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
)}
else
nil ->
Logger.error("Unable to decode body")
nil

error ->
Logger.error(error)
nil
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
28 changes: 28 additions & 0 deletions apps/gbfs/test/fixture/cassettes/smoove/stations.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
[
{
"request": {
"body": "",
"headers": [],
"method": "get",
"options": [],
"request_body": "",
"url": "https://data.montpellier3m.fr/sites/default/files/ressources/TAM_MMM_VELOMAG.xml"
},
"response": {
"binary": false,
"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><si na=\"002 Comédie\" id=\"002\" la=\"43.608148\" lg=\"3.878778\" av=\"14\" fr=\"10\" to=\"24\"></si><si na=\"003 Esplanade\" id=\"003\" la=\"43.609478\" lg=\"3.881293\" av=\"10\" fr=\"22\" to=\"32\"></si><si na=\"004 Hôtel de Ville\" id=\"004\" la=\"43.599088\" lg=\"3.894866\" av=\"8\" fr=\"8\" to=\"16\"></si><si na=\"005 Corum\" id=\"005\" la=\"43.613989\" lg=\"3.881600\" av=\"5\" fr=\"7\" to=\"12\"></si><si na=\"006 Place Albert 1er - St Charles\" id=\"006\" la=\"43.616768\" lg=\"3.873375\" av=\"22\" fr=\"9\" to=\"31\"></si><si na=\"007 Foch\" id=\"007\" la=\"43.610989\" lg=\"3.873345\" av=\"2\" fr=\"6\" to=\"8\"></si><si na=\"008 Halles Castellane\" id=\"008\" la=\"43.609935\" lg=\"3.877208\" av=\"0\" fr=\"12\" to=\"12\"></si><si na=\"009 Observatoire\" id=\"009\" la=\"43.606081\" lg=\"3.876931\" av=\"3\" fr=\"5\" to=\"8\"></si><si na=\"010 Rondelet\" id=\"010\" la=\"43.603038\" lg=\"3.875796\" av=\"1\" fr=\"15\" to=\"16\"></si><si na=\"011 Plan Cabanes\" id=\"011\" la=\"43.608491\" lg=\"3.868389\" av=\"7\" fr=\"5\" to=\"12\"></si><si na=\"012 Boutonnet\" id=\"012\" la=\"43.622629\" lg=\"3.868375\" av=\"11\" fr=\"1\" to=\"12\"></si><si na=\"013 Emile Combes\" id=\"013\" la=\"43.616742\" lg=\"3.87998\" av=\"7\" fr=\"1\" to=\"8\"></si><si na=\"014 Beaux-Arts\" id=\"014\" la=\"43.616698\" lg=\"3.884981\" av=\"5\" fr=\"3\" to=\"8\"></si><si na=\"015 Les Aubes\" id=\"015\" la=\"43.618692\" lg=\"3.893844\" av=\"4\" fr=\"4\" to=\"8\"></si><si na=\"016 Antigone centre\" id=\"016\" la=\"43.607942\" lg=\"3.890634\" av=\"6\" fr=\"10\" to=\"16\"></si><si na=\"017 Médiathèque Emile Zola\" id=\"017\" la=\"43.608218\" lg=\"3.893140\" av=\"13\" fr=\"3\" to=\"16\"></si><si na=\"018 Nombre d&#39;Or\" id=\"018\" la=\"43.607859\" lg=\"3.886644\" av=\"8\" fr=\"8\" to=\"16\"></si><si na=\"019 Louis Blanc\" id=\"019\" la=\"43.614642\" lg=\"3.877648\" av=\"4\" fr=\"12\" to=\"16\"></si><si na=\"020 Gambetta\" id=\"020\" la=\"43.607106\" lg=\"3.870693\" av=\"5\" fr=\"3\" to=\"8\"></si><si na=\"021 Port Marianne\" id=\"021\" la=\"43.600320\" lg=\"3.898510\" av=\"12\" fr=\"4\" to=\"16\"></si><si na=\"022 Clemenceau\" id=\"022\" la=\"43.603539\" lg=\"3.872394\" av=\"2\" fr=\"10\" to=\"12\"></si><si na=\"023 Les Arceaux\" id=\"023\" la=\"43.611991\" lg=\"3.867157\" av=\"4\" fr=\"12\" to=\"16\"></si><si na=\"024 Cité Mion\" id=\"024\" la=\"43.601143\" lg=\"3.884373\" av=\"2\" fr=\"6\" to=\"8\"></si><si na=\"025 Nouveau Saint-Roch\" id=\"025\" la=\"43.599817\" lg=\"3.875757\" av=\"2\" fr=\"6\" to=\"8\"></si><si na=\"026 Renouvier\" id=\"026\" la=\"43.603553\" lg=\"3.867884\" av=\"5\" fr=\"3\" to=\"8\"></si><si na=\"027 Odysseum\" id=\"027\" la=\"43.603727\" lg=\"3.918979\" av=\"1\" fr=\"7\" to=\"8\"></si><si na=\"028 Saint-Denis\" id=\"028\" la=\"43.605021\" lg=\"3.875065\" av=\"1\" fr=\"7\" to=\"8\"></si><si na=\"029 Richter\" id=\"029\" la=\"43.603424\" lg=\"3.899263\" av=\"16\" fr=\"0\" to=\"16\"></si><si na=\"030 Charles Flahault\" id=\"030\" la=\"43.618762\" lg=\"3.865971\" av=\"0\" fr=\"8\" to=\"8\"></si><si na=\"031 Voltaire\" id=\"031\" la=\"43.603767\" lg=\"3.888659\" av=\"5\" fr=\"3\" to=\"8\"></si><si na=\"032 Prés d&#39;Arènes\" id=\"032\" la=\"43.590480\" lg=\"3.884611\" av=\"2\" fr=\"6\" to=\"8\"></si><si na=\"033 Garcia Lorca\" id=\"033\" la=\"43.590757\" lg=\"3.890616\" av=\"0\" fr=\"8\" to=\"8\"></si><si na=\"034 Vert Bois\" id=\"034\" la=\"43.634580\" lg=\"3.868230\" av=\"0\" fr=\"16\" to=\"16\"></si><si na=\"035 Malbosc\" id=\"035\" la=\"43.633679\" lg=\"3.832861\" av=\"0\" fr=\"8\" to=\"8\"></si><si na=\"036 Occitanie\" id=\"036\" la=\"43.634242\" lg=\"3.849128\" av=\"0\" fr=\"32\" to=\"32\"></si><si na=\"037 FacdesSciences\" id=\"037\" la=\"43.631018\" lg=\"3.860697\" av=\"17\" fr=\"7\" to=\"24\"></si><si na=\"038 Fac de Lettres\" id=\"038\" la=\"43.630665\" lg=\"3.870230\" av=\"10\" fr=\"6\" to=\"16\"></si><si na=\"039 Aiguelongue\" id=\"039\" la=\"43.626163\" lg=\"3.882492\" av=\"1\" fr=\"7\" to=\"8\"></si><si na=\"040 Jeu de Mail des Abbés\" id=\"040\" la=\"43.619701\" lg=\"3.883831\" av=\"3\" fr=\"5\" to=\"8\"></si><si na=\"041 Euromédecine\" id=\"041\" la=\"43.639119\" lg=\"3.828199\" av=\"1\" fr=\"7\" to=\"8\"></si><si na=\"042 Marie Caizergues\" id=\"042\" la=\"43.619871\" lg=\"3.873812\" av=\"2\" fr=\"6\" to=\"8\"></si><si na=\"043 Sabines\" id=\"043\" la=\"43.584211\" lg=\"3.860031\" av=\"3\" fr=\"5\" to=\"8\"></si><si na=\"044 Celleneuve\" id=\"044\" la=\"43.614670\" lg=\"3.832624\" av=\"2\" fr=\"6\" to=\"8\"></si><si na=\"045 Jardin de la Lironde\" id=\"045\" la=\"43.605850\" lg=\"3.911576\" av=\"3\" fr=\"5\" to=\"8\"></si><si na=\"046 Père Soulas\" id=\"046\" la=\"43.621983\" lg=\"3.855603\" av=\"2\" fr=\"6\" to=\"8\"></si><si na=\"047 Place Viala\" id=\"047\" la=\"43.616812\" lg=\"3.855075\" av=\"6\" fr=\"2\" to=\"8\"></si><si na=\"048 Hôtel du Département\" id=\"048\" la=\"43.621682\" lg=\"3.834770\" av=\"0\" fr=\"8\" to=\"8\"></si><si na=\"049 Tonnelles\" id=\"049\" la=\"43.615155\" lg=\"3.839466\" av=\"4\" fr=\"4\" to=\"8\"></si><si na=\"050 Parvis Jules Ferry - Gare Saint-Roch\" id=\"050\" la=\"43.603889\" lg=\"3.879362\" av=\"5\" fr=\"3\" to=\"8\"></si><si na=\"051 Pont de Lattes - Gare Saint-Roch\" id=\"051\" la=\"43.606036\" lg=\"3.882393\" av=\"0\" fr=\"12\" to=\"12\"></si><si na=\"053 Deux Ponts - Gare Saint-Roch\" id=\"053\" la=\"43.604319\" lg=\"3.880916\" av=\"2\" fr=\"6\" to=\"8\"></si><si na=\"054 Providence - Ovalie\" id=\"054\" la=\"43.588239\" lg=\"3.853421\" av=\"2\" fr=\"6\" to=\"8\"></si><si na=\"055 Pérols Etang de l&#39;Or\" id=\"055\" la=\"43.558351\" lg=\"3.963412\" av=\"20\" fr=\"48\" to=\"68\"></si><si na=\"056 Albert 1er - Cathédrale\" id=\"056\" la=\"43.614005\" lg=\"3.873218\" av=\"1\" fr=\"11\" to=\"12\"></si><si na=\"057 Saint-Guilhem - Courreau\" id=\"057\" la=\"43.608996\" lg=\"3.872752\" av=\"2\" fr=\"6\" to=\"8\"></si><si na=\"059 Sud De France\" id=\"059\" la=\"43.595620\" lg=\"3.923500\" av=\"2\" fr=\"6\" to=\"8\"></si></sl></vcs>",
"headers": {
"Date": "Wed, 20 Oct 2021 08:59:10 GMT",
"Server": "Apache/2.4.10 (Debian)",
"Last-Modified": "Wed, 20 Oct 2021 08:59:02 GMT",
"ETag": "\"14e1-5cec4fd4b5448\"",
"Accept-Ranges": "bytes",
"Content-Length": "5345",
"Vary": "Accept-Encoding",
"Content-Type": "application/xml"
},
"status_code": 200,
"type": "ok"
}
}
]
52 changes: 52 additions & 0 deletions apps/gbfs/test/gbfs/controllers/smoove_controller_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
defmodule GBFS.SmooveControllerTest do
Copy link
Member Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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 use_cassette (cf #1853) et idem pour with_mock (car mock modifie la définition globale des modules, il n'y a pas de modification par processus jjh42/mock#129), au profit de Mox (https://github.com/dashbitco/mox).

Si tu en as l'énergie!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thbar Tu aurais un exemple de PR où on a abandonné use_cassette et utilisé autre chose ?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 with_mock que use_cassette (il manquera des points à traiter oralement à un moment, mais ça suffira pour avancer) :

  1. Identifier les points "frontière" (boundaries) dans le code, ceux qui font appel à la dépendance externe par un moyen ou un autre. Dans le cas de ce contrôleur, ça sera l'appel à HTTPoison.get, qui est ici:

transport-site/apps/gbfs/lib/gbfs/controllers/smoove_controller.ex

Lines 107 to 109 in 8a2d13e

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

  1. Dégager une "behaviour" ou un wrapper (mais là pour httpoison le travail a déjà été fait ici et il se trouve que HTTPoison lui-même est une "behaviour", ce qui fait que Mox peut le mocker directement
  2. Faire appel à ce wrapper dans le code du contrôleur (comme fait dans Utilisation d'un Mock pour tester url_extractor.ex #1863) plutôt que de faire l'appel direct à HTTPoison
  3. Modifier les tests pour créer des assertions en utilisant Mox. La configuration de "test" (ici) substitue l'implémentation de httpoison_impl avec un Mock, et on peut asserter dessus ensuite (https://github.com/etalab/transport-site/pull/1863/files#diff-cd2bd95618a14fbcafc59bb740eece7ee030f2c29af4bd5ea0add7c3062bc171R8)

Attention, il est recommandé de faire un "verify" des mocks (ce qui n'est pas fait dans #1863 je le remarque) avec setup :verify_on_exit! ou avec verify!().

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Ref: https://hexdocs.pm/mox/Mox.html)

Copy link
Member Author

Choose a reason for hiding this comment

The 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: false
alias GBFS.Router.Helpers, as: Routes
use GBFS.ExternalCase
import Mock
import GBFS.Checker

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
use_cassette "smoove/stations" do
conn = conn |> get(Routes.montpellier_path(conn, :station_information))
body = json_response(conn, 200)
check_station_information(body)
end
end

test "on station_status.json", %{conn: conn} do
use_cassette "smoove/stations" do
conn = conn |> get(Routes.montpellier_path(conn, :station_status))
body = json_response(conn, 200)
check_station_status(body)
end
end

test "on invalid response", %{conn: conn} do
mock = fn url ->
if String.match?(url, ~r|TAM_MMM_VELOMAG.xml$|) do
{:ok, %HTTPoison.Response{body: "{}", status_code: 500}}
end
end

with_mock HTTPoison, get: mock do
conn = conn |> get(Routes.montpellier_path(conn, :station_status))
assert %{"error" => "smoove service unavailable"} == json_response(conn, 502)

assert_called_exactly(HTTPoison.get(:_), 1)
end
end
end
end