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

Product totals - custom units #4609

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
38 changes: 14 additions & 24 deletions app/services/requests_total_items_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,39 +6,29 @@ def initialize(requests:)
def calculate
return unless requests

request_items_array = []

request_items.each do |items|
items.each do |json|
request_items_array << [item_name(json['item_id']), json['quantity']]
end
totals = {}
item_requests.each do |item_request|
name = item_name(item_request)
totals[name] ||= 0
totals[name] += item_request.quantity.to_i
Comment on lines +9 to +13
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
totals = {}
item_requests.each do |item_request|
name = item_name(item_request)
totals[name] ||= 0
totals[name] += item_request.quantity.to_i
totals = Hash.new(0)
item_requests.each do |item_request|
name = item_name(item_request)
totals[name] += item_request.quantity.to_i

Passing a value into Hash.new makes it the default value whenever you try to fetch anything out of it. So the last line Just Works ™️ .

end

request_items_array.inject({}) do |item, (quantity, total)|
item[quantity] ||= 0
item[quantity] += total.to_i
item
end
totals
end

private

attr_accessor :requests

def request_items
@request_items ||= requests.pluck(:request_items)
end

def request_items_ids
request_items.flat_map { |jitem| jitem.map { |item| item["item_id"] } }
def item_requests
@item_requests ||= requests.flat_map(&:item_requests)
end

def items_names
@items_names ||= Item.where(id: request_items_ids).as_json(only: [:id, :name])
end

def item_name(id)
item_found = items_names.find { |item| item["id"] == id }
item_found&.fetch('name') || '*Unknown Item*'
def item_name(item_request)
if Flipper.enabled?(:enable_packs) && item_request.request_unit.present?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we extract this into a method in a different PR?

"#{item_request.name} - #{item_request.request_unit.pluralize}"
else
item_request.name
end
end
end
2 changes: 1 addition & 1 deletion app/views/requests/_calculate_product_totals.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
</tr>
</thead>
<tbody>
<% @calculate_product_totals.sort_by { |name, quantity| -quantity}.each do |name, quantity| %>
<% @calculate_product_totals.sort_by { |name, quantity| name.downcase }.each do |name, quantity| %>
<tr>
<td><%= name %></td>
<td><%= quantity %></td>
Expand Down
43 changes: 25 additions & 18 deletions spec/services/requests_total_items_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,42 +4,49 @@
subject { described_class.new(requests: requests).calculate }

context 'when the request items is not blank' do
let(:sample_items) { create_list(:item, 3, organization: organization) }
let(:sample_items) { create_list(:item, 3, :with_unit, organization: organization, unit: "bundle") }
let(:item_names) { sample_items.pluck(:name) }
let(:item_ids) { sample_items.pluck(:id) }
let(:requests) do
[
create(:request, request_items: item_ids.map { |k| { "item_id" => k, "quantity" => 20 } }),
create(:request, request_items: item_ids.map { |k| { "item_id" => k, "quantity" => 10 } })
create(:request, :with_item_requests, request_items: item_ids.map { |k| { "item_id" => k, "quantity" => 20 } }),
create(:request, :with_item_requests, request_items: item_ids.map { |k| { "item_id" => k, "quantity" => 10, "request_unit" => "bundle" } }),
create(:request, :with_item_requests, request_items: item_ids.map { |k| { "item_id" => k, "quantity" => 50, "request_unit" => "bundle" } })
]
end

it 'return items with correct quantities calculated' do
expect(subject.first.last).to eq(30)
expect(subject.first.last).to eq(80)
end

it 'return the names of items correctly' do
expect(subject.keys).to eq(item_names)
end
end

context 'when item name is nil' do
Copy link
Collaborator Author

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

let(:item) do
i = build(:item, name: nil)
i.save(validate: false)
i
end
let(:requests) do
[create(:request, request_items: [{ "item_id" => item.id, "quantity" => 20 }])]
end

it 'return Unknown Item' do
expect(subject.first.first).to eq('*Unknown Item*')
context 'when custom request units are specified and enabled' do
before do
Flipper.enable(:enable_packs)
end

it 'return the names of items correctly' do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it 'return the names of items correctly' do
it 'returns the names of items correctly' do

expect(subject.keys).to eq(item_names + item_names.map { |k| "#{k} - bundles" })
end

it 'return items with correct quantities calculated' do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it 'return items with correct quantities calculated' do
it 'returns items with correct quantities calculated' do

expect(subject).to eq({
sample_items.first.name => 20,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we hardcode these names please?

sample_items.first.name + " - bundles" => 60,
sample_items.second.name => 20,
sample_items.second.name + " - bundles" => 60,
sample_items.third.name => 20,
sample_items.third.name + " - bundles" => 60
})
end
end
end

context 'when provided with requests that have no request items' do
let(:requests) { [create(:request, request_items: {})] }
let(:requests) { [create(:request, :with_item_requests, request_items: {})] }

it { is_expected.to be_blank }
end
Expand Down