-
-
Notifications
You must be signed in to change notification settings - Fork 473
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
Add models to support request units #4306
Conversation
@dorner Hrm. I have to apologize -- I didn't come back into this issue and address the fact that we had settled on just "units" for the default units. |
@dorner I think we're going to need something on ItemRequest as well -- that' s the level that the packs or whatever will actually be requested. |
@cielf not sure how using "units" as the default changes anything... that should be purely on the display side. We said that we'd stick with I've added the item request piece. |
Well, the description in the issue of what's required for the seed indicates diapers,and pads, rather than units. That would change. |
I've already addressed that in the seed - the defaults don't include |
@dorner Cool. I also think we don't need "reporting unit" on item -- that would always be "unit" , the way we are thinking of things now, right? |
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.
Hey @dorner -- Thanks for getting on this early!
There's one thing that I think we can yank because we won't need it if we're always using "units", 1 I think we can (/should?) do without the explicit flag I originally suggested, and 1 thing where i think we should use a different name for clarity.
safety_assured do | ||
add_column :organizations, :uses_request_units, :boolean, null: false, default: false | ||
add_column :items, :reporting_unit, :string | ||
end |
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.
As commented elsewhere, I think we can get rid of the reporting unit on items.
I also wonder if we really need the explicit uses_request_units on Organization or if that would be served as well with a wee routine that checks whether the Organization has any request units. We aren't explicitly asking the users if they use custom request units, but just have them put them in if we are. Thoughts?
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'm good with that!
# Conflicts: # db/schema.rb # spec/models/item_spec.rb
@cielf pushed the fixes! So now it seems like all we have are organization request units, and a specific request unit selected on an item request. And the only validation is that the request unit on the item request needs to exist on the org. |
Hmm. That doesn't sound right? Let me take another look. We may have had a terminology mixup. |
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.
Looking at what we have now, I see what I missed the first time, so there are some requested changes..
But also, for discussion:
I think this could benefit from changing some class names.
ItemRequestUnit is potentially confusing, given that we have ItemRequest as well -- it's currently the "requestunit" that belongs to item -- but to anyone who is looking at it new, it could well sound like the unit on an ItemRequest.
What are the pros and cons to changing "RequestUnit" to "Unit" (or maybe "CustomUnit" ?) and "ItemRequestUnit" to "ItemUnit". On the one hand-- we're only dealing with requests at this point, not distributions, donations... but if we ever had to support different units on distributions/purchases/donations/kits (which doesn't seem that likely) , we could add a unit purpose to Unit?
What do you think?
return if request_unit.blank? | ||
|
||
names = request.organization.request_units.map(&:name) | ||
unless names.include?(request_unit) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
@@ -26,6 +27,20 @@ | |||
it { should validate_numericality_of(:quantity).only_integer.is_greater_than_or_equal_to(1) } | |||
it { should validate_presence_of(:name) } | |||
it { should validate_presence_of(:partner_key) } | |||
|
|||
it "should only be able to use organization's request units" do |
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.
So this would change to "should only be able to use item's item request units" with the corresponding changes to the test...
Ooops wrong PR. @cielf changes not addressed :) |
@cielf updated! |
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.
One nit pick, and I want @awwaiid to do a pass.
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.
Yup. I think this works now. Would like @awwaiid to take a look.
db/seeds.rb
Outdated
pdx_org.items.each_with_index do |item, i| | ||
if item.name == 'Pads' | ||
%w(pad pack).each { |name| item.request_units.create!(name: name) } | ||
elsif i == 0 |
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.
Can we do names or something other than index for these?
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.
Done!
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.
Looks like I missed the seed entirely on my pass! If I'm reading things correctly, we aren't creating any ItemRequests with units in the seed. We probably should , for the benefit of people working on the later pack issues.
db/seeds.rb
Outdated
# ---------------------------------------------------------------------------- | ||
|
||
%w(pack pad flat).each do |name| | ||
RequestUnit.create!(organization: pdx_org, name: name) |
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.
Should this be Unit?
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.
Fixed! And added item requests.
@dorner This is nitpicky and maybe a bit out of scope -- I ran the setup, and inspected the results -- I see a request with two ItemRequests for pads with different units. If we go forward in time to Packs #2, we aren't going to allow that. So maybe we should just have one level of item (in this case either packs or units) for each item on a request to avoid new developer confusion later? |
I'm ok with this as-is and can help make sure that packs-#2 doesn't get confused based on the existing test data. |
@dorner: Your PR |
Resolves #4096
Description
This creates the models and fields necessary to support reporting units. For now this is a model-only change. This adds some simple validations and specs for them, as well as seed data.