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

[Spree Upgrade] Scope variants in line items availability validator #3232

Merged
merged 10 commits into from
Dec 21, 2018

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Dec 20, 2018

This PR includes all 5 commits from #3210, this PRs commits are the last 5.

What? Why?

Closes #3118

When validating line_item.variant stock levels in stock/availability_validator we need to scope variants so that variant overrides stock levels are used instead. Here we re-use the the line_item.scoper so that the variant_overrides are not loaded several times in the handling fo line_items in the same request.

Additional problems solved:

  • adapting line_item.sufficient_stock? to spree 2 by using the new variant.can_supply?
  • fix variant.in_stock? by also using the new variant.can_supply?
  • move stock_location.fill_status to VariantStock and fix it, it was totally dependent on stock_item methods and is not using already overridden variant methods (on_demand and total_on_hand)

What should we test?

This is one of the final steps in making cart and checkout handle stock levels correctly in v2!
the spec in 3118 (spec/features/consumer/shopping/variant_overrides_spec.rb:152) should be green.

Dependencies

This PR goes after #3210

@luisramos0 luisramos0 self-assigned this Dec 20, 2018
@luisramos0 luisramos0 force-pushed the 2-0-out-of-stock branch 3 times, most recently from a42b89e to 62e7277 Compare December 20, 2018 20:41
@luisramos0 luisramos0 changed the title [Spree Upgrade] WIP - Scope variants in line items availability validator [Spree Upgrade] Scope variants in line items availability validator Dec 20, 2018
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

I hope we can stop this variant override madness soon.

def validate_quantity(line_item, quantity)
quantifier = Spree::Stock::Quantifier.new(line_item.variant_id)
return if quantifier.can_supply? quantity
line_item.scoper.scope(line_item.variant)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of exposing the scoper, you could offer line_item.scope_variant.

I still have a concern about the performance of the scoper used in this way, but that can probably be solved later on.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I also think performance is best solved later when things are correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkllnk I think that would just be more lines of code, I'd need to create a new method in lineitem.

app/models/concerns/variant_stock.rb Outdated Show resolved Hide resolved
@sauloperez
Copy link
Contributor

I hope we can stop this variant override madness soon.

We all do 🙈. We need to all be aware of it when discussing priorities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants