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

Show in search check for files #92

Merged
merged 13 commits into from
May 2, 2024

Conversation

amolswnz
Copy link
Contributor

@amolswnz amolswnz commented Apr 16, 2024

Adding "Show in search" and "Last indexed in search" for DataObject and for Files

By default all files are set to "Show in search" as true https://github.com/silverstripe/silverstripe-assets/blob/2/src/File.php#L163

Allow users to easily change the indexing status of a file by checking or unchecking the "Show in search" checkbox.
When checked, the file will be included in the search index.
When unchecked, the file will be excluded from the search index.

Include a field displaying the date and time when the file was last indexed for search.

image

@amolswnz amolswnz marked this pull request as ready for review April 16, 2024 09:17
@@ -114,6 +114,17 @@ public function testShouldIndex(): void
$this->assertFalse($docTwo->shouldIndex());
// Document three should NOT be indexable (canView(): false)
$this->assertFalse($docThree->shouldIndex());

// Document should be NOT indexable (as it's value is changed to ShowInSearch: 0)

Choose a reason for hiding this comment

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

Suggested change
// Document should be NOT indexable (as it's value is changed to ShowInSearch: 0)
// Document should be NOT indexable (as its value is changed to ShowInSearch: 0)

@amolswnz amolswnz marked this pull request as draft April 17, 2024 04:26
Copy link
Collaborator

@blueo blueo left a comment

Choose a reason for hiding this comment

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

tested it out and works for file - I think this can be made a bit more generic though

src/Extensions/FileFormFactoryExtension.php Outdated Show resolved Hide resolved
src/Extensions/FileFormFactoryExtension.php Outdated Show resolved Hide resolved
src/Extensions/FileFormFactoryExtension.php Outdated Show resolved Hide resolved
tests/DataObject/DataObjectDocumentTest.php Outdated Show resolved Hide resolved
@amolswnz amolswnz force-pushed the feature/show-in-search-check branch from 75e2089 to 1f2d6ee Compare April 19, 2024 04:11
Copy link
Collaborator

@blueo blueo left a comment

Choose a reason for hiding this comment

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

really nice making it more generic. I've just got one tweak to that (adding config for excluded classes) and a fix for the test.

src/Extensions/SearchFormFactoryExtension.php Outdated Show resolved Hide resolved
tests/Fake/DataObjectFakeVersioned.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@blueo blueo left a comment

Choose a reason for hiding this comment

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

so close... just a few small things

tests/DataObject/DataObjectDocumentTest.php Show resolved Hide resolved
src/Extensions/SearchFormFactoryExtension.php Show resolved Hide resolved
Copy link
Collaborator

@blueo blueo left a comment

Choose a reason for hiding this comment

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

looking really good - just some linting fixes and it is good to go I think

@chrispenny chrispenny marked this pull request as ready for review April 29, 2024 19:45
@@ -74,8 +75,7 @@ public function updateCMSFields(FieldList $fields): void
_t(self::class . '.LastIndexed', 'Last indexed in search')
);

if ($fields->hasTabSet()) {
$fields->addFieldToTab('Root.Main', $showInSearchField);
if ($this->owner instanceof SiteTree) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you consider doing a check for a field of the same name too? eg if there is already a field don't add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the field already exists and it's added in getCMSFields to another tab, so finding the field is not an option here. Also if I remove this then for DataObject the Show in Search field is show at top of all it's fields which does not look good in ui perspective, hence this check is added.

@blueo blueo merged commit afbe384 into silverstripe:3 May 2, 2024
9 checks passed
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