Add available types and spaces to `VehicleRentalStation` by cedarbaum · Pull Request #5425 · opentripplanner/OpenTripPlanner

cedarbaum · 2023-10-12T19:53:07Z

Summary

This PR adds vehicleTypesAvailable and vehicleSpacesAvailable to the VehicleRentalStation type. These fields return the count of each vehicle type available for pickup and drop-off, respectively. As an example, this is what the new fields look like in the vehicleRentalStation query:

...
    "vehicleRentalStation": {
      "stationId": "NYC:cc7b86cf-a2fb-4738-b967-de7b197dc930",
      "name": "Ave A & E 14 St",
      "vehiclesAvailable": 2,
      "vehicleTypesAvailable": [
        {
          "vehicleType": {
            "formFactor": "BICYCLE",
            "propulsionType": "HUMAN"
          },
          "count": 0
        },
        {
          "vehicleType": {
            "formFactor": "BICYCLE",
            "propulsionType": "ELECTRIC_ASSIST"
          },
          "count": 2
        }
      ],
...

Issue

N/A

Unit tests

GraphQL integration test added that runs the vehicleRentalStation query with the new fields
Manual testing done with the interactive GraphQL explorer

Documentation

Documentation added for new GraphQL fields

codecov · 2023-10-12T20:28:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fc29339) 66.67% compared to head (8575360) 66.87%.
Report is 101 commits behind head on dev-2.x.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5425      +/-   ##
=============================================
+ Coverage      66.67%   66.87%   +0.19%     
- Complexity     15396    15495      +99     
=============================================
  Files           1794     1800       +6     
  Lines          69682    69793     +111     
  Branches        7345     7353       +8     
=============================================
+ Hits           46461    46672     +211     
+ Misses         20757    20662      -95     
+ Partials        2464     2459       -5

Files	Coverage Δ
...is/gtfs/datafetchers/VehicleRentalStationImpl.java	`91.30% <100.00%> (+5.59%)`	⬆️
...anner/apis/gtfs/generated/GraphQLDataFetchers.java	`0.00% <ø> (ø)`
...vehiclerental/model/RentalVehicleEntityCounts.java	`100.00% <100.00%> (ø)`
...ce/vehiclerental/model/RentalVehicleTypeCount.java	`100.00% <100.00%> (ø)`
...vice/vehiclerental/model/VehicleRentalStation.java	`96.82% <100.00%> (+2.38%)`	⬆️

... and 49 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

leonardehrenfried

For a very first PR this is a really good job!

I've left a few comments about things that I'd like you to change.

The failing test will be fixed if you add a mapping for VehicleTypeCount in the codegen config:

OpenTripPlanner/src/main/java/org/opentripplanner/apis/gtfs/generated/graphql-codegen.yml

Line 40 in 153a508

    
           VehicleRentalStation: org.opentripplanner.service.vehiclerental.model.VehicleRentalStation#VehicleRentalStation

We will be doing some thinking over the next few days if the want to keep the API schema or will ask you to change it.

leonardehrenfried · 2023-10-13T08:36:17Z

@@ -0,0 +1,37 @@
+{


Well done for finding out how to use these tests.

We are now adding tests for this in two different prs, since this is a more complete version, lets try to get this merged first and I'll update #5411 afterwards.

leonardehrenfried · 2023-10-13T08:37:47Z

+    """
+    Number of vehicles currently available on the rental station, grouped by vehicle type.
+    """
+    vehicleTypesAvailable: [RentalVehicleTypeCount!]
+
+    """
+    Number of free spaces currently available on the rental station, grouped by vehicle type.
+    """
+    vehicleSpacesAvailable: [RentalVehicleTypeCount!]
+


I think this is a good way of expressing the information but we will discuss this in one of our meeting if we can do better. (We leave ourselves lots of time to think about API schemas because once they are released it's almost impossible to change them later.)

I think I would model this something like this (names are up for discussion):

leonardehrenfried · 2023-10-13T08:45:05Z

Also please merge or rebase onto dev-2.x so I can get an accurate view of the code coverage.

leonardehrenfried · 2023-10-16T12:42:14Z

+  public List<RentalVehicleTypeCount> getVehicleTypeCounts() {
+    return vehicleRentalTypeMapToList(vehicleTypesAvailable);
+  }
+
+  public List<RentalVehicleTypeCount> getVehicleSpaceCounts() {
+    return vehicleRentalTypeMapToList(vehicleSpacesAvailable);
+  }


This file is not a good example but we would like to see Javadoc for all public methods.

leonardehrenfried · 2023-10-16T12:45:37Z

+    vehicleTypesAvailable: [RentalVehicleTypeCount!]
+
+    """
+    Number of free spaces currently available on the rental station, grouped by vehicle type.
+    """
+    vehicleSpacesAvailable: [RentalVehicleTypeCount!]


Just thinking out aloud: whenever I read a repeated word in the schema I wonder if these two properties should be grouped together. Perhaps something like this:

availability { types: VehicleTypeCount spaces: VehicleTypeCount }

cc @optionsome @t2gran

(Please don't do anything yet, @cedarbaum, as this thought is not fully formed yet. We will discuss in a dev meeting.)

@optionsome and I had a chat what we want the schema to look like and we have landed on the following (pseudo-graphqls language):

availableVehicles { total: Int byType: [VehicleTypeCount!]! } availableSpaces { total: Int byType: [VehicleTypeCount!]! }

We also want to deprecate the spacesAvailable and vehiclesAvailable fields so it's not too confusing for the consumers.

Does this make sense, @cedarbaum ?

This makes sense, I went ahead and updated the PR with the new query structure. I did keep the Rental prefix for the GraphQL types to be consistent with other existing types, but let me know if you have a preference for these type names (RentalVehicleEntityCounts and RentalVehicleTypeCount, specifically).

I also noticed that @t2gran proposed an alternative query structure. Is there consensus with the maintainers about the final design or is it still being discussed? If it's still being discussed and it would be helpful for me to attend a future dev meeting, I am happy to do so!

Well I thought there was a consensus until there wasn't. :)

We will definitely discuss this again today at our meeting at 3pm CET. Everybody is welcome to attend but it will not be required to get this merged.

I am fine with the suggestion made by @optionsome and @leonardehrenfried.

leonardehrenfried · 2023-10-20T08:27:17Z

Quick update, @cedarbaum: There was a bit of miscommunication between @optionsome, I and @t2gran as the latter hadn't realised that we made a proposal for the schema.

He's going to take another look at what we suggested and will let us know if he agrees or not.

t2gran · 2023-10-24T11:48:46Z

I will leave the final review to @leonardehrenfried and @optionsome. Thank you for contributing.

leonardehrenfried

I've taken the liberty to apply a few cosmetic changes directly. I hope this is ok.

Thanks for the contribution!

optionsome · 2023-10-26T09:11:36Z


+type RentalVehicleEntityCounts {
+    """The total number of entities (e.g. vehicles, spaces)."""
+    total: Int!


I think this should be nullable because I know there are rental operators who are incapable of reporting realtime status of available vehicles/spaces for the stations. We can discuss this in the developer meeting today @leonardehrenfried. Alternatively, the available spaces/vehicles fields could be nullable.

In GBFS they are mandatory. Are you thinking of your own custom importer for Vilkuu?

I'm not sure about this. The 2.3 documentation says for num_docks_available "REQUIRED except for stations that have unlimited docking capacity (for example, valet stations) ". We got rid of the Vilkku updater as they now provide gbfs but it still has similar problems and it can't be the only network in the world where they are not able to provide real numbers for everything.

Ok, lets have a chat on the call today.

cedarbaum requested a review from a team as a code owner October 12, 2023 19:53

leonardehrenfried requested changes Oct 13, 2023

View reviewed changes

cedarbaum force-pushed the vehicle_rental_station_vehicle_types branch 3 times, most recently from b356df7 to cc4f2f3 Compare October 14, 2023 00:48

leonardehrenfried reviewed Oct 16, 2023

View reviewed changes

cedarbaum force-pushed the vehicle_rental_station_vehicle_types branch from cc4f2f3 to 859b168 Compare October 17, 2023 01:29

leonardehrenfried changed the title ~~[API] Add vehicleTypesAvailable and vehicleSpacesAvailable to VehicleRentalStation~~ Add vehicleTypesAvailable and vehicleSpacesAvailable to VehicleRentalStation Oct 17, 2023

leonardehrenfried added the !Improvement A functional improvement or micro feature label Oct 17, 2023

leonardehrenfried changed the title ~~Add vehicleTypesAvailable and vehicleSpacesAvailable to VehicleRentalStation~~ Add available types and spaces to VehicleRentalStation Oct 17, 2023

Expose rental station vehicle/space availability per vehicle type

89a6b14

cedarbaum force-pushed the vehicle_rental_station_vehicle_types branch from 859b168 to 89a6b14 Compare October 19, 2023 03:59

leonardehrenfried requested a review from t2gran October 19, 2023 14:18

t2gran removed their request for review October 24, 2023 11:47

leonardehrenfried reviewed Oct 24, 2023

View reviewed changes

Comment thread src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls Outdated

leonardehrenfried reviewed Oct 24, 2023

View reviewed changes

Comment thread src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls Outdated

Use backticks to prettify generated documentation

8df18db

leonardehrenfried previously approved these changes Oct 24, 2023

View reviewed changes

leonardehrenfried requested a review from optionsome October 24, 2023 12:12

optionsome reviewed Oct 26, 2023

View reviewed changes

leonardehrenfried reviewed Oct 26, 2023

View reviewed changes

Comment thread src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls Outdated

leonardehrenfried reviewed Oct 26, 2023

View reviewed changes

Comment thread src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls Outdated

Make fields nullable

8575360

leonardehrenfried dismissed their stale review via 8575360 October 26, 2023 14:41

leonardehrenfried approved these changes Oct 26, 2023

View reviewed changes

optionsome approved these changes Oct 26, 2023

View reviewed changes

optionsome merged commit eaf1ea2 into opentripplanner:dev-2.x Oct 26, 2023

t2gran pushed a commit that referenced this pull request Oct 26, 2023

Add changelog entry for #5425 [ci skip]

64c48f9

t2gran added this to the 2.5 (next release) milestone Oct 27, 2023

Conversation

cedarbaum commented Oct 12, 2023

Summary

Issue

Unit tests

Documentation

Uh oh!

codecov Bot commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leonardehrenfried commented Oct 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried commented Oct 20, 2023

Uh oh!

t2gran commented Oct 24, 2023

Uh oh!

Uh oh!

Uh oh!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Oct 12, 2023 •

edited

Loading

leonardehrenfried Oct 16, 2023 •

edited

Loading

leonardehrenfried Oct 17, 2023 •

edited

Loading