Geta / geta-optimizely-categories

An alternative to Optimizely's default category functionality, where categories are instead stored as localizable IContent.
5 stars 10 forks source link

Geta.Optimizely.Categories.Find and trash search #21

Open zbrilhart opened 1 year ago

zbrilhart commented 1 year ago

When Geta.Optimizely.Categories.Find is installed it can modify the behavior of searches on trash in the CMS which causes missing or no results.

When that package is installed the JSON sent to search has the parent folder set to 59266, which is "For all sites". When the package is uninstalled the parent folder is set to ID 2, which is the trash folder.

See the searches which work and fail here.

trash_searches.zip

Pinkesh-Jain commented 1 year ago

Hello,

Is there any resolution to this?

GeekInTheNorth commented 1 year ago

I have several clients who use Geta Optimizely Categories who we have only just discovered as having this issue.

On a windows development machine, the issue cannot be replicated, but once it is on the Linux based DXP environments, the issue manifests.

Package Version
CMS Version 12.22.3
Search & Navigation Version 15.1.2
Environment Any DXP

Steps to Reproduce:

  1. Create a new page template and name it "test page"
  2. Publish the new page
  3. Move the page to the trash
  4. View the Trash
  5. Note that the page you created is in the trash
  6. In the search box on the trash screen, search for deleted items with the term of "test"

Expected Result: Contents of the Trash are filtered to those matching the search term "test" which should include this same page Actual Result: No results are shown in the trash

  1. In the main CMS editor interface, type the word "test" into the search bar on the content tree and note that you do actually get a result
  2. Click on the result and see you test page you created above, note that it's path is the recycle bin as expected.

Other observations:

GeekInTheNorth commented 1 year ago

@marisks following further conversation with Optimizely and setting of our local environment logging to be in debug mode, I can see that when selecting a Search Provider for the Trash, Optimizely is not specifying an Area.

Here is the URL called by the Trash Search:

https://localhost:5000/EPiServer/cms/Stores/contentstructure/?referenceId=2&query=searchdeletedcontent&allLanguages=true&isSearchable=true&matchProvider=true&queryText=mark

Note that matchProvider is set to true, but no Area has been provided.

The debug logging when this search is actioned is as below:

EPiServer.Shell.Search.SearchProvidersManager: Debug: Listing search providers within search area: 
EPiServer.Shell.Search.SearchProvidersManager: Debug: Loading search settings
EPiServer.Web.Internal.DefaultTemplateResolver: Debug: item: GeneralContentPage, Supported template 'GeneralContentPageController' with order '0', (tags='Default', category='Request'
EPiServer.DataAccess.Internal.ContentLoadDB: Debug: Loading content from database for content with id: 109
EPiServer.DataAccess.Internal.ContentLoadDB: Debug: Loading content from database for content with id: 107

The following LINQ statement is then executed to find the right Search Provider:

this.GetProviderPairs()
       .Where<SearchProviderPair>((Func<SearchProviderPair, bool>) (pp => pp.Setting.IsEnabled))
       .Where<SearchProviderPair>((Func<SearchProviderPair, bool>) (pp => !filterOnArea || pp.Provider.Area.StartsWith(searchArea, StringComparison.OrdinalIgnoreCase)))
       .OrderBy<SearchProviderPair, int>((Func<SearchProviderPair, int>) (pp => !pp.Provider.Area.StartsWith(searchArea, StringComparison.OrdinalIgnoreCase) ? 1 : 0))
       .ThenBy<SearchProviderPair, int>((Func<SearchProviderPair, int>) (pp => pp.Setting.SortIndex))
       .Select<SearchProviderPair, ISearchProvider>((Func<SearchProviderPair, ISearchProvider>) (pp => pp.Provider))
       .ToList<ISearchProvider>();

Since the Geta Categories Search Provider uses an Optimizely Base Class which has a fixed SortIndex, I suspect we are getting all search providers with the same SortIndex and since Area is null or string.Empty, both sort statements are not resulting in any discernible difference.

Something in how the original collection of ISearchProvider is set up is different between the Linux and Windows environments resulting in a different sort order.

I see two possible fixes here:

  1. Optimizely adds a second ThenBy which sorts on Area
  2. Geta overrides the Sort Order on their Search Provider so it is at least 101.

Both of which are potentially easy fixes.

Pinkesh-Jain commented 1 year ago

Thank you for digging into it. Not sure to proceed with either of the solutions. We'll try to reach out to the core team and see their point of view and if/how/when this can be remediated.

GeekInTheNorth commented 1 year ago

I suppose the real solution is that they should request the search provider for the Trash when searching on the Trash screen. So it feels like something that should be fixed in the core product.

marisks commented 1 year ago

@GeekInTheNorth I am not familiar with this library. @MattisOlsson or @valdisiljuconoks might know better what is the issue.

telleluy commented 1 year ago

There will be no fix made in Optimizely Search and Navigation package to change behaviour in external packages. I suggest going with option 2 described above

karlstal commented 1 year ago

I had a quick look at this issue and there is probably several things that can be done with the SearchProvidersManager and also how search in trash is done. I guess the philosophy is that we want all providers to search in trash.

Even if the CMS might not have the best of possible designs, I think that the major issue is in FindCategoriesSearchProvider in this project since the Query object is manipulated. This causes problems because it will be reused by all search providers. If you need to change this object you should probably clone it to a new Query object first. This feels like a more stable approach than moving it to the end of the list since there also seems to be logic for the last element in the list, and I don't think it is a good idea to mess with that.