Fix all the dates

[orenyk]

We've had numerous issues where date comparisons were not being handled properly due to the different classes they can take (see #932 and a bunch of others). We should do a complete review of the code (validations and scopes particularly) and make sure they we're always using to_date where necessary.

[orenyk]

Note that in #949 the server's date command identified UTC as the time zone whereas on my machine the date command identified EDT as the time zone. In both cases rails identified the time zone as EDT.

[orenyk]

We should use Date's wherever possible since we rarely care about the time. That would prevent us from having to call to_date on our own objects.

[orenyk]

We should also remove the date.tomorrow and date.yesterday overrides, see #888.

[orenyk]

Ok, some of our tests are failing due to the changing of the clocks; this only highlights the importance of getting these sorted out.

  1) ReservationsController#update (PUT /reservations/:id) when accessed by admin and provides valid params[:reservation] should update the reservation details
     Failure/Error: expect(@reservation.due_date.to_time.utc).to eq((Time.current.midnight + 4*24.hours).utc)

       expected: 2014-11-06 04:00:00.000000000 +0000
            got: 2014-11-06 05:00:00.000000000 +0000

       (compared using ==)

       Diff:
       @@ -1,2 +1,2 @@
       -2014-11-06 04:00:00 UTC
       +2014-11-06 05:00:00 UTC

     # ./spec/controllers/reservations_controller_spec.rb:436:in `block (5 levels) in <top (required)>'
     # ./spec/spec_helper.rb:64:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:63:in `block (2 levels) in <top (required)>'


  2) ReservationsController#update (PUT /reservations/:id) when accessed by checkout person allowed by settings and provides valid params[:reservation] should update the reservation details
     Failure/Error: expect(@reservation.due_date.to_time.utc).to eq((Time.current.midnight + 4*24.hours).utc)

       expected: 2014-11-06 04:00:00.000000000 +0000
            got: 2014-11-06 05:00:00.000000000 +0000

       (compared using ==)

       Diff:
       @@ -1,2 +1,2 @@
       -2014-11-06 04:00:00 UTC
       +2014-11-06 05:00:00 UTC

     # ./spec/controllers/reservations_controller_spec.rb:436:in `block (5 levels) in <top (required)>'
     # ./spec/spec_helper.rb:64:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:63:in `block (2 levels) in <top (required)>'


  3) AnnouncementsController with admin GET new sets the default announcement
     Failure/Error: expect(assigns(:announcement)[:ends_at]).to eq(Time.current.midnight + 24.hours)

       expected: 2014-11-02 23:00:00.000000000 -0500
            got: 2014-11-03 00:00:00.000000000 -0500

       (compared using ==)

       Diff:
       @@ -1,2 +1,2 @@
       -Sun, 02 Nov 2014 23:00:00 EST -05:00
       +Mon, 03 Nov 2014 00:00:00 EST -05:00

     # ./spec/controllers/announcements_controller_spec.rb:41:in `block (4 levels) in <top (required)>'
     # ./spec/spec_helper.rb:64:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:63:in `block (2 levels) in <top (required)>'


  4) Cart Cart actions .prepare_all the reservations should have the correct dates
     Failure/Error: expect(r.due_date).to eq(Time.current.midnight + 24.hours)

       expected: 2014-11-02 23:00:00.000000000 -0500
            got: 2014-11-03 00:00:00.000000000 -0500

       (compared using ==)

       Diff:
       @@ -1,2 +1,2 @@
       -Sun, 02 Nov 2014 23:00:00 EST -05:00
       +Mon, 03 Nov 2014 00:00:00 EST -05:00

     # ./spec/models/cart_spec.rb:106:in `block (6 levels) in <top (required)>'
     # ./spec/models/cart_spec.rb:104:in `each'
     # ./spec/models/cart_spec.rb:104:in `block (5 levels) in <top (required)>'
     # ./spec/spec_helper.rb:64:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:63:in `block (2 levels) in <top (required)>'

...

Failed examples:

rspec ./spec/controllers/reservations_controller_spec.rb:433 # ReservationsController#update (PUT /reservations/:id) when accessed by admin and provides valid params[:reservation] should update the reservation details
rspec ./spec/controllers/reservations_controller_spec.rb:433 # ReservationsController#update (PUT /reservations/:id) when accessed by checkout person allowed by settings and provides valid params[:reservation] should update the reservation details
rspec ./spec/controllers/announcements_controller_spec.rb:39 # AnnouncementsController with admin GET new sets the default announcement
rspec ./spec/models/cart_spec.rb:102 # Cart Cart actions .prepare_all the reservations should have the correct dates

[orenyk]

I've changed all of the relevant date-only columns to the :date type; interestingly, we were already using that type for blackouts. I'm just going through the app again and making sure all uses of that data make sense.

As a side note, does anyone know if the "current reservations" view (/reservations/current/:id) is actually used? The two scopes there (:checked_out_today and :checked_out_previous) are a little strange, particularly the latter which for some reason also checks to make sure the due date is no later than tomorrow. I'm fine to leave them as-is but figured some of our former MT's (cough @squidgetx cough) might have some insight.

[squidgetx]

No idea, I don't think I ever used that view but perhaps.. someone does

[orenyk]

Here's an interesting side effect that I'm not sure how to deal with: since checked_in is a DateTime but due_date is just a Date, anyone who returns their equipment on the due_date will be considered :returned_overdue since any time on a given date will be later than the Date object for that date (which is assigned a time of midnight in comparisons, I believe). We really need to check checked_in <= due_date + 1.day to see if it was checked in on time or not. Should we just save the day after the due date as the due_date parameter? I'm going to see if that breaks other scopes.

[orenyk]

Confirmed: the returned_on_time / returned_overdue scopes are broken. We convert the checked_in data to a Date when we make the comparison for the Reservation#checkin method and in the /reservations/:id view, so most users wouldn't notice, but the categorization on the /users/:id view is broken as is probably /reservations. Not sure how to handle this; I'm not sure we can easily modify the scope query. Investigating...

[orenyk]

Note that this was already broken on master.

[orenyk]

Ok, so I currently see a few ways to fix this:

1. Store the day after the due date in the database

• this allows us to have functioning scopes for returned_on_time and returned_overdue
• this will require significant tweaking to make sure that all the other places in the app that refer to the due_date parameter are correctly subtracting one day

2. Store both the due date and the day after in the database (add a column to the reservations table)

• we can fix our scopes without breaking anything else
• but we have to add a fairly trivial column to our database

3. Store reservation statuses directly in the database (see Reservation Status Overhaul #462)

• we no longer need to run comparisons to find whether or not a reservation was returned on time or overdue
• we get to simplify a bunch of scopes
• we don't break much else

I'm leaning towards option 3 since it's a change we wanted to make anyway and it would be an overall improvement to the app. If that's the way we go, I'll leave the scopes broken for now and we'll fix them in that issue. Thoughts (@squidgetx, @mnquintana, @dgoerger)?

[squidgetx]

Isn't there also

4. Adjust scopes so that they correctly use a <= comparison for dates?

Though I agree that 3 is the best

[orenyk]

Nope, I don't think it works. If you check the equipment in prior to the due date, checked_in < due_date. If you check the equipment in on the due date, checked_in > due_date. The problem is that the due date falls within the range of allowable check-in times before it's overdue.

We'll go with 3; I'll leave a comment stating that the scope is broken and we'll fix it in #462, which I'll put in the next general sprint since fixing the scopes is pretty critical.

[orenyk]

In the process of cleaning up the rest of the code. Things to search for:

• to_date
• Date.current
• Date.tomorrow
• Date.yesterday
• Time.current
• .utc

Once those have all been cleaned up I'll just make sure that Travis is happy and this will be done.

[dgoerger]

Agreed on option 3 fwiw---this sounds cleanest. 😀

[orenyk]

Looks like I have some broken tests... time to clean up :-)

[orenyk]

Ok, broken tests (almost all) fixed and rubocop violations corrected; I'm going to open up a PR now.

I have noticed that some tests were failing when the LDAP lookup timed out; I manually disabled LDAP in the authentication integration tests (spec/features/auth_spec.rb), but there's a test in users_controller_spec.rb that triggers an LDAP lookup and is still failing intermittently. I initially assumed it was because LDAP was inaccessible outside of Yale, but this can't be the case since Travis has been passing until now.

This can be solved by commenting out the USE_LDAP line in .env, but I'd prefer a way to disable it without turning it off by default in development (since it can be useful). I'll keep investigating; was ITS having uptime issues, @dgoerger?

[dgoerger]

LDAP last night? Not afaict in the logs. I don't see any broader network issues posted on the status page, either. :/

[orenyk]

Woot! I verified that this branch resolves the issues noted in this comment via Heroku, so I think it's good to go 😄.

In terms of the LDAP issues, I'm re-running the build now. We'll see if issues show up in anyone else's Travis builds, otherwise I'll have to assume that it has something to do with this specific branch 😢

[dgoerger]

It looks like it's passing now! 😀 Maybe it was a temporary glitch?

[orenyk]

That's what I'm guessing. <shakes fist>

[dgoerger]

Try changing it from testing csw3 to a valid NetID? Although I confirmed at work today that NetID lookup shouldn't work from off-campus, I'm surprised it's been working this long when the LDAP configuration at Yale hasn't changed appreciably since July 2012 (so far as I am aware). 🤷

[orenyk]

Just FYI, @elle published two awesome articles on dealing with time zones in Rails. I'm leaving this here in case anyone needs something similar and runs across this issue.