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

Fix product import date error when some but not all variants have import date #2831

Merged
merged 1 commit into from
Oct 9, 2018
Merged

Fix product import date error when some but not all variants have import date #2831

Show file tree
Hide file tree
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
5 changes: 1 addition & 4 deletions app/models/spree/product_decorator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,10 +177,7 @@ def variants_distributed_by(order_cycle, distributor)

# Get the most recent import_date of a product's variants
def import_date
variants.map do |variant|
next if variant.import_date.blank?
variant.import_date
end.sort.last
variants.map(&:import_date).compact.max
end

# Build a product distribution for each distributor
Expand Down
41 changes: 41 additions & 0 deletions spec/models/spree/product_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,4 +714,45 @@ module Spree
end
end
end

describe "product import" do
describe "finding the most recent import date of the variants" do
let!(:product) { create(:product) }

let(:reference_time) { Time.zone.now.beginning_of_day }

before do
product.reload
Copy link
Contributor

@sauloperez sauloperez Oct 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

why a reload if we just created the product? Do we really need it? This inevitably makes things slower.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this was to uncache the initially empty product.variants which was getting cached somewhere during setup of the product. 🙂 I prefer this over reloading just product.variants, which could leave you with outdated fields, e.g. (hypothetical) order.total after adding to order.line_items.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sauloperez Can I merge this now? ✔️ 🤔

end

context "when the variants do not have an import date" do
let!(:variant_a) { create(:variant, product: product, import_date: nil) }
let!(:variant_b) { create(:variant, product: product, import_date: nil) }

it "returns nil" do
expect(product.import_date).to be_nil
end
end

context "when some variants have import date and some do not" do
let!(:variant_a) { create(:variant, product: product, import_date: nil) }
let!(:variant_b) { create(:variant, product: product, import_date: reference_time - 1.hour) }
let!(:variant_c) { create(:variant, product: product, import_date: reference_time - 2.hour) }

it "returns the most recent import date" do
expect(product.import_date).to eq(variant_b.import_date)
end
end

context "when all variants have import date" do
let!(:variant_a) { create(:variant, product: product, import_date: reference_time - 2.hour) }
let!(:variant_b) { create(:variant, product: product, import_date: reference_time - 1.hour) }
let!(:variant_c) { create(:variant, product: product, import_date: reference_time - 3.hour) }

it "returns the most recent import date" do
expect(product.import_date).to eq(variant_b.import_date)
end
end
end
end
end