[Fix Cart Latency] Remove CartReservations! #587
Conversation
|
…where catalog loads properly
…ge dates with no items in cart
Current state of the branch
|
| end | ||
|
|
||
| # If the dates were illogical, sets due date to day after start date | ||
| def fix_due_date |
There was a problem hiding this comment.
do we want to get rid of fix_due_date? it's true that validations will now catch this, but if we leave this we'll need to fix it in regards to hard/soft reservations in #573.
There was a problem hiding this comment.
what about adding something to fix_cart_date in application_controller (line 112) to handle the due date as well?
There was a problem hiding this comment.
Implemented via oren's suggestion; fix due date is back in the cart model and is called inthe application controller
| @@ -1,6 +1,6 @@ | |||
| # Be sure to restart your server when you modify this file. | |||
|
|
|||
| Reservations::Application.config.session_store :active_record_store | |||
There was a problem hiding this comment.
please take this out and file a separate issue, so we can deal with the delicacies of cookies encryption and whatnot. :-)
| FactoryGirl.create(:invalid_cart_reservation).id ]} | ||
| items { { FactoryGirl.create(:equipment_model).id => 1} } | ||
| start_date Date.today | ||
| due_date Date.today + 100.day |
There was a problem hiding this comment.
might it be possible to take the max_length variable and add to that, instead of hard-coding 100.days here?
There was a problem hiding this comment.
agreed, it's currently hard-coded into the factory as 10 days (see /spec/factories/equipment_models.rb) but better to refer to the equipment model rather than an arbitrary constant.
|
otherwise I think this looks good. please see above inline comments. |
| cart.start_date = Date.strptime(params[:cart][:start_date_cart],'%m/%d/%Y') | ||
| cart.due_date = Date.strptime(params[:cart][:due_date_cart],'%m/%d/%Y') | ||
| cart.reserver_id = params[:reserver_id] | ||
| rescue ArgumentError | ||
| cart.set_start_date(Date.today) |
There was a problem hiding this comment.
don't we need to change this to cart.start_date as well? set_start_date is still a method of Cart but here we're calling it on the session cart.
|
Added a few inline comments as well, otherwise looks good to me! EDIT: Does this include an updated |
|
Good catches everybody, I'll get to work on it tomorrow morning. @orenyk this branch includes a migration (to remove the cart reservations table) but not an updated schema. Should I put that in as well? |
|
I'm not sure what best practice is; the current schema in development seems to be broken regardless (see #636) but the auto-generated comment at the top of the file says I think we should try to keep the comment as up-to-date as possible, so every time you add a migration we should update the schema file as well. |
|
|
grep for removed methods turns up clean, changes from review comments look good. please proceed with merge. :) |
[Fix Cart Latency] Remove CartReservations!
|
nice work! |
This PR removes CartReservations entirely from the project. Most calls to CartReservations have been replaced with cart.prepare_all, which builds reservations out of the contents of the cart without needing any interaction with the database