Resolves #4686 add another bank to seed #4826
Conversation
…ted new partner with new bank
…all orgs, removed redundant Product Drives section
…storage locations are selected to accommodate there being storage locations for multiple orgs
…or pdx_org and sc_org
|
Hey @Benjamin-Couey - I'm liking what I'm seeing from a functional pov. With the second city items, I have one question, though. Are we guaranteed to have at least one each of donation, purchase, distribution, and request among those 4 items? My concern is false positives when we are using them for manual testing -- that, with these extra items, we would rely on there being instances with them, and if there aren't, we might assume that everything is fine. |
|
@cielf There is currently no guarantee that the second city items will be used for the donations, purchases, distributions, and requests. I can work on explicitly adding examples of those records using the second city items. |
|
(Nods) Yes please. |
… for donations, purchases, distributions, and requests
…he new bank's items, guarantee that at least one of the items unique to the new bank has an inventory less than the recommended quantity
|
Alright, I pushed two commits to ensure that there are examples of donations, purchases, distributions, and requests using the unique items and that one of the unique items has a lower stock than its recommended quantity at all new storage locations. |
|
Thanks @Benjamin-Couey . Just to manage expectations, we're a bit snowed under on the reviewing side right now, and our main technical staff are not available for the next few days, so it might be a few days before this gets final-reviewed. |
| # At least one of the items is marked as inactive | ||
| Organization.all.each do |org| | ||
| Organization.all.find_each do |org| |
There was a problem hiding this comment.
nit: I think we weren't using find_each in this file because it's useful when you have many records in the db. And I don't believe we will have more than a few hundred records of any type during seeding.
But either way works.
There was a problem hiding this comment.
Using find_each instead of each was actually a change made by the linter. If the use of each was intentional, perhaps we need to configure the linter to not automatically make this change?
There was a problem hiding this comment.
find_each is pretty much always a better approach than each. There isn't really a case where the other way around is better.
db/seeds.rb
Outdated
| # Add a couple unique items based on random base items named after the sc_bank | ||
| # so it will be clear if they are showing up where they aren't supposed to be | ||
| 4.times.each do |time| | ||
| sc_org.seed_random_item_with_name("Second City Item ##{time + 1}") |
There was a problem hiding this comment.
nit: Not sure the value of abstracting this logic into a new method. We could just create four new items here and then call sc_org.seed_items(second_city_only_items). And having all the logic in this file makes it easier to read imo.
But up to you.
Also another nit, I think the block parameter would normally be named index or i here. But doesn't really matter.
There was a problem hiding this comment.
Yes, time should absolutely be index as that convention is used elsewhere in the file. It's even used elsewhere in this PR.
CONTRIBUTING.md
Outdated
| Password: password! | ||
| ``` | ||
|
|
||
| Second City Essentials Bank |
There was a problem hiding this comment.
Would it be worth it to specify what the difference is between these accounts?
There was a problem hiding this comment.
It certainly would.
The biggest difference is that SF Diaper Bank is a brand new bank that hasn't been set up yet.
Now that you mention it, I'd suggest putting ordering them as Pawnee, then Second City, then SF, with a note on SF Diaper bank at the end with a note that it is a freshly accepted bank (i.e. not yet set up)
We might tune Second City to have more particular characteristics as we find things that are hard to test on Pawnee, but for now its main purpose is so we have multiple in-flight organizations while we test.
app/models/organization.rb
Outdated
| @@ -223,6 +223,13 @@ def seed_items(item_collection) | |||
| reload | |||
| end | |||
|
|
|||
| def seed_random_item_with_name(name) | |||
There was a problem hiding this comment.
This shouldn't go on Organization if it's only used to create seed data. It should be a method in seeds.rb.
There was a problem hiding this comment.
Will do. Am I correct in assuming I should then discard the associated unit test, since the other functions in seed.rb lack them?
db/seeds.rb
Outdated
| org.items.order(created_at: :desc).last.update(active: false) | ||
| end | ||
|
|
||
| # Add a couple unique items based on random base items named after the sc_bank | ||
| # so it will be clear if they are showing up where they aren't supposed to be | ||
| 4.times.each do |index| |
There was a problem hiding this comment.
You don't need each, you can just do 4.times do .
|
Noting there are some conflicts to be resolved. |
|
Apologies for the delay, but commits have been made to address @dorner's requests and resolve the merge conflicts with |
|
Looks good - thanks! |
|
@Benjamin-Couey: Your PR |
Resolves #4685
Description
This PR modifies the
db/seeds.rbfile to add another diaper bank with a partner and other associated records.CONTRIBUTING.mdwas also updated to include the login information for the new diaper bank and partner.In total, the following records were added to the seed:
OrganizationSecond City Essentials BankBaseItem, named "Second City Item 1"Users for the new bank, "second_city_admin@example.com" and "second_city_user@example.com"Donations andDistributions for the new bankPurchases for the new bankDonationSites for the new bank, based on the city and state of the bank.StorageLocations for the new bank.Units,ProductDrives,ProductDriveParticipants,Manufacturers, and 'Vendor's for the new bank.Partner"Second City Senior Center"Requests made by the new partner to the new bank (this was handled by exitingPartnerseeding code).The seed uses both hand-written values and the
Fakerlibrary to populate addresses, emails, etc. When creating entirely new records, I favoredFakerfor the sake of better looking demos but didn't update existing hand-written values.For most of the records associated with an
Organization, the seed only created records for the Pawnee Diaper Bank. I updated these sections to only create records for the new bank and not the existing SF Diaper Bank as they was not requested by the issue. I tried to modify the code so that it would be easy to define a list of banks for which these records should be created.Type of change
How Has This Been Tested?
As far as I could tell, there were no automated tests associated with the seed and so relied on manual verification. I verified that
rake db:resetruns without issues with the modified seed and manually verified that the database contained the new records associated with the new bank.I created a unit test for a utility function added to
Organization.