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.

Sign up for GitHub

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

Test inventory report to use variant overrides #3654

Merged
merged 1 commit into from
Apr 3, 2019
Merged

Test inventory report to use variant overrides #3654

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 26 additions & 0 deletions spec/lib/open_food_network/products_and_inventory_report_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,32 @@ module OpenFoodNetwork
subject.stub(:params).and_return(distributor_id: distributor.id)
subject.filter(variants).should == [product2.variants.first]
end

it "ignores variant overrides without filter" do
distributor = create(:distributor_enterprise)
product = create(:simple_product, supplier: supplier, price: 5)
variant = product.variants.first
order_cycle = create(:simple_order_cycle, suppliers: [supplier], distributors: [distributor], variants: [product.variants.first])
create(:variant_override, hub: distributor, variant: variant, price: 2)

result = subject.filter(variants)

expect(result.first.price).to eq 5
end

it "considers variant overrides with distributor" do
distributor = create(:distributor_enterprise)
product = create(:simple_product, supplier: supplier, price: 5)
variant = product.variants.first
order_cycle = create(:simple_order_cycle, suppliers: [supplier], distributors: [distributor], variants: [product.variants.first])
create(:variant_override, hub: distributor, variant: variant, price: 2)

allow(subject).to receive(:params).and_return(distributor_id: distributor.id)
Copy link
Contributor

@sauloperez sauloperez Mar 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It is a bad practice to stub the subject of the test because we are then not testing the actual implementation. Do you see a way to overcome this? I guess we need to pass a different set of params instead but I haven't checked the report's implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that it's bad practice. But I think fixing it is beyond this pull request.

  1. I used the same pattern as in the rest of the spec file. Solving this problem would be an improvement but it should be done for the whole file.
  2. The params method is not provided by this controller but by one of the ancestors. Rails provides it. That's why I think it's okay to stub it. Ideally, all that filtering would happen in a service that is taking the params as input and we didn't need to stub it.

@sauloperez Are you okay to leave this as is? I don't like to get into refactoring work which then creates merge conflicts. Let's be stricter once this Spree upgrade is done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine, let's just create a tech debt issue then.

result = subject.filter(variants)

expect(result.first.price).to eq 2
end

it "filters to a specific order cycle" do
distributor = create(:distributor_enterprise)
product1 = create(:simple_product, supplier: supplier)
Expand Down