feat: filter restricted runs on APIs #4356

zawan-ila · 2024-05-13T04:24:18Z

Hide restricted runs across APIs by default. Also do not include them when calculating fields like course_run_statuses. If a query-param include_restricted=<some_value> is present, do not hide them

Testing

Create restricted runs in your system and link them with courses, programs, etc.
Run update_index
Verify that runs are hidden on all APIs by default and are revealed on adding the query params.

course_discovery/apps/api/serializers.py

DawoudSheraz · 2024-05-13T12:36:49Z

course_discovery/apps/api/utils.py

@@ -198,6 +199,9 @@ def increment_character(character):
    """
    return chr(ord(character) + 1) if character != 'z' else 'a'

+def get_forbidden_runs(request):


We should use get_restricted_runs (and restricted_runs as a variable) for consistency. Forbidden_runs can mislead when someone else is reading the code (with little to no context). (and to add to following comment, this method is only returning restriction type(s), not actual course runs. That is also misleading.

I have updated some names. Hopefully it's good enough now

DawoudSheraz · 2024-05-13T12:37:59Z

course_discovery/apps/api/utils.py

@@ -198,6 +199,9 @@ def increment_character(character):
    """
    return chr(ord(character) + 1) if character != 'z' else 'a'

+def get_forbidden_runs(request):


nit: missing docstring.

the method is not really returning runs but rather restriction types.

I have updated some names. Hopefully it's good enough now

course_discovery/apps/api/v1/tests/test_views/test_programs.py

DawoudSheraz · 2024-05-15T11:54:28Z

course_discovery/apps/course_metadata/search_indexes/documents/person.py

@@ -47,7 +47,7 @@ def prepare_position(self, obj):
            return []
        return [position.title, position.organization_override]

-    def get_queryset(self):
+    def get_queryset(self, excluded_restriction_types=[]):


why do we need this default but unused argument?

Due to this change, the get_queryset method is called with the excluded_restriction_types for all documents. We don't really use excluded_restriction_type in the get_queryset of some documents but the argument is still passed. Hence, this is to prevent an error.

We could have a condition to check the document type, and only pass it in for those documents which need it, but I think it is neater this way

Right. It would be good to have this context in docstrings/context for future's sake.

course_discovery/apps/course_metadata/search_indexes/serializers/course.py

DawoudSheraz

Played around a bit with this locally, looks good.
One thing: search/<> endpoints are missing restriction_type field on course_run serializers.

AfaqShuaib09

Did some playaround with the include_restricted in api/v1 endpoints & search endpoints on local. Changes look good.

[thinking] One thing we can do in a followup PR to check for N+1 queries.

course_discovery/apps/api/v1/tests/test_views/test_search.py

course_discovery/apps/api/v1/views/search.py

AfaqShuaib09 · 2024-05-15T20:04:58Z

course_discovery/apps/course_metadata/algolia_models.py

@@ -87,7 +87,8 @@ def _wrap(self, *args, **kwargs):


 def get_course_availability(course):
-    all_runs = course.course_runs.filter(status=CourseRunStatus.Published)
+    all_runs = course.course_runs.all()
+    all_runs = filter(lambda r: r.status == CourseRunStatus.Published, all_runs)


wondering why this change has been done?

We have prefetched course_runs on course. And in that prefetch, we have excluded restricted runs. Now if we dont make this change, it will go to the database and course.course_runs will return all runs (even restricted ones). This makes sure that the prefetched course runs are used.

course_discovery/apps/course_metadata/search_indexes/documents/learner_pathway.py

course_discovery/apps/api/v1/tests/test_views/test_catalogs.py

course_discovery/apps/api/v1/tests/test_views/test_course_runs.py

DawoudSheraz · 2024-06-10T07:40:18Z

course_discovery/apps/api/v1/tests/test_views/test_courses.py

+            self.assertEqual(set(response.data['course_run_keys']), {run_not_restricted.key, run_restricted.key})
+            self.assertEqual(


if block is doing assertListEqual, this is doing assertEqual. Since the context is same, we should try to keep the code consistent where applicable.

I have converted both to assertEqual.

DawoudSheraz · 2024-06-10T07:46:27Z

course_discovery/apps/api/v1/tests/test_views/test_programs.py

+        if include_restricted_run:
+            RestrictedCourseRunFactory(course_run=course_run, restriction_type='custom-b2c')  # pylint: disable=possibly-used-before-assignment


nit: if we move this if include_restricted_run: under if block at 55, will we still need pylint disable checks there? Considering we can't restriction without run, it makes sense to nest this if block.

I have nested the block, and have also removed the pylint disable.

course_discovery/apps/course_metadata/tests/test_algolia_models.py

zawan-ila force-pushed the anawaz/prod-4007 branch from 9c1c271 to f65796f Compare May 13, 2024 05:39

DawoudSheraz reviewed May 13, 2024

View reviewed changes

course_discovery/apps/api/serializers.py Outdated Show resolved Hide resolved

DawoudSheraz reviewed May 13, 2024

View reviewed changes

zawan-ila mentioned this pull request May 15, 2024

initial commit #4349

Closed

DawoudSheraz reviewed May 15, 2024

View reviewed changes

course_discovery/apps/api/v1/tests/test_views/test_programs.py Outdated Show resolved Hide resolved

DawoudSheraz reviewed May 15, 2024

View reviewed changes

course_discovery/apps/api/v1/tests/test_views/test_programs.py Outdated Show resolved Hide resolved

DawoudSheraz reviewed May 15, 2024

View reviewed changes

course_discovery/apps/course_metadata/search_indexes/serializers/course.py Outdated Show resolved Hide resolved

DawoudSheraz approved these changes May 15, 2024

View reviewed changes

AfaqShuaib09 approved these changes May 15, 2024

View reviewed changes

zawan-ila force-pushed the anawaz/prod-4007 branch from 3f8337a to 7e0c087 Compare June 6, 2024 10:27

DawoudSheraz reviewed Jun 10, 2024

View reviewed changes

course_discovery/apps/api/v1/tests/test_views/test_catalogs.py Outdated Show resolved Hide resolved

DawoudSheraz reviewed Jun 10, 2024

View reviewed changes

course_discovery/apps/api/v1/tests/test_views/test_course_runs.py Outdated Show resolved Hide resolved

DawoudSheraz reviewed Jun 10, 2024

View reviewed changes

course_discovery/apps/api/v1/tests/test_views/test_course_runs.py Outdated Show resolved Hide resolved

DawoudSheraz reviewed Jun 10, 2024

View reviewed changes

course_discovery/apps/course_metadata/tests/test_algolia_models.py Outdated Show resolved Hide resolved

DawoudSheraz approved these changes Jun 12, 2024

View reviewed changes

zawan-ila force-pushed the anawaz/prod-4007 branch from 1e215aa to 2295621 Compare June 14, 2024 11:45

feat: filter restricted runs on APIs

3881ed6

zawan-ila force-pushed the anawaz/prod-4007 branch from 2295621 to 3881ed6 Compare June 14, 2024 11:50

AfaqShuaib09 approved these changes Jun 14, 2024

View reviewed changes

zawan-ila merged commit 13714a7 into openedx:master Jun 14, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: filter restricted runs on APIs #4356

feat: filter restricted runs on APIs #4356

zawan-ila commented May 13, 2024 •

edited

Loading

DawoudSheraz May 13, 2024 •

edited

Loading

zawan-ila May 15, 2024

DawoudSheraz May 13, 2024 •

edited

Loading

zawan-ila May 15, 2024

DawoudSheraz May 15, 2024

zawan-ila Jun 7, 2024

DawoudSheraz Jun 10, 2024

DawoudSheraz left a comment

AfaqShuaib09 left a comment

AfaqShuaib09 May 15, 2024

zawan-ila Jun 7, 2024

DawoudSheraz Jun 10, 2024

zawan-ila Jun 11, 2024

DawoudSheraz Jun 10, 2024

zawan-ila Jun 11, 2024

		self.assertEqual(set(response.data['course_run_keys']), {run_not_restricted.key, run_restricted.key})
		self.assertEqual(

		if include_restricted_run:
		RestrictedCourseRunFactory(course_run=course_run, restriction_type='custom-b2c') # pylint: disable=possibly-used-before-assignment

feat: filter restricted runs on APIs #4356

feat: filter restricted runs on APIs #4356

Conversation

zawan-ila commented May 13, 2024 • edited Loading

Testing

DawoudSheraz May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DawoudSheraz May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DawoudSheraz left a comment

Choose a reason for hiding this comment

AfaqShuaib09 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zawan-ila commented May 13, 2024 •

edited

Loading

DawoudSheraz May 13, 2024 •

edited

Loading

DawoudSheraz May 13, 2024 •

edited

Loading