[462] status overhaul #1220
[462] status overhaul #1220
Conversation
| @@ -42,15 +42,14 @@ def bar_span_positioning_fix | |||
| def manage_reservations_btn # rubocop:disable all | |||
| return if (cannot? :manage, Reservation) || @reservation.reserver.id.nil? | |||
| if (can? :override, :reservation_errors) && | |||
| @reservation.approval_status == 'requested' | |||
| @reservation.flags & Reservation::FLAGS[:request] | |||
There was a problem hiding this comment.
Awesome bitmasking! but I think it'll be more readable and less prone to screwing up if we wrap it in a higher level model method to access flags. Something like
def flag(flag_type)
return self.flags & Reservation::FLAGS[flag_type]
end
or something
There was a problem hiding this comment.
Agreed, we could set up getter and setter methods (get_flag, set_flag) to make this easier to use / understand in the future.
|
Is it necessary to note the flag and the status as requested? (As in, we can't just have 'requested' as a first-class status? And move it to 'reserved' when it's approved?) |
| where('checked_out IS NOT NULL').not_returned.recent | ||
|
|
||
| scope :not_available, lambda { | ||
| where(status: Reservation.statuses.values_at( |
There was a problem hiding this comment.
According to this, we should be able to write this query as where(status: [:reserved, :checked_out]), unless I'm missing something?
There was a problem hiding this comment.
Also, why did we need the compact call? Shouldn't the where call return an array of Reservation objects?
There was a problem hiding this comment.
compact is unnecessary, but for some reason where(status: [:reserved, :checked_out]) is not equivalent to where(status: Reservation.statuses.values_at( *%w(reserved checked_out)))
There was a problem hiding this comment.
Really not sure what's going on there...
There was a problem hiding this comment.
Not equivalent in the sense that it's returning a different set of objects, or in the sense that it's not working?
There was a problem hiding this comment.
for the record: we can't use the more simple query because it's not supported in rails 4.1
|
Ok, this looks really good! I left a bunch of comments throughout, mostly in the scopes but a few more elsewhere. One critical issue that needs to be resolved is the fact that most of your specs aren't actually testing anything; it looks like RSpec doesn't actually evaluate assertions unless you call Overall, though, this seems pretty close to me; I think we can definitely get it merged in for v5.2.0. Nice job! |
| @@ -4,7 +4,7 @@ task email_missed_reservations: :environment do | |||
| # user to inform them | |||
| if AppConfig.first.send_notifications_for_deleted_missed_reservations | |||
| missed_reservations = Reservation.where( | |||
| "(approval_status = 'approved' or approval_status = 'auto')").missed | |||
| 'flags & ? = 0', Reservation::FLAGS[:missed_email_sent]).missed | |||
There was a problem hiding this comment.
hmmm, is there no way for us to write a scope for flags?
esoterik
force-pushed
the
462_status_overhaul
branch
from
83c202e to
1cd5c12
Compare
|
I think this is done! |
| @@ -259,7 +262,9 @@ def mark_checked_in(res, checkout_length) | |||
| def mark_checked_out(res) | |||
| if rand < MISSED_CHANCE | |||
| res.checked_out = nil | |||
| res.status = 'mmissed' | |||
|
Ok, left a few more comments; in particular, let's test out the migration at least once manually before we commit. Nice work! |
|
Looks good to me! This one is a big one, @squidgetx, do you want to take one last look at it before we squash / merge? |
esoterik
force-pushed
the
462_status_overhaul
branch
from
86a36b3 to
5e06bdb
Compare
closes #462 - updated test for reservation model - add status enum to reservation model - add flags column to reservation - replaced all other instances of not_returned scope with not_available - removed denied_requests scope, replaced with denied - status validations - removed approval status - prevent status from changing when in a final state - human readable status moved to model - replace not_available scope with active
esoterik
force-pushed
the
462_status_overhaul
branch
from
5e06bdb to
766e9ab
Compare
|
|
| Reservation.where('checked_out IS NOT NULL and checked_in IS NULL and '\ | ||
| 'due_date >= ? and due_date < ?', | ||
| Time.zone.today, Time.zone.today + 1.day) | ||
| upcoming_reservations = Reservation.due_soon |
resolves #462