Fix: GBFS: Availability wird nicht berücksichtigt #1332 #1371
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1371 +/- ##
============================================
+ Coverage 35.85% 38.52% +2.66%
- Complexity 2115 2324 +209
============================================
Files 83 83
Lines 8595 9350 +755
============================================
+ Hits 3082 3602 +520
- Misses 5513 5748 +235
|
There was a problem hiding this comment.
Nur 2 Kleinigkeiten. Eine etwas größere Frage, nach aktuellem Stand kann die tagweise slot-ermittlung doch nur für die GBFS-API genutzt werden. Wäre es sinnvoll den Teil noch parametrisierbar auch für Stundenweise/Slot-Wise Ermittlung zu implementieren.
Was mich noch gewundert hat, auf mich macht es den Eindruck, das wir diese Art von Funktionalität manchmal ins Service und manchmal ins View Package schreiben. Wenn es so (ausschließlich tagweise) würde überlegen es in Service zu packen.
Diese beiden Dinge sind aber nichts, was den Merge-Request aufhalten soll.
| @@ -34,12 +35,52 @@ class StationStatus extends BaseRoute { | |||
| public function prepare_item_for_response( $item, $request ): stdClass { | |||
| $preparedItem = new stdClass(); | |||
| $preparedItem->station_id = $item->ID . ""; | |||
| $preparedItem->num_bikes_available = count( Item::getByLocation( $item->ID ) ); // TODO should be the item availability in this moment | |||
| $preparedItem->num_bikes_available = $this->getItemCountAtLocation($item->ID); | |||
| $preparedItem->is_installed = true; | |||
| $preparedItem->is_renting = true; | |||
There was a problem hiding this comment.
Hier is_installed oder is_renting in Zukunft vielleicht sowas wie getTimeframeCount, also ob überhaupt items verfügbar sind.
There was a problem hiding this comment.
Ich habe mich dafür mit Ortwin ausgetauscht (der kennt sich ganz gut mit GBFS aus) und er meinte, dass das grundsätzlich nur beschreibt ob eine Station installiert ist und theoretisch Räder beherbergen könnte. Er meinte, dass das bei uns mehr Sinn ergeben würde das einfach immer auf true zu lassen. CommonsBooking passt da nicht so perfekt in die GBFS Logik
There was a problem hiding this comment.
Allgemein passt GBFS nicht ganz, dast stimmt und im speziellen für is_installed stimmt die Aussage ebenso, da geht es ja laut Spezifikation eher um Ausleihen von Items die z.B. am Bürgersteig "installiert" sind.
Für is_renting steht in der Spezifikation aber
Is the station currently renting vehicles?
true - Station is renting vehicles. Even if the station is empty, if it would otherwise allow rentals, this value MUST be true.
false - Station is not renting vehicles.
If the station is temporarily taken out of service and not allowing rentals, this field MUST be set to false.
If a station becomes inaccessible to users due to road construction or other factors this field SHOULD be set to false. Field SHOULD be set to false during hours or days when the system is not offering vehicles for rent.
https://gbfs.org/specification/reference/#station_statusjson
Ich würde das für CommonsBooking folgendermaßen übersetzen:
- true, wenn die Location mindestens einen Zeitrahmen mit einem Item zugeordnet hat (unabhängig von laufenden Buchungen)
- false, wenn
- die Location gar keinen Zeitrahmen zugordnet hat
- oder entweder
- alle Zeitrahmen deaktiviert sind
- oder alle Zeitrahmen für den Zeitpunkt bzw. Range der Anfrage eine Restriction zugeordnet haben.
Ich würde das aber auch nicht in diesen Pull-Request packen.
Was meinst du genau mit tageweisem Slot? Die stündliche Buchung wird auch anhand der Slots reflektiert. Ich musste feststellen, dass dafür nur noch keine Unit Tests existierten. Ich hab die mal gemacht. |
Hat sich erledigt, ich werde mal einen Pull-Request erstellen der den Code an der Stelle besser dokumentiert. |
Ziel dieses Fix ist es, dass nur Artikel, die tatsächlich gerade an der Station direkt ausleihbar wären auch als solchen angezeigt werden, weil die GBFS Spezifikationen das so fordern.
Vor diesem Patch war es so, dass einfach alle Artikel an der Station angezeigt wurden. Unabhängig von Verfügbarkeit: Gebucht / Urlaub / Reperatur.
Zu diesem Zweck habe ich die API/AvailabilityRoute refactored und das Berechnen der verfügbaren Slots in die Model/Calendar Klasse ausgelagert. So kann diese Methode auch zum Berechnen der verfügbaren Artikel für die GBFS API Route verwendet werden. Dabei werden immer nur die Daten für den heutigen Tag genommen, weil GBFS sich nicht für zukünftige Verfügbarkeiten interessiert.
Diese Methode hat vorher auch nicht das booking-offset mit einberechnet, dh., dass Artikel z.B. für den heutigen Tag als buchbar angezeigt wurden, obwohl sie einen Vorlauf von x Tagen haben.
Das wurde umgebaut und die extrahierte Methode sowie die Methode für die GBFS Route mit Unit Tests versehen.
closes #1332