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

Stop ID autocomplete #1057

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
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
30 changes: 30 additions & 0 deletions lib/arrow/shuttles.ex
Original file line number Diff line number Diff line change
Expand Up @@ -429,4 +429,34 @@ defmodule Arrow.Shuttles do
stop -> stop
end
end

@spec stops_or_gtfs_stops_by_search_string(String.t()) :: [Stop.t() | GtfsStop.t()]
def stops_or_gtfs_stops_by_search_string(string) do
# See https://github.blog/engineering/like-injection/
sanitized_string = Regex.replace(~r/([\%_])/, string, fn _, x -> "\\#{x}" end) <> "%"
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion / question

Maybe a silly question, but could we just remove all %'s? And maybe even _'s too. I wouldn't expect either of those characters to ever show up in a stop name, ID, or description.

Also, it looks like you anchor the search to the start of the string since % is only appended and not prepended to the search. I think that's ok but just wanted to check that it was an intentional choice.


stops_query =
from(s in Stop,
where:
ilike(s.stop_id, ^sanitized_string) or ilike(s.stop_desc, ^sanitized_string) or
ilike(s.stop_name, ^sanitized_string)
)

gtfs_stops_query =
from(g in GtfsStop,
where:
g.vehicle_type == :bus and g.location_type == :stop_platform and
(ilike(g.id, ^sanitized_string) or ilike(g.desc, ^sanitized_string) or
ilike(g.name, ^sanitized_string))
)

matching_stops = Repo.all(stops_query)

arrow_stop_ids = matching_stops |> Enum.map(& &1.stop_id) |> MapSet.new()

matching_gtfs_stops =
gtfs_stops_query |> Repo.all() |> Enum.filter(&(!MapSet.member?(arrow_stop_ids, &1.id)))

Enum.concat(matching_stops, matching_gtfs_stops)
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion

Would it make sense to sort the results in some way? For example, alphabetically or by Jaro distance from the user-entered string.

(Though since the search is over 3 different fields in 2 different tables, this could get complicated as you'd need to do something along the lines of

matches
|> Enum.sort_by(fn stop ->
  [stop.name, stop.id, stop.desc]
  |> Enum.map(&String.jaro_distance(&1, string)
  |> Enum.max()
end)

so feel free to leave it as-is if that seems like too much)

end
end
7 changes: 6 additions & 1 deletion lib/arrow_web/components/core_components.ex
Original file line number Diff line number Diff line change
Expand Up @@ -413,12 +413,16 @@ defmodule ArrowWeb.CoreComponents do
attr :options, :any
attr :value_mapper, :any
attr :allow_clear, :boolean
attr :target, :any, default: nil

def live_select(%{field: %Phoenix.HTML.FormField{} = field} = assigns) do
assigns =
assigns
|> assign(:errors, Enum.map(field.errors, &translate_error(&1)))
|> assign(:live_select_opts, assigns_to_attributes(assigns, [:errors, :label, :class]))
|> assign(
:live_select_opts,
assigns_to_attributes(assigns, [:errors, :label, :class, :target])
)

~H"""
<div class="form-group">
Expand All @@ -439,6 +443,7 @@ defmodule ArrowWeb.CoreComponents do
dropdown_extra_class={[
"list-unstyled"
]}
phx-target={@target}
{@live_select_opts}
/>

Expand Down
76 changes: 76 additions & 0 deletions lib/arrow_web/components/stop_input.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
defmodule ArrowWeb.StopInput do
@moduledoc """
LiveComponent providing a wrapper around live_select tailored for
stop ID autocomplete.
"""

use ArrowWeb, :live_component

alias Arrow.Gtfs.Stop, as: GtfsStop
alias Arrow.Shuttles
alias Arrow.Shuttles.Stop

attr :id, :string, required: true
attr :field, :any, required: true
attr :label, :string, default: "Stop ID"
attr :class, :string, default: nil

def render(assigns) do
assigns =
assign(
assigns,
:stop,
Phoenix.HTML.Form.input_value(assigns.field.form, :stop) ||
Phoenix.HTML.Form.input_value(assigns.field.form, :gtfs_stop)
Comment on lines +23 to +24
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion / question

Does this mean this component is safe to use only in a form that also has stop and gtfs_stop inputs?

My understanding of LiveView is still pretty tenuous, but this feels like a sort of hidden dependency for this component.

Is there a way to pass these values directly and explicitly into StopInput, as additional attrs, instead of having it reach outside of itself under the assumption that these other sibling inputs exist and have the specified names?

)

# This should only change if the selected value actually changes,
# not just when a user is typing and options change.
assigns =
assign(
assigns,
:options,
if is_nil(assigns.stop) || !Ecto.assoc_loaded?(assigns.stop) do
[]
else
[option_for_stop(assigns.stop)]
end
)
Comment on lines +27 to +38
Copy link
Member

Choose a reason for hiding this comment

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

Question

Bear with me as I continue to muddle through the live component lifecycle & data-flow models...

So, is the idea here that this expression in render/1 sets the initial options for the live_select, and the logic in handle_event("live_select_change", ...) updates the options by sending them directly to the live_select and having it essentially override the original list it received from the initial render?
This is a little weird to me, as the options attr passed by this component to live_select can be replaced without this component's knowledge... It feels very not-declarative 🤨

Based on the diagram here, it seems like render/1 will be called again if this wrapper component, or any of its ancestors, see a change in state. (Which makes sense to me as someone coming from React-land)

Would that cause the options list to be reset to the result of this expression, if it was previously updated by a "live_select_change" event?
Do we want that behavior?


~H"""
<div id={@id} class={@class}>
<.live_select
field={@field}
label={@label}
allow_clear={true}
target={@myself}
options={@options}
/>
</div>
"""
end

def handle_event("live_select_change", %{"id" => live_select_id, "text" => text}, socket) do
new_opts =
if String.length(text) < 2 do
# We only start autocomplete at 2 characters, but there are some 1-character stop IDs
case Shuttles.stop_or_gtfs_stop_for_stop_id(text) do
nil -> []
stop -> [option_for_stop(stop)]
end
else
text |> Shuttles.stops_or_gtfs_stops_by_search_string() |> Enum.map(&option_for_stop/1)
end

send_update(LiveSelect.Component, id: live_select_id, options: new_opts)

{:noreply, socket}
end

@spec option_for_stop(Stop.t() | GtfsStop.t()) :: {String.t(), String.t()}
defp option_for_stop(%Stop{stop_id: stop_id, stop_desc: stop_desc, stop_name: stop_name}),
do: {"#{stop_id} - #{stop_desc || stop_name}", stop_id}
Copy link
Member

Choose a reason for hiding this comment

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

Issue

The Arrow shuttle stops table's stop_desc column is non-null, and empty strings are truthy in Elixir, so stop_desc || stop_name will never evaluate to stop_name.

I'm not sure if we allow stop_desc to be an empty string, but if we do then this would need to be something like

Suggested change
do: {"#{stop_id} - #{stop_desc || stop_name}", stop_id}
do: {"#{stop_id} - #{if(stop_desc != "", do: stop_desc, else: stop_name)}", stop_id}


defp option_for_stop(%GtfsStop{id: id, desc: desc, name: name}),
do: {"#{id} - #{desc || name}", id}
end
7 changes: 6 additions & 1 deletion lib/arrow_web/live/shuttle_live/shuttle_live.ex
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,12 @@ defmodule ArrowWeb.ShuttleViewLive do
<div class="col-lg-1">
<.icon name="hero-bars-3" class="h-4 w-4 drag-handle cursor-grab" />
</div>
<.input field={f_route_stop[:display_stop_id]} label="Stop ID" class="col-lg-6" />
<.live_component
module={ArrowWeb.StopInput}
id={"stop-id-#{input_value(f_route, :direction_id)}-#{input_value(f_route_stop, :stop_sequence)}"}
field={f_route_stop[:display_stop_id]}
class="col-lg-6"
/>
<.input
field={f_route_stop[:time_to_next_stop]}
type="number"
Expand Down
68 changes: 68 additions & 0 deletions test/arrow/shuttles_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,74 @@ defmodule Arrow.ShuttlesTest do
end
end

describe "stops_or_gtfs_stops_by_search_string/1" do
test "finds Arrow stop by stop ID" do
insert(:stop, %{stop_id: "12"})

assert [%Arrow.Shuttles.Stop{stop_id: "12"}] =
Shuttles.stops_or_gtfs_stops_by_search_string("1")
end

test "finds Arrow stop by stop description" do
stop = insert(:stop, %{stop_desc: "Description"})
stop_id = stop.stop_id

assert [%Arrow.Shuttles.Stop{stop_id: ^stop_id}] =
Shuttles.stops_or_gtfs_stops_by_search_string("Des")
end

test "finds Arrow stop by stop name" do
stop = insert(:stop, %{stop_name: "Name"})
stop_id = stop.stop_id

assert [%Arrow.Shuttles.Stop{stop_id: ^stop_id}] =
Shuttles.stops_or_gtfs_stops_by_search_string("Na")
end

test "finds GTFS stop by stop ID" do
insert(:gtfs_stop, %{id: "12"})

assert [%Arrow.Gtfs.Stop{id: "12"}] =
Shuttles.stops_or_gtfs_stops_by_search_string("1")
end

test "finds GTFS stop by stop description" do
gtfs_stop = insert(:gtfs_stop, %{desc: "Description"})
gtfs_stop_id = gtfs_stop.id

assert [%Arrow.Gtfs.Stop{id: ^gtfs_stop_id}] =
Shuttles.stops_or_gtfs_stops_by_search_string("Des")
end

test "finds GTFS stop by stop name" do
gtfs_stop = insert(:gtfs_stop, %{name: "Name"})
gtfs_stop_id = gtfs_stop.id

assert [%Arrow.Gtfs.Stop{id: ^gtfs_stop_id}] =
Shuttles.stops_or_gtfs_stops_by_search_string("Na")
end

test "finds Arrow and GTFS stops when both match, with Arrow first" do
stop = insert(:stop, %{stop_desc: "Description A"})
stop_id = stop.stop_id

gtfs_stop = insert(:gtfs_stop, %{desc: "Description B"})
gtfs_stop_id = gtfs_stop.id

assert [%Arrow.Shuttles.Stop{stop_id: ^stop_id}, %Arrow.Gtfs.Stop{id: ^gtfs_stop_id}] =
Shuttles.stops_or_gtfs_stops_by_search_string("Des")
end

test "when a stop ID is duplicated in Arrow and GTFS, returns the Arrow one" do
insert(:stop, %{stop_id: "stop"})

insert(:gtfs_stop, %{id: "stop"})

assert [%Arrow.Shuttles.Stop{stop_id: "stop"}] =
Shuttles.stops_or_gtfs_stops_by_search_string("st")
end
end

describe "get_travel_times/1" do
test "calculates travel time between coordinates" do
expect(
Expand Down
40 changes: 26 additions & 14 deletions test/arrow_web/live/shuttle_live/shuttle_live_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -148,14 +148,18 @@ defmodule ArrowWeb.ShuttleLiveTest do

{:ok, edit_live, _html} = live(conn, ~p"/shuttles/#{shuttle}/edit")

edit_live
|> element("#shuttle-form")
|> render_change(%{
shuttle: @update_attrs,
routes_with_stops: %{
"0" => %{route_stops: %{"0" => %{display_stop_id: new_gtfs_stop.id}}}
}
})

{:ok, conn} =
edit_live
|> form("#shuttle-form",
shuttle: @update_attrs,
routes_with_stops: %{
"0" => %{route_stops: %{"0" => %{display_stop_id: new_gtfs_stop.id}}}
}
)
|> form("#shuttle-form")
|> render_submit()
|> follow_redirect(conn)

Expand Down Expand Up @@ -213,14 +217,22 @@ defmodule ArrowWeb.ShuttleLiveTest do
|> element("#shuttle-form #add_stop-0[value=\"0\"]", "Add")
|> render_click()

edit_live
|> element("#shuttle-form")
|> render_change(%{
shuttle: @update_attrs,
routes_with_stops: %{
"0" => %{
route_stops: %{
"0" => %{display_stop_id: stop_id, display_stop_id_text_input: stop_id}
}
}
}
})

{:ok, conn} =
edit_live
|> form("#shuttle-form",
shuttle: @update_attrs,
routes_with_stops: %{
"0" => %{route_stops: %{"0" => %{display_stop_id: stop_id}}}
}
)
|> form("#shuttle-form")
|> render_submit()
|> follow_redirect(conn)

Expand Down Expand Up @@ -425,7 +437,7 @@ defmodule ArrowWeb.ShuttleLiveTest do
assert [^stop_id] =
Floki.attribute(
direction_0_stop_rows,
"[data-stop_sequence=#{index}] > div.form-group > input[type=text]",
"[data-stop_sequence=#{index}] > div > div.form-group > div > div > div > input[type=text]",
"value"
)
end
Expand All @@ -434,7 +446,7 @@ defmodule ArrowWeb.ShuttleLiveTest do
assert [^stop_id] =
Floki.attribute(
direction_1_stop_rows,
"[data-stop_sequence=#{index}] > div.form-group > input[type=text]",
"[data-stop_sequence=#{index}] > div > div.form-group > div > div > div > input[type=text]",
"value"
)
end
Expand Down
Loading