feat: add default enrollment models #2259

iloveagent57 · 2024-10-07T15:11:33Z

Adds two models, DefaultEnterpriseEnrollmentIntention and DefaultEnterpriseEnrollmentRealization. The former defines, for a customer, a course/run that associated users should be automatically enrolled into. The latter represents the relationship between that intention record and a realized EnterpriseCourseEnrollment; persisting realized enrollments in this way will help to make read operations related to default enrollments much more efficient.
ENT-9577

Merge checklist:

Any new requirements are in the right place (do not manually modify the requirements/*.txt files)
- base.in if needed in production but edx-platform doesn't install it
- test-master.in if edx-platform pins it, with a matching version
- make upgrade && make requirements have been run to regenerate requirements
make static has been run to update webpack bundling if any static content was updated
./manage.py makemigrations has been run
- Checkout the Database Migration Confluence page for helpful tips on creating migrations.
- Note: This must be run if you modified any models.
  - It may or may not make a migration depending on exactly what you modified, but it should still be run.
- This should be run from either a venv with all the lms/edx-enterprise requirements installed or if you checked out edx-enterprise into the src directory used by lms, you can run this command through an lms shell.
  - It would be ./manage.py lms makemigrations in the shell.
Version bumped
Changelog record added
Translations updated (see docs/internationalization.rst but also this isn't blocking for merge atm)

Post merge:

Tag pushed and a new version released
- Note: Assets will be added automatically. You just need to provide a tag (should match your version number) and title and description.
After versioned build finishes in GitHub Actions, verify version has been pushed to PyPI
- Each step in the release build has a condition flag that checks if the rest of the steps are done and if so will deploy to PyPi.
  (so basically once your build finishes, after maybe a minute you should see the new version in PyPi automatically (on refresh))
PR created in edx-platform to upgrade dependencies (including edx-enterprise)
- Trigger the 'Upgrade one Python dependency' action against master in edx-platform with new version number to generate version bump PR
- This must be done after the version is visible in PyPi as make upgrade in edx-platform will look for the latest version in PyPi.
- Note: the edx-enterprise constraint in edx-platform must also be bumped to the latest version in PyPi.

brobro10000

A few non-blocking questions. Great job!

brobro10000 · 2024-10-11T19:06:09Z

docs/decisions/0015-default-enrollments.rst

[curious] 2 observations.

If the DefaultEnterpriseEnrollmentIntention content_key enrollable course differs from DefaultEnterpriseEnrollmentRealization, EnterpriseCourseEnrollment enrolled course, are there any consequences from the mismatch?

Were there any alternatives considered?

Here's a stab at trying to clarify any potential mismatches.

The content_key on DefaultEnterpriseEnrollmentIntention is either a top-level course key or a course run key during configuration to remain flexible for ECS; however, we will always discern the correct course run key to use for enrollment based on the provided content_key.

Post-enrollment related models (e.g., EnterpriseCourseEnrollment and DefaultEnterpriseEnrollmentRealization) will always primarily be associated with the course run associated with the DefaultEnterpriseEnrollmentIntention:

If content_key is a top-level course, the course run key used when enrolling (converting to EnterpriseCourseEnrollment and DefaultEnterpriseEnrollmentRealization) is the currently advertised course run.

If the content_key is a specific course run, we'll always try to enroll in the explicitly configured course run key (not the advertised course run).

This content_key will be passed to the EnterpriseCatalogApiClient().get_content_metadata_content_identifier method. When passing a course run key to this endpoint, it'll actually return the top-level course metadata associated with the course run, not just the specified course run's metadata (this is primarily due to catalogs containing courses, not course runs and we generally say that if the top-level course is in a customer's catalog, all of its course runs are, too (with exceptions like the ongoing restricted runs work).

If the content_key configured for a DefaultEnterpriseEnrollmentIntention is a top-level course, there is a chance the currently advertised course run used for future enrollment intentions might change over time from previous redeemed/enrolled DefaultEnterpriseEnrollmentIntentions. However, I think this is mitigated in that the DefaultEnterpriseEnrollmentRealization ensures the resulting, converted enrollment is still related to the original DefaultEnterpriseEnrollmentIntention, despite a potential content_key vs. enrollment ID match.

This is nice, so I copied it into a new section of the ADR.

brobro10000 · 2024-10-11T19:13:12Z

enterprise/models.py

+    content_type = models.CharField(
+        max_length=127,
+        blank=False,
+        null=False,
+        choices=DEFAULT_ENROLLMENT_CONTENT_TYPE_CHOICES,
+        help_text=_(
+            "The type of content (e.g. a course vs. a course run)."
+        ),
+    )
+    content_key = models.CharField(
+        max_length=255,
+        blank=False,
+        null=False,
+        help_text=_(
+            "A course or course run that related users should be automatically enrolled into."
+        ),
+    )


[questionx2] is there some benefit to including parent_content_key as a field? Should we bring the pattern of is_assigned_course_run from access here as well, is_default_course_run?

tests/test_utilities.py

adamstankiewicz · 2024-10-11T20:38:04Z

enterprise/admin/__init__.py

+
+
+@admin.register(models.DefaultEnterpriseEnrollmentIntention)
+class DefaultEnterpriseEnrollmentIntentionAdmin(admin.ModelAdmin):


nit/consideration: I wonder if it's worth getting djangoql (GitHub) integrated within edx-enterprise, starting with the new models?

adamstankiewicz · 2024-10-11T20:52:05Z

docs/decisions/0015-default-enrollments.rst

Here's a stab at trying to clarify any potential mismatches.

The content_key on DefaultEnterpriseEnrollmentIntention is either a top-level course key or a course run key during configuration to remain flexible for ECS; however, we will always discern the correct course run key to use for enrollment based on the provided content_key.

Post-enrollment related models (e.g., EnterpriseCourseEnrollment and DefaultEnterpriseEnrollmentRealization) will always primarily be associated with the course run associated with the DefaultEnterpriseEnrollmentIntention:

If content_key is a top-level course, the course run key used when enrolling (converting to EnterpriseCourseEnrollment and DefaultEnterpriseEnrollmentRealization) is the currently advertised course run.

If the content_key is a specific course run, we'll always try to enroll in the explicitly configured course run key (not the advertised course run).

This content_key will be passed to the EnterpriseCatalogApiClient().get_content_metadata_content_identifier method. When passing a course run key to this endpoint, it'll actually return the top-level course metadata associated with the course run, not just the specified course run's metadata (this is primarily due to catalogs containing courses, not course runs and we generally say that if the top-level course is in a customer's catalog, all of its course runs are, too (with exceptions like the ongoing restricted runs work).

If the content_key configured for a DefaultEnterpriseEnrollmentIntention is a top-level course, there is a chance the currently advertised course run used for future enrollment intentions might change over time from previous redeemed/enrolled DefaultEnterpriseEnrollmentIntentions. However, I think this is mitigated in that the DefaultEnterpriseEnrollmentRealization ensures the resulting, converted enrollment is still related to the original DefaultEnterpriseEnrollmentIntention, despite a potential content_key vs. enrollment ID match.

docs/decisions/0015-default-enrollments.rst

adamstankiewicz · 2024-10-11T20:54:59Z

docs/decisions/0015-default-enrollments.rst

+   the default enrollment flow should cause the "realization" of default enrollments for learners
+   into the currently-advertised, enrollable run for a course.
+2. Default Enrollments should be loosely coupled to subsidy type.
+3. Unenrollment: If a learner chooses to unenroll from a default course, they should not be automatically re-enrolled.


Might it also be worth calling out the "learner is already enrolled" case? Or do you think would be better suited for another ADR more about the proposed default-enterprise-enrollment-intentions/learner-status GET API instead?

I think there should be a distinct ADR (loosely) describing the proposed API contract.

docs/decisions/0015-default-enrollments.rst

adamstankiewicz · 2024-10-11T21:10:06Z

docs/decisions/0015-default-enrollments.rst

+   make this efficient).
+3. We're introducing more complexity in terms of how subsidized enterprise enrollments
+   can come into existence.
+4. The realization model makes the provenance of default enrollments explicit and easy to examine.


adamstankiewicz · 2024-10-11T21:14:55Z

enterprise/models.py

+            "The customer for which this default enrollment will be realized.",
+        )
+    )
+    content_type = models.CharField(


[curious] What's your thinking around how this field would be utilized? From a functionality perspective, I'm not we'd necessary need it per se for the computed properties to function BUT having this field would definitely help better understand/analyze/query these models to understand how many courses vs. course runs are configured.

Do you think this is intended to be manually set at time of configuring a DefaultEnterpriseEnrollmentIntention, or would it be programmatically set after saving based on deducing whether the provided content_key is a course vs. course run (perhaps based on the content metadata API request made to enterprise-catalog which I believe returns the content_key vs. parent_content_key relationship?

Either one - we could populate it automatically based on a simple regular expression.

adamstankiewicz · 2024-10-11T21:15:15Z

enterprise/models.py

+        )
+    )
+    content_type = models.CharField(
+        max_length=127,


nit/curious: Why 127 here?

(2^7 - 1). i.e the same reason we use max_length of 255 instead of 256. I'm not sure how much this matters, if it matters at all. There used to be some thing about using one fewer chars than a power of two to make page fetches more efficient (I think).

Interesting, TIL haha.

adamstankiewicz · 2024-10-11T21:18:06Z

docs/decisions/0015-default-enrollments.rst

+2. Default Enrollments should be loosely coupled to subsidy type.
+3. Unenrollment: If a learner chooses to unenroll from a default course, they should not be automatically re-enrolled.
+4. Graceful handling of license revocation: Upon license revocation, we currently downgrade the learner’s
+   enrollment mode to ``audit``. This fact should be visible from any new APIs exposed


Good call to make the course mode known in the proposed default-enterprise-enrollment-intentions/learner-status/ GET API.

adamstankiewicz

LGTM, one addtl nit about the name of PendingEnterpriseCourseEnrollments vs. PendingEnrollments in the ADR.

adamstankiewicz · 2024-10-15T18:00:28Z

docs/decisions/0015-default-enrollments.rst

+Enterprise needs a solution for managing automated enrollments into "default" courses or specific course runs
+for enterprise learners. Importantly, we need this solution to work for learners where we have no information
+about the identity of learner who will eventually be associated with an ``EnterpriseCustomer``. For that reason,
+the solution described below does not involve ``PendingEnterpriseCourseEnrollments`` or anything similar -


nit: the model name for pending enrollments is only PendingEnrollments (source).

adamstankiewicz · 2024-10-15T19:30:26Z

enterprise/models.py

+        )
+    )
+    content_type = models.CharField(
+        max_length=127,


Interesting, TIL haha.

Adds two models, `DefaultEnterpriseEnrollmentIntention` and `DefaultEnterpriseEnrollmentRealization`. The former defines, for a customer, a course/run that associated users should be automatically enrolled into. The latter represents the relationship between that intention record and a realized EnterpriseCourseEnrollment; persisting realized enrollments in this way will help to make read operations related to default enrollments much more efficient. ENT-9577

iloveagent57 force-pushed the aed/default-enrollment-models branch 2 times, most recently from eb54fbe to fe71e9d Compare October 10, 2024 15:03

iloveagent57 changed the title ~~feat: add default enrollment models [WIP]~~ feat: add default enrollment models Oct 10, 2024

iloveagent57 marked this pull request as ready for review October 10, 2024 15:05

iloveagent57 force-pushed the aed/default-enrollment-models branch 4 times, most recently from 619692e to ee01911 Compare October 10, 2024 16:47

brobro10000 approved these changes Oct 11, 2024

View reviewed changes

adamstankiewicz reviewed Oct 11, 2024

View reviewed changes

iloveagent57 force-pushed the aed/default-enrollment-models branch 2 times, most recently from fa285e5 to 2acd810 Compare October 15, 2024 17:57

adamstankiewicz approved these changes Oct 15, 2024

View reviewed changes

iloveagent57 force-pushed the aed/default-enrollment-models branch from 2acd810 to 36d5b35 Compare October 15, 2024 20:19

iloveagent57 force-pushed the aed/default-enrollment-models branch from 36d5b35 to 59d1d40 Compare October 15, 2024 20:27

iloveagent57 merged commit a274850 into master Oct 16, 2024
13 of 14 checks passed

iloveagent57 deleted the aed/default-enrollment-models branch October 16, 2024 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add default enrollment models #2259

feat: add default enrollment models #2259

iloveagent57 commented Oct 7, 2024 •

edited

Loading

brobro10000 left a comment •

edited

Loading

brobro10000 Oct 11, 2024

adamstankiewicz Oct 11, 2024

iloveagent57 Oct 15, 2024

brobro10000 Oct 11, 2024

adamstankiewicz Oct 11, 2024

adamstankiewicz Oct 11, 2024

adamstankiewicz Oct 11, 2024

iloveagent57 Oct 15, 2024

adamstankiewicz Oct 11, 2024

adamstankiewicz Oct 11, 2024

iloveagent57 Oct 15, 2024

adamstankiewicz Oct 11, 2024

iloveagent57 Oct 15, 2024

adamstankiewicz Oct 15, 2024

adamstankiewicz Oct 11, 2024

adamstankiewicz left a comment

adamstankiewicz Oct 15, 2024

adamstankiewicz Oct 15, 2024



		@admin.register(models.DefaultEnterpriseEnrollmentIntention)
		class DefaultEnterpriseEnrollmentIntentionAdmin(admin.ModelAdmin):

feat: add default enrollment models #2259

feat: add default enrollment models #2259

Conversation

iloveagent57 commented Oct 7, 2024 • edited Loading

brobro10000 left a comment • edited Loading

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

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

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

adamstankiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iloveagent57 commented Oct 7, 2024 •

edited

Loading

brobro10000 left a comment •

edited

Loading