From 3f7bfdfe979f5139cf7f51adf5c2a3c36bc0e5b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Menou?= Date: Thu, 6 Feb 2025 17:18:27 +0100 Subject: [PATCH] GTFS diff : nouvelle UX --- apps/transport/client/javascripts/app.js | 4 + .../stylesheets/components/_gtfs_diff.scss | 14 ++ .../transport_web/live/gtfs_diff_explain.ex | 92 +++++++++-- .../live/gtfs_diff_select_live.ex | 91 ++++++++++- .../live/gtfs_diff_select_live.html.heex | 143 +++++++++++++----- .../gettext/en/LC_MESSAGES/validations.po | 56 ++++--- .../gettext/fr/LC_MESSAGES/validations.po | 56 ++++--- apps/transport/priv/gettext/validations.pot | 56 ++++--- .../transport/test/gtfs_diff_explain_test.exs | 10 +- 9 files changed, 396 insertions(+), 126 deletions(-) diff --git a/apps/transport/client/javascripts/app.js b/apps/transport/client/javascripts/app.js index 39797152e4..c1e4ca5b19 100644 --- a/apps/transport/client/javascripts/app.js +++ b/apps/transport/client/javascripts/app.js @@ -43,6 +43,10 @@ window.addEventListener('phx:backoffice-form-owner-reset', () => { document.getElementById('js-owner-input').value = '' }) +window.addEventListener('phx:gtfs-diff-focus-steps', () => { + document.getElementById('gtfs-diff-steps').parentElement.scrollIntoView({ behavior: 'smooth' }) +}) + const csrfToken = document.querySelector('meta[name=\'csrf\']').getAttribute('content') const liveSocket = new LiveSocket('/live', Socket, { hooks: Hooks, params: { _csrf_token: csrfToken } }) liveSocket.connect() diff --git a/apps/transport/client/stylesheets/components/_gtfs_diff.scss b/apps/transport/client/stylesheets/components/_gtfs_diff.scss index 49a35ebf2e..910af9bc6a 100644 --- a/apps/transport/client/stylesheets/components/_gtfs_diff.scss +++ b/apps/transport/client/stylesheets/components/_gtfs_diff.scss @@ -1,3 +1,10 @@ +#gtfs-diff-steps { + margin: -2em 2em 2em; + li.active { + font-weight: bold; + } +} + #gtfs-diff-input { display: flex; justify-content: left; @@ -53,4 +60,11 @@ progress { padding-left: 12px; } } + .dashboard { + min-height: 50vh; + max-height: 75vh; + .main { + overflow: scroll; + } + } } diff --git a/apps/transport/lib/transport_web/live/gtfs_diff_explain.ex b/apps/transport/lib/transport_web/live/gtfs_diff_explain.ex index 64affe372c..179ce30f5e 100644 --- a/apps/transport/lib/transport_web/live/gtfs_diff_explain.ex +++ b/apps/transport/lib/transport_web/live/gtfs_diff_explain.ex @@ -18,6 +18,8 @@ defmodule TransportWeb.GTFSDiffExplain do |> explanation_delete_file(diff) |> explanation_update_stop_name(diff) |> explanation_stop_wheelchair_access(diff) + |> explanation_update_stop_longitude(diff) + |> explanation_update_stop_latitude(diff) end) end @@ -61,7 +63,7 @@ defmodule TransportWeb.GTFSDiffExplain do def explanation_add_file(explanations, %{"action" => "add", "file" => file, "target" => "file"}) do [ - dgettext("validations", ~s(A file named "%{file}" has been added), file: file) + {file, dgettext("validations", ~s(A file named "%{file}" has been added), file: file)} | explanations ] end @@ -73,7 +75,7 @@ defmodule TransportWeb.GTFSDiffExplain do "file" => file, "target" => "file" }) do - [dgettext("validations", ~s(The file "%{file}" has been deleted), file: file) | explanations] + [{file, dgettext("validations", ~s(The file "%{file}" has been deleted), file: file)} | explanations] end def explanation_delete_file(explanations, _), do: explanations @@ -90,13 +92,14 @@ defmodule TransportWeb.GTFSDiffExplain do } ) do [ - dgettext( - "validations", - ~s([stops.txt] The name of the stop_id %{stop_id} has been modified. Initial name: "%{initial_stop_name}", New name: "%{new_stop_name}"), - stop_id: stop_id, - initial_stop_name: initial_stop_name, - new_stop_name: new_stop_name - ) + {"stops.txt", + dgettext( + "validations", + ~s(The name of the stop_id %{stop_id} has been modified. Initial name: "%{initial_stop_name}", New name: "%{new_stop_name}"), + stop_id: stop_id, + initial_stop_name: initial_stop_name, + new_stop_name: new_stop_name + )} | explanations ] end @@ -118,19 +121,76 @@ defmodule TransportWeb.GTFSDiffExplain do ) when new_wheelchair_boarding in ["1", "2"] do [ - dgettext( - "validations", - ~s([stops.txt] Wheelchair_boarding information added for stop_id %{stop_id}, previously: "%{initial_wheelchair_boarding}", now: "%{new_wheelchair_boarding}"), - stop_id: stop_id, - initial_wheelchair_boarding: initial_wheelchair_boarding, - new_wheelchair_boarding: new_wheelchair_boarding - ) + {"stops.txt", + dgettext( + "validations", + ~s(Wheelchair_boarding information added for stop_id %{stop_id}, previously: "%{initial_wheelchair_boarding}", now: "%{new_wheelchair_boarding}"), + stop_id: stop_id, + initial_wheelchair_boarding: initial_wheelchair_boarding, + new_wheelchair_boarding: new_wheelchair_boarding + )} | explanations ] end def explanation_stop_wheelchair_access(explanations, _), do: explanations + def explanation_update_stop_longitude( + explanations, + %{ + "action" => "update", + "file" => "stops.txt", + "target" => "row", + "identifier" => %{"stop_id" => stop_id}, + "new_value" => %{"stop_lon" => new_stop_lon}, + "initial_value" => %{"stop_lon" => initial_stop_lon} + } + ) do + [ + {"stops.txt", + dgettext( + "validations", + ~s(The longitude of the stop_id %{stop_id} has been modified. "%{initial_stop_lon}" -> "%{new_stop_lon}"), + stop_id: stop_id, + initial_stop_lon: initial_stop_lon, + new_stop_lon: new_stop_lon + )} + | explanations + ] + end + + def explanation_update_stop_longitude(explanations, _) do + explanations + end + + def explanation_update_stop_latitude( + explanations, + %{ + "action" => "update", + "file" => "stops.txt", + "target" => "row", + "identifier" => %{"stop_id" => stop_id}, + "new_value" => %{"stop_lat" => new_stop_lat}, + "initial_value" => %{"stop_lat" => initial_stop_lat} + } + ) do + [ + {"stops.txt", + dgettext( + "validations", + ~s(The latitude of the stop_id %{stop_id} has been modified. "%{initial_stop_lat}" -> "%{new_stop_lat}"), + stop_id: stop_id, + initial_stop_lat: initial_stop_lat, + new_stop_lat: new_stop_lat + )} + | explanations + ] + end + + def explanation_update_stop_latitude(explanations, _) do + explanations + end + def try_jason_decode(""), do: nil def try_jason_decode(input), do: Jason.decode!(input) end diff --git a/apps/transport/lib/transport_web/live/gtfs_diff_select_live.ex b/apps/transport/lib/transport_web/live/gtfs_diff_select_live.ex index 4223828b30..9f2eae650b 100644 --- a/apps/transport/lib/transport_web/live/gtfs_diff_select_live.ex +++ b/apps/transport/lib/transport_web/live/gtfs_diff_select_live.ex @@ -17,6 +17,7 @@ defmodule TransportWeb.Live.GTFSDiffSelectLive do socket |> assign(:uploaded_files, []) |> assign(:diff_logs, []) + |> assign(:current_step, "preparation") |> allow_upload(:gtfs, accept: ~w(.zip), max_entries: 2, @@ -45,9 +46,24 @@ defmodule TransportWeb.Live.GTFSDiffSelectLive do {:noreply, update(socket, :uploads, &switch_uploads/1)} end + def handle_event("start-over", _, socket) do + {:noreply, clean_slate(socket)} + end + + def handle_event("select-file", %{"file" => file}, socket) do + {:noreply, assign(socket, :selected_file, file)} + end + def handle_event("gtfs_diff", _, socket) do send(self(), :enqueue_job) - {:noreply, socket |> assign(:job_running, true)} + + socket = + socket + |> assign(:job_running, true) + |> assign(:current_step, "analyse") + |> push_event("gtfs-diff-focus-steps", %{}) + + {:noreply, socket} end def handle_info(:enqueue_job, socket) do @@ -79,13 +95,38 @@ defmodule TransportWeb.Live.GTFSDiffSelectLive do def handle_info({:generate_diff_summary, diff_file_url}, socket) do http_client = Transport.Shared.Wrapper.HTTPoison.impl() - %{status_code: 200, body: body} = http_client.get!(diff_file_url) - diff = Transport.GTFSDiff.parse_diff_output(body) - socket = - socket - |> assign(:diff_summary, diff |> GTFSDiffExplain.diff_summary()) - |> assign(:diff_explanations, diff |> GTFSDiffExplain.diff_explanations()) + case http_client.get(diff_file_url) do + {:error, error} -> + socket + |> assign(:error_msg, HTTPoison.Error.message(error)) + + {:ok, %{status_code: 200, body: body}} -> + diff = Transport.GTFSDiff.parse_diff_output(body) + + diff_summary = diff |> GTFSDiffExplain.diff_summary() + diff_explanations = diff |> GTFSDiffExplain.diff_explanations() + + files_with_changes = + diff_summary + |> Map.values() + |> Enum.concat() + |> Enum.map(fn {{file, _, _}, _} -> file end) + |> Enum.sort() + |> Enum.dedup() + + selected_file = + case files_with_changes do + [] -> nil + _ -> Kernel.hd(files_with_changes) + end + + socket + |> assign(:diff_summary, diff_summary) + |> assign(:diff_explanations, diff_explanations |> drop_empty()) + |> assign(:files_with_changes, files_with_changes) + |> assign(:selected_file, selected_file) + end {:noreply, socket} end @@ -127,7 +168,9 @@ defmodule TransportWeb.Live.GTFSDiffSelectLive do |> assign(:gtfs_original_file_name_1, gtfs_original_file_name_1) |> assign(:gtfs_original_file_name_2, gtfs_original_file_name_2) |> assign(:diff_logs, []) - |> assign(:job_running, false)} + |> assign(:job_running, false) + |> assign(:current_step, "results") + |> push_event("gtfs-diff-focus-steps", %{})} end # job took too long @@ -200,4 +243,36 @@ defmodule TransportWeb.Live.GTFSDiffSelectLive do Map.update!(gtfs, :entries, &Enum.reverse/1) end) end + + def step_completion(current_step, expected_step) do + cond do + step_progression(current_step) > step_progression(expected_step) -> "done" + step_progression(current_step) == step_progression(expected_step) -> "active" + true -> "" + end + end + + defp step_progression(step) do + case step do + "preparation" -> 1 + "analyse" -> 2 + "results" -> 3 + end + end + + defp drop_empty([]), do: nil + defp drop_empty(otherwise), do: otherwise + + defp clean_slate(socket) do + socket + |> assign(:current_step, "preparation") + |> assign(:diff_explanations, nil) + |> assign(:diff_file_url, nil) + |> assign(:diff_logs, []) + |> assign(:diff_summary, nil) + |> assign(:error_msg, nil) + |> assign(:job_running, false) + |> assign(:selected_file, nil) + |> assign(:uploaded_files, []) + end end diff --git a/apps/transport/lib/transport_web/live/gtfs_diff_select_live.html.heex b/apps/transport/lib/transport_web/live/gtfs_diff_select_live.html.heex index bf8aa27374..843c1bd98e 100644 --- a/apps/transport/lib/transport_web/live/gtfs_diff_select_live.html.heex +++ b/apps/transport/lib/transport_web/live/gtfs_diff_select_live.html.heex @@ -11,7 +11,21 @@ ) ) %>.

- + + +
+ +
@@ -56,6 +70,9 @@ <% end %>
+
+ +
<%= for log <- Enum.reverse(@diff_logs) do %>
@@ -63,13 +80,13 @@
<% end %>
-
-
-
-
+
<%= @error_msg %>
+
+ +

<%= dgettext("validations", "GTFS Diff is available for") %> @@ -87,47 +104,97 @@ ) %>. <%= if assigns[:diff_summary] do %>
-

<%= dgettext("validations", "Differences Overview") %>

- <%= dgettext( - "validations", - "The GTFS file %{gtfs_original_file_name_2} has differences with the GTFS file %{gtfs_original_file_name_1}, as summarized below:", - gtfs_original_file_name_1: @gtfs_original_file_name_1, - gtfs_original_file_name_2: @gtfs_original_file_name_2 - ) %> -
- <%= for {diff_nature, translation, css_class} <- [{"add", dgettext("validations", "added"), "green"}, {"update", dgettext("validations", "updated"), "orange"}, {"delete", dgettext("validations", "deleted"), "red"}] do %> -
-
    + <%= if assigns[:diff_summary] == %{} do %> + <%= raw( + dgettext( + "validations", + "The GTFS files %{gtfs_original_file_name_2} and %{gtfs_original_file_name_1} are similar.", + gtfs_original_file_name_1: @gtfs_original_file_name_1, + gtfs_original_file_name_2: @gtfs_original_file_name_2 + ) + ) %> + <% else %> + <%= raw( + dgettext( + "validations", + "The GTFS file %{gtfs_original_file_name_2} has differences with the GTFS file %{gtfs_original_file_name_1}, as summarized below:", + gtfs_original_file_name_1: @gtfs_original_file_name_1, + gtfs_original_file_name_2: @gtfs_original_file_name_2 + ) + ) %> + <% end %> +
+ <% else %> + <%= if assigns[:error_msg] do %> +
+ <%= dgettext( + "validations", + "An error occurred while interpreting the results. Note that the report is still available as download. Error:" + ) %> + <%= @error_msg %> +
+ <% else %> +
+ <%= dgettext("validations", "analyzing found differences...") %> +
+ <% end %> + <% end %> +
+
+ +
+

<%= dgettext("validations", "Differences Overview") %>

+
    + <%= for {diff_nature, translation, css_class} <- [{"add", dgettext("validations", "added"), "green"}, {"update", dgettext("validations", "updated"), "orange"}, {"delete", dgettext("validations", "deleted"), "red"}] do %> +
    <%= for {{file, ^diff_nature, target}, n} <- @diff_summary[diff_nature] do %> -
  • - <%= translation %>  <%= translate_target(target, n) %><%= file %> +
  • + <%= translation %>  <%= translate_target(target, n) %>
    • <% end %> -
-
+
+ <% end %> + + <%= if assigns[:diff_explanations] do %> + <% active_explanations = + @diff_explanations + |> Enum.filter(fn {file, _} -> file == @selected_file end) + |> Enum.map(fn {_, explanation} -> explanation end) %> +

<%= dgettext("validations", "Detail") %>

+
    + <%= for explanation <- active_explanations do %> +
  • + <%= explanation %> +
    • + <% end %> +
<% end %>
- <% else %> -
- <%= dgettext("validations", "analyzing found differences...") %> -
- <% end %> -
-

<%= dgettext("validations", "Human friendly explanations") %>

- <%= dgettext( - "validations", - "(Work in progress). We also try to express some of the differences found in a human friendly way." - ) %> -
    - <%= for explanation <- @diff_explanations do %> -
  • - <%= explanation %> -
    • - <% end %> -
+ +