Skip to content

ENT-201: Add support for course run key in catalog contains endpoint #721

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.

Sign up for GitHub

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

Jump to bottom
Merged
zubair-arbi merged 1 commit into from
Mar 13, 2017
Merged

ENT-201: Add support for course run key in catalog contains endpoint #721

zubair-arbi merged 1 commit into from
Mar 13, 2017

Conversation

@zubair-arbi
Copy link
Contributor

@zubair-arbi zubair-arbi commented Mar 8, 2017

ENT-201

@saleem-latif @asadiqbal08 @mattdrayer @clintonb @cpennington
Update the endpoint contains of CatalogViewSet to accept a new optional parameter course_run_id. This new parameter will be used for validating if the course run keys of the format org/course/run or course-v1:org+course+run exists in a catalog.

For example in Ecom we can use this endpoint like this:

>>> course_ids = 'edX+TEST_01'
>>> course_run_ids = 'org/course/run,course-v1:edX+DemoX+Demo_Course'
>>> catalog_id = 1
>>> response = site.siteconfiguration.course_catalog_api_client.catalogs(catalog_id).contains.get(
    course_id=course_ids,
    course_run_id=course_run_ids,
)
>>> response
{u'courses': {u'edX+TEST_01': True, u'org/course/run': False, u'course-v1:edX+DemoX+Demo_Course': True}}

Related PR: ENT-211 add aggregate course key

@zubair-arbi
Copy link
Contributor Author

zubair-arbi commented Mar 8, 2017

@clintonb Please review this PR and let me know your thoughts on whether we still need the PR ENT-211 add aggregate course key?
One possibility I can think of for the aggregate course key is for validating the input key provided to the catalog contains endpoint.

Our base requirement was to validate if a course run in the basket is available for a catalog.

clintonb
clintonb suggested changes Mar 9, 2017
View reviewed changes
Copy link
Contributor

@clintonb clintonb left a comment

Choose a reason for hiding this comment

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

This will work, but it would be nice to properly define the course key in the opaque-keys library.

course_discovery/apps/api/v1/tests/test_views/test_catalogs.py
assert response.data['results'] == self.serialize_catalog_course(courses, many=True)

def test_contains(self):
""" Verify the endpoint returns a filtered list of courses contained in the catalog. """
Copy link
Contributor

@clintonb clintonb Mar 9, 2017

Choose a reason for hiding this comment

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

This change is not necessary.

Copy link
Contributor

@clintonb clintonb Mar 9, 2017

Choose a reason for hiding this comment

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

Also, split this into two separate test cases, or use ddt to achieve a similar result.

course_discovery/apps/api/v1/views/catalogs.py Outdated

if course_run_ids:
course_run_ids = course_run_ids.split(',')
course_runs = CourseRun.search(catalog.query).filter(key__in=course_run_ids).values_list('key', flat=True)
Copy link
Contributor

@clintonb clintonb Mar 9, 2017

Choose a reason for hiding this comment

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

Move this to a method on the Catalog model, similar to Catalog.contains().

@zubair-arbi zubair-arbi mentioned this pull request Mar 9, 2017
Merged
@zubair-arbi zubair-arbi force-pushed the zub/ENT-201-course_run_id_in_catalog_contains_endpoint branch 2 times, most recently from 8d77bdb to 5e4aaab Compare March 9, 2017 13:13
@zubair-arbi
Copy link
Contributor Author

zubair-arbi commented Mar 9, 2017

@clintonb Please have another look

clintonb
clintonb approved these changes Mar 9, 2017
View reviewed changes
course_discovery/apps/api/v1/tests/test_views/test_catalogs.py Outdated
self.assertEqual(catalog.query, query)
self.assertListEqual(list(catalog.viewers), [viewer])

def _assert_catalog_contains(self, catalog_contains_query_string, course_key):
Copy link
Contributor

@clintonb clintonb Mar 9, 2017

Choose a reason for hiding this comment

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

  1. Call catalog_contains_query_string query_string_kwargs, which should be a dict.
  2. Add query_string = urllib.parse.urlencode(query_string_kwargs) to de-dupe.

course_discovery/apps/catalogs/tests/test_models.py Outdated
""" Verify the method returns a mapping of course run IDs to booleans. """
partner = PartnerFactory()
course_run = CourseRunFactory(course__partner=partner, course=self.course)
uncontained_course = CourseFactory(key='d/e/f', title='ABDEF')
Copy link
Contributor

@clintonb clintonb Mar 9, 2017

Choose a reason for hiding this comment

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

This line is not necessary.

course_discovery/apps/catalogs/tests/test_models.py Outdated
def test_contains_course_runs(self):
""" Verify the method returns a mapping of course run IDs to booleans. """
partner = PartnerFactory()
course_run = CourseRunFactory(course__partner=partner, course=self.course)
Copy link
Contributor

@clintonb clintonb Mar 9, 2017

Choose a reason for hiding this comment

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

course__partner is never used since you pass course.

course_discovery/apps/catalogs/tests/test_models.py Outdated
partner = PartnerFactory()
course_run = CourseRunFactory(course__partner=partner, course=self.course)
uncontained_course = CourseFactory(key='d/e/f', title='ABDEF')
uncontained_course_run = CourseRunFactory(course__partner=partner, course=uncontained_course)
Copy link
Contributor

@clintonb clintonb Mar 9, 2017

Choose a reason for hiding this comment

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

uncontained_course_run = CourseRunFactory(title_override='ABD')

@zubair-arbi zubair-arbi force-pushed the zub/ENT-201-course_run_id_in_catalog_contains_endpoint branch 2 times, most recently from 5f8488c to 5a7633d Compare March 10, 2017 07:23
@zubair-arbi
Copy link
Contributor Author

zubair-arbi commented Mar 10, 2017

@clintonb thanks for the valuable feedback. I have addressed your suggestions.
@mattdrayer please give review.
These changes should give live before its related changes in Ecom - use catalog contains endpoint for varifying the course seats against a course catalog range.

@mattdrayer mattdrayer changed the title add support for course run key in catalog contains endpoint ENT-201: Add support for course run key in catalog contains endpoint Mar 10, 2017
@mattdrayer
Copy link
Contributor

mattdrayer commented Mar 13, 2017

Thanks for taking care of this @zubair-arbi -- LGTM 👍

@zubair-arbi zubair-arbi force-pushed the zub/ENT-201-course_run_id_in_catalog_contains_endpoint branch from 5a7633d to 6193d0a Compare March 13, 2017 12:39
@zubair-arbi zubair-arbi merged commit 5f25faf into master Mar 13, 2017
@zubair-arbi zubair-arbi deleted the zub/ENT-201-course_run_id_in_catalog_contains_endpoint branch March 13, 2017 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@clintonb clintonb clintonb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zubair-arbi @mattdrayer @clintonb