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

ENH: Provide more control over elemental block indexing. #913

Merged
merged 1 commit into from
Feb 4, 2022

Conversation

GuySartorelli
Copy link
Member

Provides a new configuration variable to exclude specific elemental block classes from being indexed in search.
Also provides a new extension point for modifying exactly what gets indexed for each block.

@GuySartorelli GuySartorelli marked this pull request as draft July 8, 2021 23:21
@GuySartorelli
Copy link
Member Author

Not sure when I'll get a chance to update this to be in line with @michalkleiner's recommendations - setting to draft until then.

@GuySartorelli
Copy link
Member Author

Tests are failing, but I don't see how the changes I've made could have caused that.... can someone who understands the ElementSiteTreeFilterSearchTest please take a look?

@GuySartorelli GuySartorelli marked this pull request as ready for review September 20, 2021 04:10
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good, some requested changes

Could you also add some unit tests? Particularly for BaseElement::getContentForSearchIndex().

Let me know if not sure how to go about setting up tests for this and I'll point you in the right direction

src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Extensions/ElementalPageExtension.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Extensions/ElementalPageExtension.php Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Member Author

@emteknetnz I'll make the requested changes as time permits, hopefully in the next week. Please refer to the discussion on the PHPDoc and let me know on that one.

I note there is also a conflict now so I'll resolve that at the same time.

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Jan 31, 2022

@emteknetnz I've made the requested changes (though see discussion about phpdoc return type which affects the related typehint) except tests. I've also added some documentation, rebased off 4, and squashed commits.

Let me know if not sure how to go about setting up tests for this and I'll point you in the right direction

Yes please. I thought the tests would be relatively straight forward but I spent a good 15 minutes and then realised I'm not really sure what exactly I want to be testing... if you could give me a direction (even just some method signatures or comments to build off) that would be great.

@GuySartorelli
Copy link
Member Author

Failed tests are because I forgot Extension isn't inherently Configurable - I'll fix that at the same time as writing tests once I know what tests to write.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny linting thing and change return type to string. Cheers

src/Models/BaseElement.php Outdated Show resolved Hide resolved
@emteknetnz
Copy link
Member

emteknetnz commented Jan 31, 2022

@GuySartorelli This would be a good level of test coverage for the work you've done here:

Setup:

  • Create a couple of test elements of ElementContent (comes with HTML db field). $element->HTML = '<p>some HTML</p><p>etc</p>'; $element->write();
  • Create a test Extension and put it on ElementContent that implements the updateContentForSearchIndex hook.
  • Add elements to an ElementalArea + SiteTree

See an example here for how to add test extensions in a unit test

To change the config for the delimiter in the unit test, simply MyClass::config()->set('config_var', $value); - it will reset between tests so won't affect other tests (we'll I'm at least 99% sure on that :-)

Tests:
a) Test that changing search_index_element_delimiter delimits the output
b) Test that getContentForSearchIndex adds spaces to HTML
c) Test that an extension implementing updateContentForSearchIndex updates the content

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Feb 2, 2022

Tests failed - working on that now. I can't get the tests to run locally so there may be a few forced pushes while I get this sorted.

@GuySartorelli GuySartorelli force-pushed the patch-1 branch 4 times, most recently from 78443e4 to 5dffae3 Compare February 2, 2022 04:56
Provides a new configuration variable to exclude specific elemental block classes from being indexed in search.
Provides a new extension point for modifying exactly what gets indexed for each block.
Provides a new configuration variable to define the delimiter used
between blocks in the search index.

A single space delimiter is non-intrusive and will not require any changes within existing projects to avoid changes to the way content is indexed - but providing an option for configuring the delimiter can help avoid false-positive results in phrase queries.
@GuySartorelli
Copy link
Member Author

@emteknetnz I've added the requested tests and those are passing. There is one test that's failing, but I haven't touched that test and (from what I can tell) none of the changes I've made should be causing this to fail. Can you please take a look?

@emteknetnz
Copy link
Member

@GuySartorelli found the issue for the travis failure, it was an existing bug - have created a PR #957

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Travis failure is an existing bug that this PR highlighted, separate PR to fix it is here #957

@emteknetnz emteknetnz merged commit 87c3e36 into silverstripe:4 Feb 4, 2022
@GuySartorelli GuySartorelli deleted the patch-1 branch February 4, 2022 02:58
GuySartorelli added a commit to GuySartorelli/silverstripe-framework that referenced this pull request Feb 6, 2022
I figured I'd write this while these changes are fairly fresh in my mind, and save someone else the trouble.
Obviously if there's some procedure ormpolicy for writing release notes that I'm not following here feel please let me know and feel free to close this PR.

Refer to silverstripe/silverstripe-admin#1251 for preview PRs, silverstripe#10204 for the `GridFieldComponent` stuff, and silverstripe/silverstripe-elemental#913 for changes to elemental search indexing.

There are some extra new lines in here that I can't remove from the GitHub editor - but I expect there will be other requested changes so I'll clean that up at the same time.
menno-rdmkr pushed a commit to busting-bytes/silverstripe-elemental that referenced this pull request Dec 15, 2022
…e#913)

Provides a new configuration variable to exclude specific elemental block classes from being indexed in search.
Provides a new extension point for modifying exactly what gets indexed for each block.
Provides a new configuration variable to define the delimiter used
between blocks in the search index.

A single space delimiter is non-intrusive and will not require any changes within existing projects to avoid changes to the way content is indexed - but providing an option for configuring the delimiter can help avoid false-positive results in phrase queries.
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.

3 participants