fix(Schedules): Fix alerts not showing up for stops with inbound trains #2029
Conversation
lib/predictions/repo.ex
Outdated
| |> 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) |
There was a problem hiding this comment.
Instead of passing nil here you can just make it a default argument.
test/predictions/repo_test.exs
Outdated
| @@ -2,6 +2,7 @@ defmodule Predictions.RepoTest do | |||
| use ExUnit.Case, async: false | |||
| @moduletag :external | |||
|
|
|||
| import Mock | |||
There was a problem hiding this comment.
We are removing Mock and replacing it with Mox. So, we shouldn't be adding new Mock instances to tests. We especially shouldn't use both in the same test.
There was a problem hiding this comment.
I am aware, please see my comment in slack
test/predictions/repo_test.exs
Outdated
| @@ -58,6 +59,67 @@ defmodule Predictions.RepoTest do | |||
| end | |||
| end | |||
|
|
|||
| test "filtes out predictions with no departure" do | |||
test/predictions/repo_test.exs
Outdated
| %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"} | ||
| ] | ||
| } | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
test/predictions/repo_test.exs
Outdated
| {Stops.Repo, [:passthrough], get_parent: fn _ -> %Stops.Stop{id: "Parent"} end}, | ||
| {Routes.Repo, [:passthrough], get: fn _ -> Routes.MockRepoApi.get("Red") end} | ||
| ]) do | ||
| predictions = Repo.all(route: "39") |
There was a problem hiding this comment.
Is this a magic string? What happens if we do Repo.all(route: "38")?
There was a problem hiding this comment.
Nope, its just a random number
test/predictions/repo_test.exs
Outdated
| five_minutes_in_future = DateTime.add(Timex.now(), 5, :minute) | ||
|
|
||
| {:ok, five_minutes_in_future_string} = | ||
| Timex.format(five_minutes_in_future, "{ISO:Extended:Z}") | ||
|
|
There was a problem hiding this comment.
Since you don't need the :ok and an error would just cause the test to fail you can do:
departure_time =
Timex.now()
|> DateTime.add(5, :minute)
|> Timex.format!("{ISO:Extended:Z}")
test/predictions/repo_test.exs
Outdated
| %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"} | ||
| ] | ||
| } | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
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.
test/predictions/repo_test.exs
Outdated
| "type" => "1", | ||
| "long_name" => "Red Test Subway", | ||
| "direction_names" => ["North Test Name", "South Test Name"], | ||
| "direction_destinations" => ["North Test", "South Test"] |
There was a problem hiding this comment.
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).
test/predictions/repo_test.exs
Outdated
| @@ -58,6 +58,102 @@ defmodule Predictions.RepoTest do | |||
| end | |||
| end | |||
|
|
|||
| test "filtes out predictions with no departure" do | |||
There was a problem hiding this comment.
This typo is still here.
lib/predictions/repo.ex
Outdated
| |> 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) |
There was a problem hiding this comment.
Using a default argument of nil rather than passing in nil makes both the function definition and it's application much clearer and cleaner.
|
If we're going to have this test in there then we should remove the |
Summary of changes
This fixes the start point of a shuttle disruption from showing predictions and showing the effect title instead
https://app.asana.com/0/home/1201123880594717/1206362494901588
General checks
New UI, or substantial UI changes
New endpoints, or non-trivial changes to current endpoints