Vinai / groupscatalog2

Magento extension to enable you to hide categories and products from customers depending on their customer group. This is a Magento 1.6 and newer compatible version of the Netzarbeiter Customer Groups Catalog extension.
139 stars 60 forks source link

When HIDE_GROUPS_ATTRIBUTE isn't set, should fall back to USE_DEFAULT instead #151

Closed kiatng closed 6 years ago

kiatng commented 6 years ago

I just installed groupscatalog2 in an existing CE 1.9.x. In the System Configuration, Hide Categories from is set to [NONE], which is netzarbeiter_groupscatalog2/general/category_default_hide = -1 in the table _core_configdata. The existing categories have the attribute HIDE_GROUPS_ATTRIBUTE like this:

image

However, all categories are hidden when groupscatalog2 is enabled. To show the categories, I need to save each category to set the attribute HIDE_GROUPS_ATTRIBUTE to [USE DEFAULT] with value -2. I expect that when HIDE_GROUPS_ATTRIBUTE isn't set, it should fall back to USE_DEFAULT instead.

I fixed with the following in Netzarbeiter_GroupsCatalog2_Helper_Data

//kiat: fixed bug: when the entity's attribute isn't set, fall back on [USE DEFAULT]
if ($groupIds === null) {
    $groupIds = array(self::USE_DEFAULT);
} elseif (!is_array($groupIds) && !is_string($groupIds)) {
    // If the value isn't set on the entity mode fall back to querying the db index table
    $visibility = Mage::getResourceSingleton('netzarbeiter_groupscatalog2/filter')
            ->isEntityVisible($entity, $customerGroupId);
    $entity->setData(self::HIDE_GROUPS_ATTRIBUTE_STATE_CACHE, $visibility);
    return $visibility;
}

In the above, the existing code elseif will always be false since when $groupIds is not null, it is either of type string or array.

In the original code, if the category was saved so that $groupIds is not null, elseif will always be false. The elseif is true only when HIDE_GROUPS_ATTRIBUTE isn't set, but in this case, $visibility is always false because the _groupid column in both idx tables cannot hold the value -2 [USE DEFAULT]. Hence, I think this is a bug.

Vinai commented 6 years ago

Hi @kiatng, thanks for opening the issue. I haven't worked with the module in many years and don't support it any more, but if you want to submit a PR I'd be happy to merge it. If you can reproduce the issue in a test case before you fix it it would be even better 👍

kiatng commented 6 years ago

Hi @Vinai Thanks for the great extension. The earlier ver has been working great all these years. Today, I installed ver 2 in another project, and deployed to production and that's when complaints started coming my way. I made a patch and deployed it. No complaint so far. So I made my very first PR.