Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initial commit #4349

Closed
wants to merge 6 commits into from
Closed

initial commit #4349

wants to merge 6 commits into from

Conversation

zawan-ila
Copy link
Contributor

PROD-4006

Expose restricted runs on non-ES Apis only when a query param asks for them

@@ -81,6 +82,7 @@ def get_queryset(self):
multiple: false
"""
q = self.request.query_params.get('q')
restriction_list = self.request.query_params.get('restriction_list', '').split(',')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we would need this on courses API as well.

Comment on lines +29 to +30
if course.advertised_course_run(restriction_list=[CourseRunRestrictionType.CustomB2C.value]) and course.advertised_course_run(restriction_list=[CourseRunRestrictionType.CustomB2C.value]).language:
return course.advertised_course_run(restriction_list=[CourseRunRestrictionType.CustomB2C.value]).language
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

advertised_run = course.advertised_course_run(restriction_list=[CourseRunRestrictionType.CustomB2C.value])
if advertised_run and advertised_run.language:
    return advertised_run.language

@@ -103,6 +107,8 @@ def should_include_course_run(course_run, params, exclude_expired):
matches_parameter = True
if matches_parameter:
break
if hasattr(course_run, 'restricted_run') and course_run.restricted_run.restriction_type in forbidden:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't we need this type of filtering on restricted_runs in get_languages and get_seat_types method.

@@ -1995,6 +2011,10 @@ def get_organization_logo_override_url(self, obj):
return logo_image_override.url
return None

def get_course_run_statuses(self, program):
restriction_list = self.context['request'].query_params.get('restriction_list', '').split(',')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we are using restriction_list = self.context['request'].query_params.get('restriction_list', '').split(',') in many places in this file. Maybe add a unified util/method to avoid repeating this?

@@ -95,10 +98,14 @@ def get_queryset(self):
queryset = CourseEditor.editable_course_runs(self.request.user, queryset)
else:
queryset = self.queryset


if self.request.method == 'GET':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: can we use edit_mode boolean here?

Comment on lines +313 to +315
restriction_list = query_params.get('restriction_list', '').split(',')
forbidden = list(set(CourseRunRestrictionType.values) - set(restriction_list))
queryset = queryset.exclude('terms', restriction_type=forbidden)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: what's the difference in doing this here vs in

? The other filters and lookup terms are defined there. For consistency purpose, we should add that there if applicable.

@@ -253,15 +255,15 @@ def active_languages(self):

@property
def active_run_key(self):
return getattr(self.advertised_course_run, 'key', None)
return getattr(self.advertised_course_run(restriction_list=[CourseRunRestrictionType.CustomB2C.value]), 'key', None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: if advertised_course_run is method in the same file, we can add B2C as a default value in the method instead of repeating across various lines.

@@ -2587,6 +2585,17 @@ def search(cls, query):
dsl_query = ESDSLQ('query_string', query=query, analyze_wildcard=True)
return queryset.query(dsl_query)

@classmethod
def get_exposed_runs(cls, runs, restriction_list=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the method name can be improved. exposed_runs does not make sense.

@@ -125,6 +126,10 @@ def prepare_seat_types(self, obj):
def prepare_skill_names(self, obj):
return get_product_skill_names(obj.course.key, ProductTypes.Course)

def prepare_restriction_type(self, obj):
if hasattr(obj, "restricted_run"):
return obj.restricted_run.restriction_type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should it be restriction_type.value? Or is that catered during indexing?

@zawan-ila
Copy link
Contributor Author

Closing in favor of 4356

@zawan-ila zawan-ila closed this May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants