-
Notifications
You must be signed in to change notification settings - Fork 10
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
Conversation
140e79b
to
b460c5e
Compare
b460c5e
to
1bff30a
Compare
1bff30a
to
cd4cd05
Compare
There was a problem hiding this 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.
There was a problem hiding this 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.
next_url = data.get('next') if traverse_pagination else None | ||
|
||
consolidated_response = { | ||
**initial_response_data, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
Returns: | ||
The transformed enterprise customer user data. | ||
""" | ||
enterprise_customer = enterprise_customer_user.get('enterprise_customer') |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Jira:
ENT-9629
Description:
HandlerContext
class attributes as private, with@property
getter functions.initialize_common_context_data
as_initialize_common_context_data
:enterprise_customer_uuid
andenterprise_customer_slug
.enterprise_features
(returned byenterprise-learner
API, described below).data
with enterprise customer metadata (mostly parity with transforms on the existing Learner Portal request) via theenterprise-learner
LMS API.active=True
enterprise customer user.identity_provider
configured.self.lms_user_api_client.get_default_enterprise_enrollment_intentions_learner_status
inBaseLearnerPortalHandler
.self.lms_user_api_client.get_enterprise_course_enrollments
inDashboardHandler
.Merge checklist:
./manage.py makemigrations
has been runPost merge: