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

feat: Disruption Diagram - Backend #1828

Merged
merged 172 commits into from
Oct 20, 2023

Conversation

jzimbel-mbta
Copy link
Member

@jzimbel-mbta jzimbel-mbta commented Aug 1, 2023

Asana task: Write the serialization for the line map

The code is split into 5 modules.
I've ordered them below by how the logic generally flows, from public interface down to detailed implementation.

Module What's in it
DisruptionDiagram The public interface of all the disruption diagram code.
DisruptionDiagram.Model Contains high-level logic reflecting Betsy's Miro flowchart.
DisruptionDiagram.Validator Logic to check that we can build a diagram from this localized alert.
DisruptionDiagram.Builder Structure used to build the final diagram from a localized alert.
DisruptionDiagram.Label Logic to determine labels for omitted segments and destination-arrow end slots.

Thoroughly tested! I needed to add some helpers to make writing test scenarios more manageable—those are in test/support/. All tests are against DisruptionDiagram.serialize/1, since it's really the only public interface of all of this code.

  • Tests added?

Copy link
Contributor

@hannahpurcell hannahpurcell left a comment

Choose a reason for hiding this comment

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

The one file I haven't fully wrapped my head around is the builder, but I think I'll pick it up through applied work on it. Also @cmaddox5 is helping give it a secondary look, so as long as enough folks on our team can understand it and answer questions about it, then we should be good.

lib/screens/v2/disruption_diagram/builder.ex Show resolved Hide resolved
lib/screens/v2/disruption_diagram/builder.ex Outdated Show resolved Hide resolved
@mbta mbta deleted a comment from github-actions bot Oct 19, 2023
@mbta mbta deleted a comment from github-actions bot Oct 19, 2023
@mbta mbta deleted a comment from github-actions bot Oct 19, 2023
@mbta mbta deleted a comment from github-actions bot Oct 19, 2023
@mbta mbta deleted a comment from github-actions bot Oct 19, 2023
@github-actions
Copy link

Coverage of commit 9461d49

Summary coverage rate:
  lines......: 45.2% (2759 of 6098 lines)
  functions..: 44.1% (1121 of 2541 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/alerts/alert.ex                                              |25.0%    108|31.1%    45|    -      0
  lib/screens/alerts/informed_entity.ex                                    | 100%      2| 100%     2|    -      0
  lib/screens/alerts/parser.ex                                             |15.3%     59|66.7%    12|    -      0
  lib/screens/route_patterns/route_pattern.ex                              |35.3%     34|21.4%    14|    -      0
  lib/screens/stops/stop.ex                                                |39.8%     83|55.2%    29|    -      0
  lib/screens/v2/disruption_diagram.ex                                     | 100%      1| 100%     1|    -      0
  lib/screens/v2/disruption_diagram/builder.ex                             |95.0%    258|95.6%    45|    -      0
  lib/screens/v2/disruption_diagram/label.ex                               |97.4%     39| 100%     2|    -      0
  lib/screens/v2/disruption_diagram/model.ex                               |92.9%     42|92.3%    13|    -      0
  lib/screens/v2/disruption_diagram/validator.ex                           | 100%     20| 100%     6|    -      0
  lib/screens/v2/localized_alert.ex                                        |82.1%     67|92.3%    13|    -      0
  lib/screens/v2/location_context.ex                                       |66.7%      3|75.0%     4|    -      0
  lib/screens/v2/widget_instance/alert.ex                                  |76.5%     68|92.0%    25|    -      0
  lib/screens/v2/widget_instance/reconstructed_alert.ex                    |88.3%    274|97.1%    35|    -      0
  lib/screens/v2/widget_instance/subway_status.ex                          |92.0%    199|95.1%    41|    -      0
  test/support/disruption_diagram_localized_alert.ex                       |79.1%     43|87.5%     8|    -      0
  test/support/parent_station_id_sigil.ex                                  | 100%      3| 100%     1|    -      0
  test/support/subway_tagged_stop_sequences.ex                             |63.2%     19|61.5%    13|    -      0

Download coverage report

@mbta mbta deleted a comment from github-actions bot Oct 19, 2023
@jzimbel-mbta
Copy link
Member Author

The Back Bay DUP is not loving a change to how its route_pattern logic for alerts is working

This should be fixed now, by this commit. My code previously defaulted to passing filter[canonical]=false, which to my surprise limits the results to only non-canonical route patterns. Definitely not what we want.

Copy link
Contributor

@hannahpurcell hannahpurcell left a comment

Choose a reason for hiding this comment

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

Great! Gave a thumbs-up on two things you suggested. Once those have been applied, I think we're ready to go. Let's merge it and start bug-bashing!! 🐛

@jzimbel-mbta
Copy link
Member Author

jzimbel-mbta commented Oct 20, 2023

I'm not sure why CI stopped running on my commits, but I manually checked and all unit tests are passing, plus Credo and Dialyzer have no complaints. Merging!

edit: Oop nvm, CI is running but the "summary box" at the bottom of the page doesn't say it is. 🤔
I'll give it a few minutes to complete.

@github-actions
Copy link

Coverage of commit 2684019

Summary coverage rate:
  lines......: 45.3% (2761 of 6100 lines)
  functions..: 44.1% (1121 of 2541 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/alerts/alert.ex                                              |25.0%    108|31.1%    45|    -      0
  lib/screens/alerts/informed_entity.ex                                    | 100%      2| 100%     2|    -      0
  lib/screens/alerts/parser.ex                                             |15.3%     59|66.7%    12|    -      0
  lib/screens/route_patterns/route_pattern.ex                              |35.3%     34|21.4%    14|    -      0
  lib/screens/stops/stop.ex                                                |39.8%     83|55.2%    29|    -      0
  lib/screens/v2/disruption_diagram.ex                                     | 100%      1| 100%     1|    -      0
  lib/screens/v2/disruption_diagram/builder.ex                             |95.0%    260|95.6%    45|    -      0
  lib/screens/v2/disruption_diagram/label.ex                               |97.4%     39| 100%     2|    -      0
  lib/screens/v2/disruption_diagram/model.ex                               |92.9%     42|92.3%    13|    -      0
  lib/screens/v2/disruption_diagram/validator.ex                           | 100%     20| 100%     6|    -      0
  lib/screens/v2/localized_alert.ex                                        |82.1%     67|92.3%    13|    -      0
  lib/screens/v2/location_context.ex                                       |66.7%      3|75.0%     4|    -      0
  lib/screens/v2/widget_instance/alert.ex                                  |76.5%     68|92.0%    25|    -      0
  lib/screens/v2/widget_instance/reconstructed_alert.ex                    |88.3%    274|97.1%    35|    -      0
  lib/screens/v2/widget_instance/subway_status.ex                          |92.0%    199|95.1%    41|    -      0
  test/support/disruption_diagram_localized_alert.ex                       |79.1%     43|87.5%     8|    -      0
  test/support/parent_station_id_sigil.ex                                  | 100%      3| 100%     1|    -      0
  test/support/subway_tagged_stop_sequences.ex                             |63.2%     19|61.5%    13|    -      0

Download coverage report

@jzimbel-mbta jzimbel-mbta merged commit 0fbad15 into prefare-alerts Oct 20, 2023
2 checks passed
@jzimbel-mbta jzimbel-mbta deleted the jz/disruption-diagram-backend branch October 20, 2023 18:53
hannahpurcell added a commit that referenced this pull request Feb 2, 2024
* feat: Pre-Fare alerts 2.0 CandidateGenerator (#1765)

* [feature] Pre-fare alert 2.0 frontend left screen (#1787)

* feat: Pre-Fare alerts 2.0 serializer (#1785)

* feat: Disruption Diagram - Frontend (#1825)

* [tweak] Disruption diagram maths (#1830)

* In LocationContext, tag stop sequences by their routes (#1832)

* tweak: PreFare simulations (#1837)

* [feature] Prefare alerts 2.0 audio (#1824)

* feat: Disruption Diagram - Backend (#1828)

* feat(WIP): Disruption diagram backend. Working for Blue Line and other basic scenarios only atm.

* fix: Update location context stop sequences field in TrainCrowdingTest (#1895)

* test: D. diagram unit test for RL trunk statio closure with home stop at Ashmont (#1894)

* Added padding-top to layout when there is no banner. (#1902)

* Cm/govt ctr gl affected pill fix (#1898)

* Cm/adjust diagram size positioning (#1896)

* Changed label to display on a single line. (#1899)

* Tweaked resizer logic so it maxHeight matches the card height on page. (#1905)

* Removed bolding of 'to' from arrow labels. (#1908)

* Fixed icon display when current stop is affected. (#1903)

* feat: Consistent description of disrupted stop ranges throughout pre-fare alerts (#1912)

* Adjusted Ashmont/Braintree destination to proper formatting (#1917)

* Refactor disruption diagram SVG (#1914)

* Log alert ID and screen's home stop when disruption diagram logic fails (#1920)

* polish: Recalculate dimensions in hook (#1921)

* Added audio column to GL & PreFare in admin table (#1916)

* Draw D. diagrams for multi-line alerts when home stop narrows it to 1 line (#1922)

* fix: PreFare Polish - Inside shuttle (#1941)

* fix: Pre-fare polish - Boundary alerts banner (#1945)

* fix: Pre-fare polish -- Text only resizes for bypassed stations (#1946)

* fix: Pre-fare polish -- Station closed takeover text wrapping (#1947)

* Downstream shuttle endpoint should be a circle (#1948)

* Label splitting logic was buggy (#1949)

* Prefare format in config is "Ashmont & Braintree" not "Ashmont/Braintree" (#1954)

* Sub-header for takeover alert is now regular weight (#1955)

* fix: Prefare alerts polish -- Fix GL headsign in various cases (#1957)

* fix: Prefare polish -- Use FreeText to prevent station text wrapping in subheaders (#1956)

* fix: Prefare polish -- Alerts that break assumptions use the fallback case (#1958)

* fix: Prefare polish -- Reorder audio with alerts before subway status (#1962)

* fix: Prefare polish -- New abbreviation rule (#1961)

* fix: Prefare Alerts polish -- RL headsigns for branch-only alerts (#1967)

* fix: Prefare alerts polish -- Screenplay sim sizing / page layout (#1966)

* fix: Prefare polish -- "via" diagram labels need abbreviating (#1968)

---------

Co-authored-by: Christian Maddox <[email protected]>
Co-authored-by: Jon Zimbel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants