Skip to content

Conversation

@mtyaka
Copy link
Member

@mtyaka mtyaka commented Dec 7, 2016

This is a follow-up to #131, addressing #131 (review)

Environments: edx.org and edge.edx.org

Merge deadline: ASAP - this is blocking a course on Edge

JIRA tickets: TNL-5932

Discussions: #131 (review)

Sandbox URL:

Note that this is the same sandbox instance that was used for #131, but was updated to use this branch of problem problem-builder. Version currently installed on the sandbox is e502a55.

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

Q(course_key=course_id) | Q(course_id=course_id)
)
if not answer.course_key:
answer.course_key = answer.course_id

Choose a reason for hiding this comment

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

AFAICT this change won't be saved on the model. Is that intentional?

Copy link
Member Author

@mtyaka mtyaka Dec 7, 2016

Choose a reason for hiding this comment

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

@maxrothman Yes, that's intentional to avoid a DB write. The course_key attribute is set here so that we can assume it's present in other parts of the code, but it does not have to be persisted.

Value of course_id will be copied and persisted to course_key in a data migration. The migration will be faked by edX and performed in batches instead.

I was thinking of doing that migration in a separate release, but I could also add it to this PR if that doesn't complicate things from edX devops perspective. What works better for you?

Edit: In order to get this out ASAP, we will add the migration in a separate release.

@bdero
Copy link

bdero commented Dec 7, 2016

👍

  • I tested this: I tested that long answers are still working with the verified user on the sandbox.
  • I read through the code
  • I checked for accessibility issues Not necessary, this is a minor rewrite.
  • Includes documentation Not necessary, this doesn't affect functionality.

@mtyaka
Copy link
Member Author

mtyaka commented Dec 12, 2016

@maxrothman @edx/devops Please review this. It fixes a bug that is affecting live courses on edx.org.

When this PR is reviewed and merged, we will create a new git tag, update https://github.com/edx/edx-platform/pull/14044 to use the new tag, then hopefully we can get https://github.com/edx/edx-platform/pull/14044 merged and released to edx.org / edge.edx.org quickly.

@mtyaka mtyaka merged commit 426b44e into master Dec 12, 2016
@mtyaka mtyaka deleted the mtyaka/extended-course-key-2 branch December 12, 2016 16:58
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.

4 participants