-
Notifications
You must be signed in to change notification settings - Fork 317
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
Add a general lms user id column to the student table #1036
Conversation
So here is the error msg when I upgrade the gradebook.db, yet it still adds the column and I can run
Running it again I get a
|
Looks good, thanks! I'll try to have a look and see why the alembic update fails. |
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.
So the error appears to be because you enforce the column to be unique (a form of constraint), but SQLite doesn't allow you to add new constraints after the table has already been created. It seems like it would be possible to work around this using alembic batch operations (https://alembic.sqlalchemy.org/en/latest/batch.html), but it would be messy and hard to implement. Thus, I would suggest just dropping the unique requirement, and then everything should work as expected.
Additionally, can you update the API docs to reflect the addition of the new column name?
I removed the unique constraint. I'm not sure why the documentation is not being generated. I ran the |
You have to manually edit the rst file here to add the column name: https://github.com/jupyter/nbgrader/blob/master/nbgrader/docs/source/api/models.rst |
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.
Looks good! I guess the last thing is just to update the tests so that they check for the lms_user_id as well.
I am not sure why the tests are failing as they seem to pass for me locally. Can you rebase (or merge master into your branch) to bring this up-to-date with the master branch of the repo? I think this time you shouldn't require too many changes, probably the main thing is you'll need to change the |
Adds a lms_user_id to the ngrader db:
See issue: #1035
How to use:
nbgrader db student add <some student.id> --lms-user-id="'123'"
Notice the extra "" if your lms user id contains only numbers.
Additions that can be made:
nbgrader/nbgrader/plugins/export.py
Line 72 in 7a81a40
Possible issues:
nbgrader db upgrade oldgradebook.db
but it still put in the column. Maybe someone else could also test it.