Multi-site aware enrollments and courses API by johnbaldwin · Pull Request #397 · appsembler/edx-platform

johnbaldwin · 2019-06-15T02:19:30Z

Up for PR is the site aware enrollment API and course API code

Included

Courses API

code for the courses api + unit tests
** GET a list of courses for the given site + basic data: 'id', 'display_name', 'org'
enrollment api
** GET a record view of a given course. Same serialized data as for in the list

This is a starting point and provides the minimum essential information so API users can get course identifiers. We can build on more functionality from here as needed

Enrollment API

Code for the enrollments API + unit tests
** POST bulk enrollment using the same interface as for Ginkgo:
*** https://github.com/appsembler/edx-platform/blob/appsembler/ginkgo/master/lms/djangoapps/appsembler_api/views.py#L377-L403
*** https://github.com/appsembler/edx-platform/blob/appsembler/ginkgo/master/lms/djangoapps/appsembler_api/apidocs.md#enrollment-codes-endpoints
** GET list of all course enrollments for the site
** GET list of all course enrollments for a given course

NOT INCLUDED

POST unenroll for the enrollment API

…very close

johnbaldwin · 2019-06-15T02:22:00Z

(8) Implement site aware courses api for Tahoe enrollment api

OmarIthawi

Thanks @johnbaldwin, I'll take another look soon, but for now here's a quick review.

OmarIthawi · 2019-06-16T16:51:26Z

+    return courses
+
+
+def get_site_for_course(course_id):


I've seen this helper existing somewhere in Tahoe. I should double check.

@OmarIthawi I'll bet one of the places you've seen it is in figures ;)

But if you happen to see it in the platform (w/out going out of your way), then we can see about using that one

It's a minor issue. Just keep it for now, thanks @johnbaldwin!

OmarIthawi · 2019-06-16T16:56:54Z

+
+COURSE_ID_STR_TEMPLATE = 'course-v1:StarFleetAcademy+SFA{}+2161'
+
+class CourseOverviewFactory(factory.DjangoModelFactory):


Can we reuse from?

edx-platform/openedx/core/djangoapps/content/course_overviews/tests/factories.py

Lines 10 to 33 in fa7b9a1

class CourseOverviewFactory(DjangoModelFactory):

class Meta(object):

model = CourseOverview

django_get_or_create = ('id', )

version = CourseOverview.VERSION

pre_requisite_courses = []

org = 'edX'

@factory.lazy_attribute

def _pre_requisite_courses_json(self):

return json.dumps(self.pre_requisite_courses)

@factory.lazy_attribute

def _location(self):

return self.id.make_usage_key('course', 'course')

@factory.lazy_attribute

def id(self):

return CourseLocator(self.org, 'toy', '2012_Fall')

@factory.lazy_attribute

def display_name(self):

return "{} Course".format(self.id)

After I get everything together, I'll drop that in and see if there's any extra work needed

@OmarIthawi No we can't without reworking tests to explicitly generate course ids since this factory has a static course id.

This means no more self.course_overviews = [CourseOverviewFactory() for i in range(X)]

Instead we either need to create a course id generator OR modify the course_overviews CourseOverviewFactory, which I call out of scope.

There may be other issues. So for now, I want to leave it with our custom one. This does give the option to see if we can enhance the default platform CourseOverviewFactory and push upstream, but we'd have to test everything that uses it.

Sorry, I'm calling this one out of scope.

Agreed with @johnbaldwin , despite how much we do like re-utilise, this one is out of scope.

OmarIthawi · 2019-06-16T16:58:19Z

+    SiteFactory,
+)
+
+from .factories import (


Please use fully qualified paths such as:

from openedx.core.djangoapps.appsembler.api.tests.factories import ...

@OmarIthawi To make you happy I made these absolute paths. Something I'm interested, and please pardon my ignorance. What is the compelling reason not to use explicit relative path imports using the leading dot, which means one is importing from the same package. Is there some definitive reference you can share? Thanks!

https://realpython.com/absolute-vs-relative-python-imports/

Generally speaking, absolute imports are recommended (PEP 8!). Relative imports are more succinct, but they can cause issues when you refactor, and make it harder to quickly find the other module, especially if you start using ...

We don't have a style guide (well, we have a PR that's outstanding that you submitted, John -- we should all revisit that), but if we did, absolute imports should be in there.

(At least py3 did away with implicit relative imports, which were not great.)

OmarIthawi · 2019-06-20T06:51:42Z

@johnbaldwin sounds like you're doing a great progress here! Thank you! Could you please let me know if you think it'll need another round of review?

…i using existing code

johnbaldwin · 2019-06-26T18:30:16Z

(21) Implement enrollment api v1 in edx-platform

johnbaldwin · 2019-06-26T21:36:28Z

+
+from django.test import RequestFactory, TestCase
+from django.test.utils import override_settings
+


cleanup whitespace

johnbaldwin · 2019-06-26T21:38:13Z

+    org = 'StarFleetAcademy'
+    # number = '2161'
+
+    version = 6L


Replace with version = CourseOverview.VERSION as done in course_overviews factory class

johnbaldwin · 2019-06-26T21:38:22Z

+        COURSE_ID_STR_TEMPLATE.format(n)))
+    display_name = factory.Sequence(lambda n: 'SFA Course {}'.format(n))
+    org = 'StarFleetAcademy'
+    # number = '2161'


remove dead code

johnbaldwin · 2019-06-26T21:39:00Z

+                                           organization=self.my_site_org)
+
+        self.other_enrollments = [CourseEnrollmentFactory()]
+        # self.other_course_overviews = [CourseOverviewFactory()]


johnbaldwin · 2019-06-26T21:40:06Z

+        self.assertEqual(res.status_code, 200)
+        enroll_list = res.data['results']
+        self.assertEqual(len(enroll_list), len(self.my_enrollments))
+        # TODO: Validate each record


Will do as follow-on (Along with other test improvements). My primary focus was getting test coverage and with that getting good test coverage on the bulk enrollment code (see next test class)

johnbaldwin · 2019-06-26T21:44:13Z

+
+    def setUp(self):
+        super(EnrollmentApiPostTest, self).setUp()
+        self.my_site = Site.objects.get(domain=u'example.com')


@OmarIthawi @melvinsoft @abeals

Note: We have redundant fixture code in this class and the previous. What I want to do is shift away from TestClass based tests to pytest based tests, then build a set of fixtures to reduce tests data set up duplication. With a healthy set of fixtures, we can also get away from class based tests toward function based tests.

The more I develop tests here, the less I see the value in class based tests as the test module provides a strong enough context and function based tests help with test development velocity and readability

Worth a broader discussion within our engineering team. An advantage to class-based tests (not an overriding advantage, but one you can use) is that you can use mix-ins to keep your test cases DRY. I've generally found inheritance in class-based tests to be more of a headache than it's worth, but the mix-ins can be nice.

@abeals This is where Pytest fixtures and using conftest.py can really shine for reuse and clarity: https://docs.pytest.org/en/latest/fixture.html

Effectively, adding fixture methods as parameters to a test function is an analog to using mixins in class based tests.

However, I think more important than the fixture based vs class mixin based is working to reduce duplication and making the test code clear to read

johnbaldwin · 2019-06-26T21:46:10Z

+            organization=self.other_site_org).count()
+
+        assert after_other_site_ce_count == before_other_site_ce_count
+        assert after_other_site_user_count == before_other_site_user_count


The above two asserts make sure that we're not creating resources in another site. We'll be able to validate that these tests work when we run on staging/simulated production environment.

TODO: Add assert for 'other_site' CourseEnrolledAllowed does not change

Got it. We should split that out into its own test case. A test case should be testing one thing, not multiple things. This one's become a bit of the latter.

Not a blocker for merging this PR, but please do flag this refactor as a TODO.

Added "TODO" in docstring with inline comment about this assertion

abeals · 2019-06-27T01:51:50Z

+_This section is a WIP_
+
+```
+pytest openedx/core/djangoapps/appsembler/api/tests/test_registration_api.py


Assuming this is just an example of usage, right? Not meant to be a full listing of all of the tests in openedx/core/djangoapps/appsembler/api/tests/ ?

Yes, just an example. But I'll update the readme to be clear

Updated to say this is an example of running an individual test. See no need to spell out each test here

abeals · 2019-06-27T02:29:42Z

+# from django.utils.timezone import utc
+
+import factory
+from factory import fuzzy


More curiosity than anything else: Why did you choose to import the fuzzy submodule, but for all other submodules in factory you referred to them via factory.Sequence, et al?

@abeals No good reason. I did it because I copied/pasted from Figures. There might have been a reason I needed to do it for figures, but I don't recall. I'll do just the import factory and see if that works ok.

Updated to use factory.fuzzy

abeals · 2019-06-27T02:30:58Z

-import factory

+import datetime
+# from django.utils.timezone import utc


abeals · 2019-06-27T02:33:56Z

+class CourseApiTest(TestCase):
+
+    def setUp(self):
+        self.my_site = Site.objects.get(domain=u'example.com')


Any reason you're using an existing site vs SiteFactory for self.my_site?

@abeals Originally because that is the default site in the request object when using the TestCase class's builtin test client. However, I ran into auth issues when running our new code that called existing edx-platform code for enrollment, so I wound up doing it the way I did in figures.

The test environment always creates the default site. I can change it to use SiteFactory. It should behave the same. And would probably be a good test to make sure there's nothing I've overlooked

Updated to use `SiteFactory and tests pass, so keeping the change. Thanks @abeals !

Noted. I should update the test_sites.py module to be more robust. Added TODO in module docstring

Hmm, ran the test again and failed after updating other tests to replace example.com with SiteFactory and running the suite of tests. Then ran just the test_course_api.py and now that fails. Hmm. Investigating

Hmm. That first time the test_course_api.py run SHOULD have failed. Think there was a docker FS issue where my host changes didn't make it to the docker container FS.

Reworking tests to fix...

This one is becoming interesting. Trying to use reverse and resolve to get the viewset object from the namespace, however, running into an error where the kwargs, which should contain the pk, are empty when the viewset retrieve method is called.

However, before calling the the view func with the request, we DO have the key for the detail view. It just seems to be getting lost in the call chain somehow. See:

(Pdb) view ResolverMatch(func=openedx.core.djangoapps.appsembler.api.v1.views.CourseViewSet, args=(), kwargs={u'pk': u'course-v1:StarFleetAcademy+SFA0+2161'}, url_name=courses-detail, app_names=[], namespaces=['tahoe-api', 'v1']) (Pdb) request <WSGIRequest: GET '/tahoe/api/v1/courses/course-v1:StarFleetAcademy+SFA0+2161/'>

In the viewset retrieve method:

(Pdb) args self = <openedx.core.djangoapps.appsembler.api.v1.views.CourseViewSet object at 0x7f316bf97fd0> request = <rest_framework.request.Request object at 0x7f314b31fd10> args = () kwargs = {}

On the surface, it appears that the call chain is dropping the PK.

Here's the stack:

/edx/app/edxapp/edx-platform/openedx/core/djangoapps/appsembler/api/tests/test_course_api.py(95)test_get_single() -> response = view.func(request) /edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/views/decorators/csrf.py(58)wrapped_view() -> return view_func(*args, **kwargs) /edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/rest_framework/viewsets.py(86)view() -> return self.dispatch(request, *args, **kwargs) /edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/rest_framework/views.py(486)dispatch() -> response = handler(request, *args, **kwargs) > /edx/app/edxapp/edx-platform/openedx/core/djangoapps/appsembler/api/v1/views.py(248)retrieve()

Dang! Silly mistake. I was missing the pk keyword from the view func call.

even though you provide the url with the key in the url:

url = reverse('tahoe-api:v1:courses-detail', args=[course_id]) request = APIRequestFactory().get(url)

You still need to give it to the view func because Django doesn't do this magic

response = view.func(request, pk=course_id)

abeals · 2019-06-27T03:11:25Z

+
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
+from xmodule.modulestore.django import modulestore


Thanks, will remove

abeals · 2019-06-27T03:18:47Z

+            organization=self.other_site_org).count()
+
+        assert after_other_site_ce_count == before_other_site_ce_count
+        assert after_other_site_user_count == before_other_site_user_count


Got it. We should split that out into its own test case. A test case should be testing one thing, not multiple things. This one's become a bit of the latter.

Not a blocker for merging this PR, but please do flag this refactor as a TODO.

abeals · 2019-06-27T03:24:12Z

@@ -0,0 +1,11 @@
+"""Paginatiors for Figures


for... Figures? :)

@abeals Yeah, a good chunk of the new code I copied over and adapted from figures. Will fix. Thx!

abeals · 2019-06-27T03:24:44Z

+class TahoeLimitOffsetPagination(LimitOffsetPagination):
+    '''Custom Tahoe paginator to make the number of records returned consistent
+    '''
+    default_limit = 20


Knowing nothing about the standards here... why 20?

It was an arbitrary number. Copied over from Figures. I chose it because it was twice the size of the default edx-platform value: https://github.com/appsembler/edx-platform/blob/appsembler/tahoe/master/lms/envs/common.py#L2343

I thought 10 was too low and that 20 was a good starting point to see how responsive the api would be.

Good for me to document this point. Thanks!

Updated class docstring to reflect why '20' was chosen

abeals · 2019-06-27T03:28:38Z

+        replaced with spaces, so we need to put the '+' back in for CourseKey
+        to be able to create a course key object from the string
+        '''
+        course_key = CourseKey.from_string(value.replace(' ', '+'))


don't we have an as_course_key helper method as part of this PR? why not use that?

Good point! Why? Copied code over from code not refactored. Will refactor

Updated to use as_course_key

abeals · 2019-06-27T03:31:11Z

+        course_keys = value
+        for course in course_keys:
+            try:
+                CourseKey.from_string(course)


As above, what's the difference between this and the helper method as_course_key? The latter seems to have some typing checking as a safety mechanism. Why do we use it in some cases and direct CourseKey.from_string() in others?

Please see my reply to your prior question. Will use as_course_key

Updated. Will now raise serializers.ValidationError on any exception raised when trying to convert to a CourseKey instance

johnbaldwin · 2019-06-28T22:14:43Z

@abeals Thanks for reviewing and your comments. A common theme: "rolling tech debt" as I copied code over from Figures that I know works. This brings the topic of what our future architectural strategy will be for common multisite handling code

…the comment on using the default site

…igate

johnbaldwin · 2019-07-01T03:58:53Z

@OmarIthawi @melvinsoft @abeals Updated added. Please review. If all is goo enough, we can merge and test on staging

melvinsoft

@johnbaldwin Thanks for putting together! The implementation is top notch. I just have some questions and requests.

melvinsoft · 2019-07-01T15:50:24Z

+    org_courses = OrganizationCourse.objects.filter(course_id=str(course_id))
+    if org_courses:
+        # Keep until this assumption analyzed
+        msg = 'Multiple orgs found for course: {}'


@johnbaldwin I don't fully understand how the messages are being managed, where do we sent them? Please clarify, thanks!

melvinsoft · 2019-07-01T15:51:57Z

+
+COURSE_ID_STR_TEMPLATE = 'course-v1:StarFleetAcademy+SFA{}+2161'
+
+class CourseOverviewFactory(factory.DjangoModelFactory):


Agreed with @johnbaldwin , despite how much we do like re-utilise, this one is out of scope.

melvinsoft · 2019-07-01T16:24:29Z

+        if serializer.is_valid():
+            # TODO: Wrap in transaction
+            # TODO: trap error on each attempt and log
+            # IMPORTANT: THIS IS A WIP to get this working quickly


@johnbaldwin Please clarify if is still a WIP, the WIP part has been removed from the PR title already. Thanks!

johnbaldwin · 2019-07-02T01:34:42Z

@melvinsoft I added invalid/wrong site course ids checking to the bulk enrollment POST along with a helper method course_belongs_to_site in the sites module, along with tests for both.

cc @OmarIthawi

melvinsoft

@johnbaldwin Thanks for implementing the requested changes! There still some pending comments from Omar and I, please address them, but I think we're ready for staging now.

I'm approving the PR.

johnbaldwin added 8 commits June 4, 2019 13:15

Enable Django REST Framework browsable API for docker devstack

3928df0

WIP Tahoe API courses resource

4abd348

Merge branch 'appsembler/tahoe/master' into john/tahoe-api-courses

508d7d6

WIP appsembler api courses endpoint testing

4800a68

Got course api and site module tests working

eb5c077

Course list view test fails. Only one course is returned. Otherwise, …

6e71c44

…very close

Tahoe API v1 courses endpoint with some basic test coverage

cb58235

Merge branch 'appsembler/tahoe/develop' into john/tahoe-api-courses

016be2c

johnbaldwin requested review from OmarIthawi and melvinsoft June 15, 2019 02:20

johnbaldwin added 2 commits June 14, 2019 23:23

test_course_api module docstring fix

393dbcb

Tahoe api sites module cleanup

bf4cda4

OmarIthawi reviewed Jun 16, 2019

View reviewed changes

OmarIthawi self-requested a review June 16, 2019 17:02

johnbaldwin added 10 commits June 16, 2019 16:02

Tahoe API updated sites and test_sites for enrollments

5e97bd2

Tahoe API WIP enrollments

2a43893

WIP Tahoe enrollment api

b386b88

Tahoe API cleanup code for test_course_api

4db632e

Tahoe API - fixed typo in v1.urls

646624a

Tahoe enrollment api - added stub for enrollment create method

d3b807b

Fixed missing import in tahoe api v1 filters

dbd89ac

Tahoe API WIP course enrollment api tests

08e7434

Tahoe API serializer - Add BulkEnrollmentSerializer

8abe180

WIP Tahoe enrollment api

94c3def

OmarIthawi changed the title ~~(WIP) Enrollments API~~ (WIP) Multi-site aware enrollments and Courses API Jun 20, 2019

OmarIthawi changed the title ~~(WIP) Multi-site aware enrollments and Courses API~~ (WIP) Multi-site aware enrollments and courses API Jun 20, 2019

johnbaldwin added 2 commits June 21, 2019 13:47

WIP Tahoe API - enrollment

b796db5

tahoe api cleanup

e82a02d

johnbaldwin added 5 commits June 24, 2019 13:43

WIP Tahoe API enrollment, added modulestore support for enrollment ap…

4640972

…i using existing code

WIP enrollment api

10cd36b

WIP Tahoe enrollment api test

fa47a32

WIP tahoe api enrollment - wrapping up testing

86f2194

Tahoe API enrollment API tests now passing

8451df9

johnbaldwin changed the title ~~(WIP) Multi-site aware enrollments and courses API~~ Multi-site aware enrollments and courses API Jun 26, 2019

abeals self-requested a review June 26, 2019 18:40

Updated tests to use absolute path for 'factories' import

ee157db

johnbaldwin commented Jun 26, 2019

View reviewed changes

abeals reviewed Jun 27, 2019

View reviewed changes

johnbaldwin added 5 commits June 30, 2019 20:36

Updated test instructions in readme

367fd42

Tahoe API - replaced relative imports with absolute imports

49525be

Tahoe enrollment api - addressed PR comments for PR #397, except for …

c540bee

…the comment on using the default site

Updated site handling in tests per PR #397 comments

4ed2561

Disabled throttling test. Getting a transaction error. Need to invest…

d7c85d5

…igate

melvinsoft suggested changes Jul 1, 2019

View reviewed changes

Tahoe enrollment api - Added invalid site check in enrollment POST

693118f

melvinsoft approved these changes Jul 2, 2019

View reviewed changes

johnbaldwin merged commit 7fbb26a into appsembler/tahoe/develop Jul 2, 2019


		COURSE_ID_STR_TEMPLATE = 'course-v1:StarFleetAcademy+SFA{}+2161'

		class CourseOverviewFactory(factory.DjangoModelFactory):

	class CourseOverviewFactory(DjangoModelFactory):
	class Meta(object):
	model = CourseOverview
	django_get_or_create = ('id', )

	version = CourseOverview.VERSION
	pre_requisite_courses = []
	org = 'edX'

	@factory.lazy_attribute
	def _pre_requisite_courses_json(self):
	return json.dumps(self.pre_requisite_courses)

	@factory.lazy_attribute
	def _location(self):
	return self.id.make_usage_key('course', 'course')

	@factory.lazy_attribute
	def id(self):
	return CourseLocator(self.org, 'toy', '2012_Fall')

	@factory.lazy_attribute
	def display_name(self):
	return "{} Course".format(self.id)


		from django.test import RequestFactory, TestCase
		from django.test.utils import override_settings

Uh oh!

Conversation

johnbaldwin commented Jun 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Included

Courses API

Enrollment API

NOT INCLUDED

Uh oh!

johnbaldwin commented Jun 15, 2019

Uh oh!

OmarIthawi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnbaldwin Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OmarIthawi commented Jun 20, 2019

Uh oh!

johnbaldwin commented Jun 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnbaldwin Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnbaldwin Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnbaldwin commented Jun 15, 2019 •

edited

Loading

johnbaldwin Jun 24, 2019 •

edited

Loading

johnbaldwin Jun 28, 2019 •

edited

Loading

johnbaldwin Jul 1, 2019 •

edited

Loading

johnbaldwin Jul 1, 2019 •

edited

Loading