Patrons unable to make reservations after being granted an exception

[dgoerger]

Patrons are unable to create additional reservations during the same time frame as extended reservations (those that break rules regarding reservation length and number of equipment items). In BMEC reservations:

Reservations # 249 and 250 are six-day reservations for the same equipment model. These reservations required admin approval. Patron attempted but failed to create reservation # 1108 herself for the standard three-day checkout and for a different equipment model. It looks like equipment restrictions are extending past a single reservation?

[adambray]

You are correct that restrictions extend past a single reservation. By their very nature they have to, otherwise patrons could create separate reservations to get around certain limits.

For example, if a patron wanted tries to bypass restrictions by reserving 3 cameras separately, rather than the defined limit of 1, we have to check each reservation against their other reservations to validate that they haven't exceeded the limit during that time period (i.e. overlapping reservations).

There are a few ways of solving this that I can think of, each with pros and cons:

1. Exclude any reservations made by admins from all other reservation validations. This would always prevent the issue above, at the risk of letting patrons create additional reservations that you didn't intent.

For example, if we built this is, the student above could reserve an additional projector during that period despite having 2 out already.

2. Another fix or workaround (and I would argue the better one) would be to implement the reservation request feature. In this case, the patron would have to make a request for Add button to search form #1108, which you would be notified of and approve. This is a bit more reliable, in the sense that there's no easy way for the code to know what additional reservations an admin would allow or not (given the complexity of the validations).

-Adam

On Dec 17, 2012, at 11:22 AM, lauratomas notifications@github.com wrote:

Patrons are unable to create additional reservations during the same time frame as extended reservations (those that break rules regarding reservation length and number of equipment items). In BMEC reservations:

Reservations # 249 and 250 are six-day reservations for the same equipment model. These reservations required admin approval. Patron attempted but failed to create reservation # 1108 herself for the standard three-day checkout and for a different equipment model. It looks like equipment restrictions are extending past a single reservation?

—
Reply to this email directly or view it on GitHub.

[lauratomas]

I agree, implementing the reservation request feature would be the best way to work with these exceptions.

There are some things we might be able to do with SN request forms and frames, but I'm not sure if y'all want go in that direction.

[orenyk]

This has been a recurring problem and we've opened up a whole bunch of issues for it (see everything merged into #318, for example). Ultimately, we need to figure out a mechanism through which admins can grant exceptions when necessary without removing the user's ability to make new valid reservations for themselves.

[orenyk]

This is a big part of #206, but since they're not quite the same I'm leaving them both open. Any solution to that issue needs to also solve this one.

[dgoerger]

I've been playing around with this (w/ #206) in gedit, and here's my thoughts for how to implement these together:

BASIC IDEA:

• add a column in the reservations table for "approval_status"
  -- reservations which pass validations and are finalized are marked "auto" for automatically-approved
  -- we then offer to file a "request" for would-be reservations which fail validations (maybe not offer, just do?)
  --- default to approval_status NULL, convert to "approved" or "denied" (keep track because clients will want a record of these)

FOR MY OWN THOUGHTS, SUGGESTIONS WELCOME:

needed for reservation requests:

• reservations controller
  -- redirect on failing validations when trying to save a reservation, to offer instead to file a request just file the request instead of "finalize"
  --- log why it was a request vs a regular reservation just calculate conflicts at approval time
  -- view / review request
  -- edit (?) (an "approve with edit" button to redirect to the reservation edit page?)
  -- approve (overriding constraints to save as a finalized reservation)
  -- deny
  -- cancel (so a user can cancel their own request)
• user controller
  -- as necessary so user can keep track of their own requests in the user page not implementing
• reservations model
  -- scopes
  --- requested
  --- approved
  --- denied
  --- missed
  -- convert to finalized reservation, skipping most validations (only what admin can override)
  -- convert to denied request
  -- convert missed requests to denied status so patrons have a record of the request, once a day if possible (how to cron job?)
  -- idea of "invalid request", e.g. not enough equipment items to go around? the kind of request that not even an admin can approve, due to physical constraints. nope
• reservations validations
  -- these run before a request can be made
  -- if a cart_reservation passes, it finalizes as approval_status 'auto'
  -- if it fails (only admin-bypassable validations?), we file it as a request (type NULL) type 'requested'
  --- saving a reservation with approval_status NULL, 'approved', or 'denied' should bypass all admin-bypassable validations (and invalid ones should bypass all validations somehow unless we just don't want to allow these to be filed) bypasses all validations for requests
• user model not implementing
  -- a user has_many requests unnecessary given implementation with tables
  -- assemble a list for the view (scopes) not implementing
• reservations view
  -- add tabs for requested, approved, denied in the lists
  -- view for individual request, with logged reasons why it's a request vs reservation no logs, reevaluates errors on the fly
  --- buttons: deny, approve as-is, approve with edit (ideally save as a reservation and then open the edit page, both in the same click)
  -- indicate valid vs invalid requests not implemented
• user view not implementing
  -- add to the user stats
• routes
  -- add as necessary for action-submits
• database
  -- new approval_status column for reservations
  --- automatically-approved reservations have status e.g. 'auto'
  --- requests default to NULL defaults to 'requested'
  --- approved e.g. 'approved' (approved_by column? Simon suggests no, that his papertrail project can probably handle that just fine)
  ---- for counting real reservations (as opposed to requested or denied), count both this and auto
  --- denied e.g. 'denied'

EDIT: updated 25 June 2014 with progress / strikethroughs.

basically what's left:

• convert missed requests to denied
• triple-check the scopes
• code review

[orenyk]

This is fantastic, great work! I'm on my phone so can't look at the whole thing while commenting, but one thing that stuck with me was the invalid request thing (e.g. reservation cannot physically be made for some reason). I think we can safely just treat those as rejected without loss of functionality, it preserves the history while allowing for us to bypass valuations. If we want, we could also add a request_note or something like that for admins to leave feedback on why a request was rejected. I'll give this a second read-through when I'm back on a computer but it's really good work.

[dgoerger]

I have not yet written tests for these, and did not include emailing the patron or admin of pending or approved or denied requests. The requests functionality is here, though.

I would like to code review this first myself, because the rebasing has been awful and I want to double-check all the changes I've committed.

[dgoerger]

it might be possible to move this check/assignment out of the do-loop (for efficiency) and add it to params[:reservation].

[shippy]

This seems to bypass any validation errors without displaying them. They should probably be displayed in both cases, one with "Reservations created successfully, despite containing the following errors: " and the other with "Reservations filed as requests pending administrator approval, as they contain the following errors: "

It's not clear if this control flow doesn't obscure error and privilege case.

[dgoerger]

I'm hesitant to output all errors to the admin again in a flash[:notice]. For one thing, flash notices don't support bullet points, but also, the error messages were all just outputted on the previous screen (the New method in the controller; this one's Create). However, I do think it's reasonable to remind the admin that they've just bypassed validations, so I'll add that case in the if/else conditional.

[squidgetx]

Just checked this out and it feels solid. I trust @shippy to have done a good job with the actual code review so if you think it's good, go ahead and sort out those merge conflicts.

[dgoerger]

okay, I'm gonna try to merge this nowish...

[squidgetx]

gl

[dgoerger]

ty! the second merge has produced a viable gemlock.