Reservation Status Overhaul

[lauratomas]

2015-02-23

We'll be overhauling the Reservation status method and converting it into a database column. This will make model scopes much simpler (and resolve an issue introduced by #939), as well as multiple rake tasks.

---

Original

"Lost Reservation"/"Missing Equipment" should be another way to categorize reservations, in addition to Checked Out, Overdue, Future, etc.

[orenyk]

I believe that the spirit of this feature will be handled through the "archive reservation" functionality (#728), as long as we can write notes when archiving. I'm not sure we need a scope specifically for lost equipment. Thoughts?

[squidgetx]

I think it would be nice to be able to differentiate easily between archived and normal reservations, especially if the reservation archive action simply sets a checked-in time. Otherwise the admin won't be able to tell easily which ones have been archived and which ones were 'normal' reservations.

It might be worth thinking about adding a status column to the reservations db. We already have an 'approval status' column that we could just rename to 'status'. Right now status is calculated based on various date criteria, and those calculations are duplicated between def status and all the reservation scopes, which is hard to maintain and leads to things like this. Imagine all our scopes simply reading, scope :overdue, lambda { where(status = 'overdue') } Then it would be trivial to also add an 'archived' status.

The only downside would be that the calculation of the 'missed' status would be dependent on a rake task :P.

[orenyk]

But aren't we relying on the approval status to override validations? I think we'd have to implement a separate column, but moving reservation status in the db is probably worthwhile. In that case it would be easy to differentiate between archived and checked-in reservations. I think you're probably right in that simply relying on notes during the archiving process isn't really sufficient.

[squidgetx]

I don't think there's ever a situation where we need both the value of the approval status and the regular status to do something; every scope uses 'approvalstatus = auto or approved' as a filter. Auto and approved requests could just be converted to checkedout requests when they get checked out.

Hmm, I just realized that the status of overdue would also need to be done via rake task. Maybe it's not such a great idea after all? What do we think about dependence on rake tasks?

Actually, just had another idea. Returned overdue means returned + overdue, regular overdue means checked out + overdue. But missed also means reserved + overdue, in a sense (that is, the current date is after the reservation due date and the equipment hasn't been checked in). We could have one rake task (and one boolean column the db) that checks for this overdue criteria for all non-returned reservations.

That way where status = checked_out out would automatically include all checked out items regardless of whether or not they're overdue, same thing goes for returned. Then we could filter for checked_out and overdue = true for checked out overdue reservations. It seems more intuitive this way and only relies on one rake task. The only thing is that missed reservations would become more confusing: reserved overdue doesn't really make any sense. We could manually set the status to 'missed' and remove the overdue flag in the rake task, though that smells kinda bad. Or we could just delete all missed reservations the day that they become missed..

[orenyk]

Since we're removing the missed category (see #594), I think that offloading the status to a DB parameter makes even more sense now. This would make it easy to differentiate between returned and archived reservations using the status (that would be set during associated actions). This would also be nice since we could simply change the status from requested to reserved when approving requests, although we might want a way to prevent old denied requests from being deleted as "missed" reservations. This needs to be fully fleshed out but it would definitely be nice to have before export.

[squidgetx]

The main issue with this is that overdue status change will have to be dependent on a rake task to scan through all the reservations each day and see if they're overdue or not. But if we think that's fine then I think it could really simplify everything else...

[orenyk]

Yea, I think that should be OK. I'm wondering if we could alternatively have a separate scope just for overdue so we don't miss them (i.e. use both status and due dates to check for overdue, but just status for everything else). We'll have to iron this out but either way we should go for it.

[orenyk]

While this sounds awesome, given resource constraints I'm bumping this to the Wish List (but with higher priority).

[orenyk]

Looking over this again, I think that using a Rake task to update statuses every night / eliminating most of our scopes would be awesome and we should try to get this done if we push out a v5.2.0.

[squidgetx]

Would having major functionality dependent on a rake task upset deployment/config? IE are there nontrivially popular server configurations that do not allow running of cron jobs/rake tasks?

[orenyk]

I don't think so... if you're running your own VM / server then you will likely have cron by default, and I'd imagine all other cloud deployment services provide some form of cron (e.g. Heroku's scheduler, etc). @dgoerger any thoughts?

[dgoerger]

afaik cron service should be available on virtually all hosting services.. it would be an odd omission from a unix server to be sure. We do however need to be careful not to have too many intensive tasks running simultaneously in shared environments.

we already have cron tasks for emails, don't we? i.e., I thought we already had a hard dependency on crond?

[orenyk]

That's a great point, we're already relying on cron for most of our e-mails so at this point I'd say it's fair to consider it a dependency.

[esoterik]

Going to use approval_status to indicate whether or not a missed reservation email was sent. See #1031

[orenyk]

Ok, confirmed: we should be considering a reservation missed once its start_date has passed. If it's simple enough we could merge #1202 into here, while we're already messing with the rake task (but don't forget tests!).

We should also make sure that our upcoming reservation e-mail makes this behavior clear (although, admittedly, that's more complex once you factor in #1202, so maybe it's better to leave them separate).

[esoterik]

all of the status changes (status column, overdue boolean, flags bitmask) are implemented, including related rake tasks, in the process of updating everything else

[esoterik]

(also, the status enum works like this)

[esoterik]

everything is now functional; need to update scopes and remove approval_status still

[esoterik]

wait, to clarify, missed reservations should still be marked missed after the due date passes for this issue, right?

[orenyk]

Nope, we're going to switch back to considering them missed after the start passes. This will preserve the current behavior of v3.4.x and we can try to make it configurable if we implement #1202.

[orenyk]

Something that came up today while I was glancing through our e-mails to respond to client feedback: we currently have a _reserved partial for the reservation status update, but the content is specifically for request approval (so if it's not actually a request that would be confusing). We should clear that up here (rename partial and/or have extra status handling code in the user mailer).

[esoterik]

opened a new issue for some of the new flags, #1216

[esoterik]

we also need to write status validations to ensure that the status is consistent with the other reservation attributes