Skip to content

Commit c0e0553

Browse files
fix: address CodeRabbit review issues in ReservationReassignmentService
- Replace MariaDB-specific @@in_transaction with MySQL-compatible @@autocommit check - Fix wrong enum value 'autore' -> 'principale' in author query - Add retry loop with max attempts in reassignOnCopyLost for race conditions - Wrap handleNoCopyAvailable UPDATE in proper transaction handling - Add new findAvailableCopyExcluding method for efficient copy exclusion - Add English translations for new log messages
1 parent 4a70c0b commit c0e0553

File tree

2 files changed

+96
-28
lines changed

2 files changed

+96
-28
lines changed

app/Services/ReservationReassignmentService.php

Lines changed: 93 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -40,18 +40,20 @@ public function setExternalTransaction(bool $external): self
4040

4141
/**
4242
* Verifica se siamo già dentro una transazione.
43+
* Compatible with both MySQL and MariaDB.
4344
*/
4445
private function isInTransaction(): bool
4546
{
4647
if ($this->externalTransaction) {
4748
return true;
4849
}
49-
// MySQL: controlla se c'è una transazione attiva
50-
$result = $this->db->query("SELECT @@autocommit as ac, @@in_transaction as it");
50+
// MySQL/MariaDB compatible: check autocommit status
51+
// When in a transaction started with begin_transaction(), autocommit is 0
52+
$result = $this->db->query("SELECT @@autocommit as ac");
5153
if ($result) {
5254
$row = $result->fetch_assoc();
53-
// in_transaction = 1 significa che siamo in una transazione
54-
return (int)($row['it'] ?? 0) === 1;
55+
// autocommit = 0 typically means we're in a transaction
56+
return (int)($row['ac'] ?? 1) === 0;
5557
}
5658
return false;
5759
}
@@ -190,10 +192,21 @@ public function reassignOnCopyLost(int $copiaId): void
190192
return;
191193
}
192194

193-
// Cerca un'altra copia disponibile per questo libro
194-
$nextCopyId = $this->findAvailableCopy((int) $reservation['libro_id'], $copiaId);
195+
$libroId = (int) $reservation['libro_id'];
196+
$reservationId = (int) $reservation['id'];
197+
$excludedCopies = [$copiaId]; // Copie da escludere dalla ricerca
198+
$maxRetries = 5; // Limite tentativi per evitare loop infiniti
199+
200+
for ($attempt = 0; $attempt < $maxRetries; $attempt++) {
201+
// Cerca un'altra copia disponibile per questo libro
202+
$nextCopyId = $this->findAvailableCopyExcluding($libroId, $excludedCopies);
203+
204+
if (!$nextCopyId) {
205+
// Nessuna copia disponibile
206+
$this->handleNoCopyAvailable($reservationId);
207+
return;
208+
}
195209

196-
if ($nextCopyId) {
197210
// Riassegna
198211
$ownTransaction = $this->beginTransactionIfNeeded();
199212
try {
@@ -207,18 +220,14 @@ public function reassignOnCopyLost(int $copiaId): void
207220
// Verifica che la copia sia ancora disponibile (potrebbe essere cambiata)
208221
if (!$copyStatus || $copyStatus['stato'] !== 'disponibile') {
209222
$this->rollbackIfOwned($ownTransaction);
210-
// Prova a cercare un'altra copia
211-
$nextCopyId = $this->findAvailableCopy((int) $reservation['libro_id'], $copiaId);
212-
if (!$nextCopyId) {
213-
// Nessuna copia disponibile
214-
$this->handleNoCopyAvailable((int) $reservation['id']);
215-
}
216-
return;
223+
// Aggiungi questa copia alle escluse e riprova
224+
$excludedCopies[] = $nextCopyId;
225+
continue;
217226
}
218227

219228
// Aggiorna prenotazione
220229
$stmt = $this->db->prepare("UPDATE prestiti SET copia_id = ? WHERE id = ?");
221-
$stmt->bind_param('ii', $nextCopyId, $reservation['id']);
230+
$stmt->bind_param('ii', $nextCopyId, $reservationId);
222231
$stmt->execute();
223232
$stmt->close();
224233

@@ -227,21 +236,29 @@ public function reassignOnCopyLost(int $copiaId): void
227236

228237
$this->commitIfOwned($ownTransaction);
229238

230-
// Notifica (opzionale, magari solo per dire "cambio copia")
231-
// $this->notifyUserCopyChanged(...)
239+
// Riassegnazione completata con successo
240+
return;
232241

233242
} catch (Exception $e) {
234243
$this->rollbackIfOwned($ownTransaction);
235244
SecureLogger::error(__('Errore riassegnazione copia persa'), [
236245
'copia_id' => $copiaId,
237-
'reservation_id' => $reservation['id'],
246+
'reservation_id' => $reservationId,
247+
'attempt' => $attempt + 1,
238248
'error' => $e->getMessage()
239249
]);
250+
// Aggiungi questa copia alle escluse e riprova
251+
$excludedCopies[] = $nextCopyId;
240252
}
241-
} else {
242-
// Nessuna copia disponibile
243-
$this->handleNoCopyAvailable((int) $reservation['id']);
244253
}
254+
255+
// Esauriti i tentativi
256+
SecureLogger::warning(__('Esauriti tentativi riassegnazione copia'), [
257+
'copia_id' => $copiaId,
258+
'reservation_id' => $reservationId,
259+
'attempts' => $maxRetries
260+
]);
261+
$this->handleNoCopyAvailable($reservationId);
245262
}
246263

247264
/**
@@ -251,12 +268,25 @@ private function handleNoCopyAvailable(int $reservationId): void
251268
{
252269
// Meglio impostare copia_id a NULL per indicare "in coda senza copia" o "in attesa"
253270
// E notificare l'utente che è tornato in lista d'attesa
254-
$stmt = $this->db->prepare("UPDATE prestiti SET copia_id = NULL WHERE id = ?");
255-
$stmt->bind_param('i', $reservationId);
256-
$stmt->execute();
257-
$stmt->close();
271+
$ownTransaction = $this->beginTransactionIfNeeded();
272+
try {
273+
$stmt = $this->db->prepare("UPDATE prestiti SET copia_id = NULL WHERE id = ?");
274+
$stmt->bind_param('i', $reservationId);
275+
$stmt->execute();
276+
$stmt->close();
277+
278+
$this->commitIfOwned($ownTransaction);
279+
280+
// Notifica DOPO il commit (fuori dalla transazione)
281+
$this->notifyUserCopyUnavailable($reservationId, 'lost_copy');
258282

259-
$this->notifyUserCopyUnavailable($reservationId, 'lost_copy');
283+
} catch (Exception $e) {
284+
$this->rollbackIfOwned($ownTransaction);
285+
SecureLogger::error(__('Errore gestione copia non disponibile'), [
286+
'reservation_id' => $reservationId,
287+
'error' => $e->getMessage()
288+
]);
289+
}
260290
}
261291

262292
/**
@@ -310,6 +340,42 @@ private function findAvailableCopy(int $libroId, ?int $excludeCopiaId = null): ?
310340
return $res ? (int) $res['id'] : null;
311341
}
312342

343+
/**
344+
* Trova una copia disponibile escludendo una lista di copie.
345+
* @param int $libroId ID del libro
346+
* @param array<int> $excludeCopiaIds Array di ID copie da escludere
347+
*/
348+
private function findAvailableCopyExcluding(int $libroId, array $excludeCopiaIds): ?int
349+
{
350+
$sql = "
351+
SELECT id
352+
FROM copie
353+
WHERE libro_id = ?
354+
AND stato = 'disponibile'
355+
";
356+
$params = [$libroId];
357+
$types = "i";
358+
359+
if (!empty($excludeCopiaIds)) {
360+
$placeholders = implode(',', array_fill(0, count($excludeCopiaIds), '?'));
361+
$sql .= " AND id NOT IN ($placeholders)";
362+
foreach ($excludeCopiaIds as $id) {
363+
$params[] = $id;
364+
$types .= "i";
365+
}
366+
}
367+
368+
$sql .= " LIMIT 1";
369+
370+
$stmt = $this->db->prepare($sql);
371+
$stmt->bind_param($types, ...$params);
372+
$stmt->execute();
373+
$res = $stmt->get_result()->fetch_assoc();
374+
$stmt->close();
375+
376+
return $res ? (int) $res['id'] : null;
377+
}
378+
313379
/**
314380
* Notifica l'utente che la copia prenotata è disponibile per il ritiro.
315381
*/
@@ -343,7 +409,7 @@ private function notifyUserCopyAvailable(int $prestitoId): void
343409
FROM autori a
344410
JOIN libri_autori la ON a.id = la.autore_id
345411
WHERE la.libro_id = ?
346-
ORDER BY la.ruolo = 'autore' DESC
412+
ORDER BY la.ruolo = 'principale' DESC
347413
LIMIT 1
348414
");
349415
$authorStmt->bind_param('i', $data['libro_id']);

locale/en_US.json

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3621,5 +3621,7 @@
36213621
"Errore prenotazione": "Reservation error",
36223622
"Riassegnazione prenotazione nuova copia fallita": "New copy reservation reassignment failed",
36233623
"Elaborazione lista attesa fallita": "Waitlist processing failed",
3624-
"La tua prenotazione per \"%s\" è stata messa in attesa. %s. Verrai notificato quando sarà disponibile una nuova copia.": "Your reservation for \"%s\" has been put on hold. %s. You will be notified when a new copy becomes available."
3624+
"La tua prenotazione per \"%s\" è stata messa in attesa. %s. Verrai notificato quando sarà disponibile una nuova copia.": "Your reservation for \"%s\" has been put on hold. %s. You will be notified when a new copy becomes available.",
3625+
"Esauriti tentativi riassegnazione copia": "Exhausted copy reassignment attempts",
3626+
"Errore gestione copia non disponibile": "Copy unavailable handling error"
36253627
}

0 commit comments

Comments
 (0)