Fundamental Rewrite of Validation System #644

squidgetx · 2014-07-08T19:11:12Z

Moving all 'soft' validations to cart validations instead of reservation validations. This enables the cart to run most validations only one time instead of for every equipment model in the cart (available, renewable, duration, max cat count, max model count, reserver has overdue, blackout dates)
Reservation.validateSet is no more
Only necessary validations run when the dates are updated and when items are added/removed (doesn't check for blackout dates when adding a new item to cart, for instance)

squidgetx · 2014-07-08T21:49:12Z

This is almost done. Still need to go over the ReservationsController spec

:

orenyk · 2014-07-09T04:36:30Z

app/models/reservation_validations.rb

+  # Checks that reservation is not in the past
+  # Does not run on checked out, checked in, overdue, or missed Reservations
+  def not_in_past
+    unless due_date >= Date.today


what about the start date?

Good catch. I was thinking that a reservation with a start date in the past and due date in the future is still 'active', but not_in_past runs only on 'create' and should check the start date as well

Actually it shouldn't matter since start_date_before_due_date would trigger in the case that the due date is before Date.today and the start_date wasn't. Since in that case its not clear whether or not that reservation was intended to be made in the past or not it would be clearer to leave out not_in_past.

Oh right! Good catch yourself :-)

orenyk · 2014-07-10T05:02:37Z

app/models/cart.rb

+  def get_items
+    # Used in cart_validations
+    # Return items where the key is the full equipment model object
+    # uses 1 database call


I really like that you've been documenting when you're making DB calls, let's try to make that the case across our codebase. It will not only be good for documentation purposes but could also potentially uncover some other inefficiencies.

orenyk · 2014-07-10T05:29:17Z

I'm getting the spec failures on my local build (freshly checked out the 573_validations_lol branch. Are you sure you were in the right branch?

EDIT: I agree with @shippy, this work is fantastic :-)

  22) ApplicationController PUT update_cart valid parameters should update cart dates
     Failure/Error: put :update_cart, cart: {start_date_cart: new_start.strftime('%m/%d/%Y'), due_date_cart: new_end.strftime('%m/%d/%Y')}, reserver_id: @new_reserver.id
     ArgumentError:
       comparison of Fixnum with nil failed
     # ./app/models/cart_validations.rb:91:in `<'
     # ./app/models/cart_validations.rb:91:in `block in validate_dates_and_items'
     # ./app/models/cart_validations.rb:88:in `each'
     # ./app/models/cart_validations.rb:88:in `validate_dates_and_items'
     # ./app/controllers/application_controller.rb:133:in `update_cart'
     # ./spec/controllers/application_controller_spec.rb:267:in `block (4 levels) in <top (required)>'

  23) ApplicationController PUT update_cart invalid parameters should set the flash
     Failure/Error: put :update_cart, cart: {start_date_cart: new_start.strftime('%m/%d/%Y'), due_date_cart: new_end.strftime('%m/%d/%Y')}, reserver_id: @new_reserver.id
     ArgumentError:
       comparison of Fixnum with nil failed
     # ./app/models/cart_validations.rb:91:in `<'
     # ./app/models/cart_validations.rb:91:in `block in validate_dates_and_items'
     # ./app/models/cart_validations.rb:88:in `each'
     # ./app/models/cart_validations.rb:88:in `validate_dates_and_items'
     # ./app/controllers/application_controller.rb:133:in `update_cart'
     # ./spec/controllers/application_controller_spec.rb:284:in `block (4 levels) in <top (required)>'

  24) CatalogController PUT remove_from_cart valid equipment_model selected 
     Failure/Error: put :add_to_cart, id: @equipment_model.id
     TypeError:
       nil can't be coerced into Fixnum
     # ./app/models/cart_validations.rb:51:in `+'
     # ./app/models/cart_validations.rb:51:in `block (2 levels) in validate_items'
     # ./app/models/cart_validations.rb:50:in `upto'
     # ./app/models/cart_validations.rb:50:in `block in validate_items'
     # ./app/models/cart_validations.rb:48:in `each'
     # ./app/models/cart_validations.rb:48:in `validate_items'
     # ./app/controllers/catalog_controller.rb:59:in `change_cart'
     # ./app/controllers/catalog_controller.rb:25:in `add_to_cart'
     # ./spec/controllers/catalog_controller_spec.rb:58:in `block (4 levels) in <top (required)>'

  25) CatalogController PUT remove_from_cart valid equipment_model selected should call cart.remove_item to remove item from cart
     Failure/Error: put :add_to_cart, id: @equipment_model.id
     TypeError:
       nil can't be coerced into Fixnum
     # ./app/models/cart_validations.rb:51:in `+'
     # ./app/models/cart_validations.rb:51:in `block (2 levels) in validate_items'
     # ./app/models/cart_validations.rb:50:in `upto'
     # ./app/models/cart_validations.rb:50:in `block in validate_items'
     # ./app/models/cart_validations.rb:48:in `each'
     # ./app/models/cart_validations.rb:48:in `validate_items'
     # ./app/controllers/catalog_controller.rb:59:in `change_cart'
     # ./app/controllers/catalog_controller.rb:25:in `add_to_cart'
     # ./spec/controllers/catalog_controller_spec.rb:58:in `block (4 levels) in <top (required)>'

  26) CatalogController PUT remove_from_cart valid equipment_model selected should set flash[:error] to the result of Reservation.validate_set if exists
     Failure/Error: put :add_to_cart, id: @equipment_model.id
     TypeError:
       nil can't be coerced into Fixnum
     # ./app/models/cart_validations.rb:51:in `+'
     # ./app/models/cart_validations.rb:51:in `block (2 levels) in validate_items'
     # ./app/models/cart_validations.rb:50:in `upto'
     # ./app/models/cart_validations.rb:50:in `block in validate_items'
     # ./app/models/cart_validations.rb:48:in `each'
     # ./app/models/cart_validations.rb:48:in `validate_items'
     # ./app/controllers/catalog_controller.rb:59:in `change_cart'
     # ./app/controllers/catalog_controller.rb:25:in `add_to_cart'
     # ./spec/controllers/catalog_controller_spec.rb:58:in `block (4 levels) in <top (required)>'

  27) CatalogController PUT remove_from_cart valid equipment_model selected 
     Failure/Error: put :add_to_cart, id: @equipment_model.id
     TypeError:
       nil can't be coerced into Fixnum
     # ./app/models/cart_validations.rb:51:in `+'
     # ./app/models/cart_validations.rb:51:in `block (2 levels) in validate_items'
     # ./app/models/cart_validations.rb:50:in `upto'
     # ./app/models/cart_validations.rb:50:in `block in validate_items'
     # ./app/models/cart_validations.rb:48:in `each'
     # ./app/models/cart_validations.rb:48:in `validate_items'
     # ./app/controllers/catalog_controller.rb:59:in `change_cart'
     # ./app/controllers/catalog_controller.rb:25:in `add_to_cart'
     # ./spec/controllers/catalog_controller_spec.rb:58:in `block (4 levels) in <top (required)>'

  28) CatalogController PUT add_to_cart valid equipment_model selected 
     Failure/Error: put :add_to_cart, id: @equipment_model.id
     TypeError:
       nil can't be coerced into Fixnum
     # ./app/models/cart_validations.rb:51:in `+'
     # ./app/models/cart_validations.rb:51:in `block (2 levels) in validate_items'
     # ./app/models/cart_validations.rb:50:in `upto'
     # ./app/models/cart_validations.rb:50:in `block in validate_items'
     # ./app/models/cart_validations.rb:48:in `each'
     # ./app/models/cart_validations.rb:48:in `validate_items'
     # ./app/controllers/catalog_controller.rb:59:in `change_cart'
     # ./app/controllers/catalog_controller.rb:25:in `add_to_cart'
     # ./spec/controllers/catalog_controller_spec.rb:28:in `block (4 levels) in <top (required)>'

  29) CatalogController PUT add_to_cart valid equipment_model selected should call cart.add_item to add item to cart
     Failure/Error: put :add_to_cart, id: @equipment_model.id
     TypeError:
       nil can't be coerced into Fixnum
     # ./app/models/cart_validations.rb:51:in `+'
     # ./app/models/cart_validations.rb:51:in `block (2 levels) in validate_items'
     # ./app/models/cart_validations.rb:50:in `upto'
     # ./app/models/cart_validations.rb:50:in `block in validate_items'
     # ./app/models/cart_validations.rb:48:in `each'
     # ./app/models/cart_validations.rb:48:in `validate_items'
     # ./app/controllers/catalog_controller.rb:59:in `change_cart'
     # ./app/controllers/catalog_controller.rb:25:in `add_to_cart'
     # ./spec/controllers/catalog_controller_spec.rb:28:in `block (4 levels) in <top (required)>'

  30) CatalogController PUT add_to_cart valid equipment_model selected should set flash[:error] to the result of Reservation.validate_set if exists
     Failure/Error: put :add_to_cart, id: @equipment_model.id
     TypeError:
       nil can't be coerced into Fixnum
     # ./app/models/cart_validations.rb:51:in `+'
     # ./app/models/cart_validations.rb:51:in `block (2 levels) in validate_items'
     # ./app/models/cart_validations.rb:50:in `upto'
     # ./app/models/cart_validations.rb:50:in `block in validate_items'
     # ./app/models/cart_validations.rb:48:in `each'
     # ./app/models/cart_validations.rb:48:in `validate_items'
     # ./app/controllers/catalog_controller.rb:59:in `change_cart'
     # ./app/controllers/catalog_controller.rb:25:in `add_to_cart'
     # ./spec/controllers/catalog_controller_spec.rb:28:in `block (4 levels) in <top (required)>'

  31) CatalogController PUT add_to_cart valid equipment_model selected 
     Failure/Error: put :add_to_cart, id: @equipment_model.id
     TypeError:
       nil can't be coerced into Fixnum
     # ./app/models/cart_validations.rb:51:in `+'
     # ./app/models/cart_validations.rb:51:in `block (2 levels) in validate_items'
     # ./app/models/cart_validations.rb:50:in `upto'
     # ./app/models/cart_validations.rb:50:in `block in validate_items'
     # ./app/models/cart_validations.rb:48:in `each'
     # ./app/models/cart_validations.rb:48:in `validate_items'
     # ./app/controllers/catalog_controller.rb:59:in `change_cart'
     # ./app/controllers/catalog_controller.rb:25:in `add_to_cart'
     # ./spec/controllers/catalog_controller_spec.rb:28:in `block (4 levels) in <top (required)>'

squidgetx · 2014-07-10T17:57:47Z

WHATTTTT

orenyk · 2014-07-10T18:06:27Z

WOO HOO! Great job!

dgoerger · 2014-07-10T19:15:23Z

app/models/equipment_model.rb

-      return false if reqs.include?(em_req)
-    end
-    return true
+    #binding.pry


please delete this, even though it's commented out. :-P

dgoerger · 2014-07-10T19:49:36Z

app/models/reservation_validations.rb

+  # Checks that reservation is not in the past
+  # Does not run on checked out, checked in, overdue, or missed Reservations
+  def not_in_past
+    if due_date < Date.today


Is there a reason not to also check for start_date < Date.today?

Check the discussion on oren's first diff comment

One still shouldn't be able to create a reservation with a start_date in the past.

Fundamental Rewrite of Validation System

squidgetx added 17 commits July 8, 2014 12:31

first round of validation editing

050e5f6

second round

83f1fc5

scaffolding cart validations

713774c

experimetning with cart validations

fbf0ea5

building structure to remove validateSet

e6e2cc5

outsource validation to cart_validations.rb

a8e53a7

appropriate changes to cart and reservation models

4ccf7df

write max count validations

cd922e2

add available, duration, and overlapping for max count

2a6ccfd

renaming files

ead90de

add ability for reservation to be validated by cart validations

a381359

prepare for testing

4d3f10c

re-enable requests functionality

79f3311

allow for unrestricted max checkout length

9cac163

fix specs from application controller

66b271f

adjusting reservation model spec

6e6669b

finish reservation model spec

3d7e684

squidgetx added 6 commits July 8, 2014 22:09

first attempts at reservations_controller'

50929dc

:

changing tests to use valid reservations

ef96a84

fix post create tests

38f077c

set available only to validate on create...

732c089

everything is valid

520db6a

when empty in reservation model spec

289d331

orenyk reviewed Jul 9, 2014
View reviewed changes

squidgetx added 3 commits July 9, 2014 12:40

refactor counting validations

79a3246

adjust not in past validation

8d1a50c

fine tuning

13a269a

orenyk reviewed Jul 10, 2014
View reviewed changes

Merge branch 'development' into 573_validations_lol

57c31db

shippy mentioned this pull request Jul 10, 2014

refactor very complex method create in ReservationsController #351

Closed

fix em requirement

c0dfabf

shippy mentioned this pull request Jul 10, 2014

Refactor ReservationsController#checkin and #checkout #666

Closed

13 tasks

squidgetx added 3 commits July 10, 2014 12:58

fix catalog spec

29193f0

Merge branch 'development' into 573_validations_lol

c14851c

remove casting of keys to string

4719214

This was referenced Jul 10, 2014

Reservation validation failing on Travis CI #647

Closed

Review validations code #573

Closed

squidgetx mentioned this pull request Jul 10, 2014

620 autocomplete #670

Merged

dgoerger mentioned this pull request Jul 10, 2014

Errors and validations on renew of reservation #229

Closed

squidgetx mentioned this pull request Jul 10, 2014

Renewals?? #674

Closed

squidgetx added 2 commits July 10, 2014 14:48

remove unnecessary blackout scope

13cb8f0

date formatting lol

7116b9d

dgoerger reviewed Jul 10, 2014
View reviewed changes

squidgetx added 2 commits July 10, 2014 15:21

remove unnecessary lines

761e8c4

pluralize

469f641

dgoerger reviewed Jul 10, 2014
View reviewed changes

fix date validation

e614951

squidgetx added a commit that referenced this pull request Jul 10, 2014

Merge pull request #644 from YaleSTC/573_validations_lol

9b3674c

Fundamental Rewrite of Validation System

squidgetx merged commit 9b3674c into development Jul 10, 2014

orenyk added this to the 3.4.0 milestone Jul 11, 2014

squidgetx mentioned this pull request Jul 11, 2014

reduces number of db queries in cart validations #680

Merged

squidgetx deleted the 573_validations_lol branch July 22, 2014 16:36

orenyk mentioned this pull request Jan 24, 2016

Availability validation not working? [v5.5] #1449

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fundamental Rewrite of Validation System #644

Fundamental Rewrite of Validation System #644

squidgetx commented Jul 8, 2014

squidgetx commented Jul 8, 2014

orenyk Jul 9, 2014

squidgetx Jul 9, 2014

squidgetx Jul 9, 2014

orenyk Jul 9, 2014

orenyk Jul 10, 2014

orenyk commented Jul 10, 2014

squidgetx commented Jul 10, 2014

orenyk commented Jul 10, 2014

dgoerger Jul 10, 2014

dgoerger Jul 10, 2014

squidgetx Jul 10, 2014

dgoerger Jul 10, 2014

Fundamental Rewrite of Validation System #644

Fundamental Rewrite of Validation System #644

Conversation

squidgetx commented Jul 8, 2014

squidgetx commented Jul 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenyk commented Jul 10, 2014

squidgetx commented Jul 10, 2014

orenyk commented Jul 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment