Add OSRA Branches #24
Conversation
| class Branch < ActiveRecord::Base | ||
|
|
||
| validates :name, presence: true | ||
| validates :code, presence: true, numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than_or_equal_to: 99} |
There was a problem hiding this comment.
Cleaner:
numericality: { only_integer: true }, inclusion: 0..99
There was a problem hiding this comment.
numericality check isn't required since the database column is an integer.
There was a problem hiding this comment.
its useful though if you wan't error messages etc. for free rather than the database just rejecting the data.
There was a problem hiding this comment.
Also would be consistent if our philosophy is to validate everything possible in the model and to use the db as dumb storage
| class Branch < ActiveRecord::Base | ||
|
|
||
| validates :name, presence: true | ||
| validates :code, presence: true, numericality: { only_integer: true}, inclusion: 0..99 |
There was a problem hiding this comment.
Is numericality: { only_integer: true} required? The database column in already an integer.
There was a problem hiding this comment.
I did a quick test. Without numericality: { only_integer: true} you can assign a character string for instance to the code and it will saved in the db as 0.
There was a problem hiding this comment.
I think you need it anyway. The discussion were having about whether we do additional db validations creates at least one common theme amongst everyone.
The models should be the one source of truth. Leaving the integer declaration out seems to me to contravene that.
|
Please rebase with latest code. |
…lper This was newly introduced in Rails 4.1: https://relishapp.com/rspec/rspec-rails/v/3-0/docs/upgrade#pending-migration-checks
motasem-salem
force-pushed
the
branches
branch
from
8941e43
to
7eddc72
Compare
PT Story: https://www.pivotaltracker.com/story/show/77972984