-
-
Notifications
You must be signed in to change notification settings - Fork 527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
5037 default the quantity per individual to 50 in the data #5080
base: main
Are you sure you want to change the base?
5037 default the quantity per individual to 50 in the data #5080
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional check: works as expected.
Over to @dorner for technical comments.
app/views/items/_form.html.erb
Outdated
@@ -27,7 +27,7 @@ | |||
<% end %> | |||
|
|||
<%= f.input :name, label: "Quantity Per Individual", wrapper: :input_group do %> | |||
<%= f.input_field :distribution_quantity, class: "form-control" %> | |||
<%= f.input_field :distribution_quantity, class: "form-control", value: f.object.distribution_quantity || (f.object.kit_id.present? ? 1 : 50) %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this if we have an after_initialize
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would be helpful for the user to see what the default was in the form. I just removed it and learned that we don't need it after all. Thank you for pointing this out.
spec/models/item_spec.rb
Outdated
describe "when distribution_quantity is set by default" do | ||
it "should set distribution_quantity to 50 for regular items" do | ||
item = Item.new | ||
item.valid? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't need to call this - your callback is after_initialize
, not before_validate
.
I closed and reopened this PR to trigger CI. Is there a better way? |
AFAIK, CI engages whenever you put up a change. |
It does, however, in this case I had a failing test in CI and was absolutely certain it should have passed and wanted to trigger another run without making a change. Hopefully closing and reopening is ok. If not I'll make note of that if it comes up in the future. |
Hmm -- I almost certainly have more rights than you do -- there is an option to rerun the job, which you might not have. We do have some flakey tests, so if the test that is failing doesn't have anything to do with your changes, I wouldn't worry too much about it until one of the reviewers calls it out. |
REFERENCE: #5037
Checklist:
[x] I have performed a self-review of my own code,
[x] I have commented my code, particularly in hard-to-understand areas,
[x] I have made corresponding changes to the documentation,
[x] I have added tests that prove my fix is effective or that my feature works,
[x] New and existing unit tests pass locally with my changes ("bundle exec rake"),
[x] Title include "WIP" if work is in progress.
[x] I acknowledge that I will not force push my branch once reviews have started.
-->
Resolves #5037
Description
Type of change
How Has This Been Tested?