Skip to content

Conversation

@Kelketek
Copy link
Member

Background: Adds Django 1.8 migrations to Problem Builder. The hope is to get this merged into dogwood. Otherwise any installations which use Problem Builder will have to run an extra management command to make their migration data accurate.

@nedbat This is the PR I was talking about. Will we be able to get someone from Platform to give it a look over? @itsjeyd This is ready for you to review-- if you could check it over when you start your day/before I start mine, that would be helpful.

How to test:

  1. In MySQL: CREATE DATABASE edxapp2;GRANT ALL ON edxapp2.* TO 'edxapp001'@'localhost';
  2. Change cms.auth.json and lms.auth.json to point to the new db, 'edxapp2'.
  3. paver update_db --settings=devstack
  4. Register a new 'staff' user.
  5. Lookup and set is_staff on the user via the Django shell. Save it.
  6. Create a Course with problem builder in it, with a long answer component.
  7. Save, test the long answer component, navigate away, return, verify long answer still has the given text.

@Kelketek
Copy link
Member Author

@nedbat @itsjeyd Please note that the CircleCI builds are not enabled on the release branch-- Travis covers those-- so they won't mark as passed. Only Travis should need to pass here. The master branch is the one that uses CircleCI.

@nedbat
Copy link

nedbat commented Jan 20, 2016

You mention getting this into Dogwood, but problem-builder isn't included in the Open edX named releases: it's referred to from edx-private.txt, so it's installed on edx.org, but generally not in Open edX installations. You can label a revision in the repo as compatible with Dogwood, that should be enough, no?

@Kelketek
Copy link
Member Author

@nedbat I've discussed your point with @bradenmacdonald and we think you are correct on this matter, in which case it is not the emergency we thought it was. It'd still be nice to include the hash update since other teams might base their private.txt on Edx's, but it is not as pressing as we feared. We can put the hash update in master if need be. Either way we'll need to coordinate with devops in order to deploy it.

Aside from this, does the PR look good for you? I'd like to get the +1s necessary for merge. :)

@Kelketek
Copy link
Member Author

@nedbat Or, because it appears to be less critical, is there someone I should ping instead?

@itsjeyd
Copy link
Member

itsjeyd commented Jan 20, 2016

@Kelketek 👍

@doctoryes
Copy link

This migration only creates the Answer model - yet there are two models defined here:
https://github.com/open-craft/problem-builder/blob/master/problem_builder/models.py

What about the other Share model? Also, did these models have South migrations before? If so, when were those deleted?

@Kelketek
Copy link
Member Author

@doctoryes This is a PR against edx-release, not master. edx-release does not contain those models. We need to make migrations for the models release has first, so that we can make the migrations for the other models on master after merging in the commit here.

@doctoryes
Copy link

I see - I was looking at the wrong branch. The second part of my question still stands:

Did this model have South migrations before? If so, when was it deleted?

@bradenmacdonald
Copy link
Member

@doctoryes South migrations were never deleted; they coexist and are present on this branch in problem_builder/south_migrations

@doctoryes
Copy link

So those South migrations were never merged to edx-release? Has problem-builder been deployed from the edx-release branch? Sorry for all the questions - just want to understand this repo's release pattern.

@bradenmacdonald
Copy link
Member

For this repo:

  • master is used on McKinsey and many other instances. It's managed by OpenCraft.
  • edx-release is the only branch that has been through a review by edX employees (plus OpenCraft) and is the only branch that has ever been deployed on edx.org
  • Whenever we make changes on edx-release we merge them back to master. On the other hand, master currently has a bunch of changes that haven't been approved for use on edx.org, so it is ahead of edx-release.

So yes, problem builder has always been deployed from the edx-release branch, at least for edx.org. When the Django 1.8 upgrade happened, I guess people didn't notice that this XBlock had South Migrations, so once that upgrade merged, django started using syncdb to manage this repo instead of migrations (since it had only South migrations). So now we are trying to fix that and restore migrations by adding in proper django 1.8 migrations.

@doctoryes
Copy link

Thanks for the explanation! Makes perfect sense...

👍

Kelketek added a commit that referenced this pull request Jan 21, 2016
Create Django migrations, deprecate South.
@Kelketek Kelketek merged commit cecdbaf into edx-release Jan 21, 2016
@Kelketek Kelketek deleted the django-migrations branch January 21, 2016 16:24
Kelketek added a commit to openedx/openedx-platform that referenced this pull request Jan 21, 2016
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.

6 participants