-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Added pacing field Course API #11985
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| # -*- coding: utf-8 -*- | ||
| from __future__ import unicode_literals | ||
|
|
||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('course_overviews', '0009_readd_facebook_url'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RemoveField( | ||
| model_name='courseoverview', | ||
| name='facebook_url', | ||
| ), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see the code change associated with this operation. Should it have been committed previously?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It appears the followup work for #11673 was not completed. @nasthagiri is there a ticket for this?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit confused about this. The original PR did have migration code for it. However, since then, the devOps team made a change to revert it in order to support production deployment to multiple workers, concurrently running different versions of the code. @jibsheet At this point, can we go ahead and re-add the removal of this field in the migration file?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, the original PR had migration code in it to remove that field from the table without previously having removed it from the model, which would have broken production. That RemoveField calls was removed. You can look at 0008 and 0009 for details. Now that we've removed the model field in a release, it should be safe to remove the column in another release. 👍 from me, provided we've done the usual work when migrating this table to deal with row versions. |
||
| migrations.AddField( | ||
| model_name='courseoverview', | ||
| name='self_paced', | ||
| field=models.BooleanField(default=False), | ||
| ), | ||
| ] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this added to an existing migration file?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a new migration file. See my comment about #11673. |
||
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.
Is this a standard terminology for MOOCs?
@catong Did you have any suggestion on naming for this?
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.
In documentation we refer to "self-paced" and "instructor-paced" courses, so those values are fine.