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

🍎 Ability to filter V3 API route patterns by date #687

Merged
merged 5 commits into from
Oct 20, 2023

Conversation

bfauble
Copy link
Contributor

@bfauble bfauble commented Oct 17, 2023

Summary of changes

Asana Ticket: 🍎 Ability to filter V3 API route patterns by date

  • Use date to determine active services
  • Use list of active services to determine relevant routes/trips

@bfauble bfauble requested review from a team and dks-mbta October 17, 2023 18:02
@github-actions
Copy link

Coverage of commit 2c8dbd5

Summary coverage rate:
  lines......: 89.2% (4119 of 4616 lines)
  functions..: 70.9% (2229 of 3143 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/route_pattern_controller.ex      |95.7%     47| 100%    21|    -      0
  apps/state/lib/state/route_pattern.ex                                 | 100%     23|63.4%    41|    -      0

Download coverage report

@bfauble bfauble requested review from nlwstein and removed request for dks-mbta October 17, 2023 18:17

test "can filter by date and route(s)", %{conn: base_conn} do
today = Parse.Time.service_date()
bad_date = ~D[2027-01-01]
Copy link
Contributor

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)
Copy link
Member

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)
Copy link
Member

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
@bfauble bfauble requested a review from paulswartz October 18, 2023 18:45
@github-actions
Copy link

Coverage of commit 931cbda

Summary coverage rate:
  lines......: 89.3% (4127 of 4624 lines)
  functions..: 70.9% (2231 of 3145 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/route_pattern_controller.ex      |96.4%     55| 100%    23|    -      0
  apps/state/lib/state/route_pattern.ex                                 | 100%     23|63.4%    41|    -      0

Download coverage report

|> Map.put(:routes, route_ids)
|> Trip.filter_by()
|> Enum.map(& &1.route_pattern_id)
|> consolidate_route_pattern_ids(acc)
Copy link
Member

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?

Copy link
Contributor Author

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"}}
Copy link
Member

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" ?

@bfauble bfauble requested a review from paulswartz October 20, 2023 17:03
Copy link
Member

@paulswartz paulswartz left a 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!

Comment on lines 173 to 174
|> Map.take([:date, :direction_id, :route_patterns])
|> Map.put(:route_patterns, ids)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
|> 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.

Copy link
Contributor Author

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

@github-actions
Copy link

Coverage of commit 8dca8c6

Summary coverage rate:
  lines......: 89.3% (4128 of 4624 lines)
  functions..: 70.9% (2231 of 3145 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/route_pattern_controller.ex      |98.2%     55| 100%    23|    -      0
  apps/state/lib/state/route_pattern.ex                                 | 100%     23|63.4%    41|    -      0

Download coverage report

@github-actions
Copy link

Coverage of commit 27978cb

Summary coverage rate:
  lines......: 89.3% (4129 of 4624 lines)
  functions..: 70.9% (2231 of 3145 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/route_pattern_controller.ex      |98.2%     55| 100%    23|    -      0
  apps/state/lib/state/route_pattern.ex                                 | 100%     23|63.4%    41|    -      0

Download coverage report

@bfauble bfauble merged commit 8d6ea1c into master Oct 20, 2023
6 checks passed
@paulswartz paulswartz deleted the bf-route-pattern-filter-by-date branch October 20, 2023 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants