Fixes #36711 - Fix searching in settings #9818

girijaasoni · 2023-08-31T13:45:49Z

This PR addresses:

Allowing description search with fuzzy operator
Displaying all the relevant suggestions for autocomplete while searching with name including the suggestions for settings with Non Nil values.

theforeman-bot · 2023-08-31T13:45:51Z

Can one of the admins verify this patch?

theforeman-bot · 2023-08-31T13:45:52Z

Can one of the admins verify this patch?

theforeman-bot · 2023-08-31T13:45:52Z

Issues: #36711

theforeman-bot · 2023-08-31T13:45:52Z

Can one of the admins verify this patch?

stejskalleos · 2023-09-01T07:25:15Z

ok to test

nofaralfasi

I have a few observations to share regarding this PR:
As I discussed previously with @girijaasoni in a private conversation, there are a couple of issues that require attention:

The 'and'/'or' operations don't seem to be functioning correctly.
When searching for descriptions containing special characters, it triggers a 500 error code or generates an error message.

One more note, I'm not sure why we didn't opt for the ext_method option, which appears to address our concerns, as detailed in this link: GitHub PR #178. Given that the description search is already operational through the code found here: Setting Presenter, I'm uncertain about the need for additional code. Wouldn't it be more efficient to resolve the error by incorporating the ext_method into the settings model?

girijaasoni · 2023-09-14T11:30:56Z

I have a few observations to share regarding this PR: As I discussed previously with @girijaasoni in a private conversation, there are a couple of issues that require attention:

The 'and'/'or' operations don't seem to be functioning correctly.

When searching for descriptions containing special characters, it triggers a 500 error code or generates an error message.

One more note, I'm not sure why we didn't opt for the ext_method option, which appears to address our concerns, as detailed in this link: GitHub PR #178. Given that the description search is already operational through the code found here: Setting Presenter, I'm uncertain about the need for additional code. Wouldn't it be more efficient to resolve the error by incorporating the ext_method into the settings model?

Thanks @nofaralfasi

Searching in the search bar is implemented by the search_for function which gives us the results, it has always been operational. However, in this PR, The concern addressed is implementing the autocomplete functionality which needs some existing scoped search functions to be overridden. IMHO, The ext method is usually used in case of searching from a different table column or searching based on creating SQL relations as per this Reference and not when we are searching from a memory object.

stejskalleos

I have a different idea:

How about turning off suggestions completely, and do not show name and description in the form at all?
The only thing users could do would be to enter value into the search form, and that's it.
The search would then be equal to the name ~ $VALUE OR description ~ $VALUE

It's simple and it's going to cover 99% cases IMHO. It looks to me like it's unnecessarily complex for a simple search in a very simple table.

nofaralfasi · 2023-09-21T10:02:25Z

It's simple and it's going to cover 99% cases IMHO. It looks to me like it's unnecessarily complex for a simple search in a very simple table.

I agree, and we can always add this functionality if users request it.

girijaasoni · 2023-09-26T10:05:14Z

I have a different idea:

How about turning off suggestions completely, and do not show name and description in the form at all? The only thing users could do would be to enter value into the search form, and that's it. The search would then be equal to the name ~ $VALUE OR description ~ $VALUE

It's simple and it's going to cover 99% cases IMHO. It looks to me like it's unnecessarily complex for a simple search in a very simple table.

Turning off auto complete would break the search pattern from the other components. In my opinion, suggestions help the user the determine what exactly they want to look for. In this PR, autocomplete works and is only an issue when there's the use of special characters which is fixable(I will update it soon). I think removing the name and description search would be a great hack if and when it's not implementable (which is not the case here) and might require additional suggestions and votes.

stejskalleos · 2023-10-02T07:36:18Z

Turning off auto complete would break the search pattern from the other components.

Is it an issue? This fix looks like overkill to me.

ofedoren · 2023-10-02T08:23:25Z

app/controllers/api/v2/settings_controller.rb

@@ -22,7 +22,6 @@ class SettingsController < V2::BaseController

      api :GET, "/settings/", N_("List all settings")
      param_group :search_and_pagination, ::Api::V2::BaseController
-      add_scoped_search_description_for(Setting)


Please, don't remove it completely, it's for documenting what can be used for searching via API/hammer.

ares · 2023-10-10T08:25:50Z

A few notes from the testing:

this makes searching by description possible again, with the ~ operator. It gives auto completion list though, which I would not expect as it shows complete descriptions.
for the name it does not work at all, the auto completion suggest the names that user sees in the table (good), these can have spacebars but the value is not wrapped in quotes (bad), even if I add quotes it does not work (bad), because the search actually responds on the setting identifier. So e.g. name = "Foreman URL" does not work while name = foreman_url works fine.
searching on name also auto suggests only ~, while other operators like = works fine

ares · 2023-10-16T15:16:22Z

To illustrate I recorded few videos.

description.webm

description shows auto completion, that feels overwhelming and does not add quotes around the multiword term

name_exact.webm

name with typing equal sign provides full name sugestions that don't work at all, even with manually added quotes, as it turns out, it searches by name, e.g. Foreman URL vs foreman_url

name_fuzzy.webm

fuzzy searching on name seems to work (not sure why = is not autosuggested), but again, does not add quotes, so "Foreman URL" will give you all settings with either Foreman or URL in their name. With added quotes, it searches correctly.

Hope this helps. Again to be clear, searching in full name is preferred over the name. I would love to search for Foreman URL instead of foreman_url, provided it works.

girijaasoni · 2023-10-17T12:55:19Z

To illustrate I recorded few videos.

description.webm
description shows auto completion, that feels overwhelming and does not add quotes around the multiword term

name_exact.webm
name with typing equal sign provides full name sugestions that don't work at all, even with manually added quotes, as it turns out, it searches by name, e.g. Foreman URL vs foreman_url

name_fuzzy.webm
fuzzy searching on name seems to work (not sure why = is not autosuggested), but again, does not add quotes, so "Foreman URL" will give you all settings with either Foreman or URL in their name. With added quotes, it searches correctly.

Hope this helps. Again to be clear, searching in full name is preferred over the name. I would love to search for Foreman URL instead of foreman_url, provided it works.

Thanks a lot @ares
These videos were really helpful

In this push I have addressed these few things:

Ditched autocomplete for description fuzzy operator as they were very long
name = "Foreman URL" or name = Foreman URL wasn't working as we didn't define searching with full name in our matches_search_query method which was unrelated to scoped search but since it was a minor fix so I added it in this PR, Now we can search for name = "Foreman URL"
Wrapped the name suggestions with quotes for multiple words

ares · 2023-11-08T14:24:21Z

From user perspective, this works now as expected. There's one issue, if user enters something that's not supported, e.g. id =`, then they see Error: Request failed with status code 500.

Previously it was showing better error message.

This should be fixed I guess

ShimShtein · 2023-11-16T13:23:50Z

app/services/setting_registry.rb

+  scoped_search :on => :id, :complete_enabled => false, :only_explicit => true, :validator => ScopedSearch::Validators::INTEGER
+  scoped_search on: :name, complete_value: :true, operators: ['~', "="]
+  scoped_search on: :description, complete_value: :true, operators: ['~']


Those definitions should be moved to setting model, since the scoped search does not use the SettingRegistry class for search definition.

we have the definitions in the setting model as well for hammer purpose, but we need it in the registry as well because we need to pass the scoped search definition in the auto_complete method (which we are overriding)

That's not a problem, just pass the definition from the Setting model:

SettingCompleter.auto_complete(self, Setting.scoped_search_definition, query, opts)

Otherwise we are creating duplication of search definitions, and it will be hard to track down which definition is used.

That was a great catch @ShimShtein
updated it :)

ShimShtein · 2023-11-16T15:43:06Z

app/services/setting_registry.rb

+      elsif field.field == :description
+        results = []
+      else
+        raise ::Foreman::Exception.new N_('Unsupported completion, only name and description are supported')


We have to send here a ScopedSearch::QueryNotSupported error instead of a general Exception, so the error would be handled correctly by the controller.

THanks @ShimShtein
that's doable

girijaasoni · 2023-11-22T18:13:12Z

From user perspective, this works now as expected. There's one issue, if user enters something that's not supported, e.g. id =`, then they see Error: Request failed with status code 500.

Previously it was showing better error message.

This should be fixed I guess

Thanks @ares
Fixed it in this commit
@stejskalleos , I also cutshort the complete_value method (the method which was causing the overkill) as much as possible

ShimShtein · 2023-11-23T12:19:00Z

app/services/setting_registry.rb

+    def complete_value_from_db(field, val)
+      count = 20
+      if field.field == :name
+        results = @registry.map { |set| ((set.full_name =~ /\s/) ? "\"#{set.full_name.gsub('"', '\"')}\"" : set.full_name) if set.name.include?(val) || set.full_name&.include?(val) }


This will return empty values when the condition is not met. e.g:
For settings => ['aaa', 'abc', 'bbb'] and val => 'b' you will get:
[nil, 'abc', 'bbb'].
We should use filter_map instead:

Suggested change

results = @registry.map { |set| ((set.full_name =~ /\s/) ? "\"#{set.full_name.gsub('"', '\"')}\"" : set.full_name) if set.name.include?(val) || set.full_name&.include?(val) }

results = @registry.filter_map { |set| ((set.full_name =~ /\s/) ? "\"#{set.full_name.gsub('"', '\"')}\"" : set.full_name) if set.name.include?(val) || set.full_name&.include?(val) }

That makes much more sense, thanks!

ShimShtein

LGTM, but I don't have the latest Foreman to test. Can someone take this one to a quick run?

stejskalleos

Tested:

Default search
name ~ & name =
description

Looks like it's working, let's get this in!

stejskalleos · 2023-11-27T08:47:28Z

Thanks @girijaasoni, @ezr-ondrej, @ShimShtein

theforeman-bot added the Not yet reviewed label Aug 31, 2023

girijaasoni force-pushed the 36711 branch 2 times, most recently from 02d2040 to 3632477 Compare August 31, 2023 14:01

girijaasoni changed the title ~~Fixes #36711 - Add search by name and description in settings~~ Fixes #36711 - Add search by description in settings Aug 31, 2023

stejskalleos self-assigned this Sep 1, 2023

stejskalleos self-requested a review September 1, 2023 07:25

nofaralfasi requested changes Sep 10, 2023

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Sep 10, 2023

stejskalleos reviewed Sep 21, 2023

View reviewed changes

ofedoren reviewed Oct 2, 2023

View reviewed changes

girijaasoni force-pushed the 36711 branch from 3632477 to f700e3c Compare October 17, 2023 11:29

pr-processor bot added Needs re-review and removed Waiting on contributor labels Oct 17, 2023

girijaasoni force-pushed the 36711 branch 2 times, most recently from 9b3a76b to 7309fad Compare October 17, 2023 12:51

girijaasoni requested review from stejskalleos and ofedoren October 17, 2023 12:57

ShimShtein requested changes Nov 16, 2023

View reviewed changes

pr-processor bot added Waiting on contributor and removed Needs re-review labels Nov 16, 2023

girijaasoni force-pushed the 36711 branch from 7309fad to b864f49 Compare November 22, 2023 18:11

pr-processor bot added Needs re-review and removed Waiting on contributor labels Nov 22, 2023

girijaasoni changed the title ~~Fixes #36711 - Add search by description in settings~~ Fixes #36711 - Fix searching in settings Nov 22, 2023

girijaasoni requested a review from ShimShtein November 22, 2023 18:13

girijaasoni force-pushed the 36711 branch from b864f49 to 989e1b4 Compare November 22, 2023 18:31

ShimShtein reviewed Nov 23, 2023

View reviewed changes

Fixes #36711 - Fix searching in settings

6f09591

girijaasoni force-pushed the 36711 branch from 989e1b4 to 6f09591 Compare November 24, 2023 08:44

girijaasoni requested a review from ShimShtein November 24, 2023 08:45

ShimShtein approved these changes Nov 26, 2023

View reviewed changes

pr-processor bot removed the Needs re-review label Nov 26, 2023

stejskalleos approved these changes Nov 27, 2023

View reviewed changes

stejskalleos merged commit 2c4139e into theforeman:develop Nov 27, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #36711 - Fix searching in settings #9818

Fixes #36711 - Fix searching in settings #9818

girijaasoni commented Aug 31, 2023 •

edited

Loading

theforeman-bot commented Aug 31, 2023

theforeman-bot commented Aug 31, 2023

theforeman-bot commented Aug 31, 2023

theforeman-bot commented Aug 31, 2023

stejskalleos commented Sep 1, 2023

nofaralfasi left a comment

girijaasoni commented Sep 14, 2023

stejskalleos left a comment

nofaralfasi commented Sep 21, 2023

girijaasoni commented Sep 26, 2023 •

edited

Loading

stejskalleos commented Oct 2, 2023

ofedoren Oct 2, 2023

ares commented Oct 10, 2023

ares commented Oct 16, 2023 •

edited

Loading

girijaasoni commented Oct 17, 2023 •

edited

Loading

ares commented Nov 8, 2023

ShimShtein Nov 16, 2023

girijaasoni Nov 22, 2023

ShimShtein Nov 22, 2023

girijaasoni Nov 22, 2023

ShimShtein Nov 16, 2023

girijaasoni Nov 22, 2023 •

edited

Loading

girijaasoni commented Nov 22, 2023 •

edited

Loading

ShimShtein Nov 23, 2023

girijaasoni Nov 24, 2023

ShimShtein left a comment

stejskalleos left a comment

stejskalleos commented Nov 27, 2023

	results = @registry.map { \|set\| ((set.full_name =~ /\s/) ? "\"#{set.full_name.gsub('"', '\"')}\"" : set.full_name) if set.name.include?(val) \|\| set.full_name&.include?(val) }
	results = @registry.filter_map { \|set\| ((set.full_name =~ /\s/) ? "\"#{set.full_name.gsub('"', '\"')}\"" : set.full_name) if set.name.include?(val) \|\| set.full_name&.include?(val) }

Fixes #36711 - Fix searching in settings #9818

Fixes #36711 - Fix searching in settings #9818

Conversation

girijaasoni commented Aug 31, 2023 • edited Loading

theforeman-bot commented Aug 31, 2023

theforeman-bot commented Aug 31, 2023

theforeman-bot commented Aug 31, 2023

theforeman-bot commented Aug 31, 2023

stejskalleos commented Sep 1, 2023

nofaralfasi left a comment

Choose a reason for hiding this comment

girijaasoni commented Sep 14, 2023

stejskalleos left a comment

Choose a reason for hiding this comment

nofaralfasi commented Sep 21, 2023

girijaasoni commented Sep 26, 2023 • edited Loading

stejskalleos commented Oct 2, 2023

Choose a reason for hiding this comment

ares commented Oct 10, 2023

ares commented Oct 16, 2023 • edited Loading

girijaasoni commented Oct 17, 2023 • edited Loading

ares commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girijaasoni Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

girijaasoni commented Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShimShtein left a comment

Choose a reason for hiding this comment

stejskalleos left a comment

Choose a reason for hiding this comment

stejskalleos commented Nov 27, 2023

girijaasoni commented Aug 31, 2023 •

edited

Loading

girijaasoni commented Sep 26, 2023 •

edited

Loading

ares commented Oct 16, 2023 •

edited

Loading

girijaasoni commented Oct 17, 2023 •

edited

Loading

girijaasoni Nov 22, 2023 •

edited

Loading

girijaasoni commented Nov 22, 2023 •

edited

Loading