Add mode and is_active to CourseEnrollment, shift enrollment logic to model #651

ormsbee · 2013-08-12T20:27:26Z

Add a mode field to allow us to support different kinds of enrollments (all current enrollments are honor code enrollments). Also add an is_active flag. This will let us do things like have an approval step before a CourseEnrollment is activated, as well as track what courses a person used to be signed up for, in case that information is necessary to determine their eligibility for other courses.

There's actually a lot more that I'd eventually like to move into CourseEnrollment (knowledge of courses, permissions, CourseEnrollmentAllowed, etc.) But shifting enroll/unenroll/is_enrolled was good enough for what I needed in the short term goal of supporting is_active as the toggle.

@dianakhuang: I still need to add some docs for this and test the migration on fake prod before I can merge this, but the code should be reviewable at this point.

@gwprice, @kevinchugh: Please review the forums code?

@sefk, @jbau: Just wanted to give you a heads up on this one.

dianakhuang · 2013-08-13T14:15:27Z

common/djangoapps/student/models.py

    created = models.DateTimeField(auto_now_add=True, null=True, db_index=True)

+    # If is_active is False, then the student is not enrolled, and 
+    # `is_enrolled()` will return False. This lets us track a record of courses
+    # that the user


This comment trails off and seems a little confusing. I think it makes sense to say something like "then the student is not considered to be enrolled in the course"

Also, the last sentence trails off.

dianakhuang · 2013-08-13T14:49:34Z

I think this looks good otherwise. We should also make sure Stanford is up to date on these changes.

ormsbee · 2013-08-13T15:54:45Z

Squashed and rebased.

dianakhuang · 2013-08-13T17:33:01Z

Btw, this has a conflict and can't be merged right now (probably in the CHANGELOG file)

kevinchugh · 2013-08-13T19:39:15Z

common/djangoapps/django_comment_common/models.py

+        instance.user.roles.remove(*course_roles)
+        return
+
+    # We've enrolled the student, so make sure they have a default role
    if instance.user.is_staff:


Do you want two consecutive ifs rather than nested ones? It will likely never matter, but semantically, do you want to make one branch if you're a staff and another if you're a student. The edge case is a staff who unregisters for a course but should still have staff access. I guess the question is do you only want to check the active flag for non-staff, more as a policy question?

Is there a reason why a staff member should continue to have Moderator privileges for a course that they are no longer enrolled in?

i was more concerned about the corollary where this essentially requires all moderators to be enrolled. Does every PM and every TA have to be enrolled explicitly to have rights in the forums? How about the instructor? I don't know the answer to that but if the answer is no, then we'll inadvertently remove all course roles for staff by virtue of their not being enrolled.

Ah. I had forgotten just how divorced forum user privileges are from everything else in our system. @jimabramson: What's the desired behavior here? Remove student roles only?

It is important for researchers studying course histories to know what roles a student had in the forum, during the course, eg community TA or other.

Ok, how about this? I'll comment this check out, I'll comment out the tests that depend on it, and the forum team can implement whatever the right thing is at a later date. This will make the situation no worse than before the merge (since it didn't take any action back then on unenrollment either).

…and mode. Features coming down the pipe will want to be able to: * Refer to enrollments before they are actually activated (approval step). * See what courses a user used to be enrolled in for when they re-enroll in the same course, or a different run of that course. * Have different "modes" of enrolling in a course, representing things like honor certificate enrollment, auditing (no certs), etc. This change adds an is_active flag and mode (with default being "honor"). The commit is only as large as it is because many parts of the codebase were manipulating enrollments by adding and removing CourseEnrollment objects directly. It was necessary to create classmethods on CourseEnrollment to encapsulate this functionality and then port everything over to using them. The migration to add columns has been tested on a prod replica, and seems to be fine for running on a live system with single digit millions of rows of enrollments.

Add mode and is_active to CourseEnrollment, shift enrollment logic to model

jbau · 2013-08-14T22:29:10Z

@ormsbee @dianakhuang
Just to ask a summary question: basically the gist of these changes (32 files) is that now, for CourseEnrollment, existence now no longer denotes enrollment. Instead

(is_active, mode ) = (True, "honor") indicates enrollment
(is_active, mode) = (False, *) indicates non-enrollment
(is_active, mode) = (True, "^honor") should not be used.

correct?

jbau · 2013-08-15T03:23:16Z

oh, and no data migration is necessary because is_active defaults to true and mode defaults to "honor", correct?

ormsbee · 2013-08-15T14:14:40Z

@jbau
Correct, existence no longer denotes enrollment. Nos. 1 and 2 are true, but a major point of this transition is to encourage people to useCourseEnrollment.is_enrolled(user, course_id) instead. It's quite possible that the internal representation of what it means to be enrolled will change again, say to a status with intermediate states like "pending" or some such.

In regards to item 3, we'll be adding different types shortly, in the next week or so.

ormsbee · 2013-08-15T14:15:08Z

And yes, no data migration necessary because of the defaults. The migration is fast enough to run on our live prod instance without trouble.

stroilova · 2013-08-15T15:36:48Z

Re: .is_enrolled()

We sometimes run SQL scripts to determine # enrolled. Some PMs also have
access to a wwc database replica and to the following query list:
(see, e.g.
https://edx-wiki.atlassian.net/wiki/pages/viewpage.action?pageId=7471154)

What is the best thing to do going forward to determine #Enrolled from the
database directly? Should we just update the query scripts according to
your documentation, or is there an API that would be queried? Thank you.

@rocha @jtauber @dave

On Thu, Aug 15, 2013 at 10:15 AM, David Ormsbee notifications@github.comwrote:

And yes, no data migration necessary because of the defaults. The defaults
also make the migration fast enough to run on our prod instance without
locking the table.

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/651#issuecomment-22705384
.

ormsbee · 2013-08-15T16:59:50Z

@stroilova: I'd like edx-platform code to use the CourseEnrollment model, but you folks should still use SQL in the aggregate queries. I updated the relevant SQL query on the linked page. Thank you.

stroilova · 2013-08-15T17:03:34Z

Just saw that, thank you!

On Thu, Aug 15, 2013 at 12:59 PM, David Ormsbee notifications@github.comwrote:

@stroilova https://github.com/stroilova: I'd like edx-platform code to
use the CourseEnrollment model, but you folks should still use SQL in the
aggregate queries. I updated the relevant SQL query on the linked page.
Thank you.

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/651#issuecomment-22713766
.

rocha · 2013-08-15T17:12:05Z

@stroilova we should discourage PMs from running that kind of query on the database since the enrollment number is accessible from the analytics tab on the instructor panel.

However, if you still need the SQL query, it something like this:

SELECT COUNT(DISTINCT user_id) AS students FROM student_courseenrollment WHERE course_id=(course_id) AND is_active !=0;

jbau · 2013-08-15T18:33:00Z

@ormsbee Yes, I'm planning on using the methods. Was just asking about the representation to make sure I understood it.

On Aug 15, 2013, at 7:14 AM, David Ormsbee notifications@github.com wrote:

@jbau
Correct, existence no longer denotes enrollment. Nos. 1 and 2 are true, but a major point of this transition is to encourage people to useCourseEnrollment.is_enrolled(user, course_id) instead. It's quite possible that the internal representation of what it means to be enrolled will change again, say to a status with intermediate states like "pending" or some such.

In regards to item 3, we'll be adding different types shortly, in the next week or so.

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

Feature/victor/proper 404s

Update static tabs APIs to cache tab contents

Confirmation email sub

dianakhuang reviewed Aug 13, 2013
View reviewed changes

ghost assigned dianakhuang Aug 13, 2013

kevinchugh reviewed Aug 13, 2013
View reviewed changes

dianakhuang mentioned this pull request Aug 14, 2013

Add Course Modes to LMS #676

Merged

ormsbee pushed a commit that referenced this pull request Aug 14, 2013

Merge pull request #651 from edx/ormsbee/enrollment_modes

57a8063

Add mode and is_active to CourseEnrollment, shift enrollment logic to model

ormsbee merged commit 57a8063 into master Aug 14, 2013

ormsbee deleted the ormsbee/enrollment_modes branch August 15, 2013 14:31

chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014

Merge pull request openedx#651 from MITx/feature/victor/proper-404s

aeefde3

Feature/victor/proper 404s

itsjeyd referenced this pull request in open-craft/edx-platform Mar 24, 2016

Merge pull request #651 from edx-solutions/msaqib52/YONK-278

500933e

Update static tabs APIs to cache tab contents

jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Mar 30, 2016

Fix bug openedx#651

0cf03eb

dfrojas pushed a commit to eduNEXT/edx-platform that referenced this pull request Aug 31, 2017

Merge pull request openedx#651 from potsui/confirmation-email-sub

1fe9a25

Confirmation email sub

snyk-bot mentioned this pull request Jan 21, 2020

[Snyk] Fix for 1 vulnerabilities ISMinisterios/edx-platform#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mode and is_active to CourseEnrollment, shift enrollment logic to model #651

Add mode and is_active to CourseEnrollment, shift enrollment logic to model #651

ormsbee commented Aug 12, 2013

dianakhuang Aug 13, 2013

dianakhuang commented Aug 13, 2013

ormsbee commented Aug 13, 2013

dianakhuang commented Aug 13, 2013

kevinchugh Aug 13, 2013

ormsbee Aug 13, 2013

kevinchugh Aug 13, 2013

ormsbee Aug 13, 2013

ichuang Aug 14, 2013

ormsbee Aug 14, 2013

jbau commented Aug 14, 2013

jbau commented Aug 15, 2013

ormsbee commented Aug 15, 2013

ormsbee commented Aug 15, 2013

stroilova commented Aug 15, 2013

ormsbee commented Aug 15, 2013

stroilova commented Aug 15, 2013

rocha commented Aug 15, 2013

jbau commented Aug 15, 2013

Add mode and is_active to CourseEnrollment, shift enrollment logic to model #651

Add mode and is_active to CourseEnrollment, shift enrollment logic to model #651

Conversation

ormsbee commented Aug 12, 2013

dianakhuang Aug 13, 2013

Choose a reason for hiding this comment

dianakhuang commented Aug 13, 2013

ormsbee commented Aug 13, 2013

dianakhuang commented Aug 13, 2013

kevinchugh Aug 13, 2013

Choose a reason for hiding this comment

ormsbee Aug 13, 2013

Choose a reason for hiding this comment

kevinchugh Aug 13, 2013

Choose a reason for hiding this comment

ormsbee Aug 13, 2013

Choose a reason for hiding this comment

ichuang Aug 14, 2013

Choose a reason for hiding this comment

ormsbee Aug 14, 2013

Choose a reason for hiding this comment

jbau commented Aug 14, 2013

jbau commented Aug 15, 2013

ormsbee commented Aug 15, 2013

ormsbee commented Aug 15, 2013

stroilova commented Aug 15, 2013

ormsbee commented Aug 15, 2013

stroilova commented Aug 15, 2013

rocha commented Aug 15, 2013

jbau commented Aug 15, 2013