-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
74 lock button vanishes #91
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good functionality wise, but I've suggested an improvement to the implementation below.
bookings/views.py
Outdated
"-reservation_time" | ||
) | ||
for booking in bookings: | ||
booking.can_lock = booking.vehicle.box.current_booking == booking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works fine I think, but from a code elegance point of view, I think it would make sense to make can_lock
a property on the Booking
model. So this loop would be replaced with a new method in bookings/models.py
along the lines of the code below. Also, thinking about it some more, I think the name can_lock
is a bit dangerous, because it doesn't technically return an exact boolean when the booking can or can't lock - e.g. you can send a lock
command to a booking that isn't the current active booking for a box if it is the booking that, based on the current time, would be the active booking. So I'd suggest a change to a longer but more precise name such as what I've suggested below. The other option would be to cover both cases (time and box.current_booking
) in the function, in which case can_lock
would be a justified name for it.
@property
def is_current_booking_on_box(self):
"""
Returns True if this booking is the currently active booking according to the associated Box, otherwise False.
"""
return self == self.vehicle.box.current_booking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Testing this, I realise we also need a change to the booking in the booking list to show it as late rather than completed, if it is in the "late" state. But that can be a separate issue. |
fixes #74
Adds booking.can_lock = True to the context if a boxes current_booking is that booking. If can_lock and the user has access to that booking then the lock button is displayed even if it is after the reservation period.