Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

nathangthomas
Copy link
Contributor

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

  1. When creating a new item through the interface, the value for quantity per individual is defaulted to 50
  2. When we create a kit, that value on the accompanying item (the one that represents the kit) is defaulted to 1
  3. Migration to set blank quantities per individual on items to 50, unless the item represents a kit, in which case a blank quantity will be set to 1.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Copy link
Collaborator

@cielf cielf left a 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.

@cielf cielf requested a review from dorner March 7, 2025 19:31
@@ -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) %>
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

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?
Copy link
Collaborator

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.

@nathangthomas nathangthomas requested a review from dorner March 7, 2025 22:25
@nathangthomas nathangthomas reopened this Mar 7, 2025
@nathangthomas
Copy link
Contributor Author

I closed and reopened this PR to trigger CI. Is there a better way?

@cielf
Copy link
Collaborator

cielf commented Mar 10, 2025

AFAIK, CI engages whenever you put up a change.

@nathangthomas
Copy link
Contributor Author

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.

@cielf
Copy link
Collaborator

cielf commented Mar 11, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default the quantity per individual to 50 in the data, except for kits, which should be 1.
3 participants