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: take exec ed course data from course run instead of additional_metadata attempt 3 #966

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

marlonkeating
Copy link
Contributor

Description

Updates references for Exec Ed courses originally contained in the additional_metadata field, to now be taken from course runs. Fixing datetime parsing issue that was found in earlier attempt #912, by incorporating fixes made in #958

Ticket Link

ENT-8248: update custom logic for non-ocm courses in enterprise-catalog

Outstanding Questions

  • Do we still need to process Exec Ed courses differently from OCM courses?

Post-review

  • Squash commits into discrete sets of changes
  • Ensure that once the changes have been deployed to stage, prod is manually deployed

based on normalized_metadata's enroll_by_date
"""
enroll_by_deadline = course_json_metadata.get('normalized_metadata')['enroll_by_date']
enroll_by_deadline_timestamp = parse_datetime(enroll_by_deadline).timestamp()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

One clarifying question from me, relates to @brobro10000's other comments. Thanks for calling out the specific fix in this PR compared to the previous attempt 😄

enterprise_catalog/apps/api/v1/export_utils.py Outdated Show resolved Hide resolved
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.

Totally non-blocking nits, but inclusion would improve clarity, great job 🥳

enterprise_catalog/apps/api/v1/export_utils.py Outdated Show resolved Hide resolved
'weeks_to_complete': weeks_to_complete,
'outcome': outcome,
'advertised_course_run': advertised_course_run,
'content_price': content_price
Copy link
Member

Choose a reason for hiding this comment

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

🥳

enterprise_catalog/apps/api/v1/export_utils.py Outdated Show resolved Hide resolved
enterprise_catalog/apps/api/v1/export_utils.py Outdated Show resolved Hide resolved
enterprise_catalog/apps/api/v1/export_utils.py Outdated Show resolved Hide resolved
…metadata attempt 3

fix: use content_price for populating price csv column

refactor: rename variable for clarity
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