Nettoyage du contrôleur de resources : actions producteurs bougées (#…

…4169) * Router: put resource edition in espace_producteur namespace * Move resource actions to espace producteur controller * rename form file template to resource_form * Move locales from resource to espace producteur * Rename when needed dataset_id to dataset_datagouv_id etc * Move auth to plug system: delete confirmation * use espace producteur plugs for resource forms * Rename some functions * Comment on the post_file function * Link directly to correct page in espace producteur * Remove space in english locale Co-authored-by: Antoine Augusti <antoine.augusti@transport.data.gouv.fr> * Remove leading underscore in new_resource route * Actually use new locale * Helpers with better names for urls * fix bug for editing resources with uploaded file --------- Co-authored-by: Antoine Augusti <antoine.augusti@transport.data.gouv.fr>
etalab · Sep 12, 2024 · 922ec4b · 922ec4b
1 parent 4446da2
commit 922ec4b
Show file tree

Hide file tree

Showing 23 changed files with 660 additions and 592 deletions.
diff --git a/apps/transport/lib/transport_web/controllers/espace_producteur_controller.ex b/apps/transport/lib/transport_web/controllers/espace_producteur_controller.ex
@@ -1,9 +1,17 @@
 defmodule TransportWeb.EspaceProducteurController do
   use TransportWeb, :controller
+  require Logger
+  alias Transport.ImportData
 
-  plug(:find_dataset_and_fetch_from_api_or_redirect when action in [:edit_dataset])
-  plug(:find_dataset_or_redirect when action in [:upload_logo, :remove_custom_logo])
-  plug(:find_datasets_or_redirect when action in [:proxy_statistics])
+  plug(:find_db_dataset_and_api_dataset_or_redirect when action in [:edit_dataset, :new_resource])
+
+  plug(
+    :find_db_dataset_and_api_dataset_and_resource_or_redirect
+    when action in [:delete_resource_confirmation, :edit_resource]
+  )
+
+  plug(:find_db_dataset_or_redirect when action in [:upload_logo, :remove_custom_logo])
+  plug(:find_db_datasets_or_redirect when action in [:proxy_statistics])
 
   def edit_dataset(%Plug.Conn{} = conn, %{"dataset_id" => _}) do
     # Awkard page, but no real choice: some parts (logo…) are from the local database
@@ -66,9 +74,99 @@ defmodule TransportWeb.EspaceProducteurController do
     |> render("proxy_statistics.html")
   end
 
+  @spec new_resource(Plug.Conn.t(), map()) :: Plug.Conn.t()
+  def new_resource(conn, %{"dataset_id" => _dataset_id}) do
+    conn
+    |> assign(:datagouv_resource, nil)
+    |> render("resource_form.html")
+  end
+
+  @spec edit_resource(Plug.Conn.t(), map()) :: Plug.Conn.t()
+  def edit_resource(conn, %{"dataset_id" => _dataset_id, "resource_datagouv_id" => _resource_datagouv_id}) do
+    conn
+    |> render("resource_form.html")
+  end
+
+  def delete_resource_confirmation(%Plug.Conn{} = conn, %{
+        "dataset_id" => _dataset_datagouv_id,
+        "resource_datagouv_id" => _resource_datagouv_id
+      }) do
+    conn
+    |> render("delete_resource_confirmation.html")
+  end
+
+  def delete_resource(%Plug.Conn{} = conn, %{
+        "dataset_datagouv_id" => dataset_datagouv_id,
+        "resource_datagouv_id" => resource_datagouv_id
+      }) do
+    with {:ok, _} <-
+           Datagouvfr.Client.Resources.delete(conn, %{
+             "dataset_id" => dataset_datagouv_id,
+             "resource_id" => resource_datagouv_id
+           }),
+         dataset when not is_nil(dataset) <-
+           DB.Repo.get_by(DB.Dataset, datagouv_id: dataset_datagouv_id),
+         {:ok, _} <- ImportData.import_dataset_logged(dataset),
+         {:ok, _} <- DB.Dataset.validate(dataset) do
+      conn
+      |> put_flash(:info, dgettext("resource", "The resource has been deleted"))
+      |> redirect(to: page_path(conn, :espace_producteur))
+    else
+      _ ->
+        conn
+        |> put_flash(:error, dgettext("resource", "Could not delete the resource"))
+        |> redirect(to: page_path(conn, :espace_producteur))
+    end
+  end
+
+  @doc """
+  The following function does a POST to the datagouv API to update or create a resource.
+  We don’t check that the user is allowed to update the dataset, as the API will do it for us.
+  We don’t check either before the POST that the dataset is imported on our side.
+  In case of error, the user is redirected to the producer space with an error message
+  instead of rendering again the form: it’s a suboptimal experience, can be improved.
+  """
+  @spec post_file(Plug.Conn.t(), map) :: Plug.Conn.t()
+  def post_file(conn, params) do
+    success_message =
+      if Map.has_key?(params, "resource_file") do
+        dgettext("resource", "File uploaded!")
+      else
+        dgettext("resource", "Resource updated with URL!")
+      end
+
+    post_params = datagouv_api_update_params(params)
+
+    with {:ok, _} <- Datagouvfr.Client.Resources.update(conn, post_params),
+         dataset when not is_nil(dataset) <-
+           DB.Repo.get_by(DB.Dataset, datagouv_id: params["dataset_datagouv_id"]),
+         {:ok, _} <- ImportData.import_dataset_logged(dataset),
+         {:ok, _} <- DB.Dataset.validate(dataset) do
+      conn
+      |> put_flash(:info, success_message)
+      |> redirect(to: dataset_path(conn, :details, params["dataset_datagouv_id"]))
+    else
+      {:error, error} ->
+        Logger.error(
+          "Unable to update resource #{params["resource_datagouv_id"]} of dataset #{params["dataset_datagouv_id"]}, error: #{inspect(error)}"
+        )
+
+        conn
+        |> put_flash(:error, dgettext("resource", "Unable to upload file"))
+        |> redirect(to: page_path(conn, :espace_producteur))
+
+      nil ->
+        Logger.error("Unable to get dataset with datagouv_id: #{params["dataset_datagouv_id"]}")
+
+        conn
+        |> put_flash(:error, dgettext("resource", "Unable to upload file"))
+        |> redirect(to: page_path(conn, :espace_producteur))
+    end
+  end
+
   defp proxy_requests_stats_nb_days, do: 15
 
-  defp find_datasets_or_redirect(%Plug.Conn{} = conn, _options) do
+  defp find_db_datasets_or_redirect(%Plug.Conn{} = conn, _options) do
     conn
     |> DB.Dataset.datasets_for_user()
     |> case do
@@ -83,7 +181,7 @@ defmodule TransportWeb.EspaceProducteurController do
     end
   end
 
-  defp find_dataset_or_redirect(%Plug.Conn{path_params: %{"dataset_id" => dataset_id}} = conn, _options) do
+  defp find_db_dataset_or_redirect(%Plug.Conn{path_params: %{"dataset_id" => dataset_id}} = conn, _options) do
     case find_dataset_for_user(conn, dataset_id) do
       %DB.Dataset{} = dataset ->
         conn |> assign(:dataset, dataset)
@@ -96,7 +194,7 @@ defmodule TransportWeb.EspaceProducteurController do
     end
   end
 
-  defp find_dataset_and_fetch_from_api_or_redirect(
+  defp find_db_dataset_and_api_dataset_or_redirect(
          %Plug.Conn{path_params: %{"dataset_id" => dataset_id}} = conn,
          _options
        ) do
@@ -114,6 +212,27 @@ defmodule TransportWeb.EspaceProducteurController do
     end
   end
 
+  defp find_db_dataset_and_api_dataset_and_resource_or_redirect(
+         %Plug.Conn{path_params: %{"dataset_id" => dataset_id, "resource_datagouv_id" => resource_datagouv_id}} = conn,
+         _options
+       ) do
+    with %DB.Dataset{datagouv_id: datagouv_id} = dataset <- find_dataset_for_user(conn, dataset_id),
+         {:ok, datagouv_dataset} <- Datagouvfr.Client.Datasets.get(datagouv_id),
+         datagouv_resource when not is_nil(datagouv_resource) <-
+           assign_resource_from_dataset_payload(datagouv_dataset, resource_datagouv_id) do
+      conn
+      |> assign(:dataset, dataset)
+      |> assign(:datagouv_dataset, datagouv_dataset)
+      |> assign(:datagouv_resource, datagouv_resource)
+    else
+      _ ->
+        conn
+        |> put_flash(:error, dgettext("alert", "Unable to get this dataset for the moment"))
+        |> redirect(to: page_path(conn, :espace_producteur))
+        |> halt()
+    end
+  end
+
   @spec find_dataset_for_user(Plug.Conn.t(), binary()) :: DB.Dataset.t() | nil
   defp find_dataset_for_user(%Plug.Conn{} = conn, dataset_id_str) do
     {dataset_id, ""} = Integer.parse(dataset_id_str)
@@ -126,4 +245,21 @@ defmodule TransportWeb.EspaceProducteurController do
     end
     |> Enum.find(fn %DB.Dataset{id: id} -> id == dataset_id end)
   end
+
+  defp assign_resource_from_dataset_payload(dataset, resource_id) do
+    Enum.find(dataset["resources"], &(&1["id"] == resource_id))
+  end
+
+  defp datagouv_api_update_params(params) do
+    post_params = Map.put(params, "dataset_id", params["dataset_datagouv_id"])
+
+    post_params =
+      if params["resource_datagouv_id"] do
+        Map.put(post_params, "resource_id", params["resource_datagouv_id"])
+      else
+        post_params
+      end
+
+    Map.take(post_params, ["title", "format", "url", "resource_file", "dataset_id", "resource_id"])
+  end
 end
diff --git a/apps/transport/lib/transport_web/controllers/resource_controller.ex b/apps/transport/lib/transport_web/controllers/resource_controller.ex
@@ -1,9 +1,7 @@
 defmodule TransportWeb.ResourceController do
   use TransportWeb, :controller
-  alias DB.{Dataset, Repo, Resource}
+  alias DB.{Repo, Resource}
   alias Transport.DataVisualization
-  alias Transport.ImportData
-  require Logger
   import Ecto.Query
 
   import TransportWeb.ResourceView, only: [issue_type: 1, latest_validations_nb_days: 0]
@@ -177,65 +175,6 @@ defmodule TransportWeb.ResourceController do
     end
   end
 
-  @spec form(Plug.Conn.t(), map()) :: Plug.Conn.t()
-  def form(conn, %{"dataset_id" => dataset_id} = params) do
-    # This shows a form with data coming directly from the datagouv API for fresh data
-    with {:ok, dataset} <- Datagouvfr.Client.Datasets.get(dataset_id),
-         # Resource and resource_id may be nil in case of a new resource
-         resource <- assign_resource_from_dataset_payload(dataset, params["resource_id"]) do
-      conn
-      |> assign_datasets(dataset)
-      |> assign(:resource, resource)
-      |> render("form.html")
-    else
-      _ ->
-        conn
-        |> put_flash(
-          :error,
-          Gettext.dgettext(TransportWeb.Gettext, "resource", "Unable to get resources, please retry.")
-        )
-        |> put_view(ErrorView)
-        |> render("404.html")
-    end
-  end
-
-  def delete_resource_confirmation(%Plug.Conn{} = conn, %{"dataset_id" => dataset_id, "resource_id" => resource_id}) do
-    with {:ok, dataset} <- Datagouvfr.Client.Datasets.get(dataset_id),
-         # Resource and resource_id may be nil in case of a new resource
-         resource when not is_nil(resource) <- assign_resource_from_dataset_payload(dataset, resource_id) do
-      conn
-      |> assign_datasets(dataset)
-      |> assign(:resource, resource)
-      |> render("delete_resource_confirmation.html")
-    else
-      _ ->
-        conn
-        |> put_flash(
-          :error,
-          Gettext.dgettext(TransportWeb.Gettext, "resource", "Unable to get resources, please retry.")
-        )
-        |> put_view(ErrorView)
-        |> render("404.html")
-    end
-  end
-
-  def delete(%Plug.Conn{} = conn, %{"dataset_id" => dataset_id, "resource_id" => _} = params) do
-    with {:ok, _} <- Datagouvfr.Client.Resources.delete(conn, params),
-         dataset when not is_nil(dataset) <-
-           Repo.get_by(Dataset, datagouv_id: dataset_id),
-         {:ok, _} <- ImportData.import_dataset_logged(dataset),
-         {:ok, _} <- Dataset.validate(dataset) do
-      conn
-      |> put_flash(:info, dgettext("resource", "The resource has been deleted"))
-      |> redirect(to: page_path(conn, :espace_producteur))
-    else
-      _ ->
-        conn
-        |> put_flash(:error, dgettext("resource", "Could not delete the resource"))
-        |> redirect(to: page_path(conn, :espace_producteur))
-    end
-  end
-
   @doc """
   `download` is in charge of downloading resources.
 
@@ -302,50 +241,4 @@ defmodule TransportWeb.ResourceController do
   end
 
   defp downcase_header({h, v}), do: {String.downcase(h), v}
-
-  @spec post_file(Plug.Conn.t(), map) :: Plug.Conn.t()
-  def post_file(conn, params) do
-    success_message =
-      if Map.has_key?(params, "resource_file") do
-        dgettext("resource", "File uploaded!")
-      else
-        dgettext("resource", "Resource updated with URL!")
-      end
-
-    with {:ok, _} <- Datagouvfr.Client.Resources.update(conn, params),
-         dataset when not is_nil(dataset) <-
-           Repo.get_by(Dataset, datagouv_id: params["dataset_id"]),
-         {:ok, _} <- ImportData.import_dataset_logged(dataset),
-         {:ok, _} <- Dataset.validate(dataset) do
-      conn
-      |> put_flash(:info, success_message)
-      |> redirect(to: dataset_path(conn, :details, params["dataset_id"]))
-    else
-      {:error, error} ->
-        Logger.error(
-          "Unable to update resource #{params["resource_id"]} of dataset #{params["dataset_id"]}, error: #{inspect(error)}"
-        )
-
-        conn
-        |> put_flash(:error, dgettext("resource", "Unable to upload file"))
-        |> form(params)
-
-      nil ->
-        Logger.error("Unable to get dataset with datagouv_id: #{params["dataset_id"]}")
-
-        conn
-        |> put_flash(:error, dgettext("resource", "Unable to upload file"))
-        |> form(params)
-    end
-  end
-
-  defp assign_datasets(%Plug.Conn{} = conn, %{"id" => dataset_id} = dataset) do
-    conn
-    |> assign(:db_dataset, DB.Repo.get_by!(DB.Dataset, datagouv_id: dataset_id))
-    |> assign(:dataset, dataset)
-  end
-
-  defp assign_resource_from_dataset_payload(dataset, resource_id) do
-    Enum.find(dataset["resources"], &(&1["id"] == resource_id))
-  end
 end
diff --git a/apps/transport/lib/transport_web/router.ex b/apps/transport/lib/transport_web/router.ex
@@ -99,6 +99,18 @@ defmodule TransportWeb.Router do
         get("/:dataset_id/edit", EspaceProducteurController, :edit_dataset)
         post("/:dataset_id/upload_logo", EspaceProducteurController, :upload_logo)
         delete("/:dataset_id/custom_logo", EspaceProducteurController, :remove_custom_logo)
+
+        scope("/:dataset_id/resources") do
+          get("/:resource_datagouv_id/delete", EspaceProducteurController, :delete_resource_confirmation)
+          get("/new_resource/", EspaceProducteurController, :new_resource)
+          get("/:resource_datagouv_id/", EspaceProducteurController, :edit_resource)
+        end
+
+        scope "/:dataset_datagouv_id/resources" do
+          post("/", EspaceProducteurController, :post_file)
+          delete("/:resource_datagouv_id/delete", EspaceProducteurController, :delete_resource)
+          post("/:resource_datagouv_id/", EspaceProducteurController, :post_file)
+        end
       end
 
       live_session :espace_producteur, session: %{"role" => :producer}, root_layout: {TransportWeb.LayoutView, :app} do
@@ -150,19 +162,6 @@ defmodule TransportWeb.Router do
       scope "/conversions" do
         get("/:resource_id/:convert_to", ConversionController, :get)
       end
-
-      scope "/update" do
-        pipe_through([:authenticated])
-
-        scope "/datasets/:dataset_id/resources" do
-          post("/", ResourceController, :post_file)
-          get("/_new_resource/", ResourceController, :form)
-          get("/:resource_id/", ResourceController, :form)
-          get("/:resource_id/delete", ResourceController, :delete_resource_confirmation)
-          delete("/:resource_id/delete", ResourceController, :delete)
-          post("/:resource_id/", ResourceController, :post_file)
-        end
-      end
     end
 
     scope "/backoffice", Backoffice, as: :backoffice do

diff --git a/apps/transport/lib/transport_web/templates/dataset/_header_links.html.heex b/apps/transport/lib/transport_web/templates/dataset/_header_links.html.heex
@@ -8,7 +8,7 @@
     <%= if @current_user do %>
       <%= if @is_producer do %>
         <%= link(dgettext("default", "Producer space"),
-          to: page_path(@conn, :espace_producteur, utm_campaign: "dataset_details"),
+          to: espace_producteur_path(@conn, :edit_dataset, @dataset.id, utm_campaign: "dataset_details"),
           target: "_blank"
         ) %>
       <% else %>

diff --git a/...templates/resource/_specify_url.html.heex → .../espace_producteur/_specify_url.html.heex b/...templates/resource/_specify_url.html.heex → .../espace_producteur/_specify_url.html.heex
@@ -1,9 +1,9 @@
 <div class="choose-option">
-  <%= dgettext("resource", "Give a link for the resource") %>
+  <%= dgettext("espace-producteurs", "Give a link for the resource") %>
   <%= text_input(
     @f,
     :url,
     placeholder: "https://data.ville.fr/gtfs.zip",
-    value: @resource["url"]
+    value: @datagouv_resource["url"]
   ) %>
 </div>
diff --git a/apps/transport/lib/transport_web/templates/espace_producteur/_upload_file.html.heex b/apps/transport/lib/transport_web/templates/espace_producteur/_upload_file.html.heex
@@ -0,0 +1,11 @@
+<div class="choose-option">
+  <%= dgettext("espace-producteurs", "Upload a file") %>
+  <%= unless is_nil(@datagouv_resource["url"]) do %>
+    <p>
+      <%= dgettext("espace-producteurs", "Current file: %{current_file}",
+        current_file: Path.basename(@datagouv_resource["url"])
+      ) %>
+    </p>
+  <% end %>
+  <%= file_input(@f, :resource_file) %>
+</div>