Fixes reserved_slots_by_reservation deleting sibling slots on partly_…

HISTORY.rst

@@ -1,6 +1,15 @@
 Changelog
 ---------
 
+1.1.2 (16.06.2026)
+~~~~~~~~~~~~~~~~~~~
+
+- Fixes reserved_slots_by_reservation deleting sibling slots on partly_available allocations
+
+  When multiple reservations share the same token on a partly available allocation, filtering by allocation_id alone returned all slots for that allocation, not just the one belonging to the specific reservation. Rejecting one reservation would therefore delete the sibling's ReservedSlot, leaving it orphaned.
+  [Tschuppi81]
+
+
 1.1.1 (27.05.2026)
 ~~~~~~~~~~~~~~~~~~~
 

src/libres/db/scheduler.py

@@ -9,7 +9,7 @@
 from sqlalchemy import exc
 from sqlalchemy import func
 from sqlalchemy.orm import selectinload
-from sqlalchemy.sql import and_, not_
+from sqlalchemy.sql import and_, not_, or_
 from uuid import uuid4 as new_uuid, UUID
 
 from libres.context.core import ContextServicesMixin
@@ -2296,10 +2296,25 @@ def reserved_slots_by_reservation(
 
         if id is None:
             return query
-        else:
-            allocations = self.allocations_by_reservation(token, id)
-            ids = allocations.with_entities(Allocation.id)
-            return query.filter(ReservedSlot.allocation_id.in_(ids))
+
+        # allocation_id is ambiguous when multiple reservations share a token
+        # on a partly_available allocation; filter by time range instead.
+        # start is None for group reservations — the or_ includes all their
+        # slots.
+        return (
+            query
+            .join(Reservation, and_(
+                Reservation.token == ReservedSlot.reservation_token,
+                Reservation.id == id
+            ))
+            .filter(or_(
+                Reservation.start.is_(None),
+                and_(
+                    ReservedSlot.start >= Reservation.start,
+                    ReservedSlot.end <= Reservation.end,
+                )
+            ))
+        )
 
     def reserved_slots_by_blocker(
         self,
@@ -2315,10 +2330,22 @@ def reserved_slots_by_blocker(
 
         if id is None:
             return query
-        else:
-            allocations = self.allocations_by_blocker(token, id)
-            ids = allocations.with_entities(Allocation.id)
-            return query.filter(ReservedSlot.allocation_id.in_(ids))
+
+        # Same rationale as reserved_slots_by_reservation.
+        return (
+            query
+            .join(ReservationBlocker, and_(
+                ReservationBlocker.token == ReservedSlot.reservation_token,
+                ReservationBlocker.id == id
+            ))
+            .filter(or_(
+                ReservationBlocker.start.is_(None),
+                and_(
+                    ReservedSlot.start >= ReservationBlocker.start,
+                    ReservedSlot.end <= ReservationBlocker.end,
+                )
+            ))
+        )
 
     def reservations_by_group(self, group: UUID) -> Query[Reservation]:
         tokens = self.managed_reservations().with_entities(Reservation.token)

tests/test_scheduler.py

@@ -9,7 +9,7 @@
 from libres.modules import errors, events
 from libres.modules import utils
 from unittest.mock import Mock
-from sqlalchemy.exc import MultipleResultsFound, StatementError
+from sqlalchemy.exc import MultipleResultsFound, NoResultFound, StatementError
 from uuid import uuid4 as new_uuid
 
 
@@ -618,6 +618,116 @@ def test_change_reservation_assertions(scheduler: Scheduler) -> None:
         )
 
 
+def test_change_reservation_with_nonexistent_id(
+    scheduler: Scheduler,
+) -> None:
+    dates = (datetime(2014, 3, 7, 8, 0), datetime(2014, 3, 7, 17, 0))
+    scheduler.allocate(dates)
+    token = scheduler.reserve('user@example.org', dates)
+    scheduler.commit()
+
+    reservation = scheduler.reservations_by_token(token).one()
+
+    with pytest.raises(NoResultFound):
+        scheduler.change_reservation(
+            token, reservation.id + 99999,
+            datetime(2014, 3, 7, 9, 0), datetime(2014, 3, 7, 16, 0)
+        )
+
+
+def test_remove_reservation_does_not_affect_sibling_reservations(
+    scheduler: Scheduler,
+) -> None:
+    """Removing one reservation on a partly_available allocation must not
+    delete the ReservedSlot of another reservation sharing the same token
+    on the same allocation (the root cause of a production NoResultFound)."""
+    dates_full = (datetime(2014, 3, 7, 8, 0), datetime(2014, 3, 7, 18, 0))
+    scheduler.allocate(dates_full, partly_available=True)
+
+    dates_a = (datetime(2014, 3, 7, 8, 0), datetime(2014, 3, 7, 10, 0))
+    dates_b = (datetime(2014, 3, 7, 10, 0), datetime(2014, 3, 7, 12, 0))
+
+    session = new_uuid()
+    token_a = scheduler.reserve(
+        'user@example.org', dates_a,
+        session_id=session, single_token_per_session=True
+    )
+    scheduler.commit()
+    # Both reservations share the same token because single_token_per_session
+    # is used — this is how onegov groups multiple dates into one booking.
+    token_b = scheduler.reserve(
+        'user@example.org', dates_b,
+        session_id=session, single_token_per_session=True
+    )
+    assert token_a == token_b
+    scheduler.commit()
+
+    scheduler.approve_reservations(token_a)
+    scheduler.commit()
+
+    reservations = sorted(
+        scheduler.reservations_by_token(token_a).all(),
+        key=lambda r: r.start
+    )
+    assert len(reservations) == 2
+    res_a, res_b = reservations  # res_a starts at 08:00, res_b at 10:00
+
+    # Each reservation on a partly_available allocation has its own slots.
+    slots_a_count = scheduler.reserved_slots_by_reservation(
+        token_a, res_a.id
+    ).count()
+    slots_b_count = scheduler.reserved_slots_by_reservation(
+        token_a, res_b.id
+    ).count()
+    assert slots_a_count > 0
+    assert slots_b_count > 0
+
+    # Remove only res_b — res_a's slots must survive intact.
+    scheduler.remove_reservation(token_a, res_b.id)
+    scheduler.commit()
+
+    remaining_count = scheduler.reserved_slots_by_reservation(token_a).count()
+    assert remaining_count == slots_a_count
+
+
+def test_remove_blocker_does_not_affect_sibling_blockers(
+    scheduler: Scheduler,
+) -> None:
+    """Removing one blocker on a partly_available allocation must not delete
+    the ReservedSlots of another blocker sharing the same token."""
+    dates_full = (datetime(2014, 3, 7, 8, 0), datetime(2014, 3, 7, 18, 0))
+    scheduler.allocate(dates_full, partly_available=True)
+
+    dates_a = (datetime(2014, 3, 7, 8, 0), datetime(2014, 3, 7, 10, 0))
+    dates_b = (datetime(2014, 3, 7, 10, 0), datetime(2014, 3, 7, 12, 0))
+
+    # A single add_blocker call with multiple date ranges shares one token.
+    blockers = scheduler.add_blocker([dates_a, dates_b])
+    scheduler.commit()
+
+    assert len(blockers) == 2
+    token = blockers[0].token
+    assert blockers[1].token == token
+
+    blocker_a, blocker_b = sorted(blockers, key=lambda b: b.start)
+
+    slots_a_count = scheduler.reserved_slots_by_blocker(
+        token, blocker_a.id
+    ).count()
+    slots_b_count = scheduler.reserved_slots_by_blocker(
+        token, blocker_b.id
+    ).count()
+    assert slots_a_count > 0
+    assert slots_b_count > 0
+
+    # Remove only blocker_b — blocker_a's slots must survive intact.
+    scheduler.remove_blocker(token, blocker_b.id)
+    scheduler.commit()
+
+    remaining_count = scheduler.reserved_slots_by_blocker(token).count()
+    assert remaining_count == slots_a_count
+
+
 def test_change_unapproved_reservation_quota(scheduler: Scheduler) -> None:
     dates = (datetime(2014, 8, 7, 8, 0), datetime(2014, 8, 7, 10, 0))
     scheduler.allocate(dates, quota=2)