Skip to content

Solution for 12825 issue. #13085

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

Closed
wants to merge 3 commits into from
Closed

Solution for 12825 issue. #13085

wants to merge 3 commits into from

Conversation

mayankzalavadia
Copy link
Contributor

@mayankzalavadia mayankzalavadia commented Jan 10, 2018

Swatch Attribute sorting is not working after updating the position of the attribute options (using drag)

Description

Fixed Issues (if relevant)

  1. Configurable options(Text swatch) type does not order on the basis of the sort order provided #12825: Configurable options(Text swatch) type does not order on the basis of the sort order provided

Manual testing scenarios

  1. Go to Store -> Attributes -> Product -> Edit Swatch Attribute -> Change option sort order (using Drag)
  2. Go to Product page does not display attribute options, sort order wise.
  3. Please follow issue number Configurable options(Text swatch) type does not order on the basis of the sort order provided #12825

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-engcom-team magento-engcom-team added mm18in Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Jan 10, 2018
@dmanners
Copy link
Contributor

Hi @mayankzalavadia could you please look into the travis failures.

There were 2 failures:
1) Magento\ConfigurableProduct\Test\Unit\Model\ResourceModel\Attribute\OptionSelectBuilderTest::testGetSelect
Zend_Db_Select::joinInner(Array (...), 'attribute_opt.option_id = ent....value', Array (), null) was not expected to be called more than 6 times.
/home/travis/build/magento/magento2/app/code/Magento/ConfigurableProduct/Model/ResourceModel/Attribute/OptionSelectBuilder.php:109
/home/travis/build/magento/magento2/app/code/Magento/ConfigurableProduct/Test/Unit/Model/ResourceModel/Attribute/OptionSelectBuilderTest.php:148
2) Magento\ConfigurableProduct\Test\Unit\Model\ResourceModel\Attribute\OptionSelectBuilderTest::testGetSelectWithBackendModel
Magento\Framework\Model\ResourceModel\Db\AbstractDb::getTable('eav_attribute_option') was not expected to be called more than 7 times.
/home/travis/build/magento/magento2/app/code/Magento/ConfigurableProduct/Model/ResourceModel/Attribute/OptionSelectBuilder.php:107
/home/travis/build/magento/magento2/app/code/Magento/ConfigurableProduct/Test/Unit/Model/ResourceModel/Attribute/OptionSelectBuilderTest.php:194

It seems that some mocked methods are now called more than previously in these tests.

Also the static tests are failing with the following error.

There was 1 failure:
1) Magento\Test\Php\LiveCodeTest::testCodeMess
PHP Code Mess has found error(s):
/home/travis/build/magento/magento2/app/code/Magento/ConfigurableProduct/Model/ResourceModel/Attribute/OptionSelectBuilder.php:45	The method getSelect() has 103 lines of code. Current threshold is set to 100. Avoid really long methods.

@mayankzalavadia
Copy link
Contributor Author

Hello @dmanners ,
Can you please help on this. I don't know how to solve this type of failures.

@dmanners
Copy link
Contributor

Sure @mayankzalavadia I will drop you a message about it once I have a moment.

@mayankzalavadia
Copy link
Contributor Author

Hello, @dmanners I am waiting for your message.

@dmanners dmanners self-assigned this Jan 11, 2018
@dmanners dmanners added this to the January 2018 milestone Jan 11, 2018
@dmanners
Copy link
Contributor

@mayankzalavadia I have just push a commit f3fb017 which should fix 1 of the tests.

Using the command ./vendor/bin/phpunit --filter testGetSelect --config dev/tests/unit/phpunit.xml.dist app/code/Magento/ConfigurableProduct/Test/Unit/Model/ResourceModel/Attribute/OptionSelectBuilderTest.php I was able to run the tests with name like testGetSelect. This allowed me to debug the two broken tests. To fix one of the tests I had to update the number of times an object expected a method to be called. $this->select->expects($this->exactly(6))->method('joinInner')->willReturnSelf(); here I updated from 5 to 6.

Do you think you would be able to have a look at the other unit test failure and the static test failure also?

Thanks

@mayankzalavadia
Copy link
Contributor Author

Hello, @dmanners
Thank you for your help. Now I am able to look at the other unit test failure.

@mayankzalavadia
Copy link
Contributor Author

mayankzalavadia commented Jan 12, 2018

@dmanners I have tested the code as per your given command and push the commit with appropriate changes but still, we got some test failure.
So, can you please help me on this.?
test ok

@dmanners
Copy link
Contributor

@mayankzalavadia it seems that there have been changes to these classes in 2.2-develop https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/ConfigurableProduct/Test/Unit/Model/ResourceModel/Attribute/OptionSelectBuilderTest.php is very different from the version on your branch.

Could you please update your fork (https://help.github.com/articles/syncing-a-fork/) and then make sure your branch is up to date with 2.2-develop.

Thank you

@dmanners
Copy link
Contributor

Actually @mayankzalavadia if seems that this problem has already been fixed. See commit 9794d38 and PR #12963 in that case I will close this pr.

Thank you for your effort.

@dmanners dmanners closed this Jan 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Progress: needs update Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants