Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #272 +/- ##
============================================
+ Coverage 73.37% 73.70% +0.33%
- Complexity 399 401 +2
============================================
Files 47 47
Lines 1292 1297 +5
============================================
+ Hits 948 956 +8
+ Misses 344 341 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| private function getPropertiesToIndex( ItemId $itemType, StatementList $statements ): array { | ||
| if ( $this->config->indexAllProperties ) { | ||
| // TODO: is this sufficient, or do we need to end up explicitly indexing properties not present as empty? | ||
| $properties = $statements->getPropertyIds(); |
There was a problem hiding this comment.
As per https://github.com/ProfessionalWiki/WikibaseFacetedSearch/pull/51/files#r1901363923, it seems we should explicitly tell elastic "there is no value" for all properties not present in the statement list, to avoid keeping data for removed statements in the index.
So we need to replace this "IDs of all properties present in the statement list" with "IDs of all properties in the wiki"
There was a problem hiding this comment.
That sounds correct. And also relates to defining all properties in #272 (comment)
3fb4652 to
c25f7be
Compare
| <p> | ||
| Indexing values for all properties is useful | ||
| when you want to be able to run structured queries for properties not shown in the UI. It is also | ||
| useful to avoid having to rebuild the search index whenever you add a new property to the UI. |
There was a problem hiding this comment.
add a new property to the UI.
I think this can be misunderstood. I get this refers to the faceted search UI, but I can see how this might look like it's referring to the general UI (including Wikibase).
Adding an existing property (already indexed) in the faceted search config works automatically. But when a new property is added on the wiki and the config, then the index must still be rebuilt.
The hook for defining the fields to be indexed is separate from the hook returning the data.
|
Does this code actually work as intended? (I did not check manually.) I just realised that I didn't mention My understanding of the code in this PR is that it will index all defined facet properties on all items, but if a property is not defined as a facet then it will not get indexed. I believe we need to return all the wiki properties there. (Unfortunately, an integration test for this is outstanding: #256) |
|
I did not test this yet and did not look at |
Fixes #271