-
Notifications
You must be signed in to change notification settings - Fork 171
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: csv loader updates for multi course runs of External LOBs #4177
Conversation
9c7996c
to
35070ac
Compare
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/tests/mixins.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/tests/test_csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/tests/test_csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/templates/course_metadata/email/loader_ingestion.html
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/tests/test_csv_loader.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally on small set of CSV, looks good.
- Ingested CSV, first row was going to create variant and it did.
- Re-run csv, no new variants
- Changed first row to create new variant (variant id change + start/end date change).
- Loader run created variant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of comments to address. Looks good to me otherwise.
if not course_run and is_course_run_created: | ||
course_run = CourseRun.objects.filter_drafts(course=course).order_by('created').last() | ||
return course_run, is_course_run_created |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just move this inside the try block? Seems redundant to add a separate check later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup we can move it to try block, but it will become unrelated there because it can't raise an exception
if hasattr(exc, 'response'): | ||
exception_message = exc.response.content.decode('utf-8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do this instead:
exception_message = getattr(exc, 'response', str(exc).encode('utf-8'))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the same convention here for the exception message as followed in the preceding methods
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
0512818
to
1158f7f
Compare
e73cabb
to
0837a93
Compare
PROD-3741
This PR adds CSV Loader updates to support multi course runs in case of external LOBs.
Details:
A New Course Run will be created if none of the existing active run have same start & end date or same variantId coming from the CSV. Otherwise it will pick the existing run and update course run in it. To enable editing of variant_id, we need to enable waffle switch from django admin
is_course_run_variant_id_editiable
Testing Instruction:
Run
ingest_getsmarter_data
on local by setting GET_SMARTER_CREDENTIALS inprivate.py
.course_metadata.is_course_run_variant_id_editable
with active value.private.py
foringest_getsmarter_data
cmd.ingest_getsmarter_data
mgmt cmd