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

fix(SiteWeb.StopController): don't show route patterns for services that aren't running today #1762

Merged
merged 7 commits into from
Oct 13, 2023
Merged
Show file tree
Hide file tree
Changes from 6 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
18 changes: 16 additions & 2 deletions apps/route_patterns/lib/route_pattern.ex
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ defmodule RoutePatterns.RoutePattern do
:route_id,
:time_desc,
:typicality,
:service_id,
sort_order: 0
]

Expand All @@ -55,7 +56,8 @@ defmodule RoutePatterns.RoutePattern do
route_id: Route.id_t(),
time_desc: String.t(),
typicality: typicality_t(),
sort_order: integer()
sort_order: integer(),
service_id: String.t()
}

def new(%Item{
Expand Down Expand Up @@ -92,7 +94,8 @@ defmodule RoutePatterns.RoutePattern do
route_id: route_id,
time_desc: time_desc,
typicality: typicality,
sort_order: sort_order
sort_order: sort_order,
service_id: service_id(trip_relationships)
}
end

Expand Down Expand Up @@ -152,4 +155,15 @@ defmodule RoutePatterns.RoutePattern do
end

defp stop_ids(_), do: nil

defp service_id(%{
"service" => [
%Item{
id: service_id
}
]
Comment on lines +160 to +164
Copy link
Contributor

Choose a reason for hiding this comment

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

This arraying of relationships is exactly what is happening in the line parsing I am working on. Should we keep this behavior around?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I see what you mean.

Should we keep this behavior around?

Changing this (via removing list() in the relevant part of the %JsonApi.Item{} struct here) would require a baffling amount of downstream changes in all of our V3 API parsing. I don't see the benefit in changing it.

}),
do: service_id

defp service_id(_), do: nil
end
12 changes: 12 additions & 0 deletions apps/services/lib/repo.ex
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@ defmodule Services.Repo do
alias Services.Service
alias V3Api.Services, as: ServicesApi

def by_id(id) when is_binary(id) do
cache(id, fn _ ->
ServicesApi.get(id)
|> handle_response()
end)
|> List.first()
end

def by_route_id(route_id, params \\ [])

def by_route_id([route_id] = route, params) when is_list(route) do
Expand All @@ -30,4 +38,8 @@ defmodule Services.Repo do
defp handle_response(%JsonApi{data: data}) do
Enum.map(data, &Service.new/1)
end

defp handle_response({:error, [%JsonApi.Error{code: "not_found"}]}) do
[]
end
end
54 changes: 54 additions & 0 deletions apps/services/lib/service.ex
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
defmodule Services.Service do
@moduledoc "Processes Services, including dates and notes"
alias JsonApi.Item
use Timex

defstruct added_dates: [],
added_dates_notes: [],
Expand Down Expand Up @@ -153,4 +154,57 @@ defmodule Services.Service do
defp typicality(5), do: :unplanned_disruption
defp typicality(6), do: :canonical
defp typicality(_), do: :unknown

@spec serves_date?(t(), Date.t()) :: boolean
def serves_date?(service, date) do
date in all_valid_dates_for_service(service)
end

@spec all_valid_dates_for_service(t()) :: [Date.t()]
defp all_valid_dates_for_service(%__MODULE__{
start_date: from,
end_date: until,
added_dates: added_dates,
removed_dates: removed_dates,
valid_days: valid_days
}) do
# fallback to today if either start or end date are nil
from = from || Timex.today()
until = until || Timex.today()

dates =
if from == until do
[from]
else
[
from: from,
until: until,
right_open: false
]
|> Interval.new()
|> Enum.map(& &1)
end

removed_dates = parse_listed_dates(removed_dates)

(dates ++ parse_listed_dates(added_dates))
|> Enum.reject(&Enum.member?(removed_dates, &1))
|> Enum.reject(fn date ->
# if valid_days is an empty array, the service's dates are likely those in
# added_dates, so we can ignore evaluating the day of the week here
if valid_days != [] do
Timex.weekday(date) not in valid_days
end
end)
|> Enum.map(&Timex.to_date/1)
|> Enum.uniq()
end

@spec parse_listed_dates([String.t()]) :: [NaiveDateTime.t()]
defp parse_listed_dates(date_strings) do
date_strings
|> Enum.map(&Timex.parse(&1, "{YYYY}-{0M}-{D}"))
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Does the {D} need the leading zero?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

..yes. I will update

|> Enum.filter(&(elem(&1, 0) == :ok))
|> Enum.map(&elem(&1, 1))
end
end
8 changes: 8 additions & 0 deletions apps/services/test/repo_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,18 @@ defmodule Services.RepoTest do
:ok
end

test "by_id fetches service by ID" do
assert %Service{} = Repo.by_id("canonical")
end

test "by_route_id fetches services for a route" do
assert [%Service{} | _] = Repo.by_route_id("Red")
end

test "by_route_id fetches services for a list" do
assert [%Service{} | _] = Repo.by_route_id(["Red"])
end

test "by_route_id fetches services for the green line" do
assert Repo.by_route_id("Green") == Repo.by_route_id("Green-B,Green-C,Green-D,Green-E")
end
Expand Down
46 changes: 46 additions & 0 deletions apps/services/test/service_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,52 @@ defmodule Services.ServiceTest do
end
end

describe "serves_date?/2" do
test "computes if the date is covered by a Service" do
# date in added_dates
assert Service.serves_date?(
%Service{
added_dates: ["2022-12-15", "2022-12-14"],
start_date: ~D[2022-12-14],
end_date: ~D[2022-12-15],
valid_days: []
},
~D[2022-12-15]
)

# date in removed_dates
refute Service.serves_date?(
%Service{
removed_dates: ["2022-12-15", "2022-12-14"],
start_date: ~D[2022-12-11],
end_date: ~D[2022-12-22],
valid_days: []
},
~D[2022-12-15]
)

# date is on right valid_days
assert Service.serves_date?(
%Service{
start_date: ~D[2022-12-11],
end_date: ~D[2022-12-22],
valid_days: [4, 5, 6]
},
~D[2022-12-15]
)

# date not on right valid_days
refute Service.serves_date?(
%Service{
start_date: ~D[2022-12-11],
end_date: ~D[2022-12-22],
valid_days: [1, 2, 3]
},
~D[2022-12-15]
)
end
end

defp test_services(_) do
[
%Service{
Expand Down
19 changes: 19 additions & 0 deletions apps/site/lib/site_web/controllers/stop_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ defmodule SiteWeb.StopController do
alias Site.JsonHelpers
alias Routes.{Group, Route}
alias RoutePatterns.RoutePattern
alias Services.Service
alias Site.TransitNearMe
alias SiteWeb.AlertView
alias SiteWeb.PartialView.HeaderTab
Expand Down Expand Up @@ -189,13 +190,31 @@ defmodule SiteWeb.StopController do
defp route_patterns_by_route_and_headsign(stop_id) do
stop_id
|> RoutePatterns.Repo.by_stop_id()
|> Enum.reject(&not_serving_today?/1)
|> Enum.reject(&ends_at?(&1, stop_id))
|> Enum.map(&add_polyline/1)
|> Enum.group_by(& &1.route_id)
|> Enum.map(fn {k, v} -> {k, Enum.group_by(v, & &1.headsign)} end)
|> Map.new()
end

@spec not_serving_today?(RoutePattern.t()) :: boolean()
# Canonical route patterns don't serve any date! Just ignore in this case
defp not_serving_today?(%RoutePattern{typicality: :canonical}), do: false

defp not_serving_today?(%RoutePattern{service_id: service_id})
when is_binary(service_id) do
case Services.Repo.by_id(service_id) do
%Service{} = service ->
not Service.serves_date?(service, Timex.today())

_ ->
false
end
end

defp not_serving_today?(_), do: false

defp ends_at?(%RoutePattern{stop_ids: stop_ids}, stop_id) when is_list(stop_ids) do
with last_stop_id <- List.last(stop_ids),
%Stop{child_ids: child_ids} <- Stops.Repo.get(stop_id) do
Expand Down
4 changes: 4 additions & 0 deletions apps/v3_api/lib/services.ex
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,8 @@ defmodule V3Api.Services do
def all(params \\ []) do
V3Api.get_json("/services/", params)
end

def get(id, params \\ []) do
V3Api.get_json("/services/#{id}", params)
end
end
20 changes: 20 additions & 0 deletions apps/v3_api/test/services_test.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
defmodule V3Api.ServicesTest do
use ExUnit.Case

alias V3Api.Services

@opts ["page[limit]": 1, sort: "id"]

describe "all/1" do
test "gets all services" do
assert %JsonApi{data: [%JsonApi.Item{}]} = Services.all(@opts)
end
end

describe "get/1" do
test "gets the services by ID" do
%JsonApi{data: [%JsonApi.Item{} = service]} = Services.get("canonical")
assert service.id == "canonical"
end
end
end