fix importing bug (STUD-1599) - #3626
Conversation
There was a problem hiding this comment.
I think instead of adding this extra condition you can just make course_prefix (#L179) to be:
course_prefix = u'{0.org}/{0.course}/'.format(course_location) a slash at the end of course to match the condition.
There was a problem hiding this comment.
Also since this can only happen when a course is imported through management command as in case of import through studio the course_id remains same, so we can move (#L179 - #L205) inside the condition
if target_location_namespace is None: since target_location_namespace is None in case of management command.
There was a problem hiding this comment.
@zubair-arbi , this can happen if imported through studio too. import_from_xml is also called here: https://github.com/edx/edx-platform/blob/master/cms/djangoapps/contentstore/views/import_export.py#L219
There was a problem hiding this comment.
@adampalay You are right but I was talking about the extra validation of course_id (#L179 - #L205), which I guess will be required for only courses imported through management command.
There was a problem hiding this comment.
For courses imported by studio have target_location (course_id) same after import. But if you import through management command then import view creates a new course with target_location same as the importing course.
There was a problem hiding this comment.
hmmm
on prod we're seeing this with studio. I can't reproduce it locally though
There was a problem hiding this comment.
@adampalay I have added reproduction steps on jira ticket.
|
Suggestion: we can call unique course_id validation a bit earlier when the course is about to get imported (perhaps at https://github.com/edx/edx-platform/blob/master/common/lib/xmodule/xmodule/modulestore/xml.py#L524 when course_id is calculated for imported course). This way we can give response in case of unique course key violation without importing all content. |
|
This looks good to me. 👍 |
|
@cahrens , may you please take a look too? |
|
I am out. Please ask Nimisha or Don. |
|
Ok, @nasthagiri or @dmitchell ? |
There was a problem hiding this comment.
replace this line w/
course_org_lower = course_location.org.lower()
course_cat_lower = course_location.course.lower() # cat as abbreviation for catalog, not sure if best name
|
@dmitchell another round? |
There was a problem hiding this comment.
Ah, I've been on opaque_keys so long that I forgot that course.id is not an object! For now, use course.location.course instead of course.number. If org works as you have it, fine.
There was a problem hiding this comment.
Got it. I'll change course.org to course.location.org for consistency too
|
👍 if you update the accessors to be slightly closer to opaque keys. |
add target_location_namespace check
fix importing bug (STUD-1599)
5 participants
@zubair-arbi