4310 - Update item quantity validation error message #4412
Conversation
vsrnth
changed the title
app/models/line_item.rb
Outdated
| @@ -21,7 +21,7 @@ class LineItem < ApplicationRecord | |||
| belongs_to :item | |||
|
|
|||
| validates :item_id, presence: true | |||
| validates :quantity, numericality: { only_integer: true, less_than: MAX_INT, greater_than: MIN_INT } | |||
| validates :quantity, numericality: { only_integer: true, less_than: MAX_INT, greater_than: MIN_INT, message: "is not a number - Note: commas are not allowed" } | |||
There was a problem hiding this comment.
Nitpick: Can we replace - with a period .? "is not a number. Note: commas are not allowed"
spec/system/donation_system_spec.rb
Outdated
| @@ -418,7 +418,7 @@ | |||
| expect(page).to have_xpath("//select[@id='donation_line_items_attributes_0_item_id']") | |||
| end.not_to change { Donation.count } | |||
| expect(page).to have_content("Start a new donation") | |||
| expect(page).to have_content("must be less than") | |||
| expect(page).to have_content("Quantity is not a number - Note: commas are not allowed") | |||
There was a problem hiding this comment.
um... why? This is a number, isn't it?
There was a problem hiding this comment.
Line 412 is setting the quantity to 10000000000000000000000, this quantity value is bigger than the configured limit of 2**31 = 2,147,483,648 in the validation
There was a problem hiding this comment.
Perhaps we need to split up the validation into checking for non-numeric characters and checking for range.
There was a problem hiding this comment.
We're checking for non-numeric characters in the unit tests. Do I need to do that for the system tests as well?
There was a problem hiding this comment.
No - it's just a weird error message since I'd expect to test a string like "abc" here instead of a number that's too large.
There was a problem hiding this comment.
i.e. let's make the tests explicit that the input is a number with commas rather than relying on implicit changes that happen in the controller / http layer.
There was a problem hiding this comment.
I'm a little confused. The original test was about overshooting the maximum. @vsnrth is changing the meaning of the test, leaving overshooting the maximum untested.... We should still have that test, no?
There was a problem hiding this comment.
Yes - we need both tests (one where it doesn't get a number and one where the number is too high).
There was a problem hiding this comment.
Updated the code to handle the above. We get a different message when the limit is crossed and when a string is entered.
- Updated validation message - Updated related unit and system specs
- Updates system specs and adds required unit tests
|
Looks good to me. @cielf did you want to kick the tires? |
|
Looks good to me -- i noticed a different issue, but it's out of scope of this change. |
|
Let's merge after the release, though. |
|
@vsrnth: Your PR |
Resolves #4310
Screenshots