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.

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

feat: display shape on map in shuttle definition pages #1037

Merged
merged 1 commit into from
Nov 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions lib/arrow/shuttles.ex
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,14 @@ defmodule Arrow.Shuttles do
"""
def get_shape!(id), do: Repo.get!(Shape, id)

@doc """
Gets the shapes specifiied by a list of ids. Does not raise if any of the ids are missing,
meaning the resulting list may be shorter than the input list.
"""
def get_shapes(ids) do
Repo.all(from s in Shape, where: s.id in ^ids)
end

@doc """
Gets a shapes upload struct associated with a given shape.

Expand Down
59 changes: 51 additions & 8 deletions lib/arrow_web/live/shuttle_live/shuttle_live.ex
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ defmodule ArrowWeb.ShuttleViewLive do
import Phoenix.HTML.Form
alias Arrow.Shuttles
alias Arrow.Shuttles.Shuttle
alias ArrowWeb.ShapeView

embed_templates "shuttle_live/*"

Expand All @@ -14,6 +15,7 @@ defmodule ArrowWeb.ShuttleViewLive do
attr :http_action, :string
attr :gtfs_disruptable_routes, :list, required: true
attr :shapes, :list, required: true
attr :map_props, :map, required: false, default: %{}

def shuttle_form(assigns) do
~H"""
Expand Down Expand Up @@ -60,6 +62,7 @@ defmodule ArrowWeb.ShuttleViewLive do
/>
</div>
</div>
<%= live_react_component("Components.ShapeViewMap", @map_props, id: "shuttle-view-map") %>
<hr />
<h2>define route</h2>
<.inputs_for :let={f_route} field={f[:routes]}>
Expand Down Expand Up @@ -152,6 +155,14 @@ defmodule ArrowWeb.ShuttleViewLive do
"""
end

defp shapes_to_shapeviews(shapes) do
shapes
|> Enum.map(&Shuttles.get_shapes_upload/1)
|> Enum.reject(&(&1 == {:ok, :disabled}))
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, do we expect to ever need to disable shape storage? Or is the :shape_storage_enabled? config sort of a "dead feature flag" at this point? (Not exactly a feature flag since changing it requires a redeploy, but you get the gist)

Not something to deal with in this PR, but it may be worth removing as part of a tech debt ticket.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's disabled by default for tests, which is why I made this deal with it gracefully. Otherwise any interaction with this form under test (without enabling shape storage first) was breaking.

|> Enum.map(&ShapeView.shapes_map_view/1)
|> Enum.map(&List.first(&1.shapes))
Copy link
Member

Choose a reason for hiding this comment

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

Do we ever expect to have more or less than one shape in &1.shapes?
Would it make sense to do a more assertive fn %{shapes: [shape]} -> shape end?

As of now, if &1.shapes is an empty list then we'll end up with a nil element, and the ShapeViewMap TS code seems to expect an array of non-null shape objects. I think it would end up throwing an exception on this line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can change the code to be more assertive.

I think we should always get exactly one shape. When storing a KML, we only ever store one shape in one file, so we shouldn't get back multiple shapes per. If that ever doesn't hold true, it'll be bad for more than just this component. Given that shapes_map_view should always return a list with exactly one shape.

Technically, we could probably have a shape record in the database that doesn't have a file in S3. It SHOULDN'T happen since we save to s3 before making the database entry, but since they are decoupled, nothing stops someone from coming along and deleting the shape file, for example. My thinking is in this case, it's OK to crash (which would happen in shapes_map_view when it receives an error changeset instead of a shape), but I'd be receptive to arguments that it's a failure case worth dealing with explicitly.

end

def mount(%{"id" => id} = _params, session, socket) do
logout_url = session["logout_url"]
shuttle = Shuttles.get_shuttle!(id)
Expand All @@ -160,6 +171,14 @@ defmodule ArrowWeb.ShuttleViewLive do
shapes = Shuttles.list_shapes()
form = to_form(changeset)

shuttle_shapes =
shuttle
|> Map.get(:routes)
|> Enum.map(&Map.get(&1, :shape))
|> Enum.reject(&is_nil/1)

shapes_map_view = shapes_to_shapeviews(shuttle_shapes)

socket =
socket
|> assign(:form, form)
Expand All @@ -170,6 +189,7 @@ defmodule ArrowWeb.ShuttleViewLive do
|> assign(:gtfs_disruptable_routes, gtfs_disruptable_routes)
|> assign(:shapes, shapes)
|> assign(:logout_url, logout_url)
|> assign(:map_props, %{shapes: shapes_map_view})

{:ok, socket}
end
Expand All @@ -196,19 +216,31 @@ defmodule ArrowWeb.ShuttleViewLive do
|> assign(:gtfs_disruptable_routes, gtfs_disruptable_routes)
|> assign(:shapes, shapes)
|> assign(:logout_url, logout_url)
|> assign(:map_props, %{shapes: []})

{:ok, socket}
end

def handle_event("validate", params, socket) do
shuttle_params = params |> combine_params()

form =
socket.assigns.shuttle
|> Shuttles.change_shuttle(shuttle_params)
|> to_form(action: :validate)
# A new shape is selected
def handle_event(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Assigning an phx-change on the input itself prevents the form's phxx-change from firing meaning it's stop validation from running when a new shape was selected. The idea here is to instead detect when the change is a shape drop down, and update the map in addition to running the normal validation.

"validate",
%{"_target" => ["shuttle", "routes", _direction_id, "shape_id"]} = params,
Copy link
Member

Choose a reason for hiding this comment

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

If I'm reading this right, aren't we validating only one direction? Will the shapes for both directions be in params, despite _target giving a path to only one shape?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

_target key has the path to the piece of data that changed in order to trigger the phx-change event. So this function clause will match when the shape_id for a direction changes, but it doesn't care which. I could access the specific changed shape with params["shuttle]["routes"][direction_id]["shape_id"] but instead of bookkeeping exactly what changed it seemed easier to just update both shapes.

The full form params are stored under the shuttle key (decided by the :as param on the form), and is extracted in the validation function here.

socket
) do
shapes =
[
params["shuttle"]["routes"]["0"]["shape_id"],
params["shuttle"]["routes"]["1"]["shape_id"]
]
|> Enum.reject(&(&1 == ""))
|> Shuttles.get_shapes()
|> shapes_to_shapeviews()

validate(params, assign(socket, :map_props, %{socket.assigns.map_props | shapes: shapes}))
end

{:noreply, assign(socket, form: form)}
def handle_event("validate", params, socket) do
validate(params, socket)
end

def handle_event("edit", params, socket) do
Expand Down Expand Up @@ -306,4 +338,15 @@ defmodule ArrowWeb.ShuttleViewLive do
route_changeset
end
end

defp validate(params, socket) do
shuttle_params = params |> combine_params()

form =
socket.assigns.shuttle
|> Shuttles.change_shuttle(shuttle_params)
|> to_form(action: :validate)

{:noreply, assign(socket, form: form)}
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
http_action={@http_action}
gtfs_disruptable_routes={@gtfs_disruptable_routes}
shapes={@shapes}
map_props={@map_props}
/>

<.back navigate={~p"/shuttles"}>Back to shuttles</.back>
13 changes: 12 additions & 1 deletion test/arrow/shuttles_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ defmodule Arrow.ShuttlesTest do
end

test "create_shape/1 with name that does not end in -S adds it" do
Application.put_env(:arrow, :shape_storage_enabled?, true)
reassign_env(:shape_storage_enabled?, true)
Application.put_env(:arrow, :shape_storage_prefix, "prefix/#{Ecto.UUID.generate()}/")

assert {:ok, %Shape{} = shape} =
Expand Down Expand Up @@ -64,6 +64,17 @@ defmodule Arrow.ShuttlesTest do
assert Shuttles.get_shape!(shape.id) == shape
end

test "get_shapes returns all shapes with matching ids" do
shapes = [shape_fixture(), shape_fixture()]
shape_ids = Enum.map(shapes, fn shape -> shape.id end)
assert MapSet.new(Shuttles.get_shapes(shape_ids)) == MapSet.new(shapes)
end

test "get_shapes returns empty list when no shapes match" do
shape_ids = [1, 2, 3]
assert [] == Shuttles.get_shapes(shape_ids)
end

test "create_shapes/1 with valid data creates a shape" do
assert {:ok, [{:ok, %Shape{} = shape}]} = Shuttles.create_shapes([@valid_attrs])
assert shape.name == "some name-S"
Expand Down
Loading