Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: initialize enterprise metadata in HandlerContext; complete auto-apply business logic #586

Merged
merged 7 commits into from
Nov 15, 2024

Conversation

adamstankiewicz
Copy link
Member

@adamstankiewicz adamstankiewicz commented Nov 12, 2024

Jira:
ENT-9629

Description:

  • Converted additional HandlerContext class attributes as private, with @property getter functions.
  • Refactors initialize_common_context_data as _initialize_common_context_data:
    • Extended to optionally support both enterprise_customer_uuid and enterprise_customer_slug.
    • Extended to include an attribute for enterprise_features (returned by enterprise-learner API, described below).
    • Initializes context data with enterprise customer metadata (mostly parity with transforms on the existing Learner Portal request) via the enterprise-learner LMS API.
      • If no slug/uuid is provided, resolves to the request user's active=True enterprise customer user.
      • If only slug is provided, finds linked enterprise customer matching the requested slug and sets the uuid.
      • If only uuid is provided, finds linked enterprise customer matching the requested uuid and sets the slug.
      • If uuid and/or slug is provided, but no matching linked enterprise customer is found, falls back to request the enterprise customer metadata as a staff user (if applicable).
  • Completes the auto-apply business logic (to be tested at the viewset layer in separate ticket) by extending the conditional check to include whether the enterprise customer has an identity_provider configured.
  • Integrates with self.lms_user_api_client.get_default_enterprise_enrollment_intentions_learner_status in BaseLearnerPortalHandler.
  • Integrates with self.lms_user_api_client.get_enterprise_course_enrollments in DashboardHandler.
  • Tests.

Merge checklist:

  • ./manage.py makemigrations has been run
    • 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.

Post merge:

  • Ensure that your changes went out to the stage instance
  • Deploy to prod instance

@adamstankiewicz adamstankiewicz marked this pull request as draft November 12, 2024 17:24
@adamstankiewicz adamstankiewicz marked this pull request as ready for review November 13, 2024 22:26
@adamstankiewicz adamstankiewicz changed the title feat: initialize enterprise metadata in HandlerContext feat: initialize enterprise metadata in HandlerContext; complete auto-apply business logic Nov 13, 2024
Copy link
Member

@brobro10000 brobro10000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job. A couple of nits that can be deferred or ignored. Approving to unblock.

enterprise_access/apps/bffs/context.py Outdated Show resolved Hide resolved
enterprise_access/apps/bffs/context.py Outdated Show resolved Hide resolved
enterprise_access/apps/bffs/context.py Outdated Show resolved Hide resolved
Copy link
Contributor

@iloveagent57 iloveagent57 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still reading through the second half of this, here's some small suggestions/questions so far.

enterprise_access/apps/api_client/lms_client.py Outdated Show resolved Hide resolved
next_url = data.get('next') if traverse_pagination else None

consolidated_response = {
**initial_response_data,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initial_response_data will contain keys like results, next, etc. right? Is that intended?

Copy link
Member Author

@adamstankiewicz adamstankiewicz Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, it is intended. Then the subsequent lines below overrides relevant fields like next, prev, etc. to reflect the traversed pagination results being a single page.

The primary reason to spread **initial_response_data is bring along any other existing pagination fields unrelated to page length, i.e. I believe **initial_response_data brings along start: 0 as well without needing to explicitly define it. The start value from the initial response is unchanged regardless of traverse_pagination.

enterprise_access/apps/bffs/context.py Show resolved Hide resolved
enterprise_access/apps/bffs/context.py Outdated Show resolved Hide resolved
enterprise_access/apps/bffs/context.py Outdated Show resolved Hide resolved
enterprise_access/apps/bffs/context.py Outdated Show resolved Hide resolved
enterprise_access/apps/bffs/context.py Outdated Show resolved Hide resolved
enterprise_access/apps/bffs/handlers.py Outdated Show resolved Hide resolved
Returns:
The transformed enterprise customer user data.
"""
enterprise_customer = enterprise_customer_user.get('enterprise_customer')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enterprise_customer dict in the ECU response payload won't be "full", right? Does that matter here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enterprise-learner API uses the same EnterpriseCustomerSerializer that the list/detail enterprise customer API uses; it's the "full" metadata. This is the same enterprise customer metadata currently consumed by the frontends (both learner/admin).

That said, when cross-checking to verify, I noticed a discrepancy between admin_users list in the enterprise-learner API (this data source) vs. the enterprise-customer detail API. The enterprise-learner API always denotes admin_users as an empty list, even if the customer has admin users...

I filed a bug ticket and fixed the bug through this PR: openedx/edx-enterprise#2287

@adamstankiewicz adamstankiewicz merged commit c99238a into main Nov 15, 2024
3 checks passed
@adamstankiewicz adamstankiewicz deleted the ags/ent-9629 branch November 15, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants