OrchardCMS / Orchard

Orchard is a free, open source, community-focused Content Management System built on the ASP.NET MVC platform.
https://orchardproject.net
BSD 3-Clause "New" or "Revised" License
2.38k stars 1.12k forks source link

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

Closed BenedekFarkas closed 7 months ago

BenedekFarkas commented 7 months ago

Fixes #8225

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

BenedekFarkas commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

I confirm migration works and query execution seems good.

Awesome, thanks!

BenedekFarkas commented 7 months ago

This change is now in dev too!