Skip to content

8225: Adding a checkbox to StringFilterForm to control whether an empty value should cause the filter to be skipped#8781

Merged
BenedekFarkas merged 6 commits into1.10.xfrom
issue/8225
Apr 17, 2024
Merged

8225: Adding a checkbox to StringFilterForm to control whether an empty value should cause the filter to be skipped#8781
BenedekFarkas merged 6 commits into1.10.xfrom
issue/8225

Conversation

@BenedekFarkas
Copy link
Copy Markdown
Member

Fixes #8225

The changes from #8213 are also removed as that functionality is now covered, as well as that of #8226.

@BenedekFarkas
Copy link
Copy Markdown
Member Author

BenedekFarkas commented Apr 11, 2024

@MatteoPiovanelli-Laser @LorenzoFrediani-Laser please see the changes here. If you're already running the code in your PRs linked above, then you can update your Queries with a recipe for example - adapting the filter configurations is a simple search and replace.
Basically, if you're using ContainsAnyIfProvided or ContainsAllIfProvided, then you should change it to use ContainsAny or ContainsAll respectively and have the checkbox enabled.

@AndreaPiovanelli
Copy link
Copy Markdown
Contributor

@MatteoPiovanelli-Laser @LorenzoFrediani-Laser please see the changes here. If you're already running the code in your PRs linked above, then you can update your Queries with a recipe for example - adapting the filter configurations is a simple search and replace. Basically, if you're using ContainsAnyIfProvided or ContainsAllIfProvided, then you should change it to use ContainsAny or ContainsAll respectively and have the checkbox enabled.

I believe the safest way to ensure compatibility of the new StringFilterForm operators and to properly add the new boolean parameter where needed inside the FilterRecords is to add a migration step to Orchard.Projections, perhaps using Orchard.Forms.Services.FormParametersHelper to correct the form state.

@BenedekFarkas
Copy link
Copy Markdown
Member Author

BenedekFarkas commented Apr 12, 2024

I believe the safest way to ensure compatibility of the new StringFilterForm operators and to properly add the new boolean parameter where needed inside the FilterRecords is to add a migration step to Orchard.Projections, perhaps using Orchard.Forms.Services.FormParametersHelper to correct the form state.

I didn't find a proper way to manipulate the form, but we can still adjust the filter in any way to cover the earlier cases.
BTW are you already using ContainsAnyIfProvided in production? Honestly I'm not really keen on adding a migration step just for this, especially on 1.10.x after the yak shaving I had to do with merging 1.10.x into dev the last time due to conflicting migrations.

Also, this is still unreleased code, so I'm not that worried about breaking changes. But if you're already running this in production and you can't update filters easily with recipes, then I'm happy to work towards a solution that works for you too.

@AndreaPiovanelli
Copy link
Copy Markdown
Contributor

I believe the safest way to ensure compatibility of the new StringFilterForm operators and to properly add the new boolean parameter where needed inside the FilterRecords is to add a migration step to Orchard.Projections, perhaps using Orchard.Forms.Services.FormParametersHelper to correct the form state.

I didn't find a proper way to manipulate the form, but we can still adjust the filter in any way to cover the earlier cases. BTW are you already using ContainsAnyIfProvided in production? Honestly I'm not really keen on adding a migration step just for this, especially on 1.10.x after the yak shaving I had to do with merging 1.10.x into dev the last time due to conflicting migrations.

Also, this is still unreleased code, so I'm not that worried about breaking changes. But if you're already running this in production and you can't update filters easily with recipes, then I'm happy to work towards a solution that works for you too.

I believe we may use a Repository, and we want to edit the column "State", which is similar to:
<Form><Description></Description><Operator>ContainsAllIfProvided</Operator><Value>{Request.QueryString:q}</Value></Form>
These rows need to be updated, with the help of FormParametersHelper, adding the parameter "IgnoreIfEmptyValue" when needed.
The migration ensures every row is updated properly and in a multi-tenant, multi-instance with tens of tenants, using a recipe for each tenant is tricky.

If needed, I can implement the migration phase and give you a branch to cherry pick from or a gist.

@BenedekFarkas
Copy link
Copy Markdown
Member Author

That's all right and I knew how to migrate the data, I just wanted to avoid having it. :)
Please check the latest changes and let me know if it's working OK for you too!

@AndreaPiovanelli
Copy link
Copy Markdown
Contributor

That's all right and I knew how to migrate the data, I just wanted to avoid having it. :) Please check the latest changes and let me know if it's working OK for you too!

I confirm migration works and query execution seems good.

@BenedekFarkas
Copy link
Copy Markdown
Member Author

I confirm migration works and query execution seems good.

Awesome, thanks!

@BenedekFarkas BenedekFarkas merged commit 3a6810e into 1.10.x Apr 17, 2024
@BenedekFarkas
Copy link
Copy Markdown
Member Author

This change is now in dev too!

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.

Return all items by projection in ContainsAll if provided value is empty

3 participants