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

Oeoeaio/uk/1031 insufficient stock in cart #1698

Conversation

@jeronimo
Copy link
Contributor

commented Jul 16, 2017

What? Why?

This is extension to PR #1583 as issue has appeared:

  1. a variant is in the user's cart
  2. the user logs out
  3. the variant is removed completely from the order cycle (not just stock reduced)
  4. the user logs back in

Approach:
Validation method inside order model seemed more appropriate way but there were so many tests failing and fixing that looked like very time consuming thing so this approach has been chosen.

P.S. was not sure about naming on methods so please give better ones if you have any in mind.

What should we test?

Checkout process when incoming/outgoing products get unselected in admin order_cycles edit page.

How is this related to the Spree upgrade?

Hopefully it is not related unless later Spree versions has Orders implemented already with double check of this functionality.

Dependencies

Extension of #1583
And follow up of #1477

@sauloperez

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2017

Validation method inside order model seemed more appropriate way but there were so many tests failing and fixing that looked like very time consuming thing so this approach has been chosen.

How did validation look like? To me the validation does seem like the way to go rather than having to pollute the application controller. What about ensuring there are no orders still to be completed that have that variant in their line items when removing it from the order cycle? I'd then tell the admin through a flash error message.

@enricostano
Copy link
Contributor

left a comment

I agree with @sauloperez , this is something that we must tackle at model level. If not, it will bite us while doing the same thing in other controllers or places in the code. If it's difficult to add, we must understand why it is so and try to fix it.

@jeronimo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2017

Cool! @sauloperez, @enricostano, may I assume that you will take this issue out of me, will analyse all requirements and whole situation again and will do it properly?

@enricostano

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2017

@jeronimo maybe we can discuss about it quickly on mumble, what do you think? Just to be on the same page and better understand the context. Is that OK for you?

@Matt-Yorkley

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2017

For reference, this is the latest PR for #1031.

@daniellemoorhead

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2017

Moved to September dot point release.

@lin-d-hop

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2017

Hi @enricostano
Could you just leave a wee update on the latest?
Maybe we could have a chat and we can respec the issue to assign to someone new?
Cheers,
Lynne

@daniellemoorhead daniellemoorhead modified the milestones: September dot point release, October dot point release Oct 5, 2017

@daniellemoorhead daniellemoorhead removed this from the October dot point release milestone Oct 18, 2017

@enricostano

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

@sauloperez can we work on prioritizing this? Maybe we can help in some way.

@myriamboure

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2017

@enricostano @lin-d-hop what shall we do with that PR ? Shall we just put #1031 into an icebox item, so that it gets specced properly when we prioritize it ? And for the bug #1491 shall we wait for it to be prioritized before we work back on this?

@myriamboure

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2017

Do I understand well: this one is blocked by #1767 so we can't review and merge before we have moved forward the other one which is in dev, right?

@myriamboure

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2017

Will reopen when #1491 is priorized, incepted and solve, and then we can work on #1031. So closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
9 participants
You can’t perform that action at this time.