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

tweak: Train Crowding Widget Logging #1839

Merged
merged 18 commits into from
Sep 8, 2023
Merged
Show file tree
Hide file tree
Changes from 12 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
3 changes: 2 additions & 1 deletion lib/screens/v2/candidate_generator.ex
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ defmodule Screens.V2.CandidateGenerator do
Fetches data and returns a list of candidate widget instances to be
considered for placement on the template.
"""
@callback candidate_instances(ScreenData.config()) :: ScreenData.candidate_instances()
@callback candidate_instances(ScreenData.config(), keyword()) ::
ScreenData.candidate_instances()

@doc """
Receives the finalized list of widget instances that were placed on
Expand Down
1 change: 1 addition & 0 deletions lib/screens/v2/candidate_generator/bus_eink.ex
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ defmodule Screens.V2.CandidateGenerator.BusEink do
# credo:disable-for-next-line Credo.Check.Design.DuplicatedCode
def candidate_instances(
config,
_opts,
now \\ DateTime.utc_now(),
fetch_stop_name_fn \\ &Stop.fetch_stop_name/1,
departures_instances_fn \\ &Widgets.Departures.departures_instances/1,
Expand Down
1 change: 1 addition & 0 deletions lib/screens/v2/candidate_generator/bus_shelter.ex
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ defmodule Screens.V2.CandidateGenerator.BusShelter do
# credo:disable-for-next-line Credo.Check.Design.DuplicatedCode
def candidate_instances(
config,
_opts,
now \\ DateTime.utc_now(),
fetch_stop_name_fn \\ &Stop.fetch_stop_name/1,
departures_instances_fn \\ &Widgets.Departures.departures_instances/1,
Expand Down
1 change: 1 addition & 0 deletions lib/screens/v2/candidate_generator/dup.ex
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ defmodule Screens.V2.CandidateGenerator.Dup do
@impl CandidateGenerator
def candidate_instances(
config,
_opts,
now \\ DateTime.utc_now(),
fetch_stop_name_fn \\ &Stop.fetch_stop_name/1,
evergreen_content_instances_fn \\ &Widgets.Evergreen.evergreen_content_instances/1,
Expand Down
1 change: 1 addition & 0 deletions lib/screens/v2/candidate_generator/gl_eink.ex
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ defmodule Screens.V2.CandidateGenerator.GlEink do
@impl CandidateGenerator
def candidate_instances(
config,
_opts,
now \\ DateTime.utc_now(),
fetch_destination_fn \\ &fetch_destination/2,
departures_instances_fn \\ &Widgets.Departures.departures_instances/3,
Expand Down
1 change: 1 addition & 0 deletions lib/screens/v2/candidate_generator/pre_fare.ex
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ defmodule Screens.V2.CandidateGenerator.PreFare do
# credo:disable-for-next-line
def candidate_instances(
config,
_opts,
now \\ DateTime.utc_now(),
subway_status_instance_fn \\ &Widgets.SubwayStatus.subway_status_instances/2,
reconstructed_alert_instances_fn \\ &Widgets.ReconstructedAlert.reconstructed_alert_instances/1,
Expand Down
1 change: 1 addition & 0 deletions lib/screens/v2/candidate_generator/solari.ex
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ defmodule Screens.V2.CandidateGenerator.Solari do
# credo:disable-for-next-line Credo.Check.Design.DuplicatedCode
def candidate_instances(
config,
_opts,
now \\ DateTime.utc_now(),
departures_instances_fn \\ &Widgets.Departures.departures_instances/1
) do
Expand Down
1 change: 1 addition & 0 deletions lib/screens/v2/candidate_generator/solari_large.ex
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ defmodule Screens.V2.CandidateGenerator.SolariLarge do
# credo:disable-for-next-line Credo.Check.Design.DuplicatedCode
def candidate_instances(
config,
_opts,
now \\ DateTime.utc_now(),
departures_instances_fn \\ &Widgets.Departures.departures_instances/1
) do
Expand Down
5 changes: 3 additions & 2 deletions lib/screens/v2/candidate_generator/triptych.ex
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,13 @@ defmodule Screens.V2.CandidateGenerator.Triptych do
@impl CandidateGenerator
def candidate_instances(
config,
crowding_widget_instances_fn \\ &Widgets.TrainCrowding.crowding_widget_instances/1,
opts,
crowding_widget_instances_fn \\ &Widgets.TrainCrowding.crowding_widget_instances/2,
evergreen_content_instances_fn \\ &Widgets.Evergreen.evergreen_content_instances/1,
local_evergreen_set_instances_fn \\ &Widgets.LocalEvergreenSet.local_evergreen_set_instances/1
) do
[
fn -> crowding_widget_instances_fn.(config) end,
fn -> crowding_widget_instances_fn.(config, opts) end,
Copy link
Contributor

Choose a reason for hiding this comment

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

From our huddle conversation, I suggested this so the train_crowding widget only has the stuff it needs

Suggested change
fn -> crowding_widget_instances_fn.(config, opts) end,
fn -> crowding_widget_instances_fn.(config, opts[:logging_options]) end,

Copy link
Contributor

Choose a reason for hiding this comment

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

And then the associated changes are needed in the train_crowding candidate_generator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I see. I misunderstood. Will make that change now.

fn -> evergreen_content_instances_fn.(config) end,
fn -> local_evergreen_set_instances_fn.(config) end
]
Expand Down
34 changes: 33 additions & 1 deletion lib/screens/v2/candidate_generator/widgets/train_crowding.ex
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
defmodule Screens.V2.CandidateGenerator.Widgets.TrainCrowding do
@moduledoc false

require Logger

alias Screens.Alerts.Alert
alias Screens.Config.Screen
alias Screens.Config.V2.{TrainCrowding, Triptych}
Expand All @@ -9,9 +11,10 @@ defmodule Screens.V2.CandidateGenerator.Widgets.TrainCrowding do
alias Screens.V2.LocalizedAlert
alias Screens.V2.WidgetInstance.TrainCrowding, as: CrowdingWidget

@spec crowding_widget_instances(Screen.t()) :: list(CrowdingWidget.t())
@spec crowding_widget_instances(Screen.t(), keyword()) :: list(CrowdingWidget.t())
def crowding_widget_instances(
config,
opts,
now \\ DateTime.utc_now(),
fetch_predictions_fn \\ &Prediction.fetch/1,
fetch_location_context_fn \\ &Stop.fetch_location_context/3,
Expand All @@ -25,13 +28,15 @@ defmodule Screens.V2.CandidateGenerator.Widgets.TrainCrowding do
_,
_,
_,
_,
_
) do
[]
end

def crowding_widget_instances(
%Screen{app_params: %Triptych{train_crowding: train_crowding}} = config,
opts,
now,
fetch_predictions_fn,
fetch_location_context_fn,
Expand All @@ -51,6 +56,12 @@ defmodule Screens.V2.CandidateGenerator.Widgets.TrainCrowding do
params |> Map.to_list() |> fetch_alerts_fn.() do
next_train_prediction = List.first(predictions)

if opts[:is_real_screen] do
hannahpurcell marked this conversation as resolved.
Show resolved Hide resolved
Logger.info(
"[train_crowding next_prediction] screen_id=#{opts[:screen_id]} triptych_pane=#{opts[:triptych_pane]} station_id=#{train_crowding.station_id} direction_id=#{train_crowding.direction_id} next_prediction_id=#{next_train_prediction.id} next_trip_id=#{next_train_prediction.trip.id}"
)
end

# If there is an upcoming train, it's headed to this station, and we're not at a temporary terminal,
# show the widget
if not is_nil(next_train_prediction) and
Expand All @@ -59,6 +70,14 @@ defmodule Screens.V2.CandidateGenerator.Widgets.TrainCrowding do
train_crowding.station_id and
next_train_prediction.vehicle.carriages != [] and
not any_alert_makes_this_a_terminal?(alerts, location_context) do
log_crowding_info(
next_train_prediction,
train_crowding,
opts[:is_real_screen],
opts[:screen_id],
opts[:triptych_pane]
)

[
%CrowdingWidget{
screen: config,
Expand Down Expand Up @@ -88,4 +107,17 @@ defmodule Screens.V2.CandidateGenerator.Widgets.TrainCrowding do
localized_alert.alert.effect in [:suspension, :shuttle] and
LocalizedAlert.location(localized_alert) in [:boundary_downstream, :boundary_upstream]
end

defp log_crowding_info(prediction, crowding_config, true, screen_id, triptych_pane) do
crowding_levels =
prediction.vehicle.carriages
|> Enum.sort_by(& &1.car_number)
hannahpurcell marked this conversation as resolved.
Show resolved Hide resolved
|> Enum.map_join(",", & &1.occupancy_status)

Logger.info(
"[train_crowding car_crowding_info] screen_id=#{screen_id} triptych_pane=#{triptych_pane} station_id=#{crowding_config.station_id} direction_id=#{crowding_config.direction_id} trip_id=#{prediction.trip.id} prediction_id=#{prediction.id} vehicle_id=#{prediction.vehicle.id} car_crowding_levels=#{crowding_levels}"
hannahpurcell marked this conversation as resolved.
Show resolved Hide resolved
)
end

defp log_crowding_info(_, _, _, _, _), do: :ok
end
6 changes: 3 additions & 3 deletions lib/screens/v2/screen_data.ex
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ defmodule Screens.V2.ScreenData do

layout_and_widgets =
config
|> fetch_data()
|> fetch_data(opts)
|> tap(&cache_visible_alert_widgets(&1, screen_id, config.hidden_from_screenplay))
|> resolve_paging(refresh_rate)

Expand All @@ -64,13 +64,13 @@ defmodule Screens.V2.ScreenData do
end

@spec fetch_data(Screens.Config.Screen.t()) :: {Template.layout(), selected_instances_map()}
def fetch_data(config) do
def fetch_data(config, opts \\ []) do
candidate_generator = Parameters.get_candidate_generator(config)
screen_template = candidate_generator.screen_template()

candidate_instances =
config
|> candidate_generator.candidate_instances()
|> candidate_generator.candidate_instances(opts)
|> Enum.filter(&WidgetInstance.valid_candidate?/1)

pick_instances(screen_template, candidate_instances)
Expand Down
2 changes: 1 addition & 1 deletion lib/screens/v2/widget_instance/train_crowding.ex
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ defmodule Screens.V2.WidgetInstance.TrainCrowding do
defp serialize_carriages(nil), do: nil

defp serialize_carriages(carriages),
do: Enum.map(carriages, fn car -> serialize_occupancy_status(car) end)
do: Enum.map(carriages, fn car -> serialize_occupancy_status(car.occupancy_status) end)

defp serialize_occupancy_status(:no_data_available), do: :no_data
defp serialize_occupancy_status(:many_seats_available), do: :not_crowded
Expand Down
21 changes: 21 additions & 0 deletions lib/screens/vehicles/carriage.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
defmodule Screens.Vehicles.Carriage do
@moduledoc false

defstruct car_number: nil,
occupancy_status: nil

@type occupancy_status ::
:many_seats_available
| :few_seats_available
| :standing_room_only
| :crushed_standing_room_only
| :full
| :no_data_available
| :not_accepting_passengers
| nil

@type t :: %__MODULE__{
car_number: String.t(),
occupancy_status: occupancy_status | nil
}
end
6 changes: 5 additions & 1 deletion lib/screens/vehicles/parser.ex
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,13 @@ defmodule Screens.Vehicles.Parser do
defp parse_carriages(data), do: Enum.map(data, &parse_car_crowding/1)

defp parse_car_crowding(%{
"label" => car_number,
"occupancy_status" => occupancy_status
}),
do: parse_occupancy_status(occupancy_status)
do: %Screens.Vehicles.Carriage{
car_number: car_number,
occupancy_status: parse_occupancy_status(occupancy_status)
}

defp trip_id_from_trip_data(%{"data" => %{"id" => trip_id}}), do: trip_id
defp trip_id_from_trip_data(_), do: nil
Expand Down
4 changes: 3 additions & 1 deletion lib/screens/vehicles/vehicle.ex
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
defmodule Screens.Vehicles.Vehicle do
@moduledoc false

alias Screens.Vehicles.Carriage

defstruct id: nil,
direction_id: nil,
current_status: nil,
Expand Down Expand Up @@ -29,7 +31,7 @@ defmodule Screens.Vehicles.Vehicle do
stop_id: Screens.Stops.Stop.id() | nil,
parent_stop_id: Screens.Stops.Stop.id() | nil,
occupancy_status: occupancy_status,
carriages: list(occupancy_status) | nil
carriages: list(Carriage.t())
}

def by_route_and_direction(route_id, direction_id) do
Expand Down
9 changes: 8 additions & 1 deletion lib/screens_web/controllers/v2/screen_api_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,14 @@ defmodule ScreensWeb.V2.ScreenApiController do
screen_side
)

json(conn, ScreenData.by_screen_id(screen_id))
json(
conn,
ScreenData.by_screen_id(screen_id,
is_real_screen: is_screen,
screen_id: screen_id,
triptych_pane: triptych_pane
Copy link
Contributor

Choose a reason for hiding this comment

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

Initially, I was worried that we're passing the triptych_pane for all screen cases, including non-triptychs. I think that means by the time fetch_data is run in ScreenData, all screens except triptych will have the option triptych_pane: nil. Fortunately, right now, the only candidate_generator that pays attention to opts is the triptych one. (But let's say one day we want to use this opts in the candidate_generator for another screen type, that screen will suddenly have this triptych_pane option out of the blue.) I'm thinking it might be better if this option is passed conditionally? I dunno, does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think one of the advantages to using an opts list is that they can be arbitrary in length and a function doing a lookup in opts only needs to look for what they know they need. Similar to params in this action. This action is used by all screen types but references very specific values only needed by certain screen types. Because a lookup of key that doesn't exist is safe for these lists, we just need to do some nil checks before using them.

I definitely see where you are coming from though. My biggest hesitation with conditionally adding it is that it will make the code here a little more complex for no drastic benefit. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I can see that. Works for me!

)
)
end
end

Expand Down
2 changes: 2 additions & 0 deletions test/screens/v2/candidate_generator/bus_eink_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,12 @@ defmodule Screens.V2.CandidateGenerator.BusEinkTest do
now = ~U[2020-04-06T10:00:00Z]
evergreen_content_instances_fn = fn _ -> [] end
subway_status_instances_fn = fn _, _ -> [] end
opts = []

actual_instances =
BusEink.candidate_instances(
config,
opts,
now,
fetch_stop_fn,
departures_instances_fn,
Expand Down
4 changes: 4 additions & 0 deletions test/screens/v2/candidate_generator/bus_shelter_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ defmodule Screens.V2.CandidateGenerator.BusShelterTest do
now = ~U[2020-04-06T10:00:00Z]
evergreen_content_instances_fn = fn _ -> [] end
subway_status_instances_fn = fn _, _ -> [] end
opts = []

expected_header = %NormalHeader{
screen: config,
Expand All @@ -93,6 +94,7 @@ defmodule Screens.V2.CandidateGenerator.BusShelterTest do
actual_instances =
BusShelter.candidate_instances(
config,
opts,
now,
fetch_stop_fn,
departures_instances_fn,
Expand All @@ -115,6 +117,7 @@ defmodule Screens.V2.CandidateGenerator.BusShelterTest do
now = ~U[2020-04-06T10:00:00Z]
evergreen_content_instances_fn = fn _ -> [] end
subway_status_instances_fn = fn _, _ -> [] end
opts = []

expected_header = %NormalHeader{
screen: config,
Expand All @@ -126,6 +129,7 @@ defmodule Screens.V2.CandidateGenerator.BusShelterTest do
actual_instances =
BusShelter.candidate_instances(
config,
opts,
now,
fetch_stop_fn,
departures_instances_fn,
Expand Down
2 changes: 2 additions & 0 deletions test/screens/v2/candidate_generator/dup_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ defmodule Screens.V2.CandidateGenerator.DupTest do
departures_instances_fn = fn _, _ -> [] end
evergreen_content_instances_fn = fn _ -> [] end
alerts_instances_fn = fn _, _ -> [] end
opts = []

expected_headers =
List.duplicate(
Expand All @@ -105,6 +106,7 @@ defmodule Screens.V2.CandidateGenerator.DupTest do
actual_instances =
Dup.candidate_instances(
config,
opts,
now,
fetch_stop_fn,
evergreen_content_instances_fn,
Expand Down
4 changes: 3 additions & 1 deletion test/screens/v2/candidate_generator/solari_large_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ defmodule Screens.V2.CandidateGenerator.SolariLargeTest do
test "returns expected header", %{config: config} do
departures_instances_fn = fn _ -> [] end
now = ~U[2020-04-06T10:00:00Z]
opts = []

expected_header = %NormalHeader{
screen: config,
Expand All @@ -42,7 +43,8 @@ defmodule Screens.V2.CandidateGenerator.SolariLargeTest do
time: ~U[2020-04-06T10:00:00Z]
}

actual_instances = SolariLarge.candidate_instances(config, now, departures_instances_fn)
actual_instances =
SolariLarge.candidate_instances(config, opts, now, departures_instances_fn)

assert expected_header in actual_instances
end
Expand Down
3 changes: 2 additions & 1 deletion test/screens/v2/candidate_generator/solari_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ defmodule Screens.V2.CandidateGenerator.SolariTest do
test "returns expected header", %{config: config} do
departures_instances_fn = fn _ -> [] end
now = ~U[2020-04-06T10:00:00Z]
opts = []

expected_header = %NormalHeader{
screen: config,
Expand All @@ -42,7 +43,7 @@ defmodule Screens.V2.CandidateGenerator.SolariTest do
time: ~U[2020-04-06T10:00:00Z]
}

actual_instances = Solari.candidate_instances(config, now, departures_instances_fn)
actual_instances = Solari.candidate_instances(config, opts, now, departures_instances_fn)

assert expected_header in actual_instances
end
Expand Down
Loading