Fixed MAGE-4238 (#109): disabled select all checkbox when the same optio... #221

Merged
merged 3 commits into from Mar 10, 2013

2 participants

@lindyk

Disabled select all checkbox when the same options show up for different simple product under configurable associated product grid

Kao Fixed MAGE-4238 (#109): disabled select all checkbox when the same op…
…tions show in the associated product grid
598eb3d
@zerkella zerkella was assigned Mar 10, 2013
@zerkella

It is better to name the method on action what it does - e.g. _loadConfigurableAttribute() or _cacheConfigurableAttributes().
However, in current case the best name is _getConfigurableAttributes(), as this method must return the array with attributes. Otherwise, the workflow is not clear - the method is called, then data is fetched from other source ($_configAttributeCodes)

@zerkella

When there are no config attributes in the product, then the caching does not work. Suggested:

  • Make _configAttributeCodes null by default
  • Make condition here as if ($this->_configAttributeCodes == null)...

And there is space needed after if

@zerkella

A very important thing is that the submitted changeset fixes UI only, but it definitely doesn't fix model or controllers logic, which still allows to run into such an inappropriate situation, as described in the issue ticket.

It is expected, that those models must be fixed as well.

It would be better to locate the methods in the order how they are referenced in the code. So _afterCollection() must be first, then the _configurableAttributes(), then retrieveRowData(), etc. Otherwise for a reader, it is hard to read the code - he reads the class as separate and completely unrelated methods at first, and only one method at the end combines them all together at the time, when a reader has already forgotten, what those methods do.

@zerkella

The description is better to be re-written. It doesn't make anything more clear.

Expected, that the description should describe purpose of the method. E.g. "Template method, called after the collection is loaded. Process product attributes and update view of the grid accordingly"

@zerkella

Not sure, that _afterLoadCollection() is the best method to put this logic to. What do you think about _beforeToHtml() - it seems to be more related to rendering, which is affected by its functionality.

@zerkella

It seems, we don't need to do anything at all, when there are no attribute codes. we can just return from the method call in such a case.

We can restructure the method like:

function () 
{
    parent::_afterLoadCollection();
    if (!$this->_configAttributeCodes()) {
        return $this;
    }

    ...disable the checkbox if needed...

    return $this;
}
@zerkella

_retrieveRowData() may produce bool, which doesn't suit to be passed to _checkRowData()

@zerkella

This method can be simplifed much.

The selected products have same attribute set id, thus they will have same attribute codes to compare values. So the arrays compared are:

  • Always of the same size
  • Always contain same attribute codes

It means, that we can create associative arrays as (code => value) rather than indexed (['code' => <code>, 'value' => <value>], ...). Thus the checking, whether some values differ, can be done just by straight comparison of the arrays. We even don't need this method at all.

$multiSelectAllow = ($needleAttributeValues == $attributeValues)
@lindyk lindyk was assigned Mar 10, 2013
@zerkella

Reopening to fix the issues found and submit the pull request again.
And, of course, we're waiting for the test :)

Kao and others added some commits Mar 10, 2013
Kao Fixed MAGE-4238 (#109): disabled select all checkbox when the same op…
…tions show in the associated product grid with functional testing
41496b8
@zerkella zerkella Fixed MAGE-4238 (#109): disabled select all checkbox when the same op…
…tions show in the associated product grid with functional testing

- improved readability of changed code
- removed unfinished test
09cff8b
@zerkella

The code change is ok, removed the non-finished taf tests - they can be made later. Accepted pull request.

Thanks!

@zerkella zerkella merged commit 30ef052 into master Mar 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment