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

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

Merged
merged 6 commits into from
Apr 17, 2024

Conversation

BenedekFarkas
Copy link
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
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
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
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
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
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
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
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
2 checks passed
@BenedekFarkas
Copy link
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