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

fix: use course-run specific metadata #633

Merged
merged 3 commits into from
Feb 11, 2025

Conversation

hamzawaleed01
Copy link
Member

Description:
Add a description of your changes here.

Jira:
ENT-9890

Merge checklist:

  • ./manage.py makemigrations has been run
    • Note: This must be run if you modified any models.
      • It may or may not make a migration depending on exactly what you modified, but it should still be run.

Post merge:

  • Ensure that your changes went out to the stage instance
  • Deploy to prod instance

@hamzawaleed01 hamzawaleed01 force-pushed the ENT-9890/fix-course-run-metadata branch 3 times, most recently from e756cd9 to e4170b2 Compare February 7, 2025 13:25
@hamzawaleed01 hamzawaleed01 force-pushed the ENT-9890/fix-course-run-metadata branch from e4170b2 to 4e6a658 Compare February 7, 2025 13:31
enterprise_access/apps/content_assignments/tasks.py Outdated Show resolved Hide resolved
enterprise_access/utils.py Outdated Show resolved Hide resolved
Comment on lines 265 to 266
for course_run in course_runs:
if course_run.get('key') == preferred_course_run_key:
Copy link
Member

Choose a reason for hiding this comment

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

nit: could opt to use next to extract the desired course run, if you wanted. might be a bit more "pythonic", e.g.:

course_run = next((run for run in course_runs if run.get('key') == preferred_course_run_key), None)
if not course_run:
    logger.warning(
        'Course run metadata not found for preferred course run key %s in content metadata %s', 
        preferred_course_run_key,
        content_metadata.get('key'),
    )
    return course_run

(Opted to add logging when the preferred_course_run_key is not found in the list of course_runs).

enterprise_access/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.

Great job with the abstraction with a few suggestions.

enterprise_access/utils.py Outdated Show resolved Hide resolved
enterprise_access/utils.py Outdated Show resolved Hide resolved
@hamzawaleed01 hamzawaleed01 force-pushed the ENT-9890/fix-course-run-metadata branch 3 times, most recently from f3cf90f to eeb5bfc Compare February 7, 2025 15:44
@hamzawaleed01 hamzawaleed01 force-pushed the ENT-9890/fix-course-run-metadata branch from eeb5bfc to 6edc5fa Compare February 7, 2025 15:51
@hamzawaleed01
Copy link
Member Author

Hey @brobro10000 @adamstankiewicz , thanks for the awesome insights! ❤️

I've resolved the comments I was sure about and left the rest for you folks to review.

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.

There is one final optimization that can be included to avoid a potential None value return for content metadata.

enterprise_access/utils.py Outdated Show resolved Hide resolved
@hamzawaleed01 hamzawaleed01 force-pushed the ENT-9890/fix-course-run-metadata branch from 4023cf9 to 75845fb Compare February 10, 2025 14:13
@hamzawaleed01 hamzawaleed01 force-pushed the ENT-9890/fix-course-run-metadata branch from 75845fb to 6993ab3 Compare February 10, 2025 14:20
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.

Great job 👍🏽

@hamzawaleed01 hamzawaleed01 merged commit 942bd29 into main Feb 11, 2025
3 checks passed
@hamzawaleed01 hamzawaleed01 deleted the ENT-9890/fix-course-run-metadata branch February 11, 2025 06:40
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