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] [Product] [ProductTranslation] Add api configuration and test for adding product and product translations #11213

Merged
merged 3 commits into from
Mar 13, 2020

Conversation

Tomanhez
Copy link
Contributor

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

@Tomanhez Tomanhez requested a review from a team as a code owner March 11, 2020 20:54
@Tomanhez Tomanhez force-pushed the crud-product branch 2 times, most recently from dc1326f to bdade86 Compare March 12, 2020 08:19
Comment on lines 53 to 54
$data = ['translations' => [$language => ['locale' => $language]]];
$data['translations'][$language]['name'] = $name;
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
$data = ['translations' => [$language => ['locale' => $language]]];
$data['translations'][$language]['name'] = $name;
$data = [
'translations' => [
$language => [
'locale' => $language,
'name' => $name,
],
],
];

Copy link
Member

Choose a reason for hiding this comment

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

What will happen if 'locale' => $language will be omitted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not working

*/
public function iSetItsSlugTo(?string $slug = null, $language = 'en_US'): void
{
$data = ['translations' => [$language => ['slug' => $slug]]];
Copy link
Member

Choose a reason for hiding this comment

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

Why is this configuration different than the configuration for the name field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is problem with doubling locale key when array is merged in addCompoundRequestData(), I think that this method should be refactored
Screenshot 2020-03-11 at 21 15 36

@GSadee GSadee added Admin AdminBundle related issues and PRs. API APIs related issues and PRs. Feature New feature proposals. labels Mar 12, 2020
@Tomanhez Tomanhez force-pushed the crud-product branch 2 times, most recently from 443576b to 50628ea Compare March 13, 2020 08:28
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.

However, I would wait for #11210 before merging

$data = [
'translations' => [
$localeCode => [
'slug' => $slug,
Copy link
Member

Choose a reason for hiding this comment

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

This should work now:

Suggested change
'slug' => $slug,
'locale' => $localeCode,
'slug' => $slug,

*/
public function theProductShouldAppearInTheShop(string $productName): void
{
$this->client->index();
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
$this->client->index();
$response = $this->client->index();

Assert::true(
$this
->responseChecker
->hasItemWithTranslation($this->client->getLastResponse(),'en_US', 'name', $productName)
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
->hasItemWithTranslation($this->client->getLastResponse(),'en_US', 'name', $productName)
->hasItemWithTranslation($response, 'en_US', 'name', $productName)

$this
->responseChecker
->hasItemWithTranslation($this->client->getLastResponse(),'en_US', 'name', $productName)
);
Copy link
Member

Choose a reason for hiding this comment

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

Missing exception message

*/
public function iShouldBeNotifiedThatItHasBeenSuccessfullyCreated(): void
{
Assert::true($this->responseChecker->isCreationSuccessful($this->client->getLastResponse()));
Copy link
Member

Choose a reason for hiding this comment

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

Missing exception message

@lchrusciel lchrusciel merged commit 498b78f into Sylius:api Mar 13, 2020
@lchrusciel
Copy link
Member

Thank you, Tomasz! 🥇

@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
Labels
Admin AdminBundle related issues and PRs. API APIs related issues and PRs. Feature New feature proposals.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants