Update select.js #9475

redelschaap · 2017-05-02T17:32:16Z

Description

Fixed a bug where options.reduce was not available in case options was generated by an array with non-numeric or missing numeric keys in PHP. In that case a TypeError get thrown, because options.reduce is not a function of an object.

Uncaught TypeError: Unable to process binding "text: function (){return $col.getLabel($row()) }"
Message: options.reduce is not a function

Manual testing scenarios

Have a select attribute where the options are being generated from an option array in PHP, where that array have non-numeric keys or missing numeric keys. For example:

$options = [
    1 => ['label' => 'Option A', 'value' => 1],
    3 => ['label' => 'Option C', 'value' => 3],
    4 => ['label' => 'Option D', 'value' => 4],
];

Load a grid where that attribute has an active column
The grid won't fully load (it loads until the first cell where that attribute is used) and JS throws an error:

Uncaught TypeError: Unable to process binding "text: function (){return $col.getLabel($row()) }"
Message: options.reduce is not a function
    at UiClass.flatOptions (select.js:58)
    at UiClass.getLabel (select.js:38)
    at UiClass.getLabel (wrapper.js:109)
    at text (eval at createBindingsStringEvaluator (knockout.js:2624), <anonymous>:3:69)
    at update (knockout.js:4260)
    at ko.dependentObservable.disposeWhenNodeIsRemoved (knockout.js:3004)
    at evaluateImmediate (knockout.js:1737)
    at Object.ko.computed.ko.dependentObservable (knockout.js:1946)
    at knockout.js:3002
    at Object.arrayForEach (knockout.js:151)

Fixed a bug where options.reduce was not available in case options was generated by an array with non-numeric or missing numeric keys in PHP.

omiroshnichenko · 2017-05-13T08:05:23Z

app/code/Magento/Ui/view/base/web/js/grid/columns/select.js

@@ -55,6 +55,10 @@ define([
        flatOptions: function (options) {
            var self = this;

+            if (!Array.isArray(options)) {


Please, use _.isArray function to check.

omiroshnichenko

Can you cover this with unit test? Test already created(dev/tests/js/jasmine/tests/app/code/Magento/Ui/base/js/grid/columns/select.test.js), but this case did not covered.

Used `_.isArray` instead of `Array.isArray` as requested

Added a scenario where options are loaded as an object instead of an array. Options are generated in PHP. This is the case when the option array in PHP has non-numeric keys or has numeric keys but misses some numeric keys. I noticed the opts test was deleted by someone in an earlier commit, but I think this is too important to remove from the test, so I added it again.

redelschaap

@omiroshnichenko I made the changes and covered it in the test file.

Wanted to add the note that the opt test is still present in the 2.1-develop branch, so I think it may be removed by mistake.

ishakhsuvarov · 2017-05-26T09:30:25Z

@redelschaap Travis CI did not execute tests on this PR due to the error:

abuse detected: known offender (request looked fishy)

We will execute tests on this PR in the internal CICD, however you may want to contact Travis CI support to figure out what is wrong.
Thanks.

redelschaap · 2017-05-26T11:11:12Z

Okay great. I guess they gave that error because I committed 4 or 5 times within half an hour. Is there anything I need to do now?

ishakhsuvarov · 2017-05-26T11:55:45Z

@redelschaap It's ok on our side. We will let you know if there is anything specific found by internal CICD jobs.

ishakhsuvarov · 2017-05-26T13:07:42Z

@redelschaap
Looks like there are some errors regarding JS Unit tests implemented, here is partial output:

Ui/js/grid/columns/select
[exec] getLabel method
[exec] - get label while options empty......✓
[exec] - get label for existed value......X
[exec] Expected '' to be 'b'. (1)
[exec] - get label for existed value in case the options are initialized as an object......X
[exec] Expected '' to be 'c'. (1)

I suggest you talking to Travis CI support to fix builds, so you could fix the errors.
Thanks

Fix for jasmine tests

redelschaap · 2017-05-26T17:39:58Z

@ishakhsuvarov I can't get grunt to work with jasmine tests, but when analyzing select.js and column.js this should do it. In the mean time I will try to reach Travis CI support.

Changed file to re-run Travis test

Fixed style issues

redelschaap · 2017-05-30T07:45:11Z

@ishakhsuvarov Travis CI is working again on my account. Travis' tests for select.js succeeded, but two integration tests for PHP failed on some tests in Magento_Sales, but I did not change anything about that. In Build #4295 you can see those integration tests succeeded, so I think Travis failed in the last build.

ishakhsuvarov · 2017-05-30T08:17:54Z

@redelschaap We are currently experiencing some issues with integration test failures. I've restarted build jobs, hope it's green this time.
Thank you for your effort.

redelschaap · 2017-05-30T08:35:10Z

@ishakhsuvarov It did :)

[EngCom] Public Pull Requests - MAGETWO-69573: Adding logo in media folder #9797 - MAGETWO-69555: Allow for referenceBlock to include template argument #9772 - MAGETWO-69540: Fix for #5897: getIdentities relies on uninitialized collection #9777 - MAGETWO-69533: [BUGFIX][6244] Fix Issue with code label display in cart checkout. #9721 - MAGETWO-69499: Update select.js #9475 - MAGETWO-69451: Replace Zend_Json in the configurable product block test #9753 - MAGETWO-69373: Customer with unique attribute can't be saved #7844 #9712 - MAGETWO-69369: Replace the direct usage of Zend_Json with a call to the Json Help class #9344 - MAGETWO-69085: Do not hardcode product link types #9600 - MAGETWO-69554: Patch to allow multiple filter_url_params to function #9723

magento-team · 2017-06-01T20:50:35Z

@redelschaap thank you for your contribution

redelschaap · 2017-06-02T08:23:45Z

You're welcome, happy to help!

Update select.js

dea38d9

Fixed a bug where options.reduce was not available in case options was generated by an array with non-numeric or missing numeric keys in PHP.

omiroshnichenko reviewed May 13, 2017

View reviewed changes

ishakhsuvarov added the Progress: needs update label May 13, 2017

ishakhsuvarov self-assigned this May 13, 2017

ishakhsuvarov added this to the May 2017 milestone May 13, 2017

redelschaap added 3 commits May 26, 2017 08:48

Update select.js

257635f

Used `_.isArray` instead of `Array.isArray` as requested

Update select.js

c8d9228

Used `_.isArray` instead of `Array.isArray` as requested

redelschaap commented May 26, 2017

View reviewed changes

ishakhsuvarov closed this May 26, 2017

ishakhsuvarov reopened this May 26, 2017

omiroshnichenko approved these changes May 26, 2017

View reviewed changes

ishakhsuvarov removed the Progress: needs update label May 26, 2017

ishakhsuvarov added the Progress: needs update label May 26, 2017

Update select.test.js

913122a

Fix for jasmine tests

redelschaap added 2 commits May 29, 2017 17:21

Update select.test.js

44aa960

Changed file to re-run Travis test

Update select.test.js

0ca74d6

Fixed style issues

ishakhsuvarov added Progress: accept and removed Progress: needs update labels May 30, 2017

okorshenko modified the milestones: May 2017, June 2017 Jun 1, 2017

okorshenko modified the milestones: June 2017, May 2017 Jun 1, 2017

magento-team merged commit 0ca74d6 into magento:develop Jun 1, 2017

magento-team pushed a commit that referenced this pull request Jun 1, 2017

MAGETWO-69499: Update select.js #9475

b48d29d

redelschaap deleted the patch-8 branch June 2, 2017 08:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update select.js #9475

Update select.js #9475

redelschaap commented May 2, 2017 •

edited by ishakhsuvarov

Loading

omiroshnichenko May 13, 2017

omiroshnichenko left a comment

redelschaap left a comment •

edited

Loading

ishakhsuvarov commented May 26, 2017

redelschaap commented May 26, 2017

ishakhsuvarov commented May 26, 2017

ishakhsuvarov commented May 26, 2017

redelschaap commented May 26, 2017

redelschaap commented May 30, 2017 •

edited

Loading

ishakhsuvarov commented May 30, 2017

redelschaap commented May 30, 2017

magento-team commented Jun 1, 2017

redelschaap commented Jun 2, 2017

Update select.js #9475

Update select.js #9475

Conversation

redelschaap commented May 2, 2017 • edited by ishakhsuvarov Loading

Description

Manual testing scenarios

omiroshnichenko May 13, 2017

Choose a reason for hiding this comment

omiroshnichenko left a comment

Choose a reason for hiding this comment

redelschaap left a comment • edited Loading

Choose a reason for hiding this comment

ishakhsuvarov commented May 26, 2017

redelschaap commented May 26, 2017

ishakhsuvarov commented May 26, 2017

ishakhsuvarov commented May 26, 2017

redelschaap commented May 26, 2017

redelschaap commented May 30, 2017 • edited Loading

ishakhsuvarov commented May 30, 2017

redelschaap commented May 30, 2017

magento-team commented Jun 1, 2017

redelschaap commented Jun 2, 2017

redelschaap commented May 2, 2017 •

edited by ishakhsuvarov

Loading

redelschaap left a comment •

edited

Loading

redelschaap commented May 30, 2017 •

edited

Loading