feat: take exec ed course data from course run instead of additional_metadata attempt 4 #988
Conversation
| indexable_course_keys.add(course_metadata.content_key) | ||
| else: | ||
| nonindexable_course_keys.add(course_metadata.content_key) | ||
| except Exception as e: # pylint: disable=broad-exception-caught |
There was a problem hiding this comment.
[inform] Change from #966 to catch issue and prevent it from stopping the whole indexing process, while logging any future parsing issues (along with the culprit's content_key).
| }, | ||
| # Error cases for invalid enrollment_end values |
There was a problem hiding this comment.
[inform] Change from #966 to test two data cases that resulted in the error we saw in Prod: TypeError: fromisoformat: argument must be str
| # The check should fail if the enroll by date isn't present or can't be parsed | ||
| return True |
There was a problem hiding this comment.
[sanity check] If a course doesn't have an enroll-by date, is it semantically correct to return True for _has_enroll_by_deadline_passed? That is, how could the enroll by deadline be passed when there is no deadline present?
I believe this might prevent courses without an enroll-by date from being indexed into Algolia due to the deadline_passed_checker method in _should_index_course; not sure if that's intended.
There was a problem hiding this comment.
Do you know if courses lacking an enroll-by date is a valid scenario, and shouldn't be disqualified from indexing?
There was a problem hiding this comment.
Conceptually, a course without an enroll-by date means there is no deadline to enroll, so as long as the other properties checks within is_course_run_active are fulfilled (e.g., status="published", is_enrollment=True, is_marketable=True) the course should still be discoverable despite null enroll-by date since it's otherwise still enrollable.
We have handled null enroll-by date in at least a couple places in the past, at least within a couple frontend spots, e.g.
- Related to which course runs appear during the assignment allocation flow in Admin Portal (source). That is,
isEnrollByDateWithinThresholdalways returns true if there is no enroll by date. - Related to determining whether an audit enrollment is potentially upgradable in the Learner Portal (source). That is, within
isEnrollmentUpgradeable,isEnrollByLapsedalways returns false when no enroll-by date is present.isEnrollByLapsedis largely synonymous with_has_enroll_by_deadline_passed.
marlonkeating
force-pushed
the
mkeating/ENT-8248-4
branch
from
d9684fe to
787e26f
Compare
…metadata attempt 4 chore: Add space in logging fix: courses without enroll-by date should be indexed
marlonkeating
force-pushed
the
mkeating/ENT-8248-4
branch
from
787e26f to
e79d60d
Compare
Description
Updates references for Exec Ed courses originally contained in the
additional_metadatafield, to now be taken from course runs. This new version fixes datetime parsing issue that was found in earlier attempt #966, and adds additional guardrails and logging around future unexpected parsing errors.Post-review