Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Abandon cart on cart clean #616

Merged
merged 1 commit into from
Nov 29, 2013
Merged

Abandon cart on cart clean #616

merged 1 commit into from
Nov 29, 2013

Conversation

winzou
Copy link
Contributor

@winzou winzou commented Nov 19, 2013

I may have missed something, but actually when clearing a cart it's deleted from the database, but not from the session (cartProvider using SessionStorage). So on the next page, CartProvider will do $this->repository->find($identifier); leading to a NotFoundException because the cart has been deleted.

This PR delete the cart identifier from the session. That way, on the next page CartProvider will create a new cart instead of trying to use an unexisting one.

The weird thing is that it's currently working well on the demo, but how? Can't find, even with a grep, where the abandonCart method is called.

@pjedrzejewski
Copy link
Member

If you look closely at the cart provider, if the cart with given identifier is not found, there is no exception, it simply creates a new cart and overrides the old identifier. So it works fine right now. That being said, I agree we should remove the deleted identifier together with the cart.

Anyway, I think we should rethink the logic of cart clearing, perhaps removing the whole cart is not a valid approach.

@winzou
Copy link
Contributor Author

winzou commented Nov 25, 2013

Yeah I've seen that. I got the error because I'd overriden the CartRepository::find() method using getSingleResult in my query instead of getOneOrNullResult. That's why it throws this exception.

But anyway, the identifier should be deleted as well and it's exactly what this PR does ;)

About the logic, I like the current all removing, it ensures all is well deleted and cleaned (think of item, promotion, etc, etc.).

pjedrzejewski pushed a commit that referenced this pull request Nov 29, 2013
Abandon cart on cart clean
@pjedrzejewski pjedrzejewski merged commit 8b292a9 into Sylius:master Nov 29, 2013
@pjedrzejewski
Copy link
Member

Thanks Alexandre!

@winzou winzou deleted the patch-4 branch January 7, 2014 07:08
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants