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][ShippingMethod] Add contract test for available shipping methods endpoint #12716

Merged

Conversation

GSadee
Copy link
Member

@GSadee GSadee commented Jun 17, 2021

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets based on #12713
License MIT

@GSadee GSadee added the API APIs related issues and PRs. label Jun 17, 2021
@GSadee GSadee requested a review from a team as a code owner June 17, 2021 11:13
@probot-autolabeler probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Jun 17, 2021
@GSadee GSadee force-pushed the api-shop-shipping-methods-contract-test branch from 120da15 to 55fa3c8 Compare June 17, 2021 11:17
@GSadee GSadee force-pushed the api-shop-shipping-methods-contract-test branch 2 times, most recently from 5193a32 to d17f037 Compare June 18, 2021 05:32
@GSadee GSadee force-pushed the api-shop-shipping-methods-contract-test branch from d17f037 to bffc4c6 Compare June 18, 2021 05:44
Copy link
Contributor

@SirDomin SirDomin left a comment

Choose a reason for hiding this comment

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

image
Do we need these 2 empty files?

@GSadee GSadee force-pushed the api-shop-shipping-methods-contract-test branch from bffc4c6 to 6ef6761 Compare June 21, 2021 05:10
@GSadee
Copy link
Member Author

GSadee commented Jun 21, 2021

image
Do we need these 2 empty files?

@SirDomin these files are not empty, just no changes were made to them except the filename

;
}

private function isNotAdminGetOperation(array $context): bool
Copy link
Member

Choose a reason for hiding this comment

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

The second normalizer where we use such a function, maybe it would worth to extract it into some separate service in the future 🖖 💃

@GSadee GSadee force-pushed the api-shop-shipping-methods-contract-test branch 2 times, most recently from 45978e5 to f979c4d Compare June 21, 2021 13:59
@GSadee GSadee force-pushed the api-shop-shipping-methods-contract-test branch from f979c4d to dedad00 Compare June 22, 2021 04:30
@AdamKasp AdamKasp merged commit bfec4ad into Sylius:master Jun 22, 2021
@AdamKasp
Copy link
Contributor

Thank you, Grzegorz! 🥇

@GSadee GSadee deleted the api-shop-shipping-methods-contract-test branch June 22, 2021 05:47
{
"@id": "\/api\/v2\/shop\/shipping-methods\/UPS",
"@type": "ShippingMethod",
"id": @integer@,
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
"id": @integer@,
"id": "@integer@",

From the matcher's point of view, such a notation would be equal to the current one. The change is, that GitHub nor phpstorm would complain about invalid json shema. On the other hand, it would look like there is a string at the integer field... Just a proposal. It is worth it to put all matchers into ", but I would love to hear your opinion about it as well

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. Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants