-
Notifications
You must be signed in to change notification settings - Fork 19
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
🍎 Ability to filter V3 API route patterns by date #687
Conversation
Coverage of commit
|
|
||
test "can filter by date and route(s)", %{conn: base_conn} do | ||
today = Parse.Time.service_date() | ||
bad_date = ~D[2027-01-01] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this cause problems when today actually is 1/1/27? You could define this to be today's date plus a year instead.
} = params | ||
) do | ||
filtered_route_ids = | ||
Enum.filter(route_ids, fn id -> Enum.member?(date_specific_route_ids, id) end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: if you're doing Enum.member?/2
checks, should date_specific_route_ids
be a MapSet
instead of a list?
defp gather_trip_based_route_pattern_ids(params), do: params | ||
|
||
defp consolidate_route_pattern_ids(route_pattern_ids, %{ids: [_ | _] = ids}) do | ||
Enum.filter(route_pattern_ids, fn rid -> Enum.member?(ids, rid) end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: if you're doing Enum.member?/2
checks, should route_pattern_ids
be a MapSet
instead of a list?
…ing filters rather than after all have been partially processed
Coverage of commit
|
|> Map.put(:routes, route_ids) | ||
|> Trip.filter_by() | ||
|> Enum.map(& &1.route_pattern_id) | ||
|> consolidate_route_pattern_ids(acc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: rather than filtering the results after the Trip
query, what about passing in the route_pattern_ids
in as part of the Trip.filter_by/1
query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, missed that this was an already existing filter here
data = ApiWeb.RoutePatternController.index_data(base_conn, params) | ||
assert [route_pattern3] == data | ||
|
||
params = %{"filter" => %{"date" => future_date_iso, "route" => "route1,route2,route3"}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: can you also add a test for "date" + "id" ?
…. Add test for date+ids
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one last thing and then I think this will be good to go!
|> Map.take([:date, :direction_id, :route_patterns]) | ||
|> Map.put(:route_patterns, ids) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|> Map.take([:date, :direction_id, :route_patterns]) | |
|> Map.put(:route_patterns, ids) | |
|> Map.take([:date, :direction_id]) | |
|> Map.put(:route_patterns, ids) |
suggestion: it looks like you're taking route_patterns
and them immediately overwriting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops yep, initially had it setup slightly different, setting that key in a different function. removed
Coverage of commit
|
Coverage of commit
|
Summary of changes
Asana Ticket: 🍎 Ability to filter V3 API route patterns by date