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.

Sign up for GitHub

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

Jump to bottom

[API] Fix cart blaming #12512

Merged
merged 5 commits into from
Apr 13, 2021
Merged

[API] Fix cart blaming #12512

merged 5 commits into from
Apr 13, 2021

Conversation

GSadee
Copy link
Member

@GSadee GSadee commented Apr 7, 2021

Q A
Branch? master
Bug fix? yes?
New feature? no
BC breaks? no
Deprecations? no
Related tickets
License MIT

Based on #12509

@GSadee GSadee added the API APIs related issues and PRs. label Apr 7, 2021
@GSadee GSadee requested a review from a team as a code owner April 7, 2021 10:17
@probot-autolabeler probot-autolabeler bot added the Admin AdminBundle related issues and PRs. label Apr 12, 2021
@probot-autolabeler probot-autolabeler bot added the Shop ShopBundle related issues and PRs. label Apr 12, 2021
@probot-autolabeler probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Apr 12, 2021
@arti0090 arti0090 force-pushed the api-fix-cart-blaming branch 2 times, most recently from 383467c to 3ae7589 Compare April 12, 2021 08:47
@arti0090 arti0090 changed the title [WIP][API] Fix cart blaming [API] Fix cart blaming Apr 12, 2021
lchrusciel
lchrusciel reviewed Apr 12, 2021
View reviewed changes
Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

We need to extract \Sylius\Bundle\CoreBundle\EventListener\UserCartRecalculationListener as well, as this service has super annoying behavior of recalculating order for every request in Api. Partially related to what we are doing here. Can be done in separate PR however.

UPGRADE-1.10.md Outdated Show resolved Hide resolved
src/Sylius/Bundle/ApiBundle/EventListener/ApiCartBlamerListener.php Outdated Show resolved Hide resolved
src/Sylius/Bundle/ApiBundle/EventListener/ApiCartBlamerListener.php Outdated Show resolved Hide resolved
src/Sylius/Bundle/ApiBundle/EventListener/ApiCartBlamerListener.php Outdated Show resolved Hide resolved
src/Sylius/Bundle/ApiBundle/SectionResolver/ShopApiUriBasedSectionResolver.php Outdated Show resolved Hide resolved
src/Sylius/Bundle/ApiBundle/spec/SectionResolver/ShopApiUriBasedSectionResolverSpec.php Show resolved Hide resolved
lchrusciel
lchrusciel approved these changes Apr 13, 2021
View reviewed changes
Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

As mentioned before, I would still consider extracting cart blaming logic to command + movement of UserCartRecalculationListener to shop namespace

src/Sylius/Bundle/ApiBundle/Resources/config/services.xml
Comment on lines +193 to +194
<tag name="kernel.event_listener" event="sylius.user.security.implicit_login" method="onImplicitLogin" />
<tag name="kernel.event_listener" event="security.interactive_login" method="onInteractiveLogin" />
Copy link
Member

Choose a reason for hiding this comment

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

Can you check if both cases are needed in Api?

@lchrusciel lchrusciel merged commit ccf0808 into Sylius:master Apr 13, 2021
@lchrusciel
Copy link
Member

lchrusciel commented Apr 13, 2021 •
edited
Loading

Thanks, Grzegorz & Artur! 🥇

@arti0090 arti0090 mentioned this pull request Apr 13, 2021
Merged
@GSadee GSadee deleted the api-fix-cart-blaming branch April 13, 2021 08:38
GSadee
GSadee commented Apr 13, 2021
View reviewed changes
Copy link
Member Author

@GSadee GSadee left a comment

Choose a reason for hiding this comment

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

There is missing PHPSpec for ApiCartBlamerListener

lchrusciel added a commit that referenced this pull request Apr 13, 2021
@lchrusciel
refactor #12526 Remove unused api cart blamer method add missing spec…
5a0ae8e 
… (arti0090)

This PR was merged into the 1.10-dev branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | master
| Bug fix?        | no
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | continues #12512
| License         | MIT

<!--
 - Bug fixes must be submitted against the 1.7 or 1.8 branch (the lowest possible)
 - Features and deprecations must be submitted against the master branch
 - Make sure that the correct base branch is set

 To be sure you are not breaking any Backward Compatibilities, check the documentation:
 https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html
-->


Commits
-------

8e764fe Remove unused api cart blamer method add missing spec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arti0090 arti0090 arti0090 left review comments

@lchrusciel lchrusciel lchrusciel approved these changes

Assignees
No one assigned
Labels
Admin AdminBundle related issues and PRs. API APIs related issues and PRs. Maintenance CI configurations, READMEs, releases, etc. Shop ShopBundle related issues and PRs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GSadee @lchrusciel @arti0090