Staff and instructor users of the master course are added to CCX created via REST APIs - #207
Staff and instructor users of the master course are added to CCX created via REST APIs#207giocalitri wants to merge 2 commits into
Conversation
| for course_user, ccx_user in izip(sorted(list_staff_master_course), sorted(list_staff_ccx_course)): | ||
| self.assertEqual(course_user, ccx_user) | ||
| self.assertEqual(len(list_instructor_master_course), len(list_instructor_ccx_course)) | ||
| for course_user, ccx_user in izip(sorted(list_staff_master_course), sorted(list_staff_ccx_course)): |
There was a problem hiding this comment.
It looks like you already did this comparison in the previous for loop
|
Can we mention in the title that this is for the REST API? |
giocalitri
changed the title
|
@pdpinch: done |
| with ccx_course(self.ccx_locator) as course_ccx: | ||
| list_staff_ccx_course = list_with_level(course_ccx, 'staff') | ||
| list_instructor_ccx_course = list_with_level(course_ccx, 'instructor') | ||
| self.assertEqual(len(list_staff_master_course), len(list_staff_ccx_course)) |
There was a problem hiding this comment.
Is it possible to compare the lists directly instead of the count, or is that cumbersome?
There was a problem hiding this comment.
Actually it looks like you could just say
self.assertEqual(sorted(list_staff_master_course), sorted(list_staff_ccx_course))
And you could remove the next couple lines as well
There was a problem hiding this comment.
I actually tried and for some reason it was failing
|
Looks good, feel free to ignore my comment about the test if it's complicated to do 👍 |
| self.assertEqual(result, ccx) | ||
|
|
||
|
|
||
| class TestStaffOnCCX(CcxTestCase, SharedModuleStoreTestCase): |
There was a problem hiding this comment.
Why did you remove SharedModuleStoreTestCase ? Doesn't that make the tests faster?
There was a problem hiding this comment.
because CcxTestCase already subclasses it:
https://github.com/mitocw/edx-platform/blob/master/lms/djangoapps/ccx/tests/utils.py#L26
|
Was this merged in edX already? |
0c604da to
65f09e0
Compare
|
closing this because included in some work @amir-qayyum-khan already merged in edx |
3 participants
What are the relevant tickets?
fixes #205
What's this PR do?
Adds the master course staff users to a new CCX created via the CCX REST APIs
Where should the reviewer start?
The added functionality is in
lms.djangoapps.ccx.api.v0.views, but there has been a significant change of code inlms.djangoapps.ccx.utils.How should this be manually tested?
A staff user on a master course should be able to view and access a CCX created via REST API using the normal dashboard.
Any background context you want to provide?
The added functionality of this PR is the equivalent of what https://github.com/edx/edx-platform/pull/10877 did with
lms.djangoapps.ccx.views.What gif best describes this PR or how it makes you feel?