feat: filter courses by user language by default #33647

navinkarkera · 2023-11-02T09:36:32Z

Description

Adds a waffle switch to filter courses by selected site language by default

Useful information to include:

Which edX user roles will this change impact? Learner

Private-ref: BB-8091

Testing instructions

Setup master devstack and checkout this PR
Add below snippet to lms/envs/private.py

FEATURES['ENABLE_COURSE_DISCOVERY'] = True

Start lms and cms using make {lms,cms}-up
Add courseware.discovery_default_language_filter waffle switch in django admin.
Add a new course or update language of any existing course to some other language.
Go to http://localhost:18000/courses
If you have never set your language via account settings, you should see all the courses without any filters applied.
Now change your language by going to Account page.
- Only English option is available in the Language field.
- You can add multiple languages by adding a new Darklang config in http://localhost:18000/admin/dark_lang/darklangconfig/

Open http://localhost:18000/courses, a default language filter should be applied.

openedx-webhooks · 2023-11-02T09:36:37Z

Thanks for the pull request, @navinkarkera! 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.

open-craft-grove · 2023-11-02T09:45:34Z

Sandbox deploy request received. Deployment will start soon.

open-craft-grove · 2023-11-02T09:53:42Z

Sandbox deployment started.

open-craft-grove · 2023-11-02T10:15:35Z

Sandbox update request received. Deployment will start soon.

open-craft-grove · 2023-11-02T10:56:47Z

Sandbox deployment successful.

Sandbox LMS is available at pr-33647-139931.staging.do.opencraft.hosting
Sandbox Studio is available at studio.pr-33647-139931.staging.do.opencraft.hosting

open-craft-grove · 2023-11-02T10:56:52Z

Sandbox deployment started.

open-craft-grove · 2023-11-02T11:25:33Z

Sandbox deployment successful.

Sandbox LMS is available at pr-33647-139931.staging.do.opencraft.hosting
Sandbox Studio is available at studio.pr-33647-139931.staging.do.opencraft.hosting

tecoholic · 2023-11-10T06:22:30Z

@navinkarkera 👍

I tested this: Verified that the discovery page filters courses automatically based on the user's language.
I read through the code
I checked for accessibility issues
Includes documentation

NOTE:

I needed to run the make lms-static for the JS files to be regenerated.
While feature flag contains the documentation about the flag, maybe there somewhere else this should be documented? eg., https://edx.readthedocs.io/projects/edx-installing-configuring-and-running/en/latest/feature_flags/feature_flag_index.html

lms/static/js/discovery/discovery_factory.js

openedx/edx-platform#33647

navinkarkera · 2023-11-13T14:17:16Z

While feature flag contains the documentation about the flag, maybe there somewhere else this should be documented? eg., https://edx.readthedocs.io/projects/edx-installing-configuring-and-running/en/latest/feature_flags/feature_flag_index.html

openedx/edx-documentation#2210

open-craft-grove · 2023-11-15T10:30:38Z

Sandbox update request received. Deployment will start soon.

mphilbrick211 · 2023-11-15T21:39:40Z

Hi @navinkarkera! Is this ready for review?

navinkarkera · 2023-11-16T04:44:52Z

@mphilbrick211 Thanks for checking. Yes it is ready.

open-craft-grove · 2023-11-29T07:37:38Z

Sandbox deployment successful.

Sandbox LMS is available at pr-33647-139931.staging.do.opencraft.hosting
Sandbox Studio is available at studio.pr-33647-139931.staging.do.opencraft.hosting

Instance config : https://grove-stage-build-logs.nyc3.digitaloceanspaces.com/34602668-5639873435-config.yml
Instance requirements : https://grove-stage-build-logs.nyc3.digitaloceanspaces.com/34602668-5639873435-requirements.txt

pomegranited · 2024-01-25T04:29:30Z

Hi @mphilbrick211 @navinkarkera may I review/merge this as CC?

navinkarkera · 2024-01-25T05:29:44Z

@pomegranited That would be super helpful. Thanks!

pomegranited · 2024-01-25T05:57:51Z

Sure thing @navinkarkera , could you direct me to the ticket?

navinkarkera · 2024-01-25T06:03:44Z

@pomegranited BB-8091. Added the ticket info to description as well.

pomegranited

@navinkarkera This code is working exactly as described, thank you for such great testing instructions!

But I have some questions about the feature flag, and a request for a better test. Please let me know if you have any questions or concerns.

pomegranited · 2024-01-26T05:56:39Z

lms/static/js/spec/discovery/discovery_factory_spec.js

+            expect($('.courses-listing .course-title')).toContainHtml('edX Demonstration Course');
+            expect($('.active-filter').length).toBe(1);
+            expect($('.active-filter .query')).toContainHtml("English");
+        });


Since there's only ever one course in the returned list, this test doesn't actually verify that the filter was applied.
Could you add another course that uses a different language so we can see that this filter actually applies?

I noticed that when my preferred language doesn't match any of my course's languages that I see header text like this:

We couldn't find any results for ""

which I think students may find confusing, because the text is confusing, and because the student didn't actually perform an explicit search.

An easy way around this would be to avoid showing that header if there's no search text.

Since there's only ever one course in the returned list, this test doesn't actually verify that the filter was applied.
Could you add another course that uses a different language so we can see that this filter actually applies?

@pomegranited Actually the test should not be about checking API response as it is mocked above by AjaxHelpers.respondWithJson(requests, JSON_RESPONSE);. So I modified to check the request body as it should include language filter automatically when default filter is enabled.

An easy way around this would be to avoid showing that header if there's no search text.

Makes sense. Done 👍

pomegranited · 2024-01-26T06:11:22Z

lms/djangoapps/courseware/views/views.py

@@ -275,6 +275,7 @@ def courses(request):
    """
    courses_list = []
    course_discovery_meanings = getattr(settings, 'COURSE_DISCOVERY_MEANINGS', {})
+    set_default_filter = settings.FEATURES.get('ENABLE_COURSE_DISCOVERY_DEFAULT_LANGUAGE_FILTER')


Is there an implementation reason why you chose to add a new settings.FEATURES flag instead of one of the toggles available from edx-toggles like SettingToggle or WaffleFlag?

It's my understanding from Feature Flags and Settings on edx-platform and OEP-17 that we should be using these edx-toggles now, and that FEATURES flags are deprecated.

@pomegranited No specific reason for using feature flags, except for maybe the relationship with ENABLE_COURSE_DISCOVERY feature flag. After reading the the OEP, I think we can using WaffleSwitch here.

pomegranited

👍 Thank you for addressing my comments @navinkarkera :) I'll merge this tomorrow if there are no objections.

Could you update the PR description to reflect the use of a Waffle Switch instead of a feature flag to enable this feature? And will need an update to openedx/edx-documentation#2210 too.

I tested this on my devstack using the excellent testing instructions in the PR.
I read through the code
I checked for accessibility issues by using my keyboard to navigate the Courses list
Includes documentation on the new switch
~~User-facing strings are extracted for translation~~ N/A

lms/djangoapps/courseware/toggles.py

navinkarkera · 2024-02-06T12:21:40Z

@pomegranited We probably don't need openedx/edx-documentation#2210 as it will be automatically documented here.

Adds a feature flag to filter courses by users preferred language by default test: add test

pomegranited · 2024-02-06T22:22:51Z

@navinkarkera Perfect, thank you for fixing that nit and updating your PR description.

We probably don't need openedx/edx-documentation#2210 as it will be automatically documented here.

Sweet! Feel free to close that then.

Merging this now :)

openedx-webhooks · 2024-02-06T22:23:07Z

@navinkarkera 🎉 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 · 2024-02-06T23:47:47Z

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

edx-pipeline-bot · 2024-02-07T00:08:34Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-02-07T00:08:34Z

2U Release Notice: This PR has been deployed to the edX production environment.

navinkarkera self-assigned this Nov 2, 2023

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 2, 2023

navinkarkera marked this pull request as ready for review November 2, 2023 09:36

navinkarkera force-pushed the navin/filter-courses-by-language branch from 0adc71b to c86d908 Compare November 2, 2023 09:37

navinkarkera force-pushed the navin/filter-courses-by-language branch from c86d908 to 6c71c68 Compare November 2, 2023 10:04

navinkarkera force-pushed the navin/filter-courses-by-language branch from 6c71c68 to 4f23108 Compare November 3, 2023 06:22

tecoholic reviewed Nov 10, 2023

View reviewed changes

lms/static/js/discovery/discovery_factory.js Show resolved Hide resolved

navinkarkera force-pushed the navin/filter-courses-by-language branch 5 times, most recently from 20597e2 to af1d124 Compare November 13, 2023 13:36

navinkarkera added a commit to open-craft/edx-documentation that referenced this pull request Nov 13, 2023

chore: add ENABLE_COURSE_DISCOVERY_DEFAULT_LANGUAGE_FILTER flag

4b50aaa

openedx/edx-platform#33647

navinkarkera mentioned this pull request Nov 13, 2023

chore: add ENABLE_COURSE_DISCOVERY_DEFAULT_LANGUAGE_FILTER flag openedx/edx-documentation#2210

Closed

10 tasks

mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Nov 20, 2023

navinkarkera force-pushed the navin/filter-courses-by-language branch 2 times, most recently from 81335fa to eca9398 Compare November 29, 2023 06:36

navinkarkera force-pushed the navin/filter-courses-by-language branch from eca9398 to 13ad42f Compare December 25, 2023 14:51

0x29a mentioned this pull request Dec 31, 2023

feat: port code drift for open-craft/edx-platform [BB-8366] open-craft/edx-platform#614

Merged

navinkarkera force-pushed the navin/filter-courses-by-language branch from 13ad42f to 9a2f9da Compare January 25, 2024 05:28

pomegranited requested changes Jan 26, 2024

View reviewed changes

navinkarkera force-pushed the navin/filter-courses-by-language branch from 9a2f9da to ed3664a Compare February 5, 2024 11:42

pomegranited approved these changes Feb 6, 2024

View reviewed changes

lms/djangoapps/courseware/toggles.py Outdated Show resolved Hide resolved

navinkarkera force-pushed the navin/filter-courses-by-language branch from 1ef9b38 to c3eb382 Compare February 6, 2024 11:10

feat: filter courses by user language by default

f1d79ca

Adds a feature flag to filter courses by users preferred language by default test: add test

navinkarkera force-pushed the navin/filter-courses-by-language branch from c3eb382 to f1d79ca Compare February 6, 2024 12:21

pomegranited merged commit 4a46ae9 into openedx:master Feb 6, 2024
46 checks passed

pomegranited deleted the navin/filter-courses-by-language branch February 6, 2024 22:23

itsjeyd added core contributor PR author is a Core Contributor (who may or may not have write access to this repo). and removed waiting for eng review PR is ready for review. Review and merge it, or suggest changes. labels Mar 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: filter courses by user language by default #33647

feat: filter courses by user language by default #33647

navinkarkera commented Nov 2, 2023 •

edited

Loading

openedx-webhooks commented Nov 2, 2023 •

edited

Loading

open-craft-grove commented Nov 2, 2023

open-craft-grove commented Nov 2, 2023

open-craft-grove commented Nov 2, 2023

open-craft-grove commented Nov 2, 2023

open-craft-grove commented Nov 2, 2023

open-craft-grove commented Nov 2, 2023

tecoholic commented Nov 10, 2023 •

edited

Loading

navinkarkera commented Nov 13, 2023

open-craft-grove commented Nov 15, 2023

mphilbrick211 commented Nov 15, 2023

navinkarkera commented Nov 16, 2023

open-craft-grove commented Nov 29, 2023

pomegranited commented Jan 25, 2024

navinkarkera commented Jan 25, 2024

pomegranited commented Jan 25, 2024

navinkarkera commented Jan 25, 2024

pomegranited left a comment

pomegranited Jan 26, 2024

pomegranited Jan 26, 2024

navinkarkera Feb 5, 2024

pomegranited Jan 26, 2024

navinkarkera Feb 5, 2024

pomegranited left a comment

navinkarkera commented Feb 6, 2024

pomegranited commented Feb 6, 2024

openedx-webhooks commented Feb 6, 2024

edx-pipeline-bot commented Feb 6, 2024

edx-pipeline-bot commented Feb 7, 2024

edx-pipeline-bot commented Feb 7, 2024

feat: filter courses by user language by default #33647

feat: filter courses by user language by default #33647

Conversation

navinkarkera commented Nov 2, 2023 • edited Loading

Description

Testing instructions

openedx-webhooks commented Nov 2, 2023 • edited Loading

open-craft-grove commented Nov 2, 2023

open-craft-grove commented Nov 2, 2023

open-craft-grove commented Nov 2, 2023

open-craft-grove commented Nov 2, 2023

open-craft-grove commented Nov 2, 2023

open-craft-grove commented Nov 2, 2023

tecoholic commented Nov 10, 2023 • edited Loading

navinkarkera commented Nov 13, 2023

open-craft-grove commented Nov 15, 2023

mphilbrick211 commented Nov 15, 2023

navinkarkera commented Nov 16, 2023

open-craft-grove commented Nov 29, 2023

pomegranited commented Jan 25, 2024

navinkarkera commented Jan 25, 2024

pomegranited commented Jan 25, 2024

navinkarkera commented Jan 25, 2024

pomegranited left a comment

Choose a reason for hiding this comment

pomegranited Jan 26, 2024

Choose a reason for hiding this comment

pomegranited Jan 26, 2024

Choose a reason for hiding this comment

navinkarkera Feb 5, 2024

Choose a reason for hiding this comment

pomegranited Jan 26, 2024

Choose a reason for hiding this comment

navinkarkera Feb 5, 2024

Choose a reason for hiding this comment

pomegranited left a comment

Choose a reason for hiding this comment

navinkarkera commented Feb 6, 2024

pomegranited commented Feb 6, 2024

openedx-webhooks commented Feb 6, 2024

edx-pipeline-bot commented Feb 6, 2024

edx-pipeline-bot commented Feb 7, 2024

edx-pipeline-bot commented Feb 7, 2024

navinkarkera commented Nov 2, 2023 •

edited

Loading

openedx-webhooks commented Nov 2, 2023 •

edited

Loading

tecoholic commented Nov 10, 2023 •

edited

Loading