silverstripe / silverstripe-fulltextsearch

Adds external full text search engine support to Silverstripe
BSD 3-Clause "New" or "Revised" License
44 stars 83 forks source link

4.12 regression: Elemental block content not showing on search summary #334

Closed maxime-rainville closed 1 year ago

maxime-rainville commented 1 year ago

In our latest round of regression test, we have come across an issue where the search summary doesn't show the content of the block.

Acceptance

GuySartorelli commented 1 year ago

The template which renders search results is not compliant with the cucumber test description:

https://github.com/silverstripe/cwp-watea-theme/blob/95b5e739537c54cf40a645f2cda361313a2c28cb/templates/Layout/Page_results.ss#L52-L60

It clearly renders $Content.Summary instead of $Excerpt. $Excerpt is the excerpt returned by solr and will include the highlighted search term as per the cucumber test description:

https://github.com/silverstripe/silverstripe-fulltextsearch/blob/6d90b3ca353811571611789b6849b73e07c9e210/src/Solr/SolrIndex.php#L796-L814

I think we should update the watea theme template from $Content.Summary to <% if $Excerpt %>$Excerpt<% else %>$Content.Summary<% end_if %> or include a separate template with this code into https://github.com/silverstripeltd/cms-testing-recipes for testing purposes (we already have instructions in that recipe to do lots of solr config stuff so it's not really a stretch to do this as well)

maxime-rainville commented 1 year ago

Looks more likely to me that the cucumber studio test is what is wrong. Standard behaviour for search engine is to render the Meta Description ... not an excerpt.

My instinct would be to live it as is and update the cucumber studio test.

GuySartorelli commented 1 year ago

"standard" based on what? I would argue that confirming fulltextsearch can correctly fetch an excerpt from solr is important. It does it by default - the only thing stopping us from seeing it is the template. If we want to be regression testing the module we should be determining that the module's behaviour is correct, not the template's behaviour. But I'll defer to whatever you decide on this

emteknetnz commented 1 year ago

Investigating on different issue - https://github.com/silverstripeltd/product-issues/issues/632#issuecomment-1341830276 - that issue is in a private account so to parapharse, the Watea theme this regression was detected on is what controls what is rendered. I don't think Watea theme was ever intended to work with elemental - I'm basing that on elemental not being in the original CMS 3 CWP recipe basic module - https://github.com/silverstripe/cwp-recipe-basic/blob/1.10/composer.json

GuySartorelli commented 1 year ago

@maxime-rainville Please review and decide what action is needed. Options seem to be:

  1. update the test description to not test this functionality
  2. update the theme to support elemental
  3. add a step in https://github.com/silverstripeltd/cms-testing-recipes to add a template which supports this behaviour so that we can test it
maxime-rainville commented 1 year ago

Standard behaviour for search engine is to render the Meta Description ... not an excerpt.

Google says that's what it uses. https://developers.google.com/search/docs/appearance/snippet

Every web writing course I've ever seen says that meta description is what search engine will want to show in search results. The idea is that when you view those search results, the snippet should give you a good idea of what you will find on the page.

If search engines just randomly guessed a part of the page that looks like it contains your search term and chop it out of context, that would be pretty unlikely to give you a good idea of what the page is about. e.g.: Let's say you have an article with 10000 words. Your search term are sprinkled throughout the document. It's pretty unlikely that chopping out a random sentence out of context from the document and putting it in your search results will give you a good idea of what you will find when follow the link.

What should happen according to benevolent dictator Maxime

In any of those scenarios, if the snippet happens to contain our search term, we can highlight them.

Short term, I'm disciplined to spend a lot of time worrying about a legacy module. My instinct would be to keep the current behaviour and update the cucumber studio test to reflect that.

Maybe we spin up a separate card to ponder the wider meta description concern. Seems like Elemental should nudge you a bit more to provide a meta description since it's a lot less clear where that should come from than for a plain page.

GuySartorelli commented 1 year ago

Google as "a search engine" and solr as "a search engine" are wildly different concepts. We're talking about the latter, which is the search within the website itself, not some external search engine. Also the template currently renders $Content.Summary which is usually not the same as the page's meta description.

In my experience, search on a site typically gives you a highlighted excerpt showing how the page's content relates to your search query.

The idea is that when you view those search results, the snippet should give you a good idea of what you will find on the page.

If the page uses elemental blocks as its primary source of content, how will your recommended approach achieve that goal?

And in any case, what we're discussing should be about what gets tested - the fullltextsearch has functionality to provide an excerpt. That is not being tested. The current description of the test is explicitly meant to test this functionality and I think we should cater to that.

That's my piece said. Now that I've said it, we can do what Max said. :p

emteknetnz commented 1 year ago

Have updated cucumber studio test, closing issue