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

Scope variant to take overrides into account in packer #7461

Merged

Conversation

andrewpbrett
Copy link
Contributor

What? Why?

Closes #7337

See comments on the issue for an extended discussion, but it appears that backoffice orders weren't accounting for inventory variant overrides ... ever?

What should we test?

Set a product (Tomatoes) to have 0 on hand and "on demand?" unchecked in the product list
Add Tomatoes to Inventory and set On Hand to 10 and On Demand to No
Add Tomatoes to a back office order
Enter customer details
Before this PR:
Get stuck at "Address" state
After this PR:
Moves to "Payment" state

Release notes

Changelog Category: User facing changes

Dependencies

Documentation updates

@codecov
Copy link

codecov bot commented Apr 22, 2021

Codecov Report

Merging #7461 (c7e6518) into master (9b52c45) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7461      +/-   ##
==========================================
+ Coverage   93.16%   93.19%   +0.02%     
==========================================
  Files         633      633              
  Lines       18101    18088      -13     
==========================================
- Hits        16864    16857       -7     
+ Misses       1237     1231       -6     
Impacted Files Coverage Δ
...ment/app/services/order_management/stock/packer.rb 100.00% <100.00%> (ø)
app/controllers/spree/admin/base_controller.rb 92.85% <0.00%> (-0.70%) ⬇️
app/models/spree/order.rb 96.27% <0.00%> (+0.95%) ⬆️
lib/spree/core/controller_helpers/auth.rb 100.00% <0.00%> (+6.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b52c45...c7e6518. Read the comment docs.

@Matt-Yorkley
Copy link
Contributor

LGTM.

It'll need tests though 😉

@andrewpbrett
Copy link
Contributor Author

Yeah I'm doing battle with one right now 🙄😆

@sauloperez
Copy link
Contributor

can we help you @andrewpbrett ?

@Matt-Yorkley
Copy link
Contributor

Matt-Yorkley commented Apr 26, 2021

Awesome 💯

I'm wondering if we should add a quick feature test as well, covering this flow:

  1. Create a new order via admin
  2. Add a variant that has zero stock but has an override with plenty of stock
  3. Hit the save/next button
  4. Check that section of the order edit UI that's been occasionally vanishing is actually there, and the line item is shown as we expect

What do you think?

@andrewpbrett
Copy link
Contributor Author

I like it, good catch @Matt-Yorkley. Added a feature spec.

Copy link
Contributor

@Matt-Yorkley Matt-Yorkley left a comment

Choose a reason for hiding this comment

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

Nice!

@Matt-Yorkley
Copy link
Contributor

Matt-Yorkley commented Apr 27, 2021

I had a look at the feature spec and it seems like it passes in master (without the bugfix). I tried writing another test for this and that also passed. So... I tried writing an even longer test that went through the whole flow described in the issue, and finally managed to replicate the bug in a test. Also confirmed it's fixed by the Stock::Packer changes in this PR 👍

I'm not sure if it's worth adding it, but it looks like this: a8daa72

@andrewpbrett
Copy link
Contributor Author

That looks a lot better, thanks @Matt-Yorkley 👍

Copy link
Contributor

@sauloperez sauloperez left a comment

Choose a reason for hiding this comment

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

awesome! this could be merged right away IMO. That feature spec gives me enough confidence. 🔴 => 🟢 => we fixed the issue. What do you say?

@andrewpbrett
Copy link
Contributor Author

this could be merged right away

SGTM 👍

@sauloperez
Copy link
Contributor

decreed then!

@sauloperez sauloperez merged commit ef9720a into openfoodfoundation:master Apr 28, 2021
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.

Backoffice order blocked at 'Address' stage
3 participants