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: sync full metadata for restricted courses #985

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

iloveagent57
Copy link
Contributor

@iloveagent57 iloveagent57 commented Oct 21, 2024

ENT-9597

Local Testing

Set up a restricted course run locally

It'll be associated with DemoX: https://2u-internal.atlassian.net/wiki/spaces/SOL/pages/1317306375/Setting+up+restricted+runs+locally

Enable the setting

In your private.py settings file, set SHOULD_FETCH_RESTRICTED_COURSE_RUNS = True. Optionally set SHOULD_INDEX_COURSES_WITH_RESTRICTED_RUNS = True if you'd like to test against a dev algolia index, too.

Sync full course metadata

  1. make worker-restart worker-logs
  2. In a different shell, do make app-shell and then ./manage.py update_content_metadata --force (note that this mgmt command runs both the update_catalog_metadata task and the update_full_content_metadata_task.)
  3. Observe that a restricted course exists here: http://localhost:18160/admin/catalog/restrictedcoursemetadata/ and that it has a relationship to a restricted run.
  4. Optionally update a test algolia index with ./manage.py reindex_algolia --force --no-async

Post-review

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

@iloveagent57 iloveagent57 force-pushed the aed/sync-full-restricted-courses branch 2 times, most recently from 73cd033 to f92b203 Compare October 21, 2024 16:37
Comment on lines -347 to -351
program_content_keys = create_course_associated_programs(
course_metadata_dict.get('programs', []),
metadata_record,
)
_update_full_content_metadata_program(program_content_keys, dry_run)
Copy link
Contributor Author

@iloveagent57 iloveagent57 Oct 21, 2024

Choose a reason for hiding this comment

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

Only change here was yanking this out of _update_single_full_course_record() and into the calling function, to better isolate responsibilities and avoid duplicate work.

@iloveagent57 iloveagent57 force-pushed the aed/sync-full-restricted-courses branch 8 times, most recently from d821a22 to e04d0f1 Compare October 23, 2024 16:53
Comment on lines +13 to +24
operations = [
migrations.AddField(
model_name='restrictedcoursemetadata',
name='restricted_run_allowed_for_restricted_course',
field=models.ManyToManyField(through='catalog.RestrictedRunAllowedForRestrictedCourse', to='catalog.contentmetadata'),
),
migrations.AlterField(
model_name='restrictedrunallowedforrestrictedcourse',
name='course',
field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, to='catalog.restrictedcoursemetadata'),
),
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a SQL no-op.

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.

Looking good to me, left some nit-level comments to consider/clarify (have not tested locally).

enterprise_catalog/apps/catalog/models.py Outdated Show resolved Hide resolved
enterprise_catalog/apps/catalog/models.py Outdated Show resolved Hide resolved
enterprise_catalog/apps/catalog/models.py Show resolved Hide resolved
enterprise_catalog/apps/catalog/models.py Show resolved Hide resolved
@iloveagent57 iloveagent57 force-pushed the aed/sync-full-restricted-courses branch 5 times, most recently from c21303f to 8c0daa7 Compare October 24, 2024 14:48
@iloveagent57
Copy link
Contributor Author

Made some recent updates to this branch:

  • the update_course_run_relationships() function was previously deleting and re-creating course run ContentMetadata records, instead of just resetting M2M relationships. This has been fixed to now use a M2M .set() call so only the relationships are re-created, and not the CM records.
  • added a update_or_create_run() helper, specifically to ensure that we pop defaults prior to update_or_create() calls, which really is necessary, as described in the related code comment.
  • adds more logging.

@iloveagent57 iloveagent57 force-pushed the aed/sync-full-restricted-courses branch from 8c0daa7 to 1b33e17 Compare October 28, 2024 15:43
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.

LGTM!

"""
Given a fetched dictionary of course content metadata and an option `course_review` record,
updates an existing course `ContentMetadata` instance with the "full" dictionary of `json_metadata`
for that course.
"""
# Merge the full metadata from discovery's /api/v1/courses into the local metadata object.
metadata_record._json_metadata.update(course_metadata_dict) # pylint: disable=protected-access
if not skip_json_metadata_update:
Copy link
Member

Choose a reason for hiding this comment

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

Nice

@iloveagent57 iloveagent57 merged commit bf5898a into master Oct 29, 2024
4 checks passed
@iloveagent57 iloveagent57 deleted the aed/sync-full-restricted-courses branch October 29, 2024 13:37
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.

2 participants