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

basket doesn't save on logout #1031

Closed
NickWeir63 opened this issue Jun 8, 2016 · 56 comments

Comments

@NickWeir63
Copy link

commented Jun 8, 2016

Stroudco shoppers want to be able to leave items in their basket that are not lost when they log out so they can add items during the week and then review and confirm them just before the OC closes

@pmackay pmackay added the uk label Jun 15, 2016

@lin-d-hop

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

@stveep Would you be willing to look into this issue a little more? Perhaps to scope it out a little more for @lebreeze.

@lin-d-hop

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2016

A little more specification from a non-technical perspective:

As a shopper, when I am logged in I would like my cart to persist for the hub I am shopping with. I would like OFN to remember the last cart that I created such then if I log out and log back in again the last cart is persisted.

If I change enterprises, order cycles or the order cycle closes my cart should not persist.

Currently if a user is shopping with one enterprise, then they switch to another enterprise their cart is cleared from the scope. This behaviour is enables multiple shopfronts to exist simultaneously. However the majority of shoppers only shop from a single hub. Therefore there is a clear benefit for a customers cart to persist if they log out and log back in to shop at the same hub.

The spree_order model enables carts to be persisted to a user and spree_line_items are also persisted. So to my understanding this issue is pertaining to reloading the cart to the scope when a user logs back in, with some checks to ensure the order cycle has not closed. Existing functions to clear the cart upon changing order_cycle or enterprise should be maintained.

I think this issue will need a couple hours of scoping first to fully understand the implications. @stveep Do you think this might be suitable for @lebreeze to ldelve further into?

@oeoeaio @mkllnk If this triggers any alarm bells to you would you mind noting them here too?

@lin-d-hop lin-d-hop added this to the UK Current milestone Nov 5, 2016

@mkllnk

This comment has been minimized.

Copy link
Member

commented Nov 5, 2016

I think that would be great. There was once a multicart feature before my time. I'm not sure if that actually worked. I definitely find the current cart system confusing and brittle. But it might be a bit difficult to work around Spree's model of only one cart.

@oeoeaio

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2016

This would be different to a multi-cart, right? We would simply be tweaking the current_order method in lib/spree/core/controller_helpers/order_decorator.rb to make it search the database for a relevant incomplete order, in the event that there is no order_id in the session.

If this search was restricted to incomplete orders owned by the current user, for the current shop and in the current order cycle, I can't see a problem with it.

@kirstenalarsen

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2016

the multi-cart never worked. it was a folly, that was abandoned ass the complexity became too much :)

On 6 Nov 2016, at 7:12 PM, Rob H notifications@github.com wrote:

This would be different to a multi-cart, right? We would simply be tweaking the current_order method in lib/spree/core/controller_helpers/order_decorator.rb to make it search the database for a relevant incomplete order, in the event that there is no order_id in the session.

If this search was restricted to incomplete orders owned by the current user, for the current shop and in the current order cycle, I can't see a problem with it.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub #1031 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/ACxryLKeJ2dphIX_lXQ-GN97gnklR84kks5q7YvlgaJpZM4IxSTA.

@lin-d-hop

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2016

While multi-cart would be amazing, this enhancement falls a long way short. Thanks for the clear spec @oeoeaio.

Tagging @levent as I tagged him incorrectly above.

@NickWeir63

This comment has been minimized.

Copy link
Author

commented Nov 7, 2016

I dont' think @levent is getting these notifications - when I spoke to him just now he has not seen these github notifications. is there something wrong with his membership of this group?

@levent

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2016

Hi Nick, notifications are fine. It was just the incorrect username being used previously.
Cheers

On 7 Nov 2016, at 11:15, Nick Weir notifications@github.com wrote:

I dont' think @levent is getting these notifications - when I spoke to him just now he has not seen these github notifications. is there something wrong with his membership of this group?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@levent

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2016

Based on @lin-d-hop and @oeoeaio's comments this doesn't seem too complicated. I will familiarise myself with the cart logic today and get back to you.

@levent

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2016

Ok. I went on a bit of a roundabout tour and discovered that there were some changes made a while ago that modified the cart logic.

Out of the box, spree attempts to merge the current session's cart with any incomplete cart when a user logs in.

In October 2012 two commits 27b7d59 and then 568d948 were added to firstly check that the carts were only merged if from the same distributor and secondly to delete any incomplete carts that didn't match the distributor.

In August 2013 this was again simplified to just delete any incomplete carts and not bother merging at all (commit: 5fac29d) This was done to fix #20.

Long story short, removing the spree override of set_current_order see diff will mean that all tests pass and logged out user's cart items are merged with any previous cart items they had last time they were logged in. It won't handle jumping from shop to shop yet.

@RohanM do you remember the rationale behind the overrides to set_current_order and what regression we would introduce by removing / reverting it? Unfortunately any specs written around that logic seem to have been lost over the years during major refactoring or redesign stages.

I also found a small sql query bug along the way where the last_incomplete_order method would always return an empty result if the user didn't have any account_invoices. If it's common not to have account_invoices then the app will always think the user does not have any incomplete orders. I can create a pull request from this fix: PR candidate

Notes for my benefit on this: https://gist.github.com/levent/a7c5e1e78db0c0e6d5a337459629a848

@lin-d-hop

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2016

Thanks @levent!
@daniellemoorhead would you mind following up @levents question above with @RohanM?

@RohanM

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2016

Hi @levent,

Nice work researching an obscure issue! I think the rationale is sitting in the commit message of 568d948. It was very common for users to have incomplete orders sitting around in the database. When they completed checkout, the system was restoring their cart from their incomplete order and they'd end up with a bunch of items mysteriously appearing in their cart. I'm thinking that with a bit of testing we could ensure we don't introduce a regression around this.

Hope that helps, let me know if you have any other questions.

@lin-d-hop

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2016

Hi all, Levent pointed out that this issue needs more clarification...
@levent @oeoeaio Can you add any scenarios that we need a case for to the list below? Thanks!
@OliverUK @sstead Would you comment on the below also? In particular the behaviour in case 2.1. Thanks :-)

As customer I create a basket then log out:

  1. I log back in without creating a new basket:
    1.1 Order cycle of last basket is still open -> Old Basket Restored
    1.2 Order cycle of last basket is now closed -> Old Basket Deleted
  2. I create a new basket in the same shop then log back in:
    2.1 Order cycle of last basket is open -> Old Basket Added to New basket
    2.2 Order cycle of last basket is closed -> Old Basket Replaced by New basket
  3. I create a new basket in a different shop then log back in:
    3.1 Order cycle of last basket is open -> Basket Replaced by New basket
    3.2 Order cycle of last basket is closed -> Basket Replaced by New basket

In the case of the old basket being restored, it is likely that some of the products previously added to the list may now be sold out. In this case I would suggest these products quietly disappear. @levent can you comment on the legacy behaviour in this case?

@NickWeir63

This comment has been minimized.

Copy link
Author

commented Nov 23, 2016

i have talked to one of the shoppers who first raised this. she think that:

  • in case 2.1 (above) the shopper is likely to have forgotten that they have items in their basket and there should be either be a warning that they are about to combine the baskets or an option either to combine baskets or delete the previous basket

  • In the case of the old basket being restored, if some of the products previously added to the basket are now sold out, then there should be a warning to say that these products have been deleted from the basket.

Sorry - I expect these are tricky demands!

@OliverUK

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2016

Hi @lin-d-hop
I wonder if it's worth the hassle letting people do this, considering the implications on limited stock items. I too think that if in 2.1 the old and new baskets get merged, this would cause confusion and would need a warning or the option to add or discard the old items.

I personally would guess most people shop in one session.

@oeoeaio

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2016

I agree with @OliverUK.

My feeling is that 2.1 will be annoying more often than it is helpful, with the possible exception of the 'new' basket being empty. If a customer is already shopping and then logs in, my preference would be to just keep their basket as is, without looking for previous baskets. I think 1.1 and 3.1 are ok, but as mentioned we will need to notify customers when stock levels mean their existing order can no longer be fulfilled.

There is a method called something like cap_quantity_at_stock! on LineItem that we can use for this, then we just need to track any changes that were made and notify the customer. I am doing a similar thing with standing orders.

1.2, 2.2 and 3.2 are existing behaviour as far as I understand. I think they're ok.

@levent

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2016

@lin-d-hop if you make the following changes to restore the default merge basket functionality:

A) This bug fixed: BUG: Incomplete cart query always empty
B) Spree default behaviour restored: Remove override that destroys incomplete orders

  • NOTE: This code knows nothing about order cycles or enterprises

In the case of the old basket being restored, it is likely that some of the products previously added > to the list may now be sold out. In this case I would suggest these products quietly disappear.
@levent can you comment on the legacy behaviour in this case?

The baskets are initially merged (including out of stock inventory) and then on a subsequent request the user is informed via a modal dialogue:

screenshot 2016-11-24 09 41 21

@levent

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2016

I am currently working on a branch that satisfies:

A) If user already has items in their basket on log in,
     do not modify the basket.
B) If user does **not** have items in their basket on login,
     use the old basket if it's the same distributor and order cycle

This would/should mean:

1.1 True - old basket restored
1.2 True - continue with empty basket
2.1 False - continue with basket used whilst logged out

I don't fully understand the user journey for 2.2, 3.1 and 3.2 but if you understand how spree determines the last_incomplete_order and follow A) and B) above, you may get the answer.

What is last_incomplete_order?

The most recently created, incomplete cart (across the entire system) associated with the user.

Am I oversimplifying or is this a useful and reasonable improvement?

@lin-d-hop

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2016

Thanks everyone.
Based on comments above I agree with y'all that 2.1 should be updated to use new basket.

So, in that case, @levent, your conditions sound good and I agree that your changes are a useful and reasonable improvement.

A) If user already has items in their basket on log in,
     do not modify the basket.
B) If user does **not** have items in their basket on login,
     use the old basket if it's the same distributor and order cycle

@levent, your notification looks good to me. @oeoeaio is this consistent with the communication of out of stock items in SO?

@lin-d-hop

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2016

Hi @levent
With the two fixes you made above is this ready for review and testing? Or is there something you are still working on here?

@levent

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2016

Hi @lin-d-hop
Not quite, I'm afraid.
Will be ready tomorrow.

@lin-d-hop

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2016

Great thanks @levent! Just wanted to check this wasn't waiting on me :-)

@OliverUK

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2017

Happy to test but can't find in buildkite, @lin-d-hop .

@lin-d-hop

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2017

@OliverUK Now has a build so over to you :-)

@OliverUK

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2017

@lin-d-hop lin-d-hop moved this from In Dev to Test Ready in UK Current Apr 24, 2017

@lin-d-hop

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

@jeronimo Could you take a look at this again please? See Sallys comment above, sounds like 'Allow backorder' is only one part of the issue. Robs comments above should give you direction on where to start to track it down. Thanks!

@OliverUK

This comment has been minimized.

Copy link
Contributor

commented May 2, 2017

@sstead @lin-d-hop
Did Lynne not turn off backorders after testing? So the problem was occurring while they were allowed?

@lin-d-hop

This comment has been minimized.

Copy link
Contributor

commented May 2, 2017

@jeronimo

This comment has been minimized.

Copy link
Contributor

commented May 2, 2017

I have added two additional feature tests with the code @levent has submitted. In both cases (login back with incomplete order) it doesn't let to proceed from shopfront nor cart if there is insufficient stock when backorders are disallowed.
I tested it manually in my local dev environment as well as the code passed in Travis (though it allows backorders by default and I had to disable inside tests).

@lin-d-hop

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2017

Would be great to dust this oldy off and see if we can move it along.....

@lin-d-hop lin-d-hop removed this from the UK Current milestone Dec 18, 2017

@myriamboure

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2017

Closing, wishlist item created here: https://community.openfoodnetwork.org/t/cart-should-save-on-log-out/1175
#gitcull2017

@lin-d-hop

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2017

@myriamboure @oeoeaio @enricostano @sauloperez
Given this issue is finished to the simple spec but is waiting on a bug that is an actual OFN bug I really don't want to close this and move it onto the wishlist.
Can we discuss this one?

@lin-d-hop lin-d-hop reopened this Dec 27, 2017

@lin-d-hop lin-d-hop closed this Dec 27, 2017

@myriamboure

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2018

Sure @lin-d-hop, I would say if a bug is blocking a feature that has been developped and just can't be merged because of that bug, it makes sense to me to prioritize. It is part of "finishing what has been already done" ... but then let's agree on our first "prioritization ceremony" to put 1491 in delivery train backlog OR create an epic if needs inception... does it make sense? I switched from wishlist to icebox this feature item, so that we don't forget it but to avoid to have lots of issues open that we can't treat, I suggest to reopen the issue and PR when the bug is treated. Or else we can create an epic and in the inception, the bug resolution will be one of the stories? So we treat them all in the feature dev process. What do you think?

@myriamboure myriamboure reopened this Jan 3, 2018

@myriamboure myriamboure closed this Jan 3, 2018

@lin-d-hop

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2018

The best solution to this bug might be quite big so your proposal is a good one! I feel reassured that this won't become lost in the wishlist ocean. Thank you :-)

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