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

[ProductAttribute] Fix select product attribute choice value rename #8737

Merged
merged 4 commits into from Sep 29, 2017

Conversation

Projects
None yet
2 participants
@GSadee
Member

GSadee commented Sep 26, 2017

Q A
Bug fix? yes
New feature? no
BC breaks? no
Related tickets partially fixes #8308
License MIT

This PR fixes losing reference to an attribute value on a product when select attribute choice value renames.

@GSadee GSadee added the Bug Fix label Sep 26, 2017

@GSadee GSadee added this to the 1.1 milestone Sep 26, 2017

@pamil

This comment has been minimized.

Show comment
Hide comment
@pamil

pamil Sep 26, 2017

Member

Can we split these two fixes to two standalone PRs? Would be easier to review & merge then :)

Member

pamil commented Sep 26, 2017

Can we split these two fixes to two standalone PRs? Would be easier to review & merge then :)

@pamil

This comment has been minimized.

Show comment
Hide comment
@pamil

pamil Sep 26, 2017

Member

Btw. as these are bugfixes, it should be targeted against 1.0

Member

pamil commented Sep 26, 2017

Btw. as these are bugfixes, it should be targeted against 1.0

@pamil pamil modified the milestones: 1.1, 1.0 Sep 26, 2017

@pamil pamil changed the base branch from master to 1.0 Sep 26, 2017

@GSadee GSadee changed the title from [WIP][ProductAttribute] Select product attribute fixes to [ProductAttribute] Fix select product attribute value rename Sep 26, 2017

Show outdated Hide outdated src/Sylius/Behat/Context/Setup/ProductAttributeContext.php
Show outdated Hide outdated src/Sylius/Behat/Context/Setup/ProductAttributeContext.php
Show outdated Hide outdated src/Sylius/Behat/Context/Setup/ProductAttributeContext.php
Show outdated Hide outdated src/Sylius/Behat/Context/Ui/Admin/ManagingProductAttributesContext.php
Show outdated Hide outdated ...ype/AttributeType/Configuration/SelectAttributeChoicesCollectionType.php
"green": "green",
"black": "black"
},
"choices": @array@,

This comment has been minimized.

@pamil

pamil Sep 28, 2017

Member

IMO we should change it to something like:

{
 "@string@": "yellow",
 "@string@": "green",
 // etc..
}
@pamil

pamil Sep 28, 2017

Member

IMO we should change it to something like:

{
 "@string@": "yellow",
 "@string@": "green",
 // etc..
}

This comment has been minimized.

@GSadee

GSadee Sep 28, 2017

Member

I know and I agree that current example doesn't check behaviour properly, but this feature is not implemented in php-matcher (there is even an issue: coduo/php-matcher#92)

@GSadee

GSadee Sep 28, 2017

Member

I know and I agree that current example doesn't check behaviour properly, but this feature is not implemented in php-matcher (there is even an issue: coduo/php-matcher#92)

This comment has been minimized.

@pamil

pamil Sep 29, 2017

Member

What about adding an additional assertion in the test file, eg. asserting that the response matches this file contents + getting $response['configuration']['choices'] and asserting they are what we expect (like https://github.com/Sylius/SyliusElasticSearchPlugin/blob/master/tests/Controller/SearchControllerApiTest.php#L436)?

@pamil

pamil Sep 29, 2017

Member

What about adding an additional assertion in the test file, eg. asserting that the response matches this file contents + getting $response['configuration']['choices'] and asserting they are what we expect (like https://github.com/Sylius/SyliusElasticSearchPlugin/blob/master/tests/Controller/SearchControllerApiTest.php#L436)?

This comment has been minimized.

@GSadee

GSadee Sep 29, 2017

Member

Done 👍

@GSadee

GSadee Sep 29, 2017

Member

Done 👍

And I name it "Mug material" in "English (United States)"
And I add material "-100% Banana Skin"
And I add value "-100% Banana Skin"

This comment has been minimized.

@pamil

pamil Sep 28, 2017

Member

Value is kinda misleading, my first - AttributeValue, the second - the same. What about naming it an "option" or a "select option"?

@pamil

pamil Sep 28, 2017

Member

Value is kinda misleading, my first - AttributeValue, the second - the same. What about naming it an "option" or a "select option"?

This comment has been minimized.

@GSadee

GSadee Sep 28, 2017

Member

But we have values in configuration of a product attribute, so I wanted to be consistent with the layout:
zrzut ekranu 2017-09-28 o 15 54 29

@GSadee

GSadee Sep 28, 2017

Member

But we have values in configuration of a product attribute, so I wanted to be consistent with the layout:
zrzut ekranu 2017-09-28 o 15 54 29

This comment has been minimized.

@pamil

pamil Sep 29, 2017

Member

I see the reasoning behind it, but it's still confusing, not sure whether we should solve it already or leave it as is :)

@pamil

pamil Sep 29, 2017

Member

I see the reasoning behind it, but it's still confusing, not sure whether we should solve it already or leave it as is :)

@GSadee GSadee changed the title from [ProductAttribute] Fix select product attribute value rename to [ProductAttribute] Fix select product attribute choice value rename Sep 29, 2017

@pamil

Let's fix that API test and it's good to go! 🎉

And I name it "Mug material" in "English (United States)"
And I add material "-100% Banana Skin"
And I add value "-100% Banana Skin"

This comment has been minimized.

@pamil

pamil Sep 29, 2017

Member

I see the reasoning behind it, but it's still confusing, not sure whether we should solve it already or leave it as is :)

@pamil

pamil Sep 29, 2017

Member

I see the reasoning behind it, but it's still confusing, not sure whether we should solve it already or leave it as is :)

"green": "green",
"black": "black"
},
"choices": @array@,

This comment has been minimized.

@pamil

pamil Sep 29, 2017

Member

What about adding an additional assertion in the test file, eg. asserting that the response matches this file contents + getting $response['configuration']['choices'] and asserting they are what we expect (like https://github.com/Sylius/SyliusElasticSearchPlugin/blob/master/tests/Controller/SearchControllerApiTest.php#L436)?

@pamil

pamil Sep 29, 2017

Member

What about adding an additional assertion in the test file, eg. asserting that the response matches this file contents + getting $response['configuration']['choices'] and asserting they are what we expect (like https://github.com/Sylius/SyliusElasticSearchPlugin/blob/master/tests/Controller/SearchControllerApiTest.php#L436)?

@pamil

pamil approved these changes Sep 29, 2017

@pamil pamil merged commit dee7829 into Sylius:1.0 Sep 29, 2017

2 checks passed

Scrutinizer 1 new issues, 16 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pamil

This comment has been minimized.

Show comment
Hide comment
@pamil

pamil Sep 29, 2017

Member

Thank you Rzegś! 🎉

Member

pamil commented Sep 29, 2017

Thank you Rzegś! 🎉

@GSadee GSadee deleted the GSadee:select-attribute-rename-fix branch Sep 29, 2017

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