-
-
Notifications
You must be signed in to change notification settings - Fork 486
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
Product totals - custom units #4609
base: main
Are you sure you want to change the base?
Conversation
Verified that in production there are zero of these instances
@@ -23,23 +23,8 @@ | |||
end | |||
end | |||
|
|||
context 'when item name is nil' 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.
From a production snapshot:
pry(main)> Partners::ItemRequest.where(name: nil).count
Partners::ItemRequest Count (15.1ms) SELECT COUNT(*) FROM "item_requests" WHERE "item_requests"."name" IS NULL
=> 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.
I'm wondering which is better, |
Which is better is a fair question. Is it a Wednesday question? |
I'd probably prefer it like this:
|
I think (in packs) implies the number of pads that happen to be in packs, as opposed to the number of packs. I kind of like the parentheses, though. This is still the whole naming things issue - but I think we should be consistent between what we use for the partners and what we use for the banks -- because they do have conversations. How about:
I don't like "units", because of the naming conflict with "Custom Request Units" , but it's what we got from the banks. We could check if "pieces" would work for them, maybe? |
From our discussion on the call today, this is now updated to use plural when units are specified and blank otherwise. |
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.
Implement #4408