Conversation
| return translated_result | ||
|
|
||
| def translate_agg_bucket(bucket, agg_result): | ||
| def translate_agg_bucket(facet_name, agg_result): |
There was a problem hiding this comment.
Do we really need to change the parameter name?
Some OeX providers may have their own customizations using the old argument name. I agree that the new name gives a better understanding of what this variable is, but unless it is urgent, I would not change it.
There was a problem hiding this comment.
Correct, I renamed it for clarity, to distinguish between facets and buckets. I changed it back as requested.
| """ | ||
| Integration tests for Elasticsearch + /course_discovery/ endpoint | ||
| """ | ||
| def setUp(self): |
There was a problem hiding this comment.
Please add a blank line between the doctring and the method, and please add docstrings to the tests, describing what is being tested.
| ElasticSearch tests. | ||
| """ | ||
| @patch("search.elastic.Elasticsearch") | ||
| def test_multivalue_aggregations_translated_correctly(self, mock_elasticsearch_class): |
| exclude_dictionary=None, | ||
| aggregation_terms=None, | ||
| exclude_ids=None, | ||
| sort_by=None, |
There was a problem hiding this comment.
Why are we adding this? We did not add sorting to MeilisearchEngine.
There was a problem hiding this comment.
Apparently, it got through during cherry-picking or rebasing. Removed.
|
I am adding here the screenshots of test runs to confirm the test pass locally: 
|
9acac1c to
344d511
Compare
f5f7711 to
d1ecf82
Compare
74739ab to
db57f57
Compare
|
Reopened in a PR covering both Meilisearch and Elasticsearch: openedx#213 |
2 participants
Implement multi-value faceted search for course discovery using Elasticsearch as a search engine.
Summary of changes: