Skip to content

fix(reservations): prevent false success for no-op updates#1486

Merged
JohnVillalovos merged 1 commit into
developfrom
jlvillal/issue_1430
Jun 14, 2026
Merged

fix(reservations): prevent false success for no-op updates#1486
JohnVillalovos merged 1 commit into
developfrom
jlvillal/issue_1430

Conversation

@JohnVillalovos

Copy link
Copy Markdown
Collaborator

When start time constraints filter all selected reservation instances out of an update or delete operation, validation still passed and the UI reported success even though nothing was changed.

Add a validation rule for update and delete operations that fails when the selected scope contains no affected reservation instances. This preserves the current restriction for active or past reservations while showing the existing start-time constraint error instead of a misleading success message.

Note: this does not grant resource administrators the ability to act on active reservations — that requires extending the Instances() bypass (currently limited to application admins) to cover resource admins as well.

Closes: #1430
Assisted-by: Codex:GPT-5

When start time constraints filter all selected reservation instances out of an
update or delete operation, validation still passed and the UI reported success
even though nothing was changed.

Add a validation rule for update and delete operations that fails when the
selected scope contains no affected reservation instances. This preserves the
current restriction for active or past reservations while showing the existing
start-time constraint error instead of a misleading success message.

Note: this does not grant resource administrators the ability to act on
active reservations — that requires extending the Instances() bypass
(currently limited to application admins) to cover resource admins as
well.

Closes: #1430
Assisted-by: Codex:GPT-5
Copilot AI review requested due to automatic review settings June 14, 2026 18:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR prevents “successful” reservation update/delete operations that are effectively no-ops because the configured start-time constraint filters all targeted instances out of the selected update scope (common for resource administrators due to AdminExcludedRule bypass behavior).

Changes:

  • Add ReservationInstancesRule to fail validation when Instances() is empty, returning the existing StartIsInPast error message.
  • Apply the new rule to update and delete validation services.
  • Add PHPUnit coverage for the new rule behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
tests/Application/Reservation/ReservationInstancesRuleTest.php Adds coverage ensuring validation fails when the selected update scope contains no instances.
lib/Application/Reservation/Validation/ReservationInstancesRule.php Introduces a new validation rule that rejects update/delete operations with zero affected instances.
lib/Application/Reservation/Validation/PreReservationFactory.php Registers the new rule in update and delete validation pipelines.
lib/Application/Reservation/Validation/namespace.php Ensures the new rule class is included via the validation namespace loader.

@JohnVillalovos JohnVillalovos merged commit e00354b into develop Jun 14, 2026
17 checks passed
@JohnVillalovos JohnVillalovos deleted the jlvillal/issue_1430 branch June 14, 2026 18:28
@JohnVillalovos

Copy link
Copy Markdown
Collaborator Author

Intentionally did not make a change to grant resource administrators the ability to modify past reservations.

Fixed the false-success behavior in this path, but intentionally did not expand resource administrator permissions to allow modifying or deleting past/already started reservations.

The current behavior is tied to reservation.start.time.constraint. With the
default future setting, reservations that have already started are not
eligible for normal update/delete operations. Resource administrators were
bypassing the start-time validation rule, but the later series-scope filtering
still selected zero affected reservation instances. That is why the UI could
report success even though nothing changed.

The fix keeps the existing policy intact and makes the result honest: when the
configured constraint leaves no reservation instances eligible to change, the
operation now fails with the existing start-time constraint message instead of
reporting success.

I do not think we should change resource administrator behavior as part of this
bug fix. Allowing resource administrators to modify active or historical
reservations is a larger product/security decision with several side effects:

  • It changes the meaning of "resource administrator" from managing resource
    availability and future bookings to being able to alter the historical record
    for that resource.
  • It can affect reports, utilization history, quota enforcement, credits, and
    any external billing/accounting workflows that depend on reservation history
    being stable after the configured constraint boundary.
  • It complicates recurring reservations. We would need precise behavior for
    "this instance", "future instances", and "full series" when some instances
    are already started or completed and others are still future reservations.
  • It complicates multi-resource reservations. If a reservation uses multiple
    resources and the user administers only one of them, changing or deleting the
    entire reservation could affect resources outside that administrator's
    authority.
  • It raises notification and calendar-sync questions. Owners, participants,
    invitees, resource admins, and external calendars may already have been
    notified about the original reservation. Changing past or active reservations
    can make those downstream records inconsistent unless we define exactly what
    gets sent and when.
  • It interacts with check-in/check-out state. A reservation that is checked in,
    checked out, auto-released, or missing check-out may need different rules than
    a normal future reservation.
  • It can surprise deployments that intentionally use
    reservation.start.time.constraint = future to prevent non-application-admin
    users from changing reservations after they begin.
  • It may require additional audit logging or a separate permission/configuration
    switch so deployments can choose whether resource administrators are trusted
    to change active/historical reservations.

For those reasons, this change only fixes the misleading success response. If
we want resource administrators to be able to change active or past
reservations, that should be designed and tested separately with explicit rules
for recurring reservations, multi-resource reservations, checked-in/out
reservations, notifications, reporting, and configuration compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Delete/modify reservations misbehavior while being resource administrator

2 participants