-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix: add data object serialization support for reversal writing #331
Conversation
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.
Seems legit, small suggestion on stuff in utils.py
.
from openedx_events.enterprise.data import ( | ||
EnterpriseCourseEnrollment, | ||
EnterpriseCustomerUser, | ||
LearnerCreditEnterpriseCourseEnrollment | ||
) |
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.
Good call.
@@ -60,14 +72,14 @@ def unenrollment_can_be_refunded( | |||
|
|||
Args: | |||
content_metadata (dict): Metadata for course from which the learner has been unenrolled. | |||
enterprise_course_enrollment: (dict-like): | |||
enterprise_course_enrollment: (dict or openedx_events.enterprise.data.EnterpriseCourseEnrollment): |
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.
What if this function did something like below to cast dicts into EnterpriseCourseEnrollments
?
record = enterprise_course_enrollment
if isinstance(record, dict):
record = EnterpriseCourseEnrollment(**record)
# then use record in all the logic below.
Or better yet, maybe the caller should be forced to do that casting, and this function can have a stricter contract, that it only accepts EnterpriseCourseEnrollments?
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.
Yea, I briefly considered forcing the onus of conversion on the caller, but I wasn't sure the object could be coerced into a dict so simply because of the multiple nested layers of objects. However, now that I look at how enterprise_course_enrollment
is actually used I think it's feasible to do it your way because only the top level keys are accessed by unenrollment_can_be_refunded()
.
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.
I had to coerce the other way around because the EnterpriseCourseEnrollments class would not accept the partial data which the /unenrolled
endpoint returns. Adding .__dict__
onto the end of an event data object seemed to work fine, for one level of data:
(Pdb) test_lc_course_enrollment.__dict__
{'uuid': UUID('07c02939-6a66-4006-9e0c-5ef3afcb05d9'), 'created': datetime.datetime(2020, 1, 1, 12, 0), 'modified': datetime.datetime(2020, 1, 1, 12, 0), 'fulfillment_type': 'learner_credit', 'is_revoked': True, 'enterprise_course_entitlement_uuid': None, 'enterprise_course_enrollment': EnterpriseCourseEnrollment(id=1, created=datetime.datetime(2020, 1, 1, 12, 0), modified=datetime.datetime(2020, 1, 1, 12, 0), enterprise_customer_user=EnterpriseCustomerUser(id=1, created=datetime.datetime(2020, 1, 1, 12, 0), modified=datetime.datetime(2020, 1, 1, 12, 0), enterprise_customer_uuid=UUID('2a4329a7-bc2e-4042-8c1e-e2e5cc335ae0'), user_id=1, active=True, linked=True, is_relinkable=True, should_inactivate_other_customers=True, invite_key=None), course_id='course-v1:bin+bar+baz', saved_for_later=False, source_slug=None, unenrolled=True, unenrolled_at=datetime.datetime(2020, 1, 1, 0, 0)), 'transaction_id': UUID('b9f7a49f-3898-40fa-85ab-82337957a3c5')}
At some point during initial development I assumed the serialized data objects from openedx_events supported dict-like access patterns. I was wrong. This commit generalizes access patterns to support both dicts and objects so that we can continue to support both event-driven and cron-driven reversal writing.
d4abab3
to
3616108
Compare
At some point during initial development I assumed the serialized data objects from openedx_events supported dict-like access patterns. I was wrong. This commit generalizes access patterns to support both dicts and objects so that we can continue to support both event-driven and cron-driven reversal writing.