tweak: PreFare simulations

[cmaddox5]

Asana task: [Pre-Fare Alerts] "New look" in Screenplay

Now that there are two different "flex zones" on PreFare screens, a little tweaking was needed so users can see all pages on both screens. To make things simpler, PreFare has its own set of simulation components and CSS. A single server change was needed so grouping logic did not incorrectly group pages with the same page number of different screens.

• Tests added?

[github-actions]

Coverage of commit f2ca8ae

Summary coverage rate:
  lines......: 45.8% (2760 of 6029 lines)
  functions..: 44.4% (1121 of 2526 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/v2/screen_data.ex                                            |61.7%    133|55.2%    29|    -      0

Download coverage report

[github-actions]

Coverage of commit f2ca8ae

Summary coverage rate:
  lines......: 42.1% (2354 of 5592 lines)
  functions..: 42.5% (1034 of 2433 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/v2/screen_data.ex                                            |61.7%    133|55.2%    29|    -      0

Download coverage report

[jzimbel-mbta]

Looks good! Just some thoughts and suggestions.

I do wonder how we would generalize the logic that divides paged content into its separate containers, but any approach that works for the narrow case of pre-fares is good enough for now.

[jzimbel-mbta]

Not sure if it's clearer, but the [] access syntax returns nil for missing map keys:

iex> map = %{a: 1}
iex> map[:q]
nil
iex> map.q
** (KeyError) key :q not found in: %{a: 1}

which, since nil is falsy, allows you do this check like

Suggested change

	if Map.has_key?(instance, :is_full_screen) and instance.is_full_screen do
	if instance[:is_full_screen] do

[cmaddox5]

O yeah, I like that much better. Thanks!

[jzimbel-mbta]

Separately: it feels a little janky to distinguish paged stuff on the left screen from that in the right screen flex-zone, by a key that happens to be on the one type of widget that can currently occupy that space.

Is there any other way to distinguish them?

If I'm following the code correctly, paged_slot_id should look something like {0, :paged_main_content_left} for a widget occupying one of the paged left screen slots. Checking for that slot ID would be a bit better, since it's independent of whatever widget is going in the slot.

So maybe something like:

|> Enum.group_by(
  fn
    {{_n, :paged_main_content_left} -> :paged_main_content_left
    {paged_slot_id, _} -> Template.get_page(paged_slot_id)
  end,
  fn {paged_slot_id, instance} -> {Template.unpage(paged_slot_id), instance} end
)

This still isn't ideal since it's specific to pre-fares, but maybe a bit better? 🤷

[cmaddox5]

Yeah this is a tricky situation and definitely the what I spent the most time figuring out. This problem only exists because we get data for both screens at the same time (or if we ever have two flexzones on one screen for other apps). Because of this, I don't see any screen besides PreFare having this issue unless we add a flexzone to another screen.

Would it be cleaner to leverage app_id in this? Maybe bring that in as a parameter, refactor the key grouping anon function into a named function, and use a guard. What do you think?

[jzimbel-mbta]

I think yeah, it might be best to just have special behavior for the pre-fare app. A separate named function leaves room for defining clauses for for other apps later, if need be, and separates the app-specific stuff from the common logic a little bit better.

[jzimbel-mbta]

tl;dr Yes, a named paged_content_key (or some better name) function that also takes app ID as an argument, assuming that's easy to access from this function, sounds good.

[cmaddox5]

Got that added. Mind taking one more look?

[github-actions]

Coverage of commit 88ded56

Summary coverage rate:
  lines......: 42.1% (2354 of 5592 lines)
  functions..: 42.5% (1034 of 2433 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/v2/screen_data.ex                                            |61.7%    133|55.2%    29|    -      0

Download coverage report

[github-actions]

Coverage of commit feba683

Summary coverage rate:
  lines......: 42.1% (2354 of 5592 lines)
  functions..: 42.5% (1034 of 2434 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/v2/screen_data.ex                                            |61.7%    133|53.3%    30|    -      0

Download coverage report

[jzimbel-mbta]

Double approved ✅✅