Skip to content
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

Implement the possibility to search for null in backend grids #1203

Merged
merged 10 commits into from
Aug 31, 2022

Conversation

digitalpianism
Copy link
Contributor

Description (*)

This modification will let users search for NULL entries in the backend grids.

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes Possibility to search for null in backend grids #1189

Manual testing scenarios (*)

  1. Access a backend grid where an entry has a NULL value
  2. Filter the grid searching for NULL in the column input
  3. Enjoy the result

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@github-actions github-actions bot added the Component: Adminhtml Relates to Mage_Adminhtml label Sep 8, 2020
@luigifab
Copy link
Collaborator

luigifab commented Dec 7, 2021

I tried but it does not work on our websites, I did that:

$filtered = array_map(static function ($value) {
    return is_object($value) ? $value->__toString() : $value;
}, is_array($cond) ? array_values($cond) : [$cond]);
if (in_array('\'%NULL%\'', $filtered, true) || in_array('NULL', $filtered, true)) {
    $this->getCollection()->addFieldToFilter($field, ['null' => true]);
} else {
    $this->getCollection()->addFieldToFilter($field, $cond);
}

In my test, $cond contains Zend_Db_Expr.
It contains also a int when filter reviews in backend (type customer).

@luigifab
Copy link
Collaborator

I still using my version, but not really used because I forgot that now I can search with NULL :)

@fballiano
Copy link
Contributor

@luigifab should we push your version to this PR?

@luigifab
Copy link
Collaborator

luigifab commented Jun 8, 2022

Why not.

@fballiano
Copy link
Contributor

done, hope correctly :-)

luigifab
luigifab previously approved these changes Jun 9, 2022
@ADDISON74
Copy link
Collaborator

This one is failing again with PHPStan.

@luigifab
Copy link
Collaborator

luigifab commented Jun 9, 2022

PHPStan is wrong! Where is the support? 😸

@ADDISON74
Copy link
Collaborator

This PR is useful only when there are no data on a column of a Backend grid. I can get this information by one click on column title. Of course is a difference between a sort and a filtering action. It is easier to do mass-actions with filtered information (Select All).

tobihille
tobihille previously approved these changes Aug 20, 2022
kiatng
kiatng previously approved these changes Aug 20, 2022
Copy link
Collaborator

@kiatng kiatng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and worked. @tobihille Thanks for the bump.

@fballiano
Copy link
Contributor

somebody understands what phpstan is complaining about?

@kiatng
Copy link
Collaborator

kiatng commented Aug 21, 2022

@fballiano It's caused by the docblock here:

/**
* get collection object
*
* @return Varien_Data_Collection
*/
public function getCollection()
{
return $this->_collection;
}

It should be

     * @return Varien_Data_Collection_Db

But I am not sure what should be done to fix that in this PR.

[edit] Also

* @param Varien_Data_Collection $collection

@sreichel
Copy link
Contributor

It should be

     * @return Varien_Data_Collection_Db

This would work, but not sure if its a good idea ... will check tomorrow

  • * @param Varien_Data_Collection|Varien_Data_Collection_Db $collection
  • * @return Varien_Data_Collection|Varien_Data_Collection_Db

@fballiano
Copy link
Contributor

@sreichel did you have time to check this?

@sreichel
Copy link
Contributor

Yes, fix phpstan-baseline.neon ... change count 1 to 2 for now. Its just a workaround, but removing this supressed errors is what i frequently do ... it will be fixed later ....

		-
			message: "#^Call to an undefined method Varien_Data_Collection\\:\\:addFieldToFilter\\(\\)\\.$#"
			count: 1
			path: ../app/code/core/Mage/Adminhtml/Block/Widget/Grid.php

kiatng
kiatng previously approved these changes Aug 24, 2022
@sreichel
Copy link
Contributor

@digitalpianism i'm also confused. 47e461c was okay. All checks passed. I've reverted this change.

@sreichel
Copy link
Contributor

Problem found: 2214b17 reverted all changes. I'll fix this in the evening ... (reapply fix)

@kiatng
Copy link
Collaborator

kiatng commented Aug 30, 2022

My bad, sorry about that. Please let me know what I did wrongly.

@sreichel
Copy link
Contributor

@kiatng i dont know. I guess you just used the "Update branch" button.

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml phpstan labels Aug 30, 2022
fballiano
fballiano previously approved these changes Aug 30, 2022
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test on customer grid, works

@fballiano
Copy link
Contributor

(but we should add something to the README)

@sreichel
Copy link
Contributor

(but we should add something to the README)

You successfully tested it, its your job :P

@fballiano
Copy link
Contributor

(but we should add something to the README)

You successfully tested it, its your job :P

fair enough 🤣

@sreichel sreichel merged commit 537e68e into OpenMage:1.9.4.x Aug 31, 2022
@github-actions

This comment has been minimized.

1 similar comment
@github-actions
Copy link

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 537e68e. ± Comparison against base commit 988e9ff.

fballiano added a commit to fballiano/openmage that referenced this pull request Aug 31, 2022
…ge#1203)

* implement OpenMage#1189

* Updated the code from luigifab comments

* Fixed baseline

* Fixed phpstan-baseline.neon

* Fixed PR

* Update README.md

Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
Co-authored-by: Ng Kiat Siong <kiatsiong.ng@gmail.com>
Co-authored-by: sv3n <github-sr@hotmail.com>
@luigifab
Copy link
Collaborator

luigifab commented Sep 1, 2022

The committed version is wrong if I remember correctly (not tested again).

We need: is_array($cond) ? array_values($cond) : [$cond]
because $cond is also a int when we filter reviews in backend on type customer.

@kiatng
Copy link
Collaborator

kiatng commented Sep 1, 2022

I have this in production (I needed this exact feature). I have searched for NULL, int ranges, string, and date ranges. All work correctly. Can you provide a test case where it fails?

@kiatng
Copy link
Collaborator

kiatng commented Sep 1, 2022

because $cond is also a int when we filter reviews in backend on type customer

I cannot test the above because I do not have customer reviews.

But filtering for store_id in order grid and customer_group in customer grid work fine.

@kiatng
Copy link
Collaborator

kiatng commented Sep 1, 2022

Looking at the code:

$cond = $column->getFilter()->getCondition();
if ($field && $cond !== null) {
$filtered = array_map(static function ($value) {
return is_object($value) ? $value->__toString() : $value;
}, array_values($cond));

If $cond is of type int or not an array, then it will throw on line 503

Warning: array_values() expects parameter 1 to be array

When searching for all the function getCondition() in folder https://github.com/OpenMage/magento-lts/tree/1.9.4.x/app/code/core/Mage/Adminhtml/Block/Widget/Grid/Column/Filter

The function return either null or array as far as I can see, except for

public function getCondition()
{
if ($this->getValue()) {
return $this->getColumn()->getValue();
}
else {
return [
['neq'=>$this->getColumn()->getValue()],
['is'=>new Zend_Db_Expr('NULL')]
];
}
//return array('like'=>'%'.$this->getValue().'%');
}

and Radio.php, which can return string|null|array.

But I have not seen a grid column that uses checkbox or radio (checkboxes in massaction doesn't count). So what do we do?

@sreichel
Copy link
Contributor

sreichel commented Sep 1, 2022

I cannot test the above because I do not have customer reviews.

Tested with sample data and @luigifab is right. PR incoming.

@sreichel
Copy link
Contributor

sreichel commented Sep 1, 2022

If $cond is of type int or not an array, then it will throw on line 503

Warning: array_values() expects parameter 1 to be array

This is what happens on customer review page + filter. :)

@kiatng
Copy link
Collaborator

kiatng commented Sep 2, 2022

I overlooked that it is possible to define your own filter:

$this->addColumn('type', [
'header' => Mage::helper('review')->__('Type'),
'type' => 'select',
'index' => 'type',
'filter' => 'adminhtml/review_grid_filter_type',
'renderer' => 'adminhtml/review_grid_renderer_type'
]);

And it returns int:

public function getCondition()
{
if ($this->getValue() == 1) {
return 1;
} elseif ($this->getValue() == 2) {
return 2;
} else {
return 3;
}
}
}

@sreichel
Copy link
Contributor

sreichel commented Sep 2, 2022

Maybe this could have been easier with correct phpdocs .... I'm working on this :)

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

Successfully merging this pull request may close these issues.

Possibility to search for null in backend grids
7 participants