Add rental system to GraphQL API #5909

sharhio · 2024-06-14T07:51:22Z

Summary

RentalVehicleSystem is returned for rental vehicles. This is so that a the system url to an operator can be provided (especially when there is no vehicle specific link available.)
Searching for nearest stations/vehicles can be filtered by networks. In some cases we only want to show the stations/vehicles of certain operators, and without filtering, the nearest search returns a very large amount of unnecessary data. The large amount of unwanted data coupled with a result limit makes most of the results unusable.
Added an option for hiding networks from maplayers so that unnecessary data doesn't always need to be returned from otp. (For example, in cases where one only wants to show the vehicles of a network and never the stations.)
Sometimes data is faulty and the name field contains an empty string, this causes an error in the FeedScopedId. Added an empty string check.

Unit tests

Added new parameters to unit tests.
Manual verification was done.

…in maplayer, handle empty name

codecov · 2024-06-14T07:57:45Z

Codecov Report

Attention: Patch coverage is 77.27273% with 5 lines in your changes missing coverage. Please review.

Project coverage is 69.51%. Comparing base (2a5c8be) to head (87bda74).
Report is 38 commits behind head on dev-2.x.

Files	Patch %	Lines
...ntripplanner/apis/gtfs/generated/GraphQLTypes.java	50.00%	2 Missing ⚠️
...e_rental/datasources/GbfsGeofencingZoneMapper.java	0.00%	1 Missing and 1 partial ⚠️
...anner/apis/transmodel/TransmodelGraphQLSchema.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5909      +/-   ##
=============================================
+ Coverage      69.45%   69.51%   +0.05%     
- Complexity     17070    17104      +34     
=============================================
  Files           1937     1938       +1     
  Lines          73692    73743      +51     
  Branches        7540     7547       +7     
=============================================
+ Hits           51184    51263      +79     
+ Misses         19880    19841      -39     
- Partials        2628     2639      +11

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

optionsome

I'm not really sure if we should have the network filtering in the map layer or just do it in UI. The problem is that even though we might not want to show some networks in our UI, some other people might. On the other hand, setting this filter on a request level is not really feasible with the map layers. There is an option that we expose filtered and unfiltered versions of the map layer but there is some push towards unifying the map layers and this would be slightly away from it by adding yet another map layer that is only used by us.

...java/org/opentripplanner/ext/vectortiles/layers/vehiclerental/VehicleRentalLayerBuilder.java

src/main/java/org/opentripplanner/apis/transmodel/TransmodelGraphQLSchema.java

src/main/java/org/opentripplanner/inspector/vector/LayerParameters.java

...in/java/org/opentripplanner/updater/vehicle_rental/datasources/GbfsGeofencingZoneMapper.java

src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

...in/java/org/opentripplanner/updater/vehicle_rental/datasources/GbfsGeofencingZoneMapper.java

leonardehrenfried · 2024-06-19T08:29:11Z

Can you add tests, like these:

leonardehrenfried · 2024-06-19T08:30:16Z

src/main/java/org/opentripplanner/apis/gtfs/datafetchers/RentalVehicleImpl.java

@@ -61,6 +62,11 @@ public DataFetcher<RentalVehicleType> vehicleType() {
    return environment -> getSource(environment).vehicleType;
  }

+  @Override


We don't want to expose core model classes directly in the API. Can you please add an Impl (=mapper) class just like RentalVehicleImpl?

I have added a mapper.

leonardehrenfried

I want to see tests and the mapper class.

…ources/GbfsGeofencingZoneMapper.java Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

…tails

optionsome · 2024-07-04T11:56:06Z

src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

@@ -1746,6 +1748,8 @@ type RentalVehicle implements Node & PlaceInterface {
  rentalUris: VehicleRentalUris
  "ID of the vehicle in the format of network:id"
  vehicleId: String
+  "The vehicle rental system information."
+  vehicleRentalSystem: VehicleRentalSystem


We need to discuss in the developer meeting what to do with the naming of this field/type and if it's already too late to create a type for this.

optionsome · 2024-07-05T14:04:47Z

This is now ready for review.

leonardehrenfried · 2024-07-08T08:29:29Z

src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

@@ -1183,6 +1183,8 @@ type QueryType {
    nearest places related to bicycling.
    """
    filterByModes: [Mode],
+    "Only include vehicle rental networks that match one of the given network names."


I think the term "network name" is confusing here. Is it the network id?

While updating it, I noticed we could also make the list items non null so I did it in the same commit 87bda74

leonardehrenfried

Just a bit of documentation.

optionsome · 2024-07-08T10:42:50Z

I've approved this now to get rid of my old review but I'll wait for @vpaturet 's review before merging.

vpaturet

This looks good

sharhio added 3 commits June 14, 2024 09:55

return systemUrl, filter neareast by network, hide unwanted networks …

01df869

…in maplayer, handle empty name

schema

d8b5fc7

docs

7f69e90

sharhio requested a review from a team as a code owner June 14, 2024 07:51

This was referenced Jun 14, 2024

DT-6182 scooter config HSLdevcom/OpenTripPlanner-data-container#403

Merged

DT-6182 rental scooters HSLdevcom/digitransit-ui#5016

Merged

optionsome requested changes Jun 14, 2024

View reviewed changes

sharhio added 6 commits June 17, 2024 12:32

Use vehicleRentalSystemType to return system url

d88617c

replace name with default if empty

814846e

filterByNetwork naming

778d0de

removed network hiding option in maplayers

fa4627f

filterByNetwork description

5ef5fb5

Fixed vehiclerentalsystem

e6603e1

leonardehrenfried self-requested a review June 18, 2024 09:04

no network filtering in transmodel

aa8d4ae

leonardehrenfried reviewed Jun 19, 2024

View reviewed changes

...in/java/org/opentripplanner/updater/vehicle_rental/datasources/GbfsGeofencingZoneMapper.java Outdated Show resolved Hide resolved

leonardehrenfried reviewed Jun 19, 2024

View reviewed changes

leonardehrenfried requested changes Jun 19, 2024

View reviewed changes

Update src/main/java/org/opentripplanner/updater/vehicle_rental/datas…

8a77828

…ources/GbfsGeofencingZoneMapper.java Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

leonardehrenfried changed the title ~~Rental vehicle result and maplayer adjustments~~ Add rental system to GraphQL API Jun 19, 2024

sharhio and others added 6 commits June 19, 2024 12:33

import fixed

f70c31d

mapper class, fix name check

8523e34

Remove unused code

3c81fca

Merge remote-tracking branch 'upstream/dev-2.x' into rentalvehicle-de…

41df32c

…tails

Add and update GraphQL integration tests

b64b08a

Add place finder tests for rental

9a3edc1

optionsome reviewed Jul 4, 2024

View reviewed changes

leonardehrenfried requested a review from vpaturet July 4, 2024 13:55

optionsome added 2 commits July 5, 2024 15:34

Call system network in schema and move network id there

7ab29bc

Add system to smoove stations

3eaa5d0

optionsome requested a review from leonardehrenfried July 5, 2024 14:04

Add more required fields to smoove system

b9a0c82

leonardehrenfried reviewed Jul 8, 2024

View reviewed changes

leonardehrenfried requested changes Jul 8, 2024

View reviewed changes

Improve documentation and make list items non null

87bda74

leonardehrenfried approved these changes Jul 8, 2024

View reviewed changes

optionsome approved these changes Jul 8, 2024

View reviewed changes

vpaturet approved these changes Jul 8, 2024

View reviewed changes

optionsome merged commit e508838 into opentripplanner:dev-2.x Jul 8, 2024
5 checks passed

optionsome deleted the rentalvehicle-details branch July 8, 2024 18:57

t2gran pushed a commit that referenced this pull request Jul 8, 2024

Add changelog entry for #5909 [ci skip]

b064330

t2gran added this to the 2.6 (next release) milestone Jul 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rental system to GraphQL API #5909

Add rental system to GraphQL API #5909

sharhio commented Jun 14, 2024 •

edited

Loading

codecov bot commented Jun 14, 2024 •

edited

Loading

optionsome left a comment

leonardehrenfried commented Jun 19, 2024

leonardehrenfried Jun 19, 2024

sharhio Jun 20, 2024

leonardehrenfried left a comment

optionsome Jul 4, 2024

optionsome commented Jul 5, 2024

leonardehrenfried Jul 8, 2024

optionsome Jul 8, 2024

leonardehrenfried left a comment

optionsome commented Jul 8, 2024

vpaturet left a comment

Add rental system to GraphQL API #5909

Add rental system to GraphQL API #5909

Conversation

sharhio commented Jun 14, 2024 • edited Loading

Summary

Unit tests

codecov bot commented Jun 14, 2024 • edited Loading

Codecov Report

optionsome left a comment

Choose a reason for hiding this comment

leonardehrenfried commented Jun 19, 2024

leonardehrenfried Jun 19, 2024

Choose a reason for hiding this comment

sharhio Jun 20, 2024

Choose a reason for hiding this comment

leonardehrenfried left a comment

Choose a reason for hiding this comment

optionsome Jul 4, 2024

Choose a reason for hiding this comment

optionsome commented Jul 5, 2024

leonardehrenfried Jul 8, 2024

Choose a reason for hiding this comment

optionsome Jul 8, 2024

Choose a reason for hiding this comment

leonardehrenfried left a comment

Choose a reason for hiding this comment

optionsome commented Jul 8, 2024

vpaturet left a comment

Choose a reason for hiding this comment

sharhio commented Jun 14, 2024 •

edited

Loading

codecov bot commented Jun 14, 2024 •

edited

Loading