Add rental system to GraphQL API#5909
Conversation
Codecov ReportAttention: Patch coverage is
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. |
optionsome
left a comment
optionsome
left a comment
There was a problem hiding this comment.
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.
| return environment -> getSource(environment).vehicleType; | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
We don't want to expose core model classes directly in the API. Can you please add an Impl (=mapper) class just like RentalVehicleImpl?
There was a problem hiding this comment.
I have added a mapper.
leonardehrenfried
left a comment
leonardehrenfried
left a comment
There was a problem hiding this comment.
I want to see tests and the mapper class.
…ources/GbfsGeofencingZoneMapper.java Co-authored-by: Leonard Ehrenfried <mail@leonard.io>
leonardehrenfried
changed the title
| "ID of the vehicle in the format of network:id" | ||
| vehicleId: String | ||
| "The vehicle rental system information." | ||
| vehicleRentalSystem: VehicleRentalSystem |
There was a problem hiding this comment.
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.
|
This is now ready for review. |
| nearest places related to bicycling. | ||
| """ | ||
| filterByModes: [Mode], | ||
| "Only include vehicle rental networks that match one of the given network names." |
There was a problem hiding this comment.
I think the term "network name" is confusing here. Is it the network id?
There was a problem hiding this comment.
While updating it, I noticed we could also make the list items non null so I did it in the same commit 87bda74
leonardehrenfried
left a comment
leonardehrenfried
left a comment
There was a problem hiding this comment.
Just a bit of documentation.
|
I've approved this now to get rid of my old review but I'll wait for @vpaturet 's review before merging. |
5 participants
Summary
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.)Unit tests