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

Cover managing the tax rates in API #14811

Merged
merged 8 commits into from Feb 23, 2023
Merged

Cover managing the tax rates in API #14811

merged 8 commits into from Feb 23, 2023

Conversation

jakubtobiasz
Copy link
Member

Q A
Branch? 1.13
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets successor of #11884
License MIT

@jakubtobiasz jakubtobiasz added the API APIs related issues and PRs. label Feb 15, 2023
@jakubtobiasz jakubtobiasz requested a review from a team as a code owner February 15, 2023 10:58
@jakubtobiasz jakubtobiasz force-pushed the SYL-2716-add-tax-rates branch 2 times, most recently from b5926a2 to 57f94db Compare February 15, 2023 13:11
@jakubtobiasz jakubtobiasz mentioned this pull request Feb 15, 2023
Closed
6 tasks
@jakubtobiasz jakubtobiasz changed the title 🚧 Cover managing the tax rates in API Cover managing the tax rates in API Feb 15, 2023
@jakubtobiasz jakubtobiasz changed the title Cover managing the tax rates in API 🚧 Cover managing the tax rates in API Feb 15, 2023
@jakubtobiasz jakubtobiasz force-pushed the SYL-2716-add-tax-rates branch 2 times, most recently from 33a3208 to fc4abf8 Compare February 16, 2023 11:28
@jakubtobiasz jakubtobiasz changed the title 🚧 Cover managing the tax rates in API Cover managing the tax rates in API Feb 16, 2023
lchrusciel
lchrusciel previously requested changes Feb 16, 2023
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 are still lacking contract tests for new endpoints

src/Sylius/Bundle/ApiBundle/Resources/config/api_resources/TaxRate.xml Outdated Show resolved Hide resolved
src/Sylius/Bundle/ApiBundle/Resources/config/api_resources/TaxRate.xml Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingTaxRateContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingTaxRateContext.php Outdated Show resolved Hide resolved
@jakubtobiasz
Copy link
Member Author

jakubtobiasz commented Feb 17, 2023 •
edited

@coldic3 @lchrusciel all suggestions applied, thanks.

P.S. Contract tests are on the way, I forgot about them xD.
P.S.2 Contract tests are present now 🫡

@jakubtobiasz jakubtobiasz force-pushed the SYL-2716-add-tax-rates branch 2 times, most recently from ebd9871 to defd0dd Compare February 17, 2023 20:29
GSadee
GSadee reviewed Feb 22, 2023
View reviewed changes
features/taxation/managing_tax_rates/adding_tax_rate.feature Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingTaxRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingTaxRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingTaxRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingTaxRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingTaxRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingTaxRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingTaxRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Page/Admin/TaxRate/UpdatePage.php Outdated Show resolved Hide resolved
src/Sylius/Bundle/ApiBundle/Resources/config/services/filters.xml Show resolved Hide resolved
@jakubtobiasz jakubtobiasz mentioned this pull request Feb 23, 2023
Closed
9 tasks
@jakubtobiasz
Copy link
Member Author

@diimpp we'll unify all gherkin tags later, we decided not to touch them once again in this PR.

GSadee
GSadee approved these changes Feb 23, 2023
View reviewed changes
src/Sylius/Behat/Page/Admin/TaxRate/UpdatePage.php
@@ -40,6 +45,7 @@ protected function getDefinedElements(): array
'code' => '#sylius_tax_rate_code',
'name' => '#sylius_tax_rate_name',
'zone' => '#sylius_tax_rate_zone',
'included_in_price' => '#sylius_tax_rate_includedInPrice'
Copy link
Member

Choose a reason for hiding this comment

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

Should be added in alphabetical order

@GSadee GSadee dismissed lchrusciel’s stale review February 23, 2023 11:50

The contract tests have been implemented for new endpoints

@GSadee GSadee added the Feature New feature proposals. label Feb 23, 2023
@GSadee GSadee merged commit aef3367 into Sylius:1.13 Feb 23, 2023
@GSadee
Copy link
Member

GSadee commented Feb 23, 2023

Thanks, Jakub! 🥇

@jakubtobiasz jakubtobiasz deleted the SYL-2716-add-tax-rates branch February 23, 2023 12:26
@jakubtobiasz jakubtobiasz mentioned this pull request Feb 23, 2023
Open
12 tasks
@Rafikooo Rafikooo mentioned this pull request Mar 16, 2023
Merged
GSadee added a commit that referenced this pull request Mar 17, 2023
@GSadee
bug #14864 [TaxRate][UI] Add missing behat step implementation (Rafik…
b215445 
…ooo)

This PR was merged into the 1.13 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.13 <!-- see the comment below -->                  |
| Bug fix?        | yes                                                       |
| New feature?    | no                                                      |
| BC breaks?      | no                                                      |
| Deprecations?   | no<!-- don't forget to update the UPGRADE-*.md file --> |
| Related tickets | missing changes #14811                   |
| License         | MIT                                                          |

<!--
 - Bug fixes must be submitted against the 1.12 branch
 - Features and deprecations must be submitted against the 1.13 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
-------

e5f67c5 [TaxRate][UI] Add missing behat step implementation
3e1e1df [Behat] Unify saving step naming
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coldic3 coldic3 coldic3 left review comments

@diimpp diimpp diimpp requested changes

@GSadee GSadee GSadee approved these changes

@Rafikooo Rafikooo Rafikooo approved these changes

@lchrusciel lchrusciel Awaiting requested review from lchrusciel

Assignees
No one assigned
Labels
API APIs related issues and PRs. Feature New feature proposals.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jakubtobiasz @GSadee @diimpp @lchrusciel @coldic3 @Rafikooo @hatem20