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

Add an option to allow the user to change the full-text search behavior in the Contents admin UI. #13919

Merged

Conversation

MikeAlhayek
Copy link
Member

Fix #13886

@jtkech
Copy link
Member

jtkech commented Jun 28, 2023

I did another review

LGTM but maybe worth to be able to also define the default operator, hmm as I remember we also needed to define a full quoted string because when searching for pete seeger the customer only want results containing the whole, no result like pete foo played music with albert seeger.

But I'm not against if we agree that it could be done in separate PRs.

@MikeAlhayek
Copy link
Member Author

I did another review

LGTM but maybe worth to be able to also define the default operator, hmm as I remember we also needed to define a full quoted string because when searching for pete seeger the customer only want results containing the whole, no result like pete foo played music with albert seeger.

But I'm not against if we agree that it could be done in separate PRs.

made some more changes and added double-quotes. Let me know if you see anything else.

@jtkech
Copy link
Member

jtkech commented Jun 28, 2023

Okay, let @sebastienros decide which options are worth to support, maybe also the default operation, and the Full Quoted String as a separate option with an higher precedence. Yes I think that if we support the Quoted String it should be an option.

Maybe the options would need to be able to return a full DefaultTermNode.

In my case I was checking if there is no explicit operator in use, maybe we don't need this, just take into account the current options, but not sure, would need to focus on it more.

@MikeAlhayek
Copy link
Member Author

@jtkech I added UseQuotationMarks option which is false by default so we don't change existing behavior. Currently, the user can achieve this by wrapping the string with double-quotes themself. This is better I think because the user has full control. The user should only turn on this option if they really need to always use double quote.

@jtkech
Copy link
Member

jtkech commented Jun 28, 2023

Currently, the user can achieve this by wrapping the string with double-quotes

Does it work? As I remember it was not working when I tried it, but not sure now

I don't like the UseQuotationMarks option name, maybe AsQuotedString

@MikeAlhayek
Copy link
Member Author

@jtkech I renamed UseQuotationMarks to UseExactMatch. I cleaned up the doc blocks and cleaned up the code. I also tested with and without UseExactMatch and it is working as expected. By default UseExactMatch is set to false so we do not change the default behavior of Orchard.

@MikeAlhayek
Copy link
Member Author

@jtkech let me know if you see anything need changes or approve it please.

@jtkech
Copy link
Member

jtkech commented Jun 29, 2023

Okay, I will look at it this night, I may do some last minor changes and then approve (or merge).

@jtkech
Copy link
Member

jtkech commented Jun 29, 2023

Okay, I did some last changes and approved, I let you review my changes

@MikeAlhayek MikeAlhayek merged commit 2e9dab1 into OrchardCMS:main Jun 29, 2023
@MikeAlhayek MikeAlhayek changed the title Add an option to allow the user to change the full-text search behavior in the admin UI. Add an option to allow the user to change the full-text search behavior in the Contents admin UI. Aug 11, 2023
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.

Add a way to search contents with fields other than just displayText
3 participants