Availability validation not working? [v5.5]

[orenyk]

Reported by BMEC:

Just a heads up that we seem to be really overbooked on projectors today. I don’t know what happened, but all of them are checked out, none are overdue, only one is due back today, and there are 4 people still listed as having a reservation that starts today. None of them have come in yet, but I wanted to let you know in case we get any angry messages, or in case you have any magical solutions.
...
Okay, I know what happened. Reservations is allowing us to create new reservations for dates where we show zero available items. I was able to create a reservation for a projector from today to tomorrow despite the red zeros for January 22-24 on the calendar. If you try to make the reservation from the catalog page, you cannot click “Add to cart” when it shows zero available, but if you go to the equipment model page, it allows you to add the item to the cart and go through and finalize the reservation, even though there are zero available until Monday. I’m adding the Developers to this email because this is definitely a problem. I had my student worker reserve one for herself, and then I changed my “view as” to a normal user and was also able to reserve the projector, so it’s not some special privilege of Admins and Checkout folks.

Confirmed on BMEC PROD, I can add said overbooked item (or any overbooked item) to my cart (through the equipment model page) and no cart validations are triggered, either in the flash or in the cart warnings. No idea what's going on, this is CRITICAL so we should get this fixed ASAP. If necessary we can push a patch for v5.5.3 over the weekend / early next week and bump the rest of this milestone to v5.5.4.

[orenyk]

Alright I'm on this.

[orenyk]

Ok, strange issue number 1 - I created a clean database, created a reservation starting today through next week for an equipment model with only a single equipment item, and then tried to create a second reservation for said equipment model. I correctly saw the failed availability validation in the flash and in the reservation confirmation. I'm thinking this might have something to do with status; I'm going to see what happens if I check out the first reservation.

[orenyk]

Yup, that's it. Once the reservation has been checked out it no longer factors into the availability equation, even though it is reflected in the number in the catalog. Now to figure out why...

[orenyk]

Looks like we broke this in #1220 (I might have even commented on it at the time), reserved_in_date_range used to return anything not a request/missed/denied that was either reserved or checked out; now it's only returning reserved reservations, not those that are checked out. This is pretty fundamental; we really need to update our specs to resolve this. Also, the fix for master is going to be a bit different because of #1288. Working to get something done ASAP so we can push a PROD deploy Monday morning if possible (or sooner).

[orenyk]

HAHAHAHA, this was literally a one-word fix (replacing reserved with active), setting up a PR and deploying to DEV now.

[orenyk]

Fix for v5.5 is finished and awaiting review, I'm going to put together a more comprehensive fix for master including decoupling the date overlap from the status and just using active everywhere we were currently using this scope, and writing tests.

[orenyk]

Turns out our "unit" tests for the Reservation availability validation were commented out, for some reason during #644 / #573, which is a shame because they would have caught this bug. I'm going to fix them now.

[orenyk]

Ok, this has been resolved on master, I added specs for the cart validation and fixed the commented out specs for the model validation. I also replaced reserved_in_date_range with overlaps_with_date_range and just added active wherever necessary to replicate the original functionality. This should prove useful in #1360 where I previously commented about this scope, so I'll rebase that branch onto master after this gets merged in.

[orenyk]

Ok, it turns out that the methods we use to calculate the number of equipment items available are somehow broken, so I'm leaving this open and we'll have to patch v5.5 again next week. While dealing with some client feedback from STC I logged in to see Reservations claiming that there are zero equipment items available despite the fact that there are clearly five of them available when looking at the list. I'm seeing 53 total items, 32 of which are checked out (8 of which are overdue), with 16 items deactivated and 5 left available. Strangely, the JS calendar on the equipment model page shows 4 available, which is also incorrect, so I'm really not sure what's going on. Adding the model to my cart does trigger the cart validation error.

This is pretty critical so I'm going to see if I can patch this tonight to squeeze it in tomorrow's deployment (going to be tight). Otherwise we'll push out a critical patch next week to fix this since availability is kinda important. We're really feeling the lack of test coverage here... I'll add that in to master in this issue.

[orenyk]

Figured it out, our code is fine (at least when it comes to evaluating availability), however there were two issues with the STC database. First, there was a single reservation where the equipment item had been deactivated but the reservation had not been checked in (I archived it). I believe that the automatic archiving of reservations after equipment deactivation was only added in #728 so this is a holdover from before the update to v5.5.

The second, and more problematic issue, is that #462 added a new boolean attribute for reservations called overdue that is automatically set each night for new reservations that are overdue. However, STC workers often extend reservations by simply editing the due dates; however, this doesn't unset the overdue flag so those reservations are being double-counted.

I'll open a new issue to address this latter problem; unfortunately due to a bug with the old version of Rails Admin we're using we can't edit reservations because of the status enum; I'm going to include an update of that dependency for the patch for v5.5 so we can resolve the issues with the STC instance immediately.

[orenyk]

Whoops, leaving this open to add still-needed tests for master.