Review validations code

[dgoerger]

We should review the code in models/reservation_validations.rb, and clean it up. Some functions may not be used any more (no_overdue_reservations? ?), we should use scopes where they exist rather than duplicate SQL queries, edge cases which say "return true" to avoid errors should maybe be reevaluated (not_renewable?), and we should make sure validations run over the correct sets rather than over everything always (not_renewable? should use the "active" scope).

Additionally, do the errors added in the validation tests get called or used anywhere? We add errors manually in validate_set. If this duplication serves no purpose, we could greatly simplify our validations by turning them into straightforward booleans.

[squidgetx]

• Remove errors and see what happens. I'm 90% sure they're never used
• Clean up .available?, it makes at least 3 queries for every model for every day the cart is set. Especially then on the catalog that's >100 queries for 2 day cart (the default) with 50 equipment models

[mnquintana]

Is this part of #586 now?

[squidgetx]

@mnquintana I think this thread is a good place to keep track of refactoring of existing code/functionality, where #586 is more for general restructuring discussion

[squidgetx]

Ok, so the errors added in the ReservationValidations model are are actually quite important and serve to validate the reservation in the truest, ActiveRecord sense of the word. (see here)

However, this is dumb because our method validateSet duplicates both the error messages themselves and the logic of the errors. The reason why there are booleans floating around everywhere is so those methods can work with validateSet.

We should distinguish the kind of validation that is 'essential' (and should be an official ActiveRecord validation) and 'softer' validations, that are dependent on many external factors and that can be overwritten by admins/checkout people. These latter validations will be easier to deal with outside of the stricter conditions required for ActiveRecord validations.

Essential ActiveRecord validations:

• not_empty?
• start_date_before_due_date
• matched_object_and_model
• ...available?

[dgoerger]

with #587, if we get rid of the fix_due_date method, we'll need this set as a hard-validation so no-one will be able to file requests per #297 over invalid date ranges.

[squidgetx]

#644