Bessere Buchungsvalidierung #1275
Conversation
…cated overlapping transient
…issue-1248 # Conflicts: # src/Model/Booking.php # src/Wordpress/CustomPostType/Booking.php
Codecov Report
@@ Coverage Diff @@
## release/2.8.1 #1275 +/- ##
================================================
Coverage ? 25.75%
Complexity ? 2054
================================================
Files ? 83
Lines ? 8382
Branches ? 0
================================================
Hits ? 2159
Misses ? 6223
Partials ? 0 |
|
@chriwen @datengraben Ich hab doch nochmal einiges gebastelt, um die ganzen Methoden einerseits testbar zu machen und habe dann auch noch die Tests dazu ergänzt. Das sind vielleicht jetzt etwas mehr Änderungen aber ich glaube das macht das ganze etwas robuster. Kannst da einer von euch nochmal rüberschauen? Dann können wir das mergen und hätten die 2.8.1 fertig. |
| // checks if it's an edit, but ignores exact start/end time | ||
| $isEdit = count( $existingBookings ) === 1 && | ||
| array_values( $existingBookings )[0]->getPost()->post_name === $requestedPostName && | ||
| array_values( $existingBookings )[0]->getPost()->post_author === get_current_user_id(); |
There was a problem hiding this comment.
Wenn es ein Admin-Edit ist, ist $isEdit immer false, obwohl es true sein sollte. Da get_current_user_id nie gleich des post_author sein kann.
There was a problem hiding this comment.
@hansmorb ist das vielleicht der Punkt aus den Tests (da wo der Test-Case fehlt).
There was a problem hiding this comment.
Hui, da bin ich überfragt @chriwen weißt du, was du damit erreichen wolltest?
There was a problem hiding this comment.
Vielleicht weil die Validierung bei Admin Buchungen ausfällt?
There was a problem hiding this comment.
TLDR: Ich würde den Change annehmen, aber die entsprechende Stelle mal überarbeiten.
Meine Vermutung ist, das der Code ab L292 eine Buchung auf unconfirmed setzen soll, wenn ein User seine Buchung bearbeitet. Aktuell verstehe ich aber nicht, (1) wieso dafür die Bedingung count( $existingBookings ) > 0 wahr sein muss und (2) wieso Zeile 299 wahr sein muss. Das ist doch ein Zustand der gar nicht eintreten darf. Also zwei Bookings zur selben Zeit. Denn die Buchung aus $booking kann ja nie in $existingBookings enthalten sein. Wenn ich es richtig verstehe.
Meiner Meinung nach gibt es auch nur einen (unwahrscheinlichen) Fall in dem sich der Code nicht wie erwartet verhält: Angenommen wir machen einen Admin-Edit. Es existiert genau eine andere Buchung, der PostName ist gleich dem requestedPostName und der PostAuthor ist aber ungleich unserem user, da wir der Admin sind. Dann ist isEdit false (obwohl es true sein müsste, es ist ja ein Edit). Damit wird die Bedingung in L298 wahr ausgewertet. $booking ist ebenfalls true (sonst wäre es ja wahrscheinlich auch kein Edit). Unser Booking Status wird damit unconfirmed, obwohl es ein Admin Edit ist (der ja ggf. gar nicht den Status ändern will).
| /** | ||
| * This class tests the form request for the frontend booking process | ||
| * | ||
| * TODO: Test the case, where one user creates an unconfirmed booking and an admin creates a booking for the same item, location and timeframe. |
There was a problem hiding this comment.
Wenn ich das TODO richtig verstehe, testen wir es nicht da es noch nicht korrekt implementiert ist?
There was a problem hiding this comment.
Exakt! Aktuell ist es so, dass wenn ein Nutzender eine unconfirmed Buchung an zb. Artikel A an Standort A von 10:00-12:00 erstellt, und gleichzeitig ein Admin für Artikel A an Standort A von 10:00-12:00 versucht eine Buchung zu erstellen, der Admin die Buchung von dem Nutzenden angezeigt bekommt.
There was a problem hiding this comment.
@hansmorb ich finde es wirklich klasse, wie detailliert ihr den Code testet und auch die Cornercases berücksichtigt. Den von dir beschriebenen Fall müssen wir aber denke ich nicht wirklich absichern. Die Eintrittswahrscheinlichkeit ist schon sehr gering, oder?
There was a problem hiding this comment.
@chriwen Das stimmt, ich hatte das auch nur in einem spezifischen Fall so! Hat auf jeden Fall keine Prio, deshalb habe ich auch nur diesen Kommentar geschrieben ;)
| // checks if it's an edit, but ignores exact start/end time | ||
| $isEdit = count( $existingBookings ) === 1 && | ||
| array_values( $existingBookings )[0]->getPost()->post_name === $requestedPostName && | ||
| array_values( $existingBookings )[0]->getPost()->post_author === get_current_user_id(); |
There was a problem hiding this comment.
TLDR: Ich würde den Change annehmen, aber die entsprechende Stelle mal überarbeiten.
Meine Vermutung ist, das der Code ab L292 eine Buchung auf unconfirmed setzen soll, wenn ein User seine Buchung bearbeitet. Aktuell verstehe ich aber nicht, (1) wieso dafür die Bedingung count( $existingBookings ) > 0 wahr sein muss und (2) wieso Zeile 299 wahr sein muss. Das ist doch ein Zustand der gar nicht eintreten darf. Also zwei Bookings zur selben Zeit. Denn die Buchung aus $booking kann ja nie in $existingBookings enthalten sein. Wenn ich es richtig verstehe.
Meiner Meinung nach gibt es auch nur einen (unwahrscheinlichen) Fall in dem sich der Code nicht wie erwartet verhält: Angenommen wir machen einen Admin-Edit. Es existiert genau eine andere Buchung, der PostName ist gleich dem requestedPostName und der PostAuthor ist aber ungleich unserem user, da wir der Admin sind. Dann ist isEdit false (obwohl es true sein müsste, es ist ja ein Edit). Damit wird die Bedingung in L298 wahr ausgewertet. $booking ist ebenfalls true (sonst wäre es ja wahrscheinlich auch kein Edit). Unser Booking Status wird damit unconfirmed, obwohl es ein Admin Edit ist (der ja ggf. gar nicht den Status ändern will).
Mit diesem PR sollten die Nutzenden nicht mehr einen Fatal Error sehen, wenn eine Buchung fehlschlägt.
closes #1248
@chriwen Schau mal bitte da rein und vielleicht kannst du ja auch sagen, wie und ob wir den alten Transient da herausnehmen können. Die Sache mit den unconfirmed Bookings würde ich auch noch auf diesem Branch machen.