Enroll the coach in the CCX on creation #10251

jamiefolsom · 2015-10-19T14:41:16Z

Problem
When a CCX coach first accesses the course, and clicks CCX Coach link on the dashboard, she is invited to give her CCX course a name, and doing so creates the CCX course.

Two things that should happen at that point don't, namely:

The coach is not enrolled in the CCX course;
The coach is not granted staff access to the course. EDIT: the scope of this PR has narrowed, and now no longer includes a solution for this problem.

Solution
This PR fixes those issues at the time that the CCX course is created, by enrolling the coach in the course. ~~and granting that coach staff access to the course.~~

How to test

Assuming that CCX is enabled:

While logged out, register as a new user.
As a logged-in instructor:
- Visit the membership view in the LMS
- Add the new user created in the first step as a CCX coach.
As the logged-in new user:
- Refresh your dashboard.
- Visit the CCX coach tab
- Give your CCX course a name and click create.
- Note that the new user is present in the list of enrolled users.~~, and has staff access to the course (ie, can access the grading, student admin and scheduling views within the course dashboard).~~

Fixes mitocw#36

openedx-webhooks · 2015-10-19T14:41:21Z

Thanks for the pull request, @jamiefolsom! I've created OSPR-884 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

supporting documentation
edx-code email threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.
We can't start reviewing your pull request until you've added yourself to the AUTHORS file . Please see the CONTRIBUTING file for more information.

sarina · 2015-10-19T15:30:24Z

lms/djangoapps/ccx/tests/test_views.py

+        pattern = "http://[^\/]+/courses/([^\/]+)/ccx_coach"
+        results = re.match(pattern, url)
+        ccx_key = results.group(1)
+        course_key = SlashSeparatedCourseKey.from_deprecated_string(ccx_key)


You shouldn't be using this syntax anymore, I don't think. @dianakhuang could you hlep out here? Shouldn't it just be CourseKey.from_string?

giocalitri · 2015-10-19T16:50:54Z

jenkins run quality

sarina · 2015-10-21T15:35:08Z

lms/djangoapps/ccx/tests/test_views.py

+        pattern = re.compile(r"http://[^\/]+/courses/([^\/]+)/ccx_coach")
+        results = re.match(pattern, url)
+        ccx_key = results.group(1)
+        course_key = SlashSeparatedCourseKey.from_deprecated_string(ccx_key)


You shouldn't be using this syntax anymore, I don't think. @dianakhuang could you help out here? Shouldn't it just be CourseKey.from_string?

Yup, this should be using CourseKey.from_string instead of using SlashSeparatedCourseKey.from_deprecated_string

jamiefolsom · 2015-10-21T20:48:41Z

All checks pass apart from bokchoy, which appears to be an unrelated failure. Still hunting for a better way to get the CCX identifier from a URL, which I need to do in the views test suite.

jamiefolsom · 2015-10-22T15:37:03Z

jenkins run lettuce

sarina · 2015-10-22T15:54:58Z

lms/djangoapps/ccx/tests/test_views.py

+
+        pattern = re.compile(r"http://[^\/]+/courses/([^\/]+)/ccx_coach")
+        results = re.match(pattern, url)
+        ccx_key = results.group(1)


@ormsbee could you do a quick drive-by here? I don't think regular expressions can possibly be the best way to get the ccx key out of this URL.

django.core.urlresolvers.resolve() maybe?

Hi, @ormsbee! I think misunderstood @sarina. I thought there was a convention within the platform for accessing the course key. Having looked at the issue and your proposed improvement, I think I've got a solution, using urlparse and os.path if that works for you guys. Pushing a commit now. Thanks!

ormsbee · 2015-10-23T17:16:18Z

lms/djangoapps/ccx/tests/test_views.py

+
+        # Then get the second to last segment of the path
+        head, tail = os.path.split(path)
+        head, ccx_key = os.path.split(head)


The reason I suggested you use resolve() when parsing URLs is that it's going to be more robust than manually parsing it yourself, especially if we decide to move URLs around. As a somewhat silly example, say we try to run this on Windows, where the path separator is a \ instead of a /. If you're parsing a URL, you might as well use exactly Django's logic for doing so.

>>> from django.core.urlresolvers import resolve >>> resolver = resolve("/courses/ccx-v1:DavidsonNext+Cal_APccx_Edge+3T2015+ccx@114/ccx_coach") >>> resolver.kwargs['course_id'] 'ccx-v1:DavidsonNext+Cal_APccx_Edge+3T2015+ccx@114'

OK, that makes sense. Will do. Thanks!

I think I still need to parse the path out of the url using urlparse, otherwise resolve(url) returns a 404 (since the url contains http://testserver/). But with that done, resolve(path) works nicely on the path. Thanks for your help with this, @ormsbee and @sarina. I just pushed a change for your review.

jamiefolsom · 2015-10-23T19:25:04Z

jenkins run quality

jamiefolsom · 2015-10-26T15:19:17Z

@sarina @ormsbee @pdpinch FYI, after some discussion of desirable coach permissions, narrowed the scope of the PR; now we don't grant staff access to the coach.

sarina · 2015-10-26T15:32:54Z

@jamiefolsom can you update the title of this PR, as well as your commit message, to indicate what this PR is actually doing?

jamiefolsom · 2015-10-26T15:34:38Z

Hi @sarina, absolutely.

Make the course URL pattern more generic. Comment newly added functionality. Fix quality issues. Address two lint errors, with regex and variable naming. Changed how we access course_key and course_id. Replace another instance of self.course.id.to_deprecated_string() Remove unused import, add missing one. Improve how the ccx key is extracted from the URL Use resolve() instead of os.path to get the course_id. Remove the granting of staff access to coaches.

sarina · 2015-10-26T15:58:31Z

👍 once commit message is updated.

@jamiefolsom can you make sure someone on your team provides a thumbs-up to this as well?

jamiefolsom · 2015-10-26T16:03:40Z

Sure, will do. @pdpinch, all checks appear to be passing. I just amended the commit message per @sarina's request and rebased again, though, so maybe when this is all green again, you could take another look? Thanks!

sarina · 2015-10-26T22:23:16Z

Great, tests pass. 👍 and please feel free to merge once someone on your side has given approval.

jamiefolsom · 2015-10-27T13:38:04Z

Hey @pdpinch -- looks like this is good to merge, after your thumbs up.

pdpinch · 2015-10-27T16:42:16Z

I've reviewed this again after the roles change 👍

Enroll the coach in the CCX on creation

jamiefolsom · 2015-10-27T16:44:22Z

Cool, thanks @pdpinch and @sarina!

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage labels Oct 19, 2015

jamiefolsom force-pushed the fix/jf/enroll-coach-on-create-ccx branch from 66b9c91 to f6432e9 Compare October 19, 2015 15:20

sarina reviewed Oct 19, 2015
View reviewed changes

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Oct 19, 2015

jamiefolsom force-pushed the fix/jf/enroll-coach-on-create-ccx branch 4 times, most recently from a52307d to dd030e8 Compare October 21, 2015 14:44

sarina reviewed Oct 21, 2015
View reviewed changes

jamiefolsom force-pushed the fix/jf/enroll-coach-on-create-ccx branch 2 times, most recently from c766b11 to 3607756 Compare October 21, 2015 19:51

jamiefolsom force-pushed the fix/jf/enroll-coach-on-create-ccx branch from 3607756 to f21ad82 Compare October 21, 2015 20:51

jamiefolsom mentioned this pull request Oct 21, 2015

When creating a CCX, automatically enroll the coach mitocw/edx-platform#36

Closed

sarina reviewed Oct 22, 2015
View reviewed changes

openedx-webhooks added community manager review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Oct 22, 2015

jamiefolsom force-pushed the fix/jf/enroll-coach-on-create-ccx branch from f21ad82 to 95339a0 Compare October 23, 2015 16:44

ormsbee reviewed Oct 23, 2015
View reviewed changes

jamiefolsom force-pushed the fix/jf/enroll-coach-on-create-ccx branch from 95339a0 to 17709ec Compare October 23, 2015 18:52

jamiefolsom force-pushed the fix/jf/enroll-coach-on-create-ccx branch from 17709ec to f3cd9e6 Compare October 26, 2015 15:11

jamiefolsom changed the title ~~Enroll the coach and grant her staff access to the CCX on creation~~ Enroll the coach in the CCX on creation Oct 26, 2015

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed community manager review labels Oct 26, 2015

jamiefolsom force-pushed the fix/jf/enroll-coach-on-create-ccx branch from f3cd9e6 to a3a7ff2 Compare October 26, 2015 15:59

openedx-webhooks added open edx community review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Oct 26, 2015

pdpinch added a commit that referenced this pull request Oct 27, 2015

Merge pull request #10251 from mitocw/fix/jf/enroll-coach-on-create-ccx

cda95d7

Enroll the coach in the CCX on creation

pdpinch merged commit cda95d7 into openedx:master Oct 27, 2015

pdpinch deleted the fix/jf/enroll-coach-on-create-ccx branch October 27, 2015 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enroll the coach in the CCX on creation #10251

Enroll the coach in the CCX on creation #10251

jamiefolsom commented Oct 19, 2015

openedx-webhooks commented Oct 19, 2015

sarina Oct 19, 2015

giocalitri commented Oct 19, 2015

sarina Oct 21, 2015

dianakhuang Oct 21, 2015

jamiefolsom Oct 21, 2015

jamiefolsom commented Oct 21, 2015

jamiefolsom commented Oct 22, 2015

sarina Oct 22, 2015

ormsbee Oct 22, 2015

jamiefolsom Oct 23, 2015

ormsbee Oct 23, 2015

jamiefolsom Oct 23, 2015

jamiefolsom Oct 23, 2015

jamiefolsom commented Oct 23, 2015

jamiefolsom commented Oct 26, 2015

sarina commented Oct 26, 2015

jamiefolsom commented Oct 26, 2015

sarina commented Oct 26, 2015

jamiefolsom commented Oct 26, 2015

sarina commented Oct 26, 2015

jamiefolsom commented Oct 27, 2015

pdpinch commented Oct 27, 2015

jamiefolsom commented Oct 27, 2015

Enroll the coach in the CCX on creation #10251

Enroll the coach in the CCX on creation #10251

Conversation

jamiefolsom commented Oct 19, 2015

openedx-webhooks commented Oct 19, 2015

Choose a reason for hiding this comment

giocalitri commented Oct 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamiefolsom commented Oct 21, 2015

jamiefolsom commented Oct 22, 2015

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

jamiefolsom commented Oct 23, 2015

jamiefolsom commented Oct 26, 2015

sarina commented Oct 26, 2015

jamiefolsom commented Oct 26, 2015

sarina commented Oct 26, 2015

jamiefolsom commented Oct 26, 2015

sarina commented Oct 26, 2015

jamiefolsom commented Oct 27, 2015

pdpinch commented Oct 27, 2015

jamiefolsom commented Oct 27, 2015