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

[API][Behat] Cover scenarios for applying correct taxes #13930

Merged
merged 2 commits into from
May 6, 2022

Conversation

GSadee
Copy link
Member

@GSadee GSadee commented May 5, 2022

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

@GSadee GSadee added Feature New feature proposals. Behat Issues and PRs aimed at improving Behat usage. API APIs related issues and PRs. labels May 5, 2022
@GSadee GSadee requested a review from a team as a code owner May 5, 2022 11:46
Scenario: Proper taxes after changing item quantity
Given I have 3 products "PHP T-Shirt" in the cart
And I have 2 products "Symfony Hat" in the cart
When I change "PHP T-Shirt" quantity to 1
When I change product "PHP T-Shirt" quantity to 1 in my cart
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
When I change product "PHP T-Shirt" quantity to 1 in my cart
When I change the number of "PHP T-Shirt" products in the cart to 1

I know we have to clarify, that we are mentioning quantity here, but still what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not convinced as we are using quantity keyword both in API and UI for the number of products in the cart, and also in Behat scenarios, so it is clear enough IMO 😃

Comment on lines +117 to +122
if (null !== $defaultAddress) {
$clonedAddress = clone $defaultAddress;
$clonedAddress->setCustomer(null);
}

return $clonedAddress ?? null;
Copy link
Member

Choose a reason for hiding this comment

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

We could reduce one "if" statement :P

Suggested change
if (null !== $defaultAddress) {
$clonedAddress = clone $defaultAddress;
$clonedAddress->setCustomer(null);
}
return $clonedAddress ?? null;
$clonedAddress = null;
if (null !== $defaultAddress) {
$clonedAddress = clone $defaultAddress;
$clonedAddress->setCustomer(null);
}
return $clonedAddress;

or

Suggested change
if (null !== $defaultAddress) {
$clonedAddress = clone $defaultAddress;
$clonedAddress->setCustomer(null);
}
return $clonedAddress ?? null;
if (null !== $defaultAddress) {
$clonedAddress = clone $defaultAddress;
$clonedAddress->setCustomer(null);
return $clonedAddress;
}
return null;

@@ -77,6 +79,7 @@ function it_picks_up_a_new_cart_for_logged_in_shop_user(
$channel->getDefaultLocale()->willReturn($locale);

$customerRepository->findOneBy(['email' => 'sample@email.com'])->willReturn($customer);
$customer->getDefaultAddress()->willReturn($address);
Copy link
Member

Choose a reason for hiding this comment

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

that's funny, that this works, as we are cloning it :P

Anyway, we should cover the null case well

@lchrusciel lchrusciel merged commit dfa8dc5 into Sylius:master May 6, 2022
@lchrusciel
Copy link
Member

Thanks, Grzegorz! 🥇

@GSadee GSadee deleted the api-applying-taxes branch May 9, 2022 04:35
lchrusciel added a commit that referenced this pull request May 9, 2022
…the logged in user has no default address + minor refactor (GSadee)

This PR was merged into the 1.12-dev branch.

Discussion
----------

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

<!--
 - Bug fixes must be submitted against the 1.10 or 1.11 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
-------

3df61fb [API] Refactor getting address in PickupCartHandler
5a8dd5b [API] Add additional PHPSpec for PickupCartHandler when the logged in user has no default address
GSadee pushed a commit to Sylius/SyliusApiBundle that referenced this pull request May 10, 2022
…the logged in user has no default address + minor refactor (GSadee)

This PR was merged into the 1.12-dev branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | master|
| Bug fix?        | no                                                       |
| New feature?    | no                                                       |
| BC breaks?      | no                                                       |
| Deprecations?   | no|
| Related tickets | after Sylius/Sylius#13930                      |
| License         | MIT                                                          |

<!--
 - Bug fixes must be submitted against the 1.10 or 1.11 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
-------

3df61fb91cc0c523e949097d27e086e34d2c49c2 [API] Refactor getting address in PickupCartHandler
5a8dd5ba278b76e3d48afd2c030c91bab932b7f6 [API] Add additional PHPSpec for PickupCartHandler when the logged in user has no default address
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API APIs related issues and PRs. Behat Issues and PRs aimed at improving Behat usage. Feature New feature proposals.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants