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(Schedules): Fix alerts not showing up for stops with inbound trains #2029

Merged
merged 4 commits into from
May 9, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
33 changes: 22 additions & 11 deletions lib/predictions/repo.ex
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,15 @@ defmodule Predictions.Repo do
opts
|> add_all_optional_params()
|> cache_fetch()
|> filter_by_min_time(Keyword.get(opts, :min_time))
|> filter_predictions(Keyword.get(opts, :min_time))
|> load_from_other_repos
end

def all_no_cache(opts) when is_list(opts) and opts != [] do
opts
|> add_all_optional_params()
|> fetch()
|> filter_predictions(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of passing nil here you can just make it a default argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using a default argument of nil rather than passing in nil makes both the function definition and it's application much clearer and cleaner.

|> load_from_other_repos
end

Expand All @@ -53,6 +54,18 @@ defmodule Predictions.Repo do
end
end

@spec filter_predictions([Parser.record()] | {:error, any}, DateTime.t() | nil) ::
[Parser.record()] | {:error, any}
defp filter_predictions({:error, error}, _) do
{:error, error}
end

defp filter_predictions(predictions, min_time) do
Enum.filter(predictions, fn prediction ->
has_departure_time?(prediction) && after_min_time?(prediction, min_time)
end)
end

defp fetch(params) do
_ = Logger.info("predictions_repo_all_cache=cache_miss")

Expand Down Expand Up @@ -98,19 +111,17 @@ defmodule Predictions.Repo do
[]
end

@spec filter_by_min_time([Parser.record()] | {:error, any}, DateTime.t() | nil) ::
[Parser.record()] | {:error, any}
defp filter_by_min_time({:error, error}, _) do
{:error, error}
defp has_departure_time?(
{_id, _trip_id, _stop_id, _route_id, _direction_id, _arrival, departure, _time,
_stop_sequence, _schedule_relationship, _track, _status, _departing?,
_vehicle_id} = _prediction
) do
departure != nil
end

defp filter_by_min_time(predictions, nil) do
predictions
end
defp has_departure_time?(_), do: false

defp filter_by_min_time(predictions, %DateTime{} = min_time) do
Enum.filter(predictions, &after_min_time?(&1, min_time))
end
defp after_min_time?(_, nil), do: true

defp after_min_time?(
{
Expand Down
96 changes: 96 additions & 0 deletions test/predictions/repo_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,102 @@ defmodule Predictions.RepoTest do
end
end

test "filtes out predictions with no departure" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo

Copy link
Contributor

Choose a reason for hiding this comment

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

This typo is still here.

five_minutes_in_future = DateTime.add(Timex.now(), 5, :minute)

five_minutes_in_future_string =
Timex.format!(five_minutes_in_future, "{ISO:Extended:Z}")

expect(MBTA.Api.Mock, :get_json, fn _, _ ->
%JsonApi{
data: [
%JsonApi.Item{
attributes: %{
"departure_time" => nil,
"arrival_time" => five_minutes_in_future_string,
"direction_id" => 1
},
relationships: %{
"route" => [
%{
id: "Red"
}
],
"trip" => [],
"vehicle" => [],
"stop" => [
%{id: "StopID"}
]
}
},
%JsonApi.Item{
attributes: %{
"departure_time" => five_minutes_in_future_string,
"arrival_time" => five_minutes_in_future_string,
"direction_id" => 1
},
relationships: %{
"route" => [
%{
id: "Red"
}
],
"trip" => [],
"vehicle" => [],
"stop" => [
%{id: "StopID"}
]
}
}
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all of these attributes necessary for exercising the test? Would it work just setting the departure_time? These two structs are exactly the same except for that one attribute, so there should be a shorter and clearer way to communicate that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, all these fields are required. The parsing errors out if any of these fields are missing. Ideally I'd like to mock away some of that, but it seemed outside the scope of this fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

These two structs are exactly the same except for that one attribute, so there should be a shorter and clearer way to communicate that.

Because these two structs are identical except for one attribute, you could define it once and then override that attribute for the second. It will make the test shorter. That also helps to communicate the difference between the two.

end)

# Route for calculating display time
expect(MBTA.Api.Mock, :get_json, fn _, _ ->
%JsonApi{
data: [
%JsonApi.Item{
id: "Red",
attributes: %{
"type" => "1",
"long_name" => "Red Test Subway",
"direction_names" => ["North Test Name", "South Test Name"],
"direction_destinations" => ["North Test", "South Test"]
Copy link
Contributor

Choose a reason for hiding this comment

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

These are all magic strings. As per the testing doc we all reviewed and agreed to, we should be using Faker for these (and all others in this file).

https://www.notion.so/mbta-downtown-crossing/The-Four-Legs-of-Quality-Programming-b35248799da84e02b93887361d5bf343

},
relationships: %{}
}
]
}
end)

# Parent Stop for prediction
expect(MBTA.Api.Mock, :get_json, fn _, _ ->
%JsonApi{
data: [
%JsonApi.Item{
id: "place-subway",
attributes: %{
"name" => "Test Subway Stop",
"location_type" => 0,
"platform_name" => "Test Subway Platform",
"platform_code" => "Test Code",
"description" => "Test Description"
},
relationships: %{
"facilities" => %{},
"zone" => "Test Zone"
}
}
]
}
end)

predictions = Repo.all(route: "39")

assert Kernel.length(predictions) == 1
end

test "returns a list even if the server is down" do
expect(MBTA.Api.Mock, :get_json, fn _, _ ->
{:error, %HTTPoison.Error{reason: :econnrefused}}
Expand Down
Loading