feat: display shape on map in shuttle definition pages #1037
Conversation
| |> Shuttles.change_shuttle(shuttle_params) | ||
| |> to_form(action: :validate) | ||
| # A new shape is selected | ||
| def handle_event( |
There was a problem hiding this comment.
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.
Whoops
requested review from
a team and
jzimbel-mbta
and removed request for
a team
| defp shapes_to_shapeviews(shapes) do | ||
| shapes | ||
| |> Enum.map(&Shuttles.get_shapes_upload/1) | ||
| |> Enum.reject(&(&1 == {:ok, :disabled})) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(&Shuttles.get_shapes_upload/1) | ||
| |> Enum.reject(&(&1 == {:ok, :disabled})) | ||
| |> Enum.map(&ShapeView.shapes_map_view/1) | ||
| |> Enum.map(&List.first(&1.shapes)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| # A new shape is selected | ||
| def handle_event( | ||
| "validate", | ||
| %{"_target" => ["shuttle", "routes", _direction_id, "shape_id"]} = params, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
_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.
Whoops
force-pushed
the
wh-shuttle-map-view
branch
from
3e2d75b
to
49242a1
Compare
Whoops
force-pushed
the
wh-shuttle-map-view
branch
from
49242a1
to
4d89d8f
Compare
Summary of changes
Asana Ticket: 🏹 Implement "Create/Edit Shuttle Route Definition" Map View + Route Shape Display
[Please include a brief description of what was changed]
Reviewer Checklist