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

Double cart fix #401

Merged
merged 4 commits into from
Mar 7, 2019
Merged

Double cart fix #401

merged 4 commits into from
Mar 7, 2019

Conversation

mamazu
Copy link
Member

@mamazu mamazu commented Feb 16, 2019

This fixes #393

In this pull request I introduced a new Request and Command object for picking up a cart of a customer that is logged in. This way I registered a new handler. The PickupLoggedInCart Command and Request object inherit from the "non logged in"-version.

@mamazu mamazu requested a review from a team as a code owner February 16, 2019 21:30
@mamazu mamazu changed the title [WIP] Double cart Double cart Feb 17, 2019
@mamazu mamazu changed the title Double cart Double cart fix Feb 17, 2019
@mamazu
Copy link
Member Author

mamazu commented Feb 18, 2019

@lchrusciel This is the current best fix. What do you think? The other approach would be to use a different context that does't automatically create a cart or exclude the cart pickup form those routes. Which I both find not optimal.

@lchrusciel lchrusciel closed this Feb 22, 2019
@lchrusciel lchrusciel reopened this Feb 22, 2019
@lchrusciel
Copy link
Member

If I understood you correctly, the main issue is with UserCartRecalculationListener. This service is called each time we send authorization token, so it makes absolute sense. However, in this case, this solution seems to be more like a workaround. We should resolve this issue by customizing UserCartRecalculationListener instead. IMHO we should just turn off UserCartRecalculationListener for Shop API context. Still, I don't have an idea how :D

PS. Sorry, for misclick and closing PR :)

@mamazu
Copy link
Member Author

mamazu commented Feb 22, 2019

Correct, the UserCartRecalculationListener also has a fail save, when there is no cart then there is nothing to calculate and therefore returns. The problem is that the cartContext that the listener is provided with, doesn't throw an exception if the cart is not found but silently creates a new one.

@mamazu
Copy link
Member Author

mamazu commented Feb 22, 2019

Alternatively you could also remove the sylius.context.cart.new context from the composite cart (see context.Sylius/Sylius#10192) which is the best solution in my opinion.

This also fixes the problem in a different place. Currently with the service enabled you get a cart whenever you log in. Even if you never have ever put something into the cart or visited the cart overview once. So this fix also reduces the overhead on any logged in request.

The easier way would be to fix the sylius cart context. For more
information see (Sylius#401)
@lchrusciel lchrusciel merged commit a4b2f62 into Sylius:master Mar 7, 2019
@lchrusciel
Copy link
Member

Thanks, @mamazu! 🥇


$response = $this->client->getResponse();

$this->assertResponse($response, 'cart/deprecated_empty_response', Response::HTTP_CREATED);
}

/**
* @deprecated
Copy link
Member

Choose a reason for hiding this comment

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

💖

@tophsic
Copy link

tophsic commented Mar 7, 2019

Big thanks!

@mamazu mamazu deleted the double_cart branch March 13, 2019 15:08
@@ -63,5 +63,10 @@
<argument type="service" id="validator" />
<argument type="service" id="tactician.commandbus" />
</service>

<!-- Removing the create cart context from composite context (see: https://github.com/Sylius/Sylius/issues/10192) -->
<service id="sylius.context.cart.new" class="Sylius\Component\Order\Context\CartContext">
Copy link

@tophsic tophsic Mar 14, 2019

Choose a reason for hiding this comment

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

What happens if a project use in the same project shop bundle and shop api? This create cart context is removed for both context, isnt'it?

This how I understand your comment Sylius/Sylius#10192 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

If you want to use the shop front-end and the API as well those will collide. Or so we think. I have not tested it.

Copy link
Member Author

Choose a reason for hiding this comment

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

But we also have a ticket for removing the shop from the API package as a dependency (#264)

Copy link
Member

Choose a reason for hiding this comment

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

@tophsic that's correct, right now ShopApi will collide with default shop implementation

@mamazu mamazu mentioned this pull request Jun 22, 2019
GSadee added a commit that referenced this pull request Jul 25, 2019
…assignment (lchrusciel)

This PR was merged into the 1.0-dev branch.

Discussion
----------

Closing #401, #259 and #406

Commits
-------

f80d609 [Customer] Refactor customer assignment to command
bf72278 [Cart] Simplify cart recalculation after customer cart assignment
e97cd65 [Cart] Auto assign customer to cart during cart pick up
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.

POST /cart with jwt create 2 order records
3 participants