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] [Exchange Rate] Add api configuration and tests #11178

Merged
merged 5 commits into from
Mar 11, 2020
Merged

[API] [Exchange Rate] Add api configuration and tests #11178

merged 5 commits into from
Mar 11, 2020

Conversation

Tomanhez
Copy link
Contributor

@Tomanhez Tomanhez commented Mar 5, 2020 •
edited

Q A
Branch? api
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
License MIT

Screenshot 2020-03-05 at 09 09 22

@Tomanhez Tomanhez requested a review from a team as a code owner March 5, 2020 08:22
@Tomanhez Tomanhez force-pushed the exchange-rates-api branch 3 times, most recently from 44fb232 to 16f3952 Compare March 5, 2020 09:31
@Zales0123 Zales0123 added API APIs related issues and PRs. Feature New feature proposals. labels Mar 5, 2020
Zales0123
Zales0123 requested changes Mar 5, 2020
View reviewed changes
src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated
*/
public function iWantToEditThisExchangeRate(ExchangeRateInterface $exchangeRate): void
{
$this->sharedStorage->set('exchange_rate_id', $exchangeRate->getId());
Copy link
Member

Choose a reason for hiding this comment

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

Why it's not the same step as the one below?

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 share the answer to Mateusz question?

Copy link
Contributor Author

@Tomanhez Tomanhez Mar 9, 2020 •
edited

Choose a reason for hiding this comment

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

Because after this step I must repeat twice all flow from PUT request to check field immutablity in this two steps: https://github.com/Sylius/Sylius/blob/master/features/currency/managing_exchange_rates/editing_exchange_rate.feature#L23

Copy link
Member

Choose a reason for hiding this comment

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

But it doesn't change anything, I would be for merging these two steps because now, this step is confusing to me

src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Bundle/ApiBundle/Filters/SearchFilter.php Outdated Show resolved Hide resolved
src/Sylius/Bundle/ApiBundle/Filters/SearchFilter.php Outdated Show resolved Hide resolved
src/Sylius/Bundle/ApiBundle/Filters/SearchFilter.php Outdated Show resolved Hide resolved
@Tomanhez Tomanhez force-pushed the exchange-rates-api branch 5 times, most recently from a9ed475 to 00790a2 Compare March 9, 2020 09:51
lchrusciel
lchrusciel reviewed Mar 9, 2020
View reviewed changes
src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated
*/
public function iWantToEditThisExchangeRate(ExchangeRateInterface $exchangeRate): void
{
$this->sharedStorage->set('exchange_rate_id', $exchangeRate->getId());
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 share the answer to Mateusz question?

src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated Show resolved Hide resolved
GSadee
GSadee reviewed Mar 10, 2020
View reviewed changes
src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated
*/
public function iWantToEditThisExchangeRate(ExchangeRateInterface $exchangeRate): void
{
$this->sharedStorage->set('exchange_rate_id', $exchangeRate->getId());
Copy link
Member

Choose a reason for hiding this comment

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

But it doesn't change anything, I would be for merging these two steps because now, this step is confusing to me

src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php Outdated Show resolved Hide resolved
src/Sylius/Bundle/ApiBundle/Resources/config/api_resources/ExchangeRate.xml Outdated Show resolved Hide resolved
src/Sylius/Bundle/ApiBundle/Resources/config/serializer/ExchangeRate.xml Outdated Show resolved Hide resolved
@Tomanhez Tomanhez mentioned this pull request Mar 10, 2020
Merged
Zales0123
Zales0123 approved these changes Mar 11, 2020
View reviewed changes
src/Sylius/Behat/Client/ApiPlatformClient.php
@@ -123,14 +123,14 @@ public function countCollectionItems(): int
return (int) $this->getResponseContentValue('hydra:totalItems');
}

public function getCollection(): array
public function getCollectionItems(): array
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure about this change :D 💃

src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php
public function iDeleteTheExchangeRateBetweenAnd(CurrencyInterface $sourceCurrency, CurrencyInterface $targetCurrency) : void
{
/** @var ExchangeRateInterface */
$exchangeRate = $this->getExchangeRateBetweenCurrencies($sourceCurrency->getCode(), $targetCurrency->getCode());
Copy link
Member

Choose a reason for hiding this comment

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

I wonder, maybe we can do it with transform context? It would be wonderful to not have any repositories in the main contexts 🖖

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its good idea, we don't have transform for ExchangeRate yet, but I can make it

src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php
public function iShouldSeeASingleExchangeRateInTheList(): void
{
$this->client->index('exchange_rates');
Assert::count($this->client->getCollectionItems(), 1);
Copy link
Member

Choose a reason for hiding this comment

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

Why not countCollectionItems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it after : #11178 (comment)

src/Sylius/Behat/Context/Api/Admin/ManagingExchangeRatesContext.php
$exchangeRate->getTargetCurrency()
),
sprintf(
'Exchange rate with ratio %s between %s and %s still exists',
Copy link
Member

Choose a reason for hiding this comment

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

still exists -> exists, but it should not.? We don't know is it "still" or not 😄

@GSadee GSadee merged commit 33e8487 into Sylius:api Mar 11, 2020
@GSadee
Copy link
Member

GSadee commented Mar 11, 2020

Thank you, Tomasz! 🥇

lchrusciel added a commit that referenced this pull request Mar 11, 2020
@lchrusciel
feature #11202 [API] [ExchangeRate] Add currency filter (Tomanhez)
1c9c295 
This PR was merged into the api branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | api
| Bug fix?        | no
| New feature?    | yes
| BC breaks?      | no
| Deprecations?   | no
| License         | MIT

This PR based on #11178

<img width="969" alt="Screenshot 2020-03-10 at 11 15 10" src="https://user-images.githubusercontent.com/39232096/76302525-725a8800-62c0-11ea-8190-86ed67a81548.png">

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


Commits
-------

a5a0fef Add behat contexts for api ExchangeRates
291852a Add behat configurations
c60cf88 Add test for filters
21c56e5 Add filter for currencies
b30cfc6 Add fixes and ExchangeRate transform
@lchrusciel
Copy link
Member

Part of #11250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GSadee GSadee GSadee approved these changes

@Zales0123 Zales0123 Zales0123 approved these changes

@lchrusciel lchrusciel lchrusciel approved these changes

@AdamKasp AdamKasp AdamKasp approved these changes

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
5 participants
@Tomanhez @GSadee @lchrusciel @Zales0123 @AdamKasp