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.

Sign up for GitHub

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

Jump to bottom

fix: 500 when getting vehicle with OL crowding data #663

Merged
merged 6 commits into from
Sep 1, 2023
Merged

fix: 500 when getting vehicle with OL crowding data #663

merged 6 commits into from
Sep 1, 2023

Conversation

Whoops
Copy link
Collaborator

@Whoops Whoops commented Aug 22, 2023 •
edited
Loading

Summary of changes

Asana Ticket: 👥🔮 Surface car-level crowding data through RTR

This fixes the 500 error when attempting to encode vehicles with carriages. It also adds carriages to the tests, and adds the remaining legal occupancy statuses.

@Whoops Whoops temporarily deployed to dev-green August 22, 2023 17:53 — with GitHub Actions Inactive
@github-actions
Copy link

Coverage of commit ac261fa

Summary coverage rate:
  lines......: 89.6% (4022 of 4489 lines)
  functions..: 70.6% (2204 of 3121 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/parse/lib/parse/vehicle_positions_json.ex                        |69.6%     23| 100%     7|    -      0

Download coverage report

@Whoops Whoops temporarily deployed to dev-green August 22, 2023 18:30 — with GitHub Actions Inactive
@Whoops Whoops temporarily deployed to dev-green August 22, 2023 18:59 — with GitHub Actions Inactive
@github-actions
Copy link

Coverage of commit 53051ad

Summary coverage rate:
  lines......: 89.6% (4029 of 4497 lines)
  functions..: 70.6% (2206 of 3123 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/views/vehicle_view.ex                        | 100%     18|81.6%    49|    -      0
  apps/model/lib/model/vehicle.ex                                       | 0.0%      3| 0.0%    10|    -      0
  apps/parse/lib/parse/vehicle_positions_json.ex                        |66.7%     24| 100%     7|    -      0

Download coverage report

@Whoops Whoops marked this pull request as ready for review August 22, 2023 19:08
@Whoops Whoops requested a review from a team August 22, 2023 19:09
paulswartz
paulswartz reviewed Aug 22, 2023
View reviewed changes
apps/api_web/lib/api_web/views/vehicle_view.ex Outdated Show resolved Hide resolved
apps/api_web/lib/api_web/views/vehicle_view.ex Outdated Show resolved Hide resolved
apps/api_web/lib/api_web/views/vehicle_view.ex Outdated
defp encode_carriage(carraige) do
%{
label: carraige.label,
carriage_sequence: carraige.carriage_sequence,
Copy link
Member

Choose a reason for hiding this comment

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

question:

  • if the order is already present given that this is an ordered list, does the carriage_sequence need to be included?
  • and if it does need to be included, couldn't the key be sequence since the item is already known to be a carriage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Normally I wouldn't want to change the API contact in a bugfix, but I suppose this is a bit of a special case, since this information wasn't available before, and wouldn't have worked if it was, so there shouldn't be any downstream consumers depending on the shape.

For my 2c, I'd probably leave carriage_sequence out entirely, since it's implied by the array and the API breaks from the GTFS spec in other ways (such as putting these on the carriage field). That said these fields to map directly to the corresponding GTFS fields, and carriage_sequence is actually the only field required in the spec. For context it looks like this was originally implemented in this ticket that calls for those fields.

I'll defer to you on the final shape. If you want it dropped it's easy enough to do.

Copy link
Member

Choose a reason for hiding this comment

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

Assuming that there's some other part of the system (maybe the consist/carriage parsing?) that's ensuring the list is in carriage_sequence order, I'd prefer to drop it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The GTFS spec requires they be in order, and RTR is generating the sequence numbers based on their position in the array so we should be covered there. The field has been removed.

apps/api_web/lib/api_web/views/vehicle_view.ex Outdated Show resolved Hide resolved
@github-actions
Copy link

Coverage of commit 26ef56d

Summary coverage rate:
  lines......: 89.6% (4030 of 4497 lines)
  functions..: 70.7% (2207 of 3123 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/views/vehicle_view.ex                        | 100%     18|83.7%    49|    -      0
  apps/model/lib/model/vehicle.ex                                       | 0.0%      3| 0.0%    10|    -      0
  apps/parse/lib/parse/vehicle_positions_json.ex                        |66.7%     24| 100%     7|    -      0

Download coverage report

@Whoops Whoops requested a review from paulswartz August 23, 2023 17:39
@github-actions
Copy link

Coverage of commit 16de1b4

Summary coverage rate:
  lines......: 89.6% (4028 of 4496 lines)
  functions..: 70.7% (2207 of 3123 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/vehicle_controller.ex            |89.1%     46| 100%    17|    -      0
  apps/api_web/lib/api_web/swagger_helpers.ex                           |98.9%     94| 100%    35|    -      0
  apps/api_web/lib/api_web/views/vehicle_view.ex                        | 100%     17|83.7%    49|    -      0
  apps/model/lib/model/vehicle.ex                                       | 0.0%      3| 0.0%    10|    -      0
  apps/parse/lib/parse/vehicle_positions_json.ex                        |66.7%     24| 100%     7|    -      0

Download coverage report

paulswartz
paulswartz approved these changes Aug 24, 2023
View reviewed changes
Copy link
Member

@paulswartz paulswartz left a comment

Choose a reason for hiding this comment

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

🍰 should we put this up on dev-green for more testing before merging?

@Whoops Whoops temporarily deployed to dev-green August 24, 2023 17:17 — with GitHub Actions Inactive
@Whoops
Copy link
Collaborator Author

Whoops commented Aug 24, 2023

🍰 should we put this up on dev-green for more testing before merging?

The initial version has been in dev-green for the last couple days. I've gone ahead and sent the new version. I don't think there's any particular rush for this to go out, just needs to happen before the RTR crowding changes land.

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

Coverage of commit b3cf3db

Summary coverage rate:
  lines......: 89.6% (4034 of 4501 lines)
  functions..: 70.7% (2211 of 3127 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/vehicle_controller.ex            |89.1%     46| 100%    17|    -      0
  apps/api_web/lib/api_web/swagger_helpers.ex                           |98.9%     94| 100%    35|    -      0
  apps/api_web/lib/api_web/views/vehicle_view.ex                        | 100%     17|83.7%    49|    -      0
  apps/model/lib/model/vehicle.ex                                       | 0.0%      3| 0.0%    10|    -      0
  apps/parse/lib/parse/vehicle_positions_json.ex                        |66.7%     24| 100%     7|    -      0

Download coverage report

@bklebe bklebe merged commit 61f9b45 into master Sep 1, 2023
7 checks passed
@bklebe bklebe deleted the ol-crowding-500 branch September 1, 2023 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulswartz paulswartz paulswartz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Whoops @paulswartz @bklebe