[SE-4682] Removing invitation-only and non-catalog courses from search results

[ha-D]

Description

Using the following options allows us to enable searching for courses in the LMS:
COURSES_ARE_BROWSABLE=true
ENABLE_COURSE_DISCOVERY=true

However, there are two types of courses which we would like to exclude from search results:

• invitation-only courses
• Courses which are not supposed to be shown in the catalog (i.e., catalog_visibility="none")

This PR adds the two fields invitation_only and catalog_visibility to the indexed data for each course.

It also adds two extra settings: SEARCH_SKIP_INVITATION_ONLY_FILTERING and SEARCH_SKIP_SHOW_IN_CATALOG_FILTERING. These can be configured to specify whether the filtering on these fields should be performed or not.

Testing Instructions

Sandbox: LMS

The sandbox has four courses with the following names, with their "invitation" and "catalog" settings matching their titles:
Test - Doesn't show in Catalog - Needs Invitation
Test - Doesn't show in Catalog - Doesn't need Invitation
Test - Shows in Catalog - Needs Invitation
Test - Shows in Catalog - Doesn't need invitation

By using the search term "test" in the course discovery page, you should only see the Test - Shows in Catalog - Doesn't need invitation course.

---

Settings

EDXAPP_FEATURES_EXTRA:
  COURSES_ARE_BROWSABLE: true
  ENABLE_COURSE_DISCOVERY: true
  ENABLE_SEPARATE_ARCHIVED_COURSES: true
EDXAPP_LMS_ENV_EXTRA:
  SEARCH_SKIP_INVITATION_ONLY_FILTERING: false
  SEARCH_SKIP_SHOW_IN_CATALOG_FILTERING: false

Reviewers

• @nizarmah

[openedx-webhooks]

Thanks for the pull request, @ha-D! I've created OSPR-5983 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

⚠️ We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation. Please see the CONTRIBUTING file for more information. If you've signed an agreement in the past, you may need to re-sign. See The New Home of the Open edX Codebase for details.

[nizarmah]

@ha-D I've tested the change and it functions as expected. However, I do have additional comments mentioned below.

I apologize for the mistake on my end regarding the django app comment. There are two main comments in the same comment:

• First is regarding the naming of the search filter generator class
• Second is regarding the django app

Let me know please when this is ready for another review 👍🏼

[nizarmah]

Sorry I should've been clearer. Can you move the custom search class to the same directory the LmsSearchFilterGenerator is, lms/lib/courseware_search/lms_filter_generator.py?

Also, would it be possible to rename this filter generator to a name that's more descriptive regarding what the filter generator does, such as CatalogInviteOnlySearchFilterGenerator? Please note that I don't have any better suggestions, so if you have any other suggestion, please go with it instead.

---

Just for clarification, initially, my suggestion to create a django app was in order to utilize this filter in case the change was not upstreamed. But since we're working on upstreaming it, there's no need for creating a new django app for it.

Sorry, I missed it in the initial internal pull request you created.

[ha-D]

Oh, got it 👍

I'm not sure if it's better but just to put it out there, there's another approach we can take as well:

There's currently a SEARCH_SKIP_ENROLLMENT_START_DATE_FILTERING options in the settings. We can add similar settings:
SEARCH_SHOW_INVITATION_ONLY_COURSES
SEARCH_SHOW_NON_CATALOG_COURSES

Then instead of creating a new SearchFilterGenerator we can modify the LmsSearchFilterGenerator to respect these settings. This way we'd be able to configure these options separately if needed, e.g, disabling invitation-only while still allowing non-catalog courses.

@nizarmah Any thoughts on this?

[nizarmah]

@ha-D yeah I'm a fan of adding those feature flags.

I'd like to ask though, what are your thoughts about following a similar naming pattern, SEARCH_SKIP_INVITATION_ONLY_FILTERING and SEARCH_SKIP_SHOW_IN_CATALOG_FILTERING?

We can set their default values to True so that users interested in having invitation only and show in catalog filtering turned on can set their values False.

But yeah, in the meantime, please proceed in that direction.

We'll still have to see what edX developers think of the changes once they are done.

[ha-D]

Made the changes with the naming pattern you suggested

[nizarmah]

Can you please follow this guide to document new feature toggles?

[ha-D]

I added the this, but I'm a little unsure about the toggle_use_cases parameter I used. Its mentioned here that its best to avoid using open_edx however I couldn't find a better one than fit.

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[nizarmah]

I'm not sure I can resolve the toggle_use_cases issue. In addition, edX might want us to follow a different approach.

Accordingly, I think this is ready for edX's review @ha-D. Thanks for addressing my comments 👍🏼

[ha-D]

@natabene This is ready for edX review

[natabene]

@ha-D Thanks for letting me know, I am lining this up for our review.

[natabene]

@bradenmacdonald Would you be interested in reviewing this?

[bradenmacdonald]

@natabene Sure, I can review this.

[bradenmacdonald]

👍 Looks good to me, thanks! Sorry for the slow review here.

• I tested this: on sandbox only.
• I read through the code
• I checked for accessibility issues: n/a
• Includes documentation: yes, of the feature toggle.

@pkulkark I think you are taking this PR over from @ha-D; could you please rebase it and ping me to merge once the tests are all green?

[pkulkark]

@bradenmacdonald Rebased and all tests are passing except for the CLA one. Can we get an exception for this? Or should I close this and open a new one?

[bradenmacdonald]

@natabene Is it OK if I merge this here even without the CLA check passing? The original author was covered under our CLA at the time the PR was created, but is no longer at OpenCraft. @pkulkark has taken it over and she is under our CLA :)

[natabene]

@bradenmacdonald In this case it is fine to merge.

[openedx-webhooks]

@ha-D 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.