fix: unenrolled defaults are now not enrollable #2301
Conversation
iloveagent57
force-pushed
the
aed/unenrolled-defaults
branch
from
61bb513 to
c9ad71e
Compare
| @ddt.data( | ||
| {'run_is_enrollable': False, 'unenrollment_exists': False}, | ||
| {'run_is_enrollable': True, 'unenrollment_exists': True}, | ||
| ) | ||
| @mock.patch('enterprise.content_metadata.api.EnterpriseCatalogApiClient') | ||
| @mock.patch('enterprise.models.utils.get_best_mode_from_course_key') | ||
| @mock.patch.object(EnterpriseCourseEnrollment, 'course_enrollment', new_callable=mock.PropertyMock) | ||
| @ddt.unpack | ||
| def test_default_enrollment_intentions_learner_status_content_not_enrollable( |
There was a problem hiding this comment.
This is the only changed test, everything else has just been refactored into this new file.
There was a problem hiding this comment.
Good call to start breaking up the giant test_views file :)
| @@ -211,6 +211,12 @@ def _partition_default_enrollment_intentions_by_enrollment_status( | |||
| already_enrolled.append((intention, non_audit_enrollment)) | |||
| continue | |||
|
|
|||
| if non_audit_enrollment and non_audit_enrollment.unenrolled: | |||
There was a problem hiding this comment.
[sanity check] If the existing enrollment is audit (vs. the non_audit_enrollment here), the learners unenrolls, and there's non-audit modes that exist, would we also want to prevent automatic re-enrollment, too?
For example, would the conditions below the NOTE comment also need to take into account audit_enrollment.unenrolled?
There was a problem hiding this comment.
I thought about that but decided against it, I can leave a comment in the code. My reasoning is that default enrollments should be "exactly once" per (user, enterprise), if possible. If they enrolled in audit and unenrolled, it means they likely never had a default intention realized for them. This isn't a strongly-held position though.
There was a problem hiding this comment.
If they enrolled in audit and unenrolled, it means they likely never had a default intention realized for them.
Ah yeah, this probably represents the source of most audit enrollments. The exception would be for courses that are audit-only, which likely rarely happens in production; it does happen somewhat frequently on stage, though we shouldn't optimize for bad stage data 😉
Let's leave it as is for now, with a comment!
| @ddt.data( | ||
| {'run_is_enrollable': False, 'unenrollment_exists': False}, | ||
| {'run_is_enrollable': True, 'unenrollment_exists': True}, | ||
| ) | ||
| @mock.patch('enterprise.content_metadata.api.EnterpriseCatalogApiClient') | ||
| @mock.patch('enterprise.models.utils.get_best_mode_from_course_key') | ||
| @mock.patch.object(EnterpriseCourseEnrollment, 'course_enrollment', new_callable=mock.PropertyMock) | ||
| @ddt.unpack | ||
| def test_default_enrollment_intentions_learner_status_content_not_enrollable( |
There was a problem hiding this comment.
Good call to start breaking up the giant test_views file :)
The default enrollment ``learner_status`` view now considers intended courses from which the requested user has unenrolled as no-longer realizable. ENT-9839
iloveagent57
force-pushed
the
aed/unenrolled-defaults
branch
from
c9ad71e to
ccd4dc3
Compare
The default enrollment
learner_statusview now considers intended courses from which the requested user has unenrolled as no-longer realizable.ENT-9839
Merge checklist:
requirements/*.txtfiles)base.inif needed in production but edx-platform doesn't install ittest-master.inif edx-platform pins it, with a matching versionmake upgrade && make requirementshave been run to regenerate requirementsmake statichas been run to update webpack bundling if any static content was updated./manage.py makemigrationshas been run./manage.py lms makemigrationsin the shell.Post merge:
(so basically once your build finishes, after maybe a minute you should see the new version in PyPi automatically (on refresh))
make upgradein edx-platform will look for the latest version in PyPi.