1608 extract checkout object - #1667
Conversation
1c87731 to
bd43812
Compare
resolves #1608 --create manage route for managing users' reservations --extract checkin/checkout functionality to manage controller --extract as much functionality to checkoutHelper service object as possible --replace set_reservation before action with ||= --put repeated before_action into application controller
80003de to
a4ab639
Compare
esoterik
left a comment
esoterik
left a comment
There was a problem hiding this comment.
Looking good overall, except for the missing tests
Left a few suggestions to make a new form object and to restructure your current service object a bit
| return unless @user.role == 'banned' | ||
| flash[:error] = 'This user is banned and cannot check out equipment.' | ||
| params[:banned] = true | ||
| end |
There was a problem hiding this comment.
this should be renamed -- it does more than set the user
| before_action :set_user, only: [:show, :checkout] | ||
| before_action :set_reservation, only: [:send_receipt] | ||
|
|
||
| private |
There was a problem hiding this comment.
you should swap the order of the public and private sections
| # Overdue validation | ||
| return unless handle_overdue_reservations | ||
| true | ||
| end |
There was a problem hiding this comment.
all these checks can live inside a service object -- check out these examples from vesta:
service object, controller method, and handle_action
you can probably do something very similar by adding a less-generalized handle_action to this controller
| render :show, layout: 'application' | ||
| end | ||
|
|
||
| def checkout |
There was a problem hiding this comment.
this process should also live in a service object (example)
| end | ||
|
|
||
| CheckoutHelper.update_tos(@user) | ||
| CheckoutHelper.send_checkout_receipts(checked_out_reservations) |
There was a problem hiding this comment.
these methods belong in the batch/bulk service object
| prep_receipt_page(check_in_set: [], check_out_set: checked_out_reservations) | ||
| end | ||
|
|
||
| def checkin |
There was a problem hiding this comment.
same as the above -- this method should live inside a service object
|
|
||
| # for editing reservations; not for checkout or check-in | ||
| def update # rubocop:disable all | ||
| @reservation = Reservation.find(params[:id]) |
There was a problem hiding this comment.
you should be able to just call reservation, right?
| @@ -0,0 +1,3 @@ | |||
| # frozen_string_literal: true | |||
| class Manage < ActiveRecord::Base | |||
| end | |||
There was a problem hiding this comment.
this should be a form object that handles all of the checks and bulk processing that you currently have in the controller (example)
| @@ -0,0 +1,3 @@ | |||
| # frozen_string_literal: true | |||
| class Manage < ActiveRecord::Base | |||
There was a problem hiding this comment.
I don't think this needs to inherit from ActiveRecord::Base?
There was a problem hiding this comment.
also this should probably be renamed to something that implies that this is for reservations only
| @@ -0,0 +1,67 @@ | |||
| # frozen_string_literal: true | |||
| class CheckoutHelper | |||
There was a problem hiding this comment.
this is really good but needs a bit of reorganizing:
- The only methods on
selfshould becheck_in!andcheck_out!, which should read like this:
def self.check_out!(**params)
new(**params).check_out!
end- There should be an
initializemethod that sets some instance variables so you don't have to pass them around. It should use keyword args. - Any method that can raise an un-rescued exception should end in a
!
There was a problem hiding this comment.
This also needs to be tested!
3 participants
resolves #1608
--create manage route for managing users' reservations
--extract checkin/checkout functionality to manage controller
--extract as much functionality to checkoutHelper service object as possible