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

Filter DataObjects on ShowInSearch #275

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Mar 26, 2020

This was originally branched off 3.6, have since rebased and retargeted to 3

Issue: #257

Inlet filtering - cms save
SearchUpdateProcessor.php

Inlet filtering - solr reindex
SolrReindexBase.php

Outlet filtering
SolrIndex.php

@emteknetnz emteknetnz force-pushed the pulls/3.6/show-in-search branch 2 times, most recently from 4e091e8 to 82288aa Compare March 26, 2020 03:38
Copy link
Member

@chillu chillu left a comment

Choose a reason for hiding this comment

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

Good work so far! Don't forget to reflect any changes in the docs as well. For example, https://www.cwp.govt.nz/developer-docs/en/2/features/solr_search/basic_usage/ mentions using Index->search() directly, without either mentioning canView or ShowInSearch there, so setting you up for failure. As a rule of thumb, on the search results side whenever it does canView it should also check ShowInSearch.

And we should probably add some clear docs on CWP around not using the raw SolrIndex for querying unless you really know what you're doing. CWP\Search\Extensions\SearchControllerExtension does some magic, and then CwpSearchEngine does more. We should make it clear that it's the preferred, supported way of searching, no matter how horrible it might be in terms of layering.

src/Search/Processors/SearchUpdateProcessor.php Outdated Show resolved Hide resolved
src/Search/Processors/SearchUpdateProcessor.php Outdated Show resolved Hide resolved
src/Solr/Reindex/Handlers/SolrReindexBase.php Outdated Show resolved Hide resolved
src/Solr/SolrIndex.php Outdated Show resolved Hide resolved
@emteknetnz emteknetnz changed the title Filter DataObjects on ShowInSearch WIP: Filter DataObjects on ShowInSearch Mar 29, 2020
Copy link
Member

@Cheddam Cheddam left a comment

Choose a reason for hiding this comment

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

Looking pretty solid! Agreed with @chillu on shifting one of the checks, one minor code suggestion, and have suggested some tweaks to the new documentation.

docs/en/03_configuration.md Outdated Show resolved Hide resolved
docs/en/03_configuration.md Outdated Show resolved Hide resolved
docs/en/03_configuration.md Outdated Show resolved Hide resolved
docs/en/03_configuration.md Outdated Show resolved Hide resolved
src/Solr/Reindex/Handlers/SolrReindexBase.php Outdated Show resolved Hide resolved
src/Solr/Reindex/Handlers/SolrReindexBase.php Outdated Show resolved Hide resolved
src/Solr/SolrIndex.php Outdated Show resolved Hide resolved
@Cheddam Cheddam linked an issue Apr 2, 2020 that may be closed by this pull request
23 tasks
dhensby
dhensby previously requested changes Apr 2, 2020
src/Solr/Reindex/Handlers/SolrReindexBase.php Outdated Show resolved Hide resolved
Copy link
Member

@Cheddam Cheddam left a comment

Choose a reason for hiding this comment

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

Haven't completed manual testing yet, will pick that up next, but looking pretty solid! I've suggested some further adjustments to the documentation, recommended we drop the commented placeholder canView() code, and asked for some clarity regarding the storage of the $indexableState.

docs/en/03_configuration.md Outdated Show resolved Hide resolved
src/Search/Extensions/IndexableExtension.php Outdated Show resolved Hide resolved
src/Search/Extensions/IndexableExtension.php Outdated Show resolved Hide resolved
tests/IndexableExtensionTest.php Outdated Show resolved Hide resolved
@Cheddam
Copy link
Member

Cheddam commented Apr 5, 2020

Sidenote - this needs to be rebased and retargeted to the 3 branch, as it introduces new API surface.

Copy link
Member

@chillu chillu left a comment

Choose a reason for hiding this comment

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

Good start, left a comment below

src/Search/Extensions/IndexableExtension.php Outdated Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/3.6/show-in-search branch from a73f893 to 1073c90 Compare April 6, 2020 06:17
@emteknetnz emteknetnz changed the base branch from 3.6 to 3 April 6, 2020 06:18
@emteknetnz
Copy link
Member Author

@Cheddam have rebased and retargeted to 3 (used to be 3.6) cos as you say, new API = minor release

chillu
chillu previously requested changes Apr 7, 2020
src/Solr/Reindex/Handlers/SolrReindexBase.php Outdated Show resolved Hide resolved
tests/SolrIndexTest.php Outdated Show resolved Hide resolved
@dhensby dhensby dismissed their stale review April 7, 2020 09:22

feedback addressed

@emteknetnz emteknetnz force-pushed the pulls/3.6/show-in-search branch 3 times, most recently from e7b16da to a2e341f Compare April 7, 2020 22:45
@Cheddam Cheddam changed the title WIP: Filter DataObjects on ShowInSearch Filter DataObjects on ShowInSearch Apr 7, 2020
@emteknetnz emteknetnz force-pushed the pulls/3.6/show-in-search branch from a2e341f to bfa2ede Compare April 8, 2020 21:04
@Cheddam Cheddam dismissed chillu’s stale review April 8, 2020 22:30

All feedback has been addressed.

@Cheddam Cheddam merged commit e56a369 into silverstripe:3 Apr 8, 2020
@Cheddam Cheddam deleted the pulls/3.6/show-in-search branch April 8, 2020 22:31
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.

Don't index records with ShowInSearch=false
4 participants