-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Dictionary: Add configurable value search functionality #21200
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
Dictionary: Add configurable value search functionality #21200
Conversation
|
Hi there @Nis-Knowit, thank you for this contribution! 👍 While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution. If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request. Thanks, from your friendly Umbraco GitHub bot 🤖 🙂 |
Added integration tests to verify search results.
AndyButland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me @Nis-Knowit, apart from one issue that I'll raise as a concern.
Firstly though I have pushed a small update to:
- Rename the configuration key to
EnableValueSearch- I felt we didn't need to repeatDictionarygiven we are in that configuration section, so the name could be a bit terser.- I noted you had already made a docs PR which is great, but if we go with this changed name, that'll need an update too.
- I added a couple of integration tests to verify the functionality.
The problem I found though in testing is that, when I have the feature enabled, if I search for a value that exists on one language, the other language is marked as not having a translation, even though it does.
E.g. in this case, I have both an English and Italian translation, but having search for the Italian one, the English one shows as missing:
So we would need to find a solution to that. I think it may have to be done with a sub-query.
|
@AndyButland I've added a fix to the query that makes sure it returns all languages with a value in the overview :) |
AndyButland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to work nicely @Nis-Knowit. I just pushed an update to use IOptionsMonitor and to extend the integration test to verify the fix to the issue I found.
Just curious how this update has affected the performance you provided a benchmark for when you first raised the PR? I'm assuming the sub-query will be slower than what you first had, but would like to understand by how much.
@AndyButland
|
AndyButland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds promising @Nis-Knowit, thanks. Then I see no reason not to merge this in, so I'll approve, and we'll have this available from 17.2.
Thanks for the docs PR too. I'll add some comments on there just to get that ready for when we make available the release candidate for 17.2.
Summary
This PR adds the ability to search dictionary items by their translation values in addition to keys.
The feature is opt-in via configuration and is disabled by default to preserve backward compatibility and performance.
Changes
1. New Configuration Setting
DictionarySettings.csEnableValueSearchpropertyfalseConstants-Configuration.csConfigDictionaryconstantUmbracoBuilder.Configuration.cs2. Repository Updates
DictionaryRepository.csIOptions<DictionarySettings>into constructorGetDictionaryItemDescendantsto support value search when enabledcmsDictionary.keyusingLIKE 'filter%'cmsLanguageText.valueusingLIKE '%filter%'3. Test Updates
DictionaryRepositoryTest.csDictionarySettingsparameter to test setupConfiguration
To enable dictionary value search, add the following to your
appsettings.json:Search Behavior
When
EnableValueSearch == false(Default)key LIKE 'searchterm%'(starts with)When
EnableValueSearch == truekey LIKE 'searchterm%'value LIKE '%searchterm%'Performance Testing
Tested with:
en-US,sq,da-DK,nb-NO,sv-SE,de-DE,fr-FR,es-ESResults:
Umbraco Docs link: umbraco/UmbracoDocs#7734