Skip to content

Conversation

@mtyaka
Copy link
Member

@mtyaka mtyaka commented Jan 12, 2017

This is a follow-up to #131. In #131, we deprecated the Answer.course_id field and replaced it with a new Answer.course_key field with extended length to accommodate courses with course keys longer than 50 characters (for example some CCX courses on edx.org have course keys longer than 50 characters).

This PR includes a migration that copies contents of course_id column into course_key, for any Answer object where course_key is NULL (these are Answer objects that were created before problem-builder v2.6.5 was deployed).

NOTE: Before deploying v2.6.6 to production, edX will have to fake this migration and manually copy course_id values into course_key in batches because the production problem_builder_answer table is large and performing the migration in one go would degrade performance!

Next, this PR also includes a migration that makes the course_key mandatory and removes transition code that supports both course_id and course_key columns.

The course_id column is only deprecated at this point, but could safely be dropped in the future.

Environments: edx.org and edge.edx.org

JIRA tickets: TNL-5932

Discussions: See https://github.com/edx/edx-platform/pull/14013 and JIRA ticket.

Sandbox URLs:

Built with c733391.

Partner information: hosted on edX Edge (Davidson College)

Testing instructions:

This patch should not change functionality in any way. Verify that long Answer objects still work correctly on the sandbox.

Reviewers

The migration copies content of the Answer.course_id column into the new
extended Answer.course_key column.

It skips Answer records that already have the course_key column set.
Previous migration populates any null course_key fields, so we can now
change the column to not allow NULL values.

We make the deprecated course_id nullable, to be able to start ignoring
it later.
Now that all data has been migrated to the new course_key column, we can
remove the temporary transition code.
@mtyaka mtyaka force-pushed the mtyaka/copy-course-keys-migration branch from 0b5554c to c733391 Compare January 13, 2017 07:29
@mtyaka mtyaka changed the title WIP: Copy course_id into course_key and remove temporary transition code Copy course_id into course_key and remove temporary transition code Jan 13, 2017
@bdero
Copy link

bdero commented Jan 19, 2017

Looks good @mtyaka. :) 👍

  • I tested this: Long answers are still working as expected with course names > 50 characters (on the sandbox).
  • I read through the code
  • I checked for accessibility issues There are no user facing changes.
  • Includes documentation Not necessary - this PR is a code base touch up that eliminates tech debt. It doesn't change anything external facing.

@mtyaka
Copy link
Member Author

mtyaka commented Jan 20, 2017

@edx/devops Can somebody please review this follow-up to the hotfix we did for problem-builder. It removes the temporary transition code and includes a migration that copies values from the deprecated column into the new, extended course_key column - this migration will have to be faked and performed in batches when deploying to edx.org!.

/cc @adampalay

@adampalay
Copy link

code looks ok to me! But this should definitely get ops approval :)

@mtyaka mtyaka merged commit 52045c0 into master Jan 25, 2017
@mtyaka mtyaka deleted the mtyaka/copy-course-keys-migration branch January 25, 2017 08:13
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.

5 participants