feat: [FC-86] add new version of course discovery API by Serj-N · Pull Request #227 · openedx/edx-search

Serj-N · 2025-09-18T09:46:15Z

PR Summary
Implement a new API endpoint to support multi-value faceted search for course discovery.

Key Changes:

Added a new url: unstable/v0/course_list_search/
Added two thin wrappers around the old view function to determine whether the search is single-value or multi-value.
For the new endpoint, always get facet values as a list from request, even if it is only one value.
Added integration tests emulating API calls on both endpoints (old and new) using Meilisearch engine.

Note

Related to: #213

openedx-webhooks · 2025-09-18T09:46:20Z

Thanks for the pull request, @Serj-N!

This repository is currently maintained by @Ali-Salman29.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

brian-smith-tcril

I left a couple comments with questions about test changes that aren't mentioned in the PR description.

brian-smith-tcril · 2025-09-19T15:19:41Z

    Integration tests using real Meilisearch engine.
    """

+    meilisearch_client = get_meilisearch_client()


Could you provide some context as to why this is being moved up here instead of following the previous pattern of calling get_meilisearch_client() in tearDown and wait_for_meilisearch_indexing?

This is a slight optimization: instead of calling get_meilisearch_client() multiple times, we call it once, store the return value on the test class and then reuse it whenever necessary.

brian-smith-tcril · 2025-09-19T15:23:12Z

+    def tearDown(self):  # pragma: no cover
+        try:
+            self.meilisearch_client.index(TEST_INDEX_NAME).delete()
+        except Exception:  # pylint: disable=broad-exception-caught


A few questions here:

Why wasn't this try/except needed before?

Should we really be catching everything?

Instead of just passing here, should we log something?

Technically, it was needed - to ensure a proper teardown after each test so that we start the following test with a clean slate. This makes our test suite more robust and futureproof (i.e. in case more test methods are added or tests are rearranged).

and 3. All we care about is to delete the index if it exists, and if an exception is caught - that is okay, and expected, if the index didn't exist in the first place.

I think we should at least log the exceptions. If there's a bunch of exceptions happening during a test case, that usually indicates some code issue that needs to be cleaned up, and it's better to know about it than quietly ignore it.

brian-smith-tcril · 2025-09-19T15:24:10Z

+        if not task:
+            return


Why wasn't this needed before?

This is to ensure that no error is raised in cases where we try to get the last task after all tasks have been completed. Such cases may never occur, but it is a nice-to-have safeguard.

It would be much more robust to pass in the ID of a specific task to wait for, rather than to generically wait for the most recent task. But I guess this is fine for test purposes.

Totally agree, and it was my initial intention, but the problem is that create_indexes() does not return anything

peterkulko · 2025-09-22T09:12:03Z

@bradenmacdonald Hi! This PR is ready for review. When you have a moment, I’d really appreciate it if you could take a look 💯

bradenmacdonald · 2025-09-22T21:20:46Z

@PKulkoRaccoonGang I'll gladly take a look, but I have about 6-8 PRs ahead of this in my review queue so it may take a couple days.

bradenmacdonald · 2025-09-22T21:23:40Z

Is it too late to call this API something different btw? "Course Discovery" is a fine name, but makes me think this API is related to the Discovery Service. I would prefer "Course Search API" but I guess that is also easily confused with the "Courseware Search API". Maybe "Course List Search API" ? 😬

cmltaWt0 · 2025-09-23T07:02:33Z

Is it too late to call this API something different btw? "Course Discovery" is a fine name, but makes me think this API is related to the Discovery Service. I would prefer "Course Search API" but I guess that is also easily confused with the "Courseware Search API". Maybe "Course List Search API" ? 😬

Personally, I like the suggestion. I have fallen into this trap several times by mixing it with Discovery...

Serj-N · 2025-09-24T09:37:40Z

Is it too late to call this API something different btw?

@bradenmacdonald @sarina @cmltaWt0 @PKulkoRaccoonGang
I agree that the current naming is confusing and "Course List Search" seems much clearer. However before we proceed,
I would like to make sure we realize what this would entail. We have two urls: course_discovery/ (legacy) and v1/course_discovery/ (new). At this point, we can rename the new one with minimal impact, but changing the legacy endpoint will require some adjustments in other services that use this endpoint. If we keep the old name for the legacy endpoint, we will have two urls that are almost identical functionally but are named inconsistently, which may create confusion. So I guess we will have to make a decision here:

Either rename both endpoints and then make corresponding changes to whatever services are using the legacy endpoint.
Or only rename the new endpoint and live with the inconsistent naming.

sarina · 2025-09-24T13:06:58Z

My vote would be to

only rename the new endpoint

and to file a DEPR for the legacy endpoint, which we could remove once the MFE is default.

BUT please let @bradenmacdonald and/or @feanil weigh in on this.

bradenmacdonald · 2025-09-24T18:21:22Z

I agree with DEPR the old endpoint without renaming it, and rename the new endpoint.

I would also prefer to mark the new endpoint as unstable/v0 because I'm hoping we'll redevelop the whole edx-search API in the future. But I guess we can always DEPR it if and when we get there.

Serj-N · 2025-09-25T10:23:03Z

@bradenmacdonald Done, the new endpoint is now unstable/v0/course_list_search/. The old one is kept as is.

sarina · 2025-09-29T12:46:39Z

Hi @bradenmacdonald - I know you've got a lot on your plate. So we can effectively plan, could you kindly provide an estimate of when we might expect a review, so we can plan to be prompt on responding? Thanks!

bradenmacdonald

The main code looks fine. Just some questions about the tests.

I'm wondering, could we combine the test cases and use ddt or pytest.mark.parametrize to switch between the search engines, while still using the same basic data, query, and aggregation test code ?

Also, I haven't tried running this yet. Is there anything I should do to test it in particular, or just verify no regressions with the current API?

bradenmacdonald · 2025-09-30T02:22:32Z

+        try:
+            self.meilisearch_client.get_index(TEST_INDEX_NAME).delete()
+        except Exception:  # pylint: disable=broad-exception-caught
+            pass


pylint is right: We should only pass if this is an expected exception like IndexDoesNotExistException . Otherwise, it's not good to suppress the error here.

Changed, now catching MeilisearchApiError

bradenmacdonald · 2025-09-30T02:23:39Z

+
+    def tearDown(self):  # pragma: no cover
+        try:
+            self.meilisearch_client.index(TEST_INDEX_NAME).delete()


I don't think we need to delete the index in the tearDown and the setUp. Why not just in the setUp ?

bradenmacdonald · 2025-09-30T02:24:44Z

+        try:
+            self.meilisearch_client.get_index(TEST_INDEX_NAME).delete()
+        except Exception:  # pylint: disable=broad-exception-caught
+            pass


Same comment

Now catching specifically MeilisearchApiError

Serj-N · 2025-09-30T19:00:00Z

@bradenmacdonald For better or for worse, I reworked test_course_discovery_views.py to combine tests by single-value and multi-value and switch engines via ddt. However, currently, some tests fail if run together, even though they pass when run separately. It seems like indices do not get cleared properly between test modules. I might be missing something obvious, but I'll have to leave it as is for now, and come back a little later with fresh eyes.

As for testing anything in particular, I believe some basic api calls via Postman should be enough.

Serj-N · 2025-10-02T13:08:31Z

@bradenmacdonald I refactored the tests once more. Short summary:

Split test_course_discovery_views into three modules: test_api_single_value, test_api_multi_value, test_api_mock_engine. This allows us to use a separate index for each test case and ensure test isolation.
Moved DemoCourse class to factories.py
Adjusted the waiting function to simply wait for 1 second flat. Not very elegant, I admit, but Meilisearch async indexing is not very test-friendly, and we need something to make the tests pass consistently.

bradenmacdonald · 2025-10-02T20:48:04Z

but Meilisearch async indexing is not very test-friendly, and we need something to make the tests pass consistently.

It's probably more a limitation of the current edx-search API? Our Meilisearch code in https://github.com/openedx/edx-platform/blob/master/openedx/core/djangoapps/content/search/api.py waits as needed for tasks just fine - see any usage of _wait_for_meili_task in that api.py file.

bradenmacdonald

Thanks for those test refactors. This is good with me.

sarina · 2025-10-02T21:17:42Z

@bradenmacdonald can I put in a nomination to expand your merge rights to this repo? 🙏🏻

bradenmacdonald · 2025-10-02T22:20:02Z

@sarina It would be helpful to have merge rights, yes. Thanks!

Serj-N added 2 commits September 18, 2025 11:26

feat: add v1 url, view wrappers, get lists from POST

c846384

test: add integration tests

376ba3f

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 18, 2025

openedx-webhooks added this to Contributions Sep 18, 2025

github-project-automation Bot moved this to Needs Triage in Contributions Sep 18, 2025

fix: disable unexpected-keyword-arg

7a6f1cd

Serj-N force-pushed the nanai/axm-2532/discovery-api branch 2 times, most recently from 15c7542 to f5f7711 Compare September 18, 2025 12:12

test: ensure teardown, reduce sleep, put client on class, add no cover

d1ecf82

Serj-N force-pushed the nanai/axm-2532/discovery-api branch from f5f7711 to d1ecf82 Compare September 18, 2025 12:20

Serj-N self-assigned this Sep 18, 2025

Serj-N requested a review from peterkulko September 18, 2025 13:17

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Sep 18, 2025

mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Sep 18, 2025

Serj-N requested review from bradenmacdonald and cmltaWt0 September 19, 2025 08:48

brian-smith-tcril reviewed Sep 19, 2025

View reviewed changes

peterkulko mentioned this pull request Sep 25, 2025

feat: [FC-86] Home page course list section openedx/frontend-app-catalog#18

Merged

Serj-N added 2 commits September 25, 2025 12:56

refactor: rename new endpoint to course_list_search

132f0b5

test: exclude a helper method from coverage

41b00b0

mphilbrick211 moved this from Waiting on Author to In Eng Review in Contributions Sep 26, 2025

bradenmacdonald reviewed Sep 30, 2025

View reviewed changes

Serj-N added 5 commits September 30, 2025 11:57

test: catch MeilisearchApiError, remove teardowns

4e1f0d6

test: combine all test cases into one

34f6a91

fix: fix pylint complaints

4a25f4f

test: log broad exception on index delete

e04cefb

refactor: add wait_for_task

f5b2d0b

Serj-N added 3 commits October 2, 2025 14:21

refactor: split api tests into three modules

edd0fe7

style: fix linter issues

b9b164c

refactor: move DemoClass to factories

c2edd1a

Serj-N force-pushed the nanai/axm-2532/discovery-api branch 2 times, most recently from e358f6a to 74739ab Compare October 2, 2025 12:35

test: exclude except from coverage

db57f57

Serj-N force-pushed the nanai/axm-2532/discovery-api branch from 74739ab to db57f57 Compare October 2, 2025 12:42

test: exclude from coverage a line in DemoCourse

3d21851

bradenmacdonald approved these changes Oct 2, 2025

View reviewed changes

ormsbee approved these changes Oct 2, 2025

View reviewed changes

ormsbee merged commit 046f623 into openedx:master Oct 2, 2025
10 checks passed

github-project-automation Bot moved this from In Eng Review to Done in Contributions Oct 2, 2025

Serj-N mentioned this pull request Oct 15, 2025

feat: [FC-86] create new version of course discovery API raccoongang/edx-search#13

Closed

cmersinli mentioned this pull request Jan 22, 2026

Can not use new catalog mfe in ulmo.1 #242

Closed

Uh oh!

Conversation

Serj-N commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Sep 18, 2025

Uh oh!

brian-smith-tcril left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterkulko commented Sep 22, 2025

Uh oh!

bradenmacdonald commented Sep 22, 2025

Uh oh!

bradenmacdonald commented Sep 22, 2025

Uh oh!

cmltaWt0 commented Sep 23, 2025

Uh oh!

Serj-N commented Sep 24, 2025

Uh oh!

sarina commented Sep 24, 2025

Uh oh!

bradenmacdonald commented Sep 24, 2025

Uh oh!

Serj-N commented Sep 25, 2025

Uh oh!

sarina commented Sep 29, 2025

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Serj-N commented Sep 30, 2025

Uh oh!

Serj-N commented Oct 2, 2025

Uh oh!

bradenmacdonald commented Oct 2, 2025

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

sarina commented Oct 2, 2025

Uh oh!

Uh oh!

bradenmacdonald commented Oct 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Serj-N commented Sep 18, 2025 •

edited

Loading