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

Adds query for all an organization's items to the CSV export service, updates spec to test new behavior #4070

Merged
merged 4 commits into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions app/services/exports/export_distributions_csv_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ def initialize(distributions:, filters: [])
# service object.
@distributions = distributions
@filters = filters
@organization = @distributions.first.organization
end

def generate_csv
Expand Down Expand Up @@ -114,15 +115,7 @@ def base_headers
def item_headers
return @item_headers if @item_headers

item_names = Set.new

distributions.each do |distribution|
distribution.line_items.each do |line_item|
item_names.add(line_item.item.name)
end
end

@item_headers = item_names.sort
@item_headers = @organization.items.uniq.sort_by(&:created_at).pluck(:name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't doing what you think it is :) It's first loading all items into memory and then sorting and mapping them. I think you wanted

@item_headers = @organization.items.order(:created_at).distinct.select([:created_at, :name]).map(&:name)

which does it purely in the database.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For posterity: My comment came across as condescending. A better wording would be something like: "This is doing queries in the app instead of in the database; it would be much faster to do the query this way."

end

def build_row_data(distribution)
Expand All @@ -133,6 +126,8 @@ def build_row_data(distribution)
distribution.line_items.each do |line_item|
item_name = line_item.item.name
item_column_idx = headers_with_indexes[item_name]
next unless item_column_idx

row[item_column_idx] += line_item.quantity
end

Expand Down
1 change: 1 addition & 0 deletions spec/requests/partners_requests_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@

context "csv" do
let(:response_format) { 'csv' }
before { create(:distribution, partner: partner) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this added? It doesn't seem relevant to the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without creating a distro for the partner, the export method that's called in the csv format fails with nil distributions. Instead let me put a nil catch in the export.


it { is_expected.to be_successful }
end
Expand Down
53 changes: 45 additions & 8 deletions spec/services/exports/export_distributions_csv_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,13 @@
let(:item_id) { distributions.flatten.first.line_items.first.item_id }
let(:filters) { {by_item_id: item_id} }
let(:item_name) { Item.find(item_id).name }
let(:organization) { distributions.first.organization }

let(:all_org_items) { Item.where(organization:).uniq.sort_by(&:created_at) }

let(:total_item_quantities) do
template = item_names.index_with(0)
# binding.pry
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line can be removed.

template = all_org_items.pluck(:name).index_with(0)

items_lists.map do |items_list|
row = template.dup
Expand All @@ -64,7 +68,7 @@
end
end

let(:expected_headers) do
let(:non_item_headers) do
[
"Partner",
"Date of Distribution",
Expand All @@ -76,14 +80,10 @@
"State",
"Agency Representative",
"Comments"
] + expected_item_headers
]
end

let(:expected_item_headers) do
expect(item_names).not_to be_empty

item_names
end
let(:expected_headers) { non_item_headers + all_org_items.pluck(:name) }

it 'should match the expected content for the csv' do
expect(subject[0]).to eq(expected_headers)
Expand All @@ -107,5 +107,42 @@
expect(subject[idx + 1]).to eq(row)
end
end

context 'when a new item is added' do
let!(:original_columns_count) { organization.items.size + non_item_headers.size }
let(:new_item_name) { "new item" }
before do
create(:item, name: new_item_name, organization:)
end

it 'should add it to the end of the row' do
expect(subject[0].uniq).to eq(expected_headers)
.and end_with(new_item_name)
.and have_attributes(size: original_columns_count + 1)
end

it 'should show up with a 0 quantity if there are no distributions' do
distributions.zip(total_item_quantities).each_with_index do |(distribution, total_item_quantity), idx|
row = [
distribution.partner.name,
distribution.issued_at.strftime("%m/%d/%Y"),
distribution.storage_location.name,
distribution.line_items.where(item_id: item_id).total,
distribution.cents_to_dollar(distribution.line_items.total_value),
distribution.delivery_method,
"$#{distribution.shipping_cost.to_f}",
distribution.state,
distribution.agency_rep,
distribution.comment
]

row += total_item_quantity

expect(subject[idx + 1]).to eq(row)
.and end_with(0)
.and have_attributes(size: original_columns_count + 1)
end
end
end
end
end
Loading