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

[GridBundle] Allow filtering by fields from associated objects #5501

Merged
merged 2 commits into from Jul 17, 2016
Merged

[GridBundle] Allow filtering by fields from associated objects #5501

merged 2 commits into from Jul 17, 2016

Conversation

aramalipoor
Copy link
Contributor

@aramalipoor aramalipoor commented Jul 12, 2016

Q A
Bug fix? yes
New feature? no
BC breaks? no
Related tickets mentioned in #5476
License MIT

I also had some issues when filter form was submitting with empty string value so I thought maybe do not restrict DataSource when filer data is empty?!

$this->queryBuilder->andWhere($this->getFieldName($field).' =< :'.$field)->setParameter($field, $value);
$this->setParameter($field, $value);

$this->queryBuilder->andWhere($this->getFieldName($field).' =< :'.$field);

Choose a reason for hiding this comment

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

This should be <=

@liverbool
Copy link
Contributor

@pjedrzejewski why this PR not merge? What do you think?

@@ -74,7 +74,7 @@ public function comparison($field, $operator, $value)
*/
public function equals($field, $value)
{
$this->queryBuilder->setParameter($field, $value);
$this->setParameter($field, $value);

return $this->queryBuilder->expr()->eq($this->getFieldName($field), ':'.$field);
Copy link
Contributor

@liverbool liverbool Jul 14, 2016

Choose a reason for hiding this comment

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

BUG: $field (':'.$field) still not replaced by _

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OOPS!! MY BAD!!

@pjedrzejewski
Copy link
Member

Build is failing, otherwise looks good! 👍

@aramalipoor
Copy link
Contributor Author

aramalipoor commented Jul 16, 2016

@pjedrzejewski The fail is about it filters data containing empty strings which seems legit, and now I don't know what to decide here :D ?

The problem is I do not want to apply any filter when form input of filter is not filled by user (e.g. filling only another field) because it will automatically only show data with empty strings! But on the other hand what to do if user really wants to filter empty strings?

Should I not support filtering for empty strings? Or to add another field (checkbox) as look for empty strings so that user can actually look for empty strings?

@pjedrzejewski
Copy link
Member

The problems is that string filter type: empty / non-empty has no value - so your if (empty... breaks the tests. I guess we should check if filter type is not these 2.

@aramalipoor
Copy link
Contributor Author

@pjedrzejewski Thanks for the feedback :) I think it's ready now

@pjedrzejewski pjedrzejewski merged commit 8b0a0d4 into Sylius:master Jul 17, 2016
@pjedrzejewski
Copy link
Member

Thanks Aram! Tests FTW!

@aramalipoor aramalipoor deleted the fix/grid-datasource branch July 18, 2016 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants