Use the standard enrollment table instead of a custom CCX table

[pdpinch]

Depends on #43

[cewing]

Now that the custom opaque-key implementation for ccx is in place, this should be approachable.

[cguardia]

Ok, I will be taking care of this issue. I'm not familiar with it, so first I'll review the code and see what's required.

[pdpinch]

Dave Ormsbee keeps claiming that all we need to do is remove the code for the custom CCX enrollment table.

On Jun 29, 2015, at 2:19 PM, Carlos de la Guardia [email protected] wrote:

Ok, I will be taking care of this issue. I'm not familiar with it, so first I'll review the code and see what's required.

—
Reply to this email directly or view it on GitHub #78 (comment).

[cguardia]

Started work on this. After removing membership models, there are a bunch of tests that need fixing, so I'm doing that now.

[cguardia]

In addition to the tests, I've been having to rewire functionality from the enrollment pages that required queries to the ccx membership tables. So it was not just a matter of removing the models, because they power up the whole membership tab and some other places, like the list of currently subscribed ccx courses. I have not finished test fixing either, so I will be working on this for at least the rest of the week.

[bdero]

@cguardia Let me know when this is ready for @pwilkins and I to test on a sandbox and we'll give it a spin.

[cguardia]

I'm running a bit behind, but shooting for Thursday morning.

[cguardia]

I still have more work to do here, plus #80. Will keep you posted.

[cguardia]

Sorry that this has taken me more time than expected. I had a meeting with @cewing late last week and that helped me get unstuck. I have pushed my progress to this branch:

https://github.com/jazkarta/edx-platform/tree/remove-ccx-enrollment

There is only one failing ccx test at this point. I will keep working on this today, but I think it can be tested now.

[cguardia]

All tests are passing now. This is definitely ready for testing. I'll work on #80 while you try it out.

[pdpinch]

Thanks Carlos. We are reviewing this now on sandbox2o.mitx.mit.edu

[pdpinch]

Carlos, we're getting an error on creating CCXs -- although we're running off yestersday's version of your branch. Do you think a pull and re-deploy will fix this?

https://gist.github.com/bdero/d63c3c0cc393bd26e9b0

[cewing]

Looks to me like a typo in the create_course view:

https://gist.github.com/bdero/d63c3c0cc393bd26e9b0#file-ccx_traceback-L11

The comma in the kwargs argument should be a colon.

[cguardia]

Yes, @cewing is right. Fixed the typo. The reason I didn't get that on my testing is that this url is only used when trying to create a ccx from a course with a deprecated id, which is not allowed.

[pdpinch]

With a completely new student, I'm getting duplicate enrollments in my course dashboard -- although they both lead to the same URL (https://sandbox2o.mitx.mit.edu/courses/ccx-v1:ODLEng+DemoX+2015_T2+ccx@37/info) ??

Is this because we are double-enrolling the student -- in the root course and in the CCX? Is that still necessary?

[cguardia]

I think we could probably remove our custom list from the dashboard. However, we would be losing the CCX name. What do you think @cewing?

[cewing]

@cguardia, @pwilkins I do not know that we can yet. If we do, we'll need to figure out how to fork the display logic for a given "course" based on whether it's a CCX or a regular course. CCXs do not support the same exact API that a course does (yet).

I think what's likely happening here is that there is a single enrollment only for a CCX. That enrollment is being found once by the list of CCXs that the student is enrolled in (that's our code) and that is producing the second listing in the screenshot. The same enrollment is also being found by the code that lists a student's enrollments in courses, and it is returning the course from which the ccx was made, which results in the first listing.

If we are going to get rid of our list of CCX enrollments on the dashboard, then we'll need to figure out how to make enrollments for CCXs that appear in that first list grab their display information from the CCX, not from the course object.

[pdpinch]

That seems like a likely explanation, as you see in the screenshot there's different titles and start dates, but the same links.

On Jul 15, 2015, at 2:30 PM, Cris Ewing [email protected] wrote:

@cguardia https://github.com/cguardia, @pwilkins https://github.com/pwilkins I do not know that we can yet. If we do, we'll need to figure out how to fork the display logic for a given "course" based on whether it's a CCX or a regular course. CCXs do not support the same exact API that a course does (yet).

I think what's likely happening here is that there is a single enrollment only for a CCX. That enrollment is being found once by the list of CCXs that the student is enrolled in (that's our code) and that is producing the second listing in the screenshot. The same enrollment is also being found by the code that lists a student's enrollments in courses, and it is returning the course from which the ccx was made, which results in the first listing.

If we are going to get rid of our list of CCX enrollments on the dashboard, then we'll need to figure out how to make enrollments for CCXs that appear in that first list grab their display information from the CCX, not from the course object.

—
Reply to this email directly or view it on GitHub #78 (comment).

[tssheth]

Enrolling a student with the "Student List Management" works but "revoking access" does not. At the moment, it is only possible to unenroll a student using "Batch Enrollment". In addition, after the student is unenrolled, the student still appears in "Student List Management" despite having no access to the CCX.

[cguardia]

For the dashboard, am I correct that students are never supposed to join both the regular course and the CCX at the same time? If that's the case, should we look into modifying the main dashboard to show the CCX info instead when the user is actually enrolled in that?

[cewing]

I'm not sure that's a safe assumption, though obviously @pdpinch would know better.

I do think that if we've retrieved the enrollment object using a CCXLocator rather than a CourseID object, we should be able to assert that we want to see the CCX data.

[pdpinch]

Assuming the student shouldn't enroll in the master course is an acceptable compromise, BUT I would prefer a solution that allows the student to enroll in the CCX without an enrollment in the master course. Is that feasible?

In other words, I can live with hiding the incidental enrollment, but I'd rather not require it at all.

---

I am mobile.

On Jul 16, 2015, at 6:07 PM, Cris Ewing [email protected] wrote:

I'm not sure that's a safe assumption, though obviously @pdpinch would know better.

I do think that if we've retrieved the enrollment object using a CCXLocator rather than a CourseID object, we should be able to assert that we want to see the CCX data.

—
Reply to this email directly or view it on GitHub.

[pdpinch]

In further testing, I've confirmed that my CCX student, when enrolled in the CCX is not enrolled in the master course. This is a good thing, but it makes the duplicate in the student dashboard all the more puzzling.

[cguardia]

In that case, I propose to remove our list of courses form the dashboard, then change the original dashboard to check for a ccx for each course and use its information if found. We can do this on the same branch that we are working on, and see how it goes.

[cewing]

@pdpinch I actually don't think it's completely surprising. But it is troubling. Here's what I believe is happening.

The list of 'courses' in which a student is enrolled is arrived at by selecting records from the enrollment table, and then looking up the courses by 'course id'. Because the CCXLocator (which is the custom version of a course id for a ccx) is "unwrapped" on the way through the modulestore wrapper the information for the course from which a ccx was created is found when looking up the enrollment that has a CCX id. The student isn't actually enrolled in that course, but the information for that course is retrieved and displayed. The links are rendered correctly for the CCX because the CCXLocator is re-wrapped as the data for the course comes out of the modulestore wrapper.

I agree with @cguardia that the best approach is to make the dashboard listing of courses aware of the difference between a CCXLocator and a regular Course ID, and then to remove the secondary listing of CCXs.

[pdpinch]

Ok. This makes sense to me now, and I agree with the solution.

I presume the conditional in the dashboard listing template will be keyed off the use of a CCXLocator instead of a CourseLocator? When there is a CCXLocator, we want to be sure to display the overridden course name and start dates (as we do currently in the "secondary listing of CCXs").

I agree with @cguardia https://github.com/cguardia that the best approach is to make the dashboard listing of courses aware of the difference between a CCXLocator and a regular Course ID, and then to remove the secondary listing of CCXs.

[cguardia]

My plan is, for each course, we will check if the course_id is a CCXLocator. If it is, we will use the CCX data, if not we'll simply use the old display code to show the course info.

[cewing]

This is sensible. There is an alternate template used by our listing that should be used an example of how to display the CCX information. It skips a great deal of the display options for the standard course because those options don't make sense for a CCX

[cguardia]

I did the dashboard change and pushed into this same remove-ccx-enrollment branch. It seems to work OK.

[pdpinch]

Great! I think this is ready to be PRed to edX.

---

I am mobile.

On Jul 18, 2015, at 1:30 PM, Carlos de la Guardia [email protected] wrote:

I did the dashboard change and pushed into this same remove-ccx-enrollment branch. It seems to work OK.

—
Reply to this email directly or view it on GitHub.

[cguardia]

@pdpinch PR was just made. It took a bit longer than expected because it turns out edx very recently refactored the dashboard code and I had to make sure what we did worked under the new structure.

[cguardia]

https://github.com/edx/edx-platform/pull/8986

[pwilkins]

I'm encountering the same issue that @jetej reported above. Revoking access in CCX doesn't remove the student from the Student List Management control even though the student can no longer view the CCX in their dashboard.

[cguardia]

@pdpinch I added a data migration, rebased and have all tests passing. Should I continue fixing issues like the one above as part of this PR?

[pdpinch]

We opened a new issue for this (#119) and it doesn't seem to me to be a blocker, so I'd like to handle it in another PR.

On Jul 28, 2015, at 11:24 PM, Carlos de la Guardia [email protected] wrote:

@pdpinch https://github.com/pdpinch I added a data migration, rebased and have all tests passing. Should I continue fixing issues like the one above as part of this PR?

—
Reply to this email directly or view it on GitHub #78 (comment).

[pdpinch]

Closed via 34526dd