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

Fixing product discard and classifications issue #3439

Merged
merged 1 commit into from
Dec 12, 2019
Merged

Fixing product discard and classifications issue #3439

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
2 changes: 1 addition & 1 deletion core/app/models/spree/product.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class Product < Spree::Base
variants_including_master.discard_all
self.product_option_types = []
self.product_properties = []
self.classifications = []
self.classifications.destroy_all
Copy link

Choose a reason for hiding this comment

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

Style/RedundantSelf: Redundant self detected.

Copy link
Member

Choose a reason for hiding this comment

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

Great catch, I think we should use discard_all instead. Also, in another PR I think it's a good idea to do the same with the rest of the associations in that block, if they are soft-deletable, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Classifications is not discard'able, that's why I used destroy_all so act_as_list callbacks are executed.

I also had a look at the other relationships and the only one with callbacks is product properties but position is only used for sorting, it does not affect like in classifications

Copy link
Member

Choose a reason for hiding this comment

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

You are right @softr8, the rest of associations should be handled in a different context, if needed. Thanks!

self.product_promotion_rules = []
end

Expand Down
12 changes: 12 additions & 0 deletions core/spec/models/spree/classification_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,18 @@ def positions_to_be_valid(taxon)
end
end

context "Discard'ing a product" do
before :each do
element = taxon_with_5_products.products[1]
expect(element.classifications.first.position).to eq(2)
element.discard
end

it "resets positions" do
expect positions_to_be_valid(taxon_with_5_products)
end
end

context "replacing taxon's products" do
before :each do
products = taxon_with_5_products.products.to_a
Expand Down