facets: provide CombinedTermsFacet to fix #798 [+] #549
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fenekku
force-pushed
the
798_incorrect_nested_aggregations_option_2
branch
2 times, most recently
from
e4b7d03 to
8a2d045
Compare
This was referenced Nov 29, 2023
fenekku
commented
Nov 30, 2023
| label=_('Resource types'), | ||
| value_labels=VocabularyL10NLabels(current_service) | ||
| ), | ||
| ) |
There was a problem hiding this comment.
just repeated docstring - cleaning up 🧹
| @@ -149,7 +144,7 @@ def _parse_values(self, filter_values): | |||
| .. code-block:: python | |||
|
|
|||
| { | |||
| 'publication': ['publication::book', 'publication::journal'], | |||
| 'publication': ['book', 'journal'], | |||
There was a problem hiding this comment.
This is actually what this function does.
| @@ -246,6 +239,211 @@ def get_labelled_values( | |||
| return ret_val | |||
|
|
|||
|
|
|||
| class CombinedTermsFacet(NestedTermsFacet): | |||
| """ | |||
There was a problem hiding this comment.
I think the best place to explain more complex things and reasoning behind approaches is in the code itself so I placed the explainer here.
Comment on lines
+328
to
+339
| def get_parents(self): | ||
| """Return parents. | ||
|
|
||
| We have to delay getting the parents since it may require an application | ||
| context. | ||
| """ | ||
| if not self._cached_parents: | ||
| if callable(self._parents): | ||
| self._cached_parents = self._parents() | ||
| else: | ||
| self._cached_parents = self._parents | ||
| return self._cached_parents |
There was a problem hiding this comment.
Tradeoff here was in favor of simplicity of "caching". The parents are computed once, so a restart of the application is needed if new subjects are installed. Maybe this can be documented somewhere, if so where?
| @@ -303,3 +248,100 @@ def test_facets_post_filtering(app, service, identity_simple, records): | |||
| # Test hits are filtered | |||
| assert 1 == len(res) | |||
| assert set(["001"]) == set([h["metadata"]["title"] for h in res]) | |||
|
|
|||
|
|
|||
| def test_combined_terms_facets(app, service, identity_simple, search_clear): | |||
There was a problem hiding this comment.
Test other combination of scheme+subject than in resource test to be thorough.
fenekku
force-pushed
the
798_incorrect_nested_aggregations_option_2
branch
from
8a2d045 to
6624851
Compare
fenekku
force-pushed
the
798_incorrect_nested_aggregations_option_2
branch
from
6624851 to
d389762
Compare
lnielsen
approved these changes
Mar 19, 2024
This approach doesn't use 'nested' fields, but instead relies on a field containing <parent><split char><child> to aggregate correctly.
fenekku
force-pushed
the
798_incorrect_nested_aggregations_option_2
branch
from
d389762 to
1d370c4
Compare
|
the mergening has started |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This approach doesn't use 'nested' fields, but instead relies on a
field containing
<parent><split char><child>to aggregate correctly.