-
-
Notifications
You must be signed in to change notification settings - Fork 573
Bug-5152-update-purchase-export-with-inactive-unpurchased-items #5201
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
013f77c
00c0acc
f71fca3
42d24bc
43b57e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,18 @@ | ||
| module Exports | ||
| class ExportPurchasesCSVService | ||
| def initialize(purchase_ids:) | ||
| def initialize(purchase_ids:, organization:) | ||
| # Use a where lookup so that I can eager load all the resources | ||
| # needed rather than depending on external code to do it for me. | ||
| # This makes this code more self contained and efficient! | ||
| @purchases = Purchase.includes( | ||
| @organization = organization | ||
| @purchases = organization.purchases.includes( | ||
| :storage_location, | ||
| :vendor, | ||
| line_items: [:item] | ||
| ).where( | ||
| id: purchase_ids | ||
| ).order(created_at: :asc) | ||
| @item_headers = @organization.items.select("DISTINCT ON (LOWER(name)) items.name").order("LOWER(name) ASC").map(&:name) | ||
| end | ||
|
|
||
| def generate_csv | ||
|
|
@@ -34,7 +36,7 @@ def generate_csv_data | |
|
|
||
| private | ||
|
|
||
| attr_reader :purchases | ||
| attr_reader :purchases, :item_headers | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this read from outside the class? |
||
|
|
||
| def headers | ||
| # Build the headers in the correct order | ||
|
|
@@ -60,7 +62,6 @@ def headers_with_indexes | |
| # (or on the order of the literal). | ||
| def base_table | ||
| { | ||
|
|
||
| "Purchases from" => ->(purchase) { | ||
| purchase.vendor.try(:business_name) | ||
| }, | ||
|
|
@@ -101,20 +102,6 @@ def base_headers | |
| base_table.keys | ||
| end | ||
|
|
||
| def item_headers | ||
| return @item_headers if @item_headers | ||
|
|
||
| item_names = Set.new | ||
|
|
||
| purchases.each do |purchase| | ||
| purchase.line_items.each do |line_item| | ||
| item_names.add(line_item.item.name) | ||
| end | ||
| end | ||
|
|
||
| @item_headers = item_names.sort | ||
| end | ||
|
|
||
| def build_row_data(purchase) | ||
| row = base_table.values.map { |closure| closure.call(purchase) } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,11 @@ | ||
| RSpec.describe Exports::ExportPurchasesCSVService do | ||
| describe "#generate_csv_data" do | ||
| subject { described_class.new(purchase_ids: purchase_ids).generate_csv_data } | ||
| let(:organization) { create(:organization) } | ||
| subject { described_class.new(purchase_ids: purchase_ids, organization: organization).generate_csv_data } | ||
| let(:purchase_ids) { purchases.map(&:id) } | ||
| let(:duplicate_item) do | ||
| FactoryBot.create( | ||
| :item, name: Faker::Appliance.unique.equipment | ||
| :item, name: Faker::Appliance.unique.equipment, organization: organization | ||
| ) | ||
| end | ||
| let(:items_lists) do | ||
|
|
@@ -13,15 +14,15 @@ | |
| [duplicate_item, 5], | ||
| [ | ||
| FactoryBot.create( | ||
| :item, name: Faker::Appliance.unique.equipment | ||
| :item, name: Faker::Appliance.unique.equipment, organization: organization | ||
| ), | ||
| 7 | ||
| ], | ||
| [duplicate_item, 3] | ||
| ], | ||
| *(Array.new(3) do |i| | ||
| [[FactoryBot.create( | ||
| :item, name: Faker::Appliance.unique.equipment | ||
| :item, name: Faker::Appliance.unique.equipment, organization: organization | ||
| ), i + 1]] | ||
| end) | ||
| ] | ||
|
|
@@ -35,8 +36,9 @@ | |
| items_lists.each_with_index.map do |items, i| | ||
| purchase = create( | ||
| :purchase, | ||
| organization: organization, | ||
| vendor: create( | ||
| :vendor, business_name: "Vendor Name #{i}" | ||
| :vendor, business_name: "Vendor Name #{i}", organization: organization | ||
| ), | ||
| issued_at: start_time + i.days, | ||
| comment: "This is the #{i}-th purchase in the test.", | ||
|
|
@@ -114,5 +116,84 @@ | |
| expect(subject[idx + 1]).to eq(row) | ||
| end | ||
| end | ||
|
|
||
| context "when an organization's item exists but isn't in any purchase" do | ||
| let(:unused_item) { create(:item, name: "Unused Item", organization: organization) } | ||
| let(:generated_csv_data) do | ||
| # Force unused_item to be created first | ||
| unused_item | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we replace line 121 with |
||
| described_class.new(purchase_ids: purchases.map(&:id), organization: organization).generate_csv_data | ||
| end | ||
|
|
||
| it "should include the unused item as a column with 0 quantities" do | ||
| expect(generated_csv_data[0]).to include(unused_item.name) | ||
|
|
||
| purchases.each_with_index do |_, idx| | ||
| row = generated_csv_data[idx + 1] | ||
| item_column_index = generated_csv_data[0].index(unused_item.name) | ||
| expect(row[item_column_index]).to eq(0) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| context "when an organization's item is inactive" do | ||
| let(:inactive_item) { create(:item, name: "Inactive Item", active: false, organization: organization) } | ||
| let(:generated_csv_data) do | ||
| # Force inactive_item to be created first | ||
| inactive_item | ||
| described_class.new(purchase_ids: purchases.map(&:id), organization: organization).generate_csv_data | ||
| end | ||
|
|
||
| it "should include the inactive item as a column with 0 quantities" do | ||
| expect(generated_csv_data[0]).to include(inactive_item.name) | ||
|
|
||
| purchases.each_with_index do |_, idx| | ||
| row = generated_csv_data[idx + 1] | ||
| item_column_index = generated_csv_data[0].index(inactive_item.name) | ||
| expect(row[item_column_index]).to eq(0) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| context "when generating CSV output" do | ||
| let(:generated_csv) { described_class.new(purchase_ids: purchase_ids, organization: organization).generate_csv } | ||
|
|
||
| it "returns a valid CSV string" do | ||
| expect(generated_csv).to be_a(String) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we do a test against "real" CSV data instead of just testing bits of it?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally can if you want -- it's kinda unnecessary imo. Most of the test contentions are against I rearranged the file a bit so you can see this more clearly, with a #generate_csv_data and a #generate_csv context blocks. If you still want that in, let me know :)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's really kind of turning a set of unit tests into an integration test. Each individual part might be tested, but until we see a full CSV that we're comparing, with as few dynamic values as possible, we can't really know that we're doing it right. So we create data with specific values, generate the CSV, and compare against our result. Our comparison CSV shouldn't have anything dynamic except auto-generated IDs. |
||
| expect { CSV.parse(generated_csv) }.not_to raise_error | ||
| end | ||
|
|
||
| it "includes headers as first row" do | ||
| csv_rows = CSV.parse(generated_csv) | ||
| expect(csv_rows.first).to eq(expected_headers) | ||
| end | ||
|
|
||
| it "includes data for all purchases" do | ||
| csv_rows = CSV.parse(generated_csv) | ||
| expect(csv_rows.count).to eq(purchases.count + 1) # +1 for headers | ||
| end | ||
| end | ||
|
|
||
| context "when items have different cases" do | ||
| let(:item_names) { ["Zebra", "apple", "Banana"] } | ||
| let(:expected_order) { ["apple", "Banana", "Zebra"] } | ||
| let(:purchase) { create(:purchase, organization: organization) } | ||
| let(:case_sensitive_csv_data) do | ||
| # Create items in random order to ensure sort is working | ||
| item_names.shuffle.each do |name| | ||
| create(:item, name: name, organization: organization) | ||
| end | ||
|
|
||
| described_class.new(purchase_ids: [purchase.id], organization: organization).generate_csv_data | ||
| end | ||
|
|
||
| it "should sort item columns case-insensitively, ASC" do | ||
| # Get just the item columns by removing the known base headers | ||
| item_columns = case_sensitive_csv_data[0] - expected_headers[0..-4] # plucks out the 3 items at the end | ||
|
|
||
| # Check that the remaining columns match our expected case-insensitive sort | ||
| expect(item_columns).to eq(expected_order) | ||
| end | ||
| end | ||
| end | ||
| 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.
Does this need to be an instance variable?