[BB-8488] Link course enrollments to learning pathways #6
Conversation
pkulkark
force-pushed
the
pooja/link-course-enrollments-to-pathway
branch
2 times, most recently
from
5a849e5 to
3d0fb15
Compare
pkulkark
force-pushed
the
pooja/link-course-enrollments-to-pathway
branch
from
3d0fb15 to
2725429
Compare
| try: | ||
| learning_path_step = LearningPathStep.objects.get(course_key=course_key) | ||
| except LearningPathStep.DoesNotExist: | ||
| logger.info("Course is not part of any Learning Path. Ignoring.") | ||
| return |
There was a problem hiding this comment.
Isn't possible for a Course to correspond to multiple LearningSteps in different LearningPaths?
There was a problem hiding this comment.
@CefBoud That's a good point you brought up. Let's say there's a course that's part of 2 different LearningPaths. If a user enrolls into it, should they be automatically enrolled into both the LearningPaths? Or should the enrollments in LearningPaths be only through the admin (i.e. should be done explicitly)? I'm inclined towards keeping it explicit but that would mean doing away with this whole signal.py file. What do you think?
| if created: # Only react to new enrollments | ||
| course_key = instance.course_id |
There was a problem hiding this comment.
Is there a reason to only react to new enrollments? How about unenrollment?
There was a problem hiding this comment.
The idea was to automatically create a learningpathcourseenrollment object, if a course was added to a learning path (after initial learning path enrollments were done) and a user enrolled into it. But the changes here might not be needed at all (see previous comment).
| LearningPathEnrollment, on_delete=models.CASCADE | ||
| ) | ||
| course_key = CourseKeyField(max_length=255) | ||
| status = models.CharField(max_length=9, choices=STATUS_CHOICES) |
There was a problem hiding this comment.
Can this be maybe replaced with an is_active BooleanField?
There was a problem hiding this comment.
Yep sure. I was just following the same things as ProgramCourseEnrollment field.
| learning_path=form.instance | ||
| ).values_list("course_key", flat=True) | ||
| for course_key in courses: | ||
| LearningPathCourseEnrollment.objects.create( |
There was a problem hiding this comment.
Maybe create only if it doesn't already exist? Otherwise, this will fail if a user is added a second time to the form (by mistake or intentionally)
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| @receiver(post_save, sender=CourseEnrollment) |
There was a problem hiding this comment.
Nit: how about using the COURSE_ENROLLMENT_CREATED signal?
There was a problem hiding this comment.
I'm wondering if we should just do away with the CourseEnrollment foreign key and only use the COURSE_ENROLLMENT_CREATED and COURSE_UNENROLLMENT_COMPLETED signals to link the learning path enrollments to course enrollments. While the current approach works within lms, it breaks the modularity of the plugin and creates too much of a dependency on lms (CI tests can't pass without lms). What do you think?
There was a problem hiding this comment.
If we don't need the CourseEnrollement object, yeah, definitely.
| for course_key in courses: | ||
| LearningPathCourseEnrollment.objects.create( |
There was a problem hiding this comment.
What do you think about using bulk_create here?
Description
This PR links course enrollments to learning pathways for more granular control over individual course enrollments. It extends the admin feature to bulk enroll users into the courses in the pathway.
Testing Instructions
<devstack root>/src$ make dev.up.lms$ make dev.shell.lms# pip install -e /edx/src/learning-paths-plugin/# ./manage.py lms migrate learning_paths$ make dev.stop.lms dev.up.lmsverifieduser and check dashboard for enrolled courses.verifiedusername and save the learning path.verifieduser and verify that the user has been enrolled in all the courses listed in the learning steps section.