feat: take exec ed course data from course run instead of additional_metadata (take 2) #912
Conversation
marlonkeating
force-pushed
the
mkeating/ENT-8248-2
branch
2 times, most recently
from
47e3477
to
181df4e
Compare
There was a problem hiding this comment.
Thanks for updating 😺 @brobro10000 and I did a pair PR review on the changes and left some additional questions/clarifications/suggestions/etc.
Let us know if you want to rubber duck on anything!
| if course_json_metadata.get('course_type') == EXEC_ED_2U_COURSE_TYPE: | ||
| additional_metadata = course_json_metadata.get('additional_metadata') or {} | ||
| registration_deadline = additional_metadata.get('registration_deadline') | ||
| if registration_deadline: | ||
| enrollment_end = advertised_course_run.get('end') or {} | ||
| if enrollment_end: | ||
| enroll_by_deadline_timestamp = datetime.datetime.strptime( | ||
| registration_deadline, | ||
| enrollment_end, | ||
| '%Y-%m-%dT%H:%M:%S%z', | ||
| ).timestamp() | ||
| else: | ||
| enroll_by_deadline_timestamp = _get_verified_upgrade_deadline(advertised_course_run) | ||
|
|
||
| return enroll_by_deadline_timestamp < localized_utcnow().timestamp() |
There was a problem hiding this comment.
[clarification] Could we use the normalized_metadata that exists from the course_json_metadata passed into this method? That is, could we refer to the consolidated content metadata across course types already exposed without having to introduce similar conditional branching logic based on course_type?
normalized_metadata has this branching logic abstracted away.
There was a problem hiding this comment.
From what I can see we should be able to use normalized_metadata... I just read through https://github.com/openedx/enterprise-catalog/blob/master/docs/decisions/0007-normalized-metadata-course-types.rst, and is it still the position that we aren't making an active effort to change over to using normalized_metadata in business logic, it is available for use where convenient?
There was a problem hiding this comment.
When normalized_metadata was introduced, we limited the scope to be an additive-only change, with the plan to incrementally migrate to normalized_metadata anywhere we had existing branching logic between course types (i.e., OCM vs EE) around metadata like dates/price/etc. We have switched over to normalized_metadata in several places already on various consuming services, so this is another incremental opportunity to simplify the existing implementation by relying on the abstraction. The hope is that relying on normalized_metadata makes content metadata easier to reason about when parsing it within business logic such as here.
marlonkeating
force-pushed
the
mkeating/ENT-8248-2
branch
3 times, most recently
from
6c9fe89
to
47055b6
Compare
| return upgrade_deadline or self.course_run_metadata.get('enrollment_end') | ||
|
|
||
| enrollment_end = self.course_run_metadata.get('enrollment_end') | ||
| return min(filter(None, [upgrade_deadline, enrollment_end]), default=None) |
| def _days_from_now_ts(days_from_now=0): | ||
| deadline = _days_from_now(days_from_now) | ||
| return {'str': deadline, 'timestamp': to_timestamp(deadline)} | ||
|
|
||
|
|
||
| TOMORROW, NEXT_WEEK, NEXT_MONTH = [_days_from_now_ts(days) for days in [1, 7, 30]] | ||
| NEXT_MONTH_DATE = _days_from_now(30) | ||
|
|
||
|
|
| if 'normalized_metadata' in course_json_metadata: | ||
| enroll_by_deadline = course_json_metadata['normalized_metadata']['enroll_by_date'] | ||
| enroll_by_deadline_timestamp = datetime.datetime.strptime( | ||
| enroll_by_deadline, | ||
| '%Y-%m-%dT%H:%M:%S%z', | ||
| ).timestamp() | ||
| elif course_json_metadata.get('course_type') == EXEC_ED_2U_COURSE_TYPE: |
There was a problem hiding this comment.
[curious/suggestion] When will normalized_metadata not exist in course_json_metadata, where the fallback elif is needed? I think the conditional around whether normalized_metadata exists and the fallback elif could probably be removed 🤔 Rationale: normalized_metadata is added to ContentMetadata's json_metadata during the update_content_metadata so I believe it's a fair assumption it will always exist (source).
Given this, I believe the pre-existing Exec Ed vs. OCM checks here and below (i.e., _get_verified_upgrade_deadline) could be removed to only rely on normalized_metadata as the logic below is essentially duplicative of normalized_metadata.
There was a problem hiding this comment.
Got it, removed the check so we're only going by normalized_metadata now.
| @@ -1103,6 +1107,7 @@ def _get_course_run(full_course_run): | |||
| 'availability': full_course_run.get('availability'), | |||
| 'start': full_course_run.get('start'), | |||
| 'end': full_course_run.get('end'), | |||
| 'enrollment_end': full_course_run.get('enrollment_end'), | |||
There was a problem hiding this comment.
[clarification] Is enrollment_end already abstracted into the enroll_by field below? I would consider this enrollment_end deprecated similar to upgrade_deadline, in favor of relying on the transformed "interface" for enroll_by.
There was a problem hiding this comment.
Good point! Removed it.
| @@ -130,12 +130,18 @@ def get_end_date(self, obj) -> str: # pylint: disable=unused-argument | |||
| def get_enroll_by_date(self, obj) -> str: # pylint: disable=unused-argument | |||
| if not self.course_run_metadata: | |||
| return None | |||
|
|
|||
| if self.course.is_exec_ed_2u_course: | |||
There was a problem hiding this comment.
Nice cleanup here to short-circuit for the Exec Ed case!
marlonkeating
force-pushed
the
mkeating/ENT-8248-2
branch
2 times, most recently
from
8eccd3a
to
5b721e1
Compare
…metadata attempt 2 fix: calculate enroll_by correctly test: fix unit tests pt2 fix: use normalized_metadata for deadline check fix: remove logging fix: use normalized_metadata for calculating course deadlines
marlonkeating
force-pushed
the
mkeating/ENT-8248-2
branch
from
5b721e1
to
e9e9e1e
Compare
Description
Updates references for Exec Ed courses originally contained in the
additional_metadatafield, to now be taken from course runs. Fixing issues that were found in earlier attempt: #785Ticket Link
ENT-8248: update custom logic for non-ocm courses in enterprise-catalog
Outstanding Questions
Post-review