diff --git a/search/tests/factories.py b/search/tests/factories.py new file mode 100644 index 00000000..55de022e --- /dev/null +++ b/search/tests/factories.py @@ -0,0 +1,57 @@ +""" Classes to populate test data """ +import copy +from datetime import datetime + + +class DemoCourse: + """ Class for dispensing demo courses """ + DEMO_COURSE_ID = "edX/DemoX/Demo_Course" + DEMO_COURSE = { + "start": datetime(2014, 2, 1), + "number": "DemoX", + "content": { + "short_description": "Short description", + "overview": "Long overview page", + "display_name": "edX Demonstration Course", + "number": "DemoX" + }, + "course": "edX/DemoX/Demo_Course", + "image_url": "/c4x/edX/DemoX/asset/images_course_image.jpg", + "effort": "5:30", + "id": DEMO_COURSE_ID, + "enrollment_start": datetime(2014, 1, 1), + } + + demo_course_count = 0 + + @classmethod + def get(cls, update_dict=None, remove_fields=None): + """ get a new demo course """ + cls.demo_course_count += 1 + course_copy = copy.deepcopy(cls.DEMO_COURSE) + if update_dict: + if "content" in update_dict: + course_copy["content"].update(update_dict["content"]) + del update_dict["content"] + course_copy.update(update_dict) + course_copy.update({"id": "{}_{}".format(course_copy["id"], cls.demo_course_count)}) + if remove_fields: + for remove_field in remove_fields: + if remove_field in course_copy: # pragma: no cover + del course_copy[remove_field] + return course_copy + + @classmethod + def reset_count(cls): + """ go back to zero """ + cls.demo_course_count = 0 + + @staticmethod + def index(searcher, course_info): + """ Adds course info dictionary to the index """ + searcher.index(sources=course_info) + + @classmethod + def get_and_index(cls, searcher, update_dict=None, remove_fields=None): + """ Adds course info dictionary to the index """ + cls.index(searcher, [cls.get(update_dict, remove_fields)]) diff --git a/search/tests/test_course_discovery_views.py b/search/tests/test_api_mock_engine.py similarity index 91% rename from search/tests/test_course_discovery_views.py rename to search/tests/test_api_mock_engine.py index f01a442a..59824db1 100644 --- a/search/tests/test_course_discovery_views.py +++ b/search/tests/test_api_mock_engine.py @@ -1,11 +1,10 @@ """ High-level view tests""" - from django.test.utils import override_settings from search.tests.tests import TEST_INDEX_NAME -from search.tests.utils import post_discovery_request +from search.tests.utils import post_discovery_request, setup_democourse +from search.tests.factories import DemoCourse from .test_views import MockSearchUrlTest -from .test_course_discovery import DemoCourse @override_settings(ELASTIC_FIELD_MAPPINGS={ @@ -23,16 +22,7 @@ class DiscoveryUrlTest(MockSearchUrlTest): def setUp(self): super().setUp() - DemoCourse.reset_count() - DemoCourse.get_and_index( - self.searcher, {"org": "OrgA", "content": {"short_description": "Find this one with the right parameter"}} - ) - DemoCourse.get_and_index( - self.searcher, {"org": "OrgB", "content": {"short_description": "Find this one with another parameter"}} - ) - DemoCourse.get_and_index( - self.searcher, {"content": {"short_description": "Find this one somehow"}} - ) + setup_democourse(self.searcher) def test_search_from_url(self): """ test searching using the url """ diff --git a/search/tests/test_api_multi_value.py b/search/tests/test_api_multi_value.py new file mode 100644 index 00000000..fdd3e662 --- /dev/null +++ b/search/tests/test_api_multi_value.py @@ -0,0 +1,155 @@ +""" High-level view tests""" +import uuid +import logging +import ddt + +from django.test import TestCase +from django.test.utils import override_settings +from django.urls import reverse + +from search.search_engine_base import SearchEngine +from search.tests.utils import post_discovery_request, setup_meilisearch, setup_elasticsearch, setup_democourse + + +index_name = f"test_index_{uuid.uuid4().hex}" +logger = logging.getLogger(__name__) + + +@ddt.ddt +@override_settings(COURSEWARE_CONTENT_INDEX_NAME=index_name, COURSEWARE_INFO_INDEX_NAME=index_name) +class CourseListSearchMultiValueTest(TestCase): + """ + Multi-value tests (/course_list_search/) for both Meilisearch and Elasticsearch engines. + """ + url = reverse("course_list_search") + searcher = ... + wait = ... + + def _init_engine(self, config): + """Helper method to initialize the search engine""" + from django.conf import settings # pylint: disable=import-outside-toplevel + settings.SEARCH_ENGINE = config["search_engine"] + self.searcher = SearchEngine.get_search_engine(settings.COURSEWARE_INFO_INDEX_NAME) + setup_democourse(self.searcher) + self.wait = config["wait"] + self.wait() + + def _post(self, params): + """Helper method to send a post request""" + return post_discovery_request(params, address=self.url) + + @ddt.data(("meili", setup_meilisearch(index_name, logger)), ("es", setup_elasticsearch(index_name))) + @ddt.unpack + def test_search_string(self, label, config): # pylint: disable=unused-argument + """Tests that keyword search returns correct number of matching documents.""" + self._init_engine(config) + + code, results = self._post({}) + self.assertEqual(code, 200) + self.assertEqual(results["total"], 3) + + code, results = self._post({"search_string": "right"}) + self.assertEqual(code, 200) + self.assertEqual(results["total"], 1) + + code, results = self._post({"search_string": "parameter"}) + self.assertEqual(code, 200) + self.assertEqual(results["total"], 2) + + @ddt.data(("meili", setup_meilisearch(index_name, logger)), ("es", setup_elasticsearch(index_name))) + @ddt.unpack + def test_org_filter(self, label, config): # pylint: disable=unused-argument + """Tests filtering results by the 'org' facet.""" + self._init_engine(config) + + code, results = self._post({"org": "OrgA"}) + self.assertEqual(code, 200) + self.assertEqual(results["total"], 1) + self.assertEqual(results["results"][0]["data"]["org"], "OrgA") + + code, results = self._post({"org": "OrgB"}) + self.assertEqual(code, 200) + self.assertEqual(results["total"], 1) + self.assertEqual(results["results"][0]["data"]["org"], "OrgB") + + @ddt.data(("meili", setup_meilisearch(index_name, logger)), ("es", setup_elasticsearch(index_name))) + @ddt.unpack + def test_search_with_pagination(self, label, config): # pylint: disable=unused-argument + """Tests that pagination limits and offsets results correctly.""" + self._init_engine(config) + + code, results = self._post({"page_size": 2}) + self.assertEqual(code, 200) + self.assertEqual(len(results["results"]), 2) + + code, results = self._post({"page_size": 2, "page_index": 1}) + self.assertEqual(code, 200) + self.assertEqual(len(results["results"]), 1) + + @ddt.data(("meili", setup_meilisearch(index_name, logger)), ("es", setup_elasticsearch(index_name))) + @ddt.unpack + def test_bad_search_string(self, label, config): # pylint: disable=unused-argument + """Tests that non-matching search terms return no results.""" + self._init_engine(config) + + code, results = self._post({"search_string": "doesnotexist123"}) + self.assertEqual(code, 200) + self.assertEqual(results["total"], 0) + + @ddt.data(("meili", setup_meilisearch(index_name, logger)), ("es", setup_elasticsearch(index_name))) + @ddt.unpack + def test_no_filters_returns_all_aggregations(self, label, config): # pylint: disable=unused-argument + """Tests that full facet counts are returned when no filters are applied.""" + self._init_engine(config) + + code, results = self._post({}) # pylint: disable=unused-variable + aggs = results.get("aggs", {}) + self.assertIn("org", aggs) + self.assertIn("language", aggs) + self.assertEqual(aggs["org"]["terms"]["OrgA"], 1) + self.assertEqual(aggs["org"]["terms"]["OrgB"], 1) + self.assertEqual(aggs["org"]["terms"]["OrgC"], 1) + self.assertEqual(aggs["language"]["terms"]["en"], 2) + self.assertEqual(aggs["language"]["terms"]["fr"], 1) + + @ddt.data(("meili", setup_meilisearch(index_name, logger)), ("es", setup_elasticsearch(index_name))) + @ddt.unpack + def test_single_value_filter_keeps_full_facet(self, label, config): # pylint: disable=unused-argument + """Tests that single-value filters preserve all facet options in aggregations.""" + self._init_engine(config) + + _, results = self._post({"language": ["en"]}) + aggs = results.get("aggs", {}) + self.assertIn("fr", aggs["language"]["terms"]) + + @ddt.data(("meili", setup_meilisearch(index_name, logger)), ("es", setup_elasticsearch(index_name))) + @ddt.unpack + def test_multi_value_filter_keeps_full_facet(self, label, config): # pylint: disable=unused-argument + """Tests that multi-value filters preserve all facet options in aggregations.""" + self._init_engine(config) + + code, results = self._post({"language": ["en", "fr"]}) + self.assertEqual(code, 200) + self.assertEqual(results["total"], 3) + + aggs = results.get("aggs", {}) + self.assertIn("language", aggs) + self.assertIn("en", aggs["language"]["terms"]) + self.assertIn("fr", aggs["language"]["terms"]) + self.assertEqual(aggs["language"]["terms"]["en"], 2) + self.assertEqual(aggs["language"]["terms"]["fr"], 1) + + @ddt.data(("meili", setup_meilisearch(index_name, logger)), ("es", setup_elasticsearch(index_name))) + @ddt.unpack + def test_combined_facet_filter_aggregated_correctly(self, label, config): # pylint: disable=unused-argument + """Tests that combining multiple facet filters returns correct aggregations.""" + self._init_engine(config) + + code, results = self._post({"language": ["en"], "org": ["OrgA", "OrgC"]}) + self.assertEqual(code, 200) + self.assertEqual(results["total"], 2) + + aggs = results.get("aggs", {}) + self.assertIn("org", aggs) + self.assertIn("OrgA", aggs["org"]["terms"]) + self.assertIn("OrgC", aggs["org"]["terms"]) diff --git a/search/tests/test_api_single_value.py b/search/tests/test_api_single_value.py new file mode 100644 index 00000000..c33d2f22 --- /dev/null +++ b/search/tests/test_api_single_value.py @@ -0,0 +1,127 @@ +""" High-level view tests""" +import uuid +import logging +import ddt + +from django.test import TestCase +from django.test.utils import override_settings +from django.urls import reverse + +from search.search_engine_base import SearchEngine +from search.tests.utils import post_discovery_request, setup_meilisearch, setup_elasticsearch, setup_democourse + + +index_name = f"test_index_{uuid.uuid4().hex}" +logger = logging.getLogger(__name__) + + +@ddt.ddt +@override_settings(COURSEWARE_CONTENT_INDEX_NAME=index_name, COURSEWARE_INFO_INDEX_NAME=index_name) +class CourseListSearchSingleValueTest(TestCase): + """ + Single-value tests (/course_discovery/) for both Meilisearch and Elasticsearch engines. + """ + + url = reverse("course_discovery") + searcher = ... + wait = ... + + def _init_engine(self, config): + """Helper method to initialize the search engine""" + from django.conf import settings # pylint: disable=import-outside-toplevel + settings.SEARCH_ENGINE = config["search_engine"] + self.searcher = SearchEngine.get_search_engine(settings.COURSEWARE_INFO_INDEX_NAME) + setup_democourse(self.searcher) + self.wait = config["wait"] + self.wait() + + def _post(self, params): + """Helper method to send a post request""" + return post_discovery_request(params, address=self.url) + + @ddt.data(("meili", setup_meilisearch(index_name, logger)), ("es", setup_elasticsearch(index_name))) + @ddt.unpack + def test_search_string(self, label, config): # pylint: disable=unused-argument + """Tests that keyword search returns correct number of matching documents.""" + self._init_engine(config) + + code, results = self._post({}) + self.assertEqual(code, 200) + self.assertEqual(results["total"], 3) + + code, results = self._post({"search_string": "right"}) + self.assertEqual(results["total"], 1) + + code, results = self._post({"search_string": "parameter"}) + self.assertEqual(code, 200) + self.assertEqual(results["total"], 2) + + @ddt.data(("meili", setup_meilisearch(index_name, logger)), ("es", setup_elasticsearch(index_name))) + @ddt.unpack + def test_org_filter(self, label, config): # pylint: disable=unused-argument + """Tests filtering results by the 'org' facet.""" + self._init_engine(config) + + code, results = self._post({"org": "OrgA"}) # pylint: disable=unused-variable + self.assertEqual(results["total"], 1) + + code, results = self._post({"org": "OrgB"}) + self.assertEqual(results["total"], 1) + + @ddt.data(("meili", setup_meilisearch(index_name, logger)), ("es", setup_elasticsearch(index_name))) + @ddt.unpack + def test_search_with_pagination(self, label, config): # pylint: disable=unused-argument + """Tests that pagination limits and offsets results correctly.""" + self._init_engine(config) + + code, results = self._post({"page_size": 2}) # pylint: disable=unused-variable + self.assertEqual(len(results["results"]), 2) + + code, results = self._post({"page_size": 2, "page_index": 1}) + self.assertEqual(len(results["results"]), 1) + + @ddt.data(("meili", setup_meilisearch(index_name, logger)), ("es", setup_elasticsearch(index_name))) + @ddt.unpack + def test_bad_search_string(self, label, config): # pylint: disable=unused-argument + """Tests that non-matching search terms return no results.""" + self._init_engine(config) + + code, results = self._post({"search_string": "doesnotexist123"}) # pylint: disable=unused-variable + self.assertEqual(results["total"], 0) + + @ddt.data(("meili", setup_meilisearch(index_name, logger)), ("es", setup_elasticsearch(index_name))) + @ddt.unpack + def test_aggregations_basic(self, label, config): # pylint: disable=unused-argument + """Tests that facet aggregations include all indexed orgs.""" + self._init_engine(config) + + code, results = self._post({}) + self.assertEqual(code, 200) + aggs = results.get("aggs", {}) + + self.assertIn("org", aggs) + self.assertEqual(aggs["org"]["terms"].get("OrgA", 0), 1) + self.assertEqual(aggs["org"]["terms"].get("OrgB", 0), 1) + + @ddt.data(("meili", setup_meilisearch(index_name, logger)), ("es", setup_elasticsearch(index_name))) + @ddt.unpack + def test_aggregations_filtered_down(self, label, config): # pylint: disable=unused-argument + """Tests that aggregations reflect active filters correctly.""" + self._init_engine(config) + + code, results = self._post({"org": "OrgA"}) # pylint: disable=unused-variable + aggs = results.get("aggs", {}) + self.assertIn("org", aggs) + self.assertEqual(aggs["org"]["terms"].get("OrgA", 0), 1) + self.assertNotIn("OrgB", aggs["org"]["terms"]) + + @ddt.data(("meili", setup_meilisearch(index_name, logger)), ("es", setup_elasticsearch(index_name))) + @ddt.unpack + def test_aggregations_empty_search(self, label, config): # pylint: disable=unused-argument + """Tests that aggregations are returned even if there are no matches.""" + self._init_engine(config) + code, results = self._post({"org": "DoesNotExist"}) + self.assertEqual(code, 200) + aggs = results.get("aggs", {}) + self.assertIn("org", aggs) + self.assertEqual(aggs["org"]["terms"], {}) diff --git a/search/tests/test_course_discovery.py b/search/tests/test_course_discovery.py index 22705086..719972da 100644 --- a/search/tests/test_course_discovery.py +++ b/search/tests/test_course_discovery.py @@ -3,75 +3,26 @@ # error, but they do get used when included as part of the override_settings """ Tests for search functionalty """ -import copy import time from datetime import datetime +import logging import ddt from django.core.cache import cache from django.test import TestCase from django.test.utils import override_settings from elasticsearch import Elasticsearch +from meilisearch.errors import MeilisearchApiError from search.api import course_discovery_search, NoSearchEngineError from search.elastic import ElasticSearchEngine +from search.tests.factories import DemoCourse from search.tests.utils import SearcherMixin, TEST_INDEX_NAME from search.meilisearch import get_meilisearch_client, create_indexes from .mock_search_engine import MockSearchEngine -class DemoCourse: - """ Class for dispensing demo courses """ - DEMO_COURSE_ID = "edX/DemoX/Demo_Course" - DEMO_COURSE = { - "start": datetime(2014, 2, 1), - "number": "DemoX", - "content": { - "short_description": "Short description", - "overview": "Long overview page", - "display_name": "edX Demonstration Course", - "number": "DemoX" - }, - "course": "edX/DemoX/Demo_Course", - "image_url": "/c4x/edX/DemoX/asset/images_course_image.jpg", - "effort": "5:30", - "id": DEMO_COURSE_ID, - "enrollment_start": datetime(2014, 1, 1), - } - - demo_course_count = 0 - - @classmethod - def get(cls, update_dict=None, remove_fields=None): - """ get a new demo course """ - cls.demo_course_count += 1 - course_copy = copy.deepcopy(cls.DEMO_COURSE) - if update_dict: - if "content" in update_dict: - course_copy["content"].update(update_dict["content"]) - del update_dict["content"] - course_copy.update(update_dict) - course_copy.update({"id": "{}_{}".format(course_copy["id"], cls.demo_course_count)}) - if remove_fields: - for remove_field in remove_fields: - if remove_field in course_copy: - del course_copy[remove_field] - return course_copy - - @classmethod - def reset_count(cls): - """ go back to zero """ - cls.demo_course_count = 0 - - @staticmethod - def index(searcher, course_info): - """ Adds course info dictionary to the index """ - searcher.index(sources=course_info) - - @classmethod - def get_and_index(cls, searcher, update_dict=None, remove_fields=None): - """ Adds course info dictionary to the index """ - cls.index(searcher, [cls.get(update_dict, remove_fields)]) +logger = logging.getLogger(__name__) @override_settings(SEARCH_ENGINE="search.tests.mock_search_engine.MockSearchEngine") @@ -492,8 +443,17 @@ class TestMeilisearchCourseDiscoverySearch(TestCase, SearcherMixin): Integration tests using real Meilisearch engine. """ - def setUp(self): + meilisearch_client = get_meilisearch_client() + + def setUp(self): # pragma: no cover super().setUp() + try: + self.meilisearch_client.index(TEST_INDEX_NAME).delete() + except MeilisearchApiError: + pass + except Exception as e: # pylint: disable=broad-exception-caught + logger.warning(f"Unexpected error deleting Meilisearch index: {e}") + create_indexes({TEST_INDEX_NAME: [ "language", "modes", @@ -504,18 +464,13 @@ def setUp(self): ]}) self.wait_for_meilisearch_indexing() - def tearDown(self): - client = get_meilisearch_client() - client.index(TEST_INDEX_NAME).delete() - super().tearDown() - - @staticmethod - def wait_for_meilisearch_indexing(): + def wait_for_meilisearch_indexing(self): # pragma: no cover """Helper method adding a tiny delay for Meilisearch to finish updating the index.""" - client = get_meilisearch_client() - task = client.index(TEST_INDEX_NAME).get_tasks().results[-1] - client.wait_for_task(task.uid) - time.sleep(0.2) + task = self.meilisearch_client.index(TEST_INDEX_NAME).get_tasks().results[-1] + if not task: + return + self.meilisearch_client.wait_for_task(task.uid) + time.sleep(0.1) def test_course_matching_empty_index(self): """ Check for empty result count before indexing """ diff --git a/search/tests/utils.py b/search/tests/utils.py index 5d38d616..7fd4ec94 100644 --- a/search/tests/utils.py +++ b/search/tests/utils.py @@ -1,11 +1,15 @@ """ Test utilities """ import json +import time from django.test import Client from elasticsearch import Elasticsearch, exceptions +from meilisearch.errors import MeilisearchApiError from search.search_engine_base import SearchEngine from search.tests.mock_search_engine import MockSearchEngine +from search.tests.factories import DemoCourse from search.elastic import ElasticSearchEngine +from search.meilisearch import create_indexes, get_meilisearch_client TEST_INDEX_NAME = "test_index" @@ -21,9 +25,8 @@ def post_request(body, course_id=None): return getattr(response, "status_code", 500), json.loads(getattr(response, "content", None).decode('utf-8')) -def post_discovery_request(body): +def post_discovery_request(body, address='/course_discovery/'): """ Helper method to post the request and process the response """ - address = '/course_discovery/' response = Client().post(address, body) return getattr(response, "status_code", 500), json.loads(getattr(response, "content", None).decode('utf-8')) @@ -83,3 +86,69 @@ class ErroringElasticImpl(Elasticsearch): def search(self, **kwargs): # pylint: disable=arguments-differ """ this will definitely fail """ raise exceptions.ElasticsearchException("This search operation failed") + + +def setup_meilisearch(index_name, logger): # pragma: no cover + """Helper method to set up Meilisearch engine""" + client = get_meilisearch_client() + try: + task_info = client.get_index(index_name).delete() + client.wait_for_task(task_info.task_uid, timeout_in_ms=5000) + except MeilisearchApiError: + pass + except Exception as e: # pylint: disable=broad-exception-caught + logger.warning(f"Unexpected error deleting Meilisearch index: {e}") + + create_indexes({index_name: [ + "language", "modes", "org", "catalog_visibility", "enrollment_start", "enrollment_end", + ]}) + + def wait(seconds=1): + """Add small delay to wait for Meilisearch tasks to complete""" + time.sleep(seconds) + + return {"search_engine": "search.meilisearch.MeilisearchEngine", "wait": wait} + + +def setup_elasticsearch(index_name): + """Helper method to set up Elasticsearch engine""" + es = Elasticsearch() + es.indices.delete(index=index_name, ignore=[400, 404]) # pylint: disable=unexpected-keyword-arg + es.indices.create(index=index_name, ignore=400, body={}) # pylint: disable=unexpected-keyword-arg + + return {"search_engine": "search.tests.utils.ForceRefreshElasticSearchEngine", "wait": lambda: None} + + +def setup_democourse(searcher): + """Set up a demo course to use in api tests""" + DemoCourse.reset_count() + DemoCourse.get_and_index( + searcher, + { + "org": "OrgA", + "language": "en", + "content": { + "short_description": "Find this one with the right parameter" + } + } + ) + DemoCourse.get_and_index( + searcher, + { + "org": "OrgB", + "language": "fr", + "content": { + "short_description": "Find this one with another parameter" + } + } + ) + DemoCourse.get_and_index( + searcher, + { + "org": "OrgC", + "language": "en", + "content": { + "short_description": "Find this one somehow" + } + } + ) diff --git a/search/urls.py b/search/urls.py index 10439d54..c2fe2ff1 100644 --- a/search/urls.py +++ b/search/urls.py @@ -12,4 +12,5 @@ path('', views.do_search, name='do_search'), re_path(r'^{}$'.format(COURSE_ID_PATTERN), views.do_search, name='do_search'), path('course_discovery/', views.course_discovery, name='course_discovery'), + path('unstable/v0/course_list_search/', views.course_list_search, name='course_list_search'), ] diff --git a/search/views.py b/search/views.py index f694aa37..4d0499e9 100644 --- a/search/views.py +++ b/search/views.py @@ -34,10 +34,11 @@ def _process_pagination_values(request): return size, from_, page -def _process_field_values(request): +def _process_field_values(request, is_multivalue=False): """ Create separate dictionary of supported filter values provided """ + get_value = request.POST.getlist if is_multivalue else request.POST.get return { - field_key: request.POST[field_key] + field_key: get_value(field_key) for field_key in request.POST if field_key in course_discovery_filter_fields() } @@ -138,11 +139,24 @@ def do_search(request, course_id=None): @require_POST def course_discovery(request): + """ Legacy single-value search endpoint """ + return _course_discovery(request, is_multivalue=False) + + +@require_POST +def course_list_search(request): + """ Main endpoint for multi-value faceted search """ + return _course_discovery(request, is_multivalue=True) + + +@require_POST +def _course_discovery(request, is_multivalue=False): """ Search for courses Args: request (required) - django request object + is_multivalue (optional) - boolean indicating whether to use multi-value faceted search Returns: http json response with the following fields @@ -171,7 +185,7 @@ def course_discovery(request): try: size, from_, page = _process_pagination_values(request) - field_dictionary = _process_field_values(request) + field_dictionary = _process_field_values(request, is_multivalue=is_multivalue) # Analytics - log search request track.emit( @@ -189,6 +203,7 @@ def course_discovery(request): from_=from_, field_dictionary=field_dictionary, enable_course_sorting_by_start_date=enable_course_sorting_by_start_date, + is_multivalue=is_multivalue, ) # Analytics - log search results before sending to browser