-
Notifications
You must be signed in to change notification settings - Fork 58
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
Introduce Answer.course_key and deprecate course_id. #131
Conversation
e4fd2d5
to
f0d0ce5
Compare
Migration locks the table, which is not desired since the production table on edx.org is quite large. The course_id field will be extended in a multiple steps instead. For more info see: - https://github.com/edx/edx-platform/pull/14013 - #131
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.
👍 @mtyaka
- I tested this on my devstack:
- Installed problem-builder v2.6.1
- Created a course, and imported the Problem Builder Demo Course.
- Answered several questions.
- Created a test course with a very long course ID
- Added Unit with Problem Builder and a Long Answer element - watched it fail.
- Installed problem-builder from this PR
- Ran migrations
- Verified that my answers are still present in the Demo course.
- Verified that the Long Answer element in the course with the very long course ID is working now, and I can submit answers to it.
- I tested this on the sandbox instance:
- Created a course with a course ID > 50 chars
- Added Unit with Problem Builder and a Long Answer element.
- Verified that I could answer the question in the LMS.
- I read through the code
-
I checked for accessibility issues - Includes documentation
# course_key is the new course_id replacement with extended max_length. | ||
# We need to allow NULL values during the transition period, | ||
# but will be set to null=False in next release. | ||
course_key = models.CharField(max_length=255, db_index=True, null=True) |
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.
Will the uniqueness constraint ('student_id', 'course_key', 'name')
get us into trouble if all the initial course_key
values are null?
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.
No, multiple NULL
values are not considered to be identical to each other so they don't violate the uniqueness constraint.
This introduces a new 255-char course_key field to the Answer model. The old 50-char course_id field is still present, but is deprecated and will be removed in next release. We cannot simply extend the existing course_id field because we have some large problem_builder_answer tables in production and the migration to extend the column would lock the table causing issues in production. The code is updated so that it uses the new course_key column, but falls back to course_id. In next release, a data migration to copy course_id data to course_key will be provided and the course_id column dropped.
f0d0ce5
to
b855567
Compare
Rebased from f0d0ce5. |
Thank you @pomegranited! |
WHERE name = %s | ||
AND student_id = %s | ||
AND (course_key = %s | ||
OR course_id = %s)''' |
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.
Why are you writing raw SQL here rather than using the ORM?
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.
@maxrothman Only because I was not familiar with django Q
objects and didn't know django ORM supports OR
queries. Thanks for the hint, I opened #136 to convert the raw SQL query to Q
objects - can you please review?
If it looks good, I'll update the version number at https://github.com/edx/edx-platform/pull/14044 and we can finally get that one merged.
It includes support for course keys longer than 50 characters. See: open-craft/problem-builder#131
It includes support for course keys longer than 50 characters. See: open-craft/problem-builder#131
It includes support for course keys longer than 50 characters. See: open-craft/problem-builder#131
This patch introduces a new 255-char
course_key
field to theAnswer
model. The old 50-charcourse_id
field is still present, but is deprecated and will be removed in next release.It removes the previous migration (which was never deployed to production) which extended the existing
course_id
field to 255 characters, which was found to not be the optimal approach on the edx.org production database due to the relevant table's size. We cannot simply extend the existingcourse_id
field because we have some largeproblem_builder_answer
tables in production and the migration to extend the column would lock the table causing issues in production.The code is updated so that it uses the new
course_key
column, but falls back tocourse_id
.In next release, a data migration to copy course_id data to
course_key
will be provided and thecourse_id
column dropped.Below is the output of
sqlmigrate
for the new migration:Next release will add a data migration to copy contents of
course_id
to the newcourse_key
column. EdX will fake this migration and copy data in batches instead.Next release will also drop the deprecated
course_id
column.See https://github.com/edx/edx-platform/pull/14013#issuecomment-261635454 for more information.
Environments: edx.org and edge.edx.org
Merge deadline: ASAP - this is blocking a course on Edge
JIRA tickets: TNL-5932
Discussions: See https://github.com/edx/edx-platform/pull/14013 and JIRA ticket.
Sandbox URL:
Partner information: hosted on edX Edge (Davidson College)
Testing instructions:
Check that you can successfully add a Problem Builder "Long Answer" component to a course with a course key that is longer than 50 character and that answers answered on an older version of problem-builder continue to work correctly with this patch.
Reviewers