Reproduce issue

[Tschuppi81]

Org: Fix reservation form crashing on expired session

Move remove_expired_reservation_sessions to run before bound_reservations so an expired session yields a clean 403 instead of potentially crashing when the template accesses attributes of a detached SQLAlchemy object.

TYPE: Bugfix
LINK: https://seantis-gmbh.sentry.io/issues/7432727459/activity/

[Tschuppi81]

@Daverball Claude helped to potentially fix this issue.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.48%. Comparing base (5f39f21) to head (79818dc).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

Files with missing lines	Coverage Δ
src/onegov/org/views/reservation.py	68.63% <100.00%> (+0.03%)	⬆️

... and 1 file with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f39f21...79818dc. Read the comment docs.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Daverball]

That is one the possible ways I suggested solving this in the sentry issue. It may be a little controversial though, that we're mutating things on GET requests now.

I still think the better solution is to move this cleanup into a cronjob and to do it for all resources at once. It's also a little weird that this is a method on the resource. We have all our other maintenance tasks on objects that can expire in cronjobs.

This is fine as a quick fix, but I would probably put a second if request.POST condition around this call, if we're going with this, just so we're not cleaning up more often than we did before.

[Tschuppi81]

That cron job would need to run every 15 minutes then, isnt't it a little heavy as the issue might happen very rarely?

[Daverball]

I think the expiration time is a little too tight right now anyways, especially if there's a lot of information to input. An hour would be perfectly fine, or even multiple hours, I'm honestly not sure why we have set it up this tightly, it's not very user friendly. It might be so it's less likely to block the deletion of an allocation/resource, but I'm not even sure if it does block the deletion of an allocation. And deleting a resource is rare, you can just set it private first and then delete it later, after everything has expired.

With the current implementation it's more likely that some stale reservations will stick around, since we only clean them up on demand in a couple of views. So you need to know which views to hit in order to clean them up.

[Tschuppi81]

Oh, I wasn't aware of the 15 minutes expiration time in libres. I agree to increase it. Any form could take you longer to fill than 1h. Creating a resource is one that crosses my mind..

[Tschuppi81]

So beside my change, we would bump the session expiration time and release libres. Then I need to update the test, as it tests for 15 minutes right now

[Tschuppi81]

Let's assume we increase to 1 hour. This means someone blocks a resource slot for an hour even when not finishing the reservation/booking.

Conclusion: I would not increase it to multiple hours as it might block others from booking

[Daverball]

Pending reservations don't block each other as far as I know. They only block once you submit them, so it should not matter if they stick around for a while. You also don't need to make a new libres release, find_expired_reservation_sessions/remove_expired_reservation_sessions accept an expiration_date argument, so you can set it to whatever you want, 15 minutes is just the default.