Skip to content
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

Restrict access for students to different courses #1040

Merged
merged 45 commits into from
May 29, 2019

Conversation

sigurdurb
Copy link
Contributor

Multiple courses

This is a more finalised version of #893 with permission from zonca to post this with his code here as a new PR.

Add to documentation:

  • Only api_tokens made for users in the admin_users list will work.

  • You must generate an 'api_token' with with the command jupyterhub token <some admin user>. The default api token that is created even if the user that you are using nbgrader in is in the admin_users list does not work, so generate it manually with that command wherever your jupyterhub.sqlite file cd nbis.

  • For teachers/TA's have 'formgrade-<course-id>' as the name of your group.

  • For the students have 'nbgrader-<course-id>' as the name, you can also ommit this because it is made for you when adding a student if they are not already in Jupyterhub config when the server is started.

Here is an example how to put both the 'api_token' and the 'formgrade-<course-id>'group in when you create the service.

c.Jupyterhub.load_groups = {
     'formgrade-gagr':['sigurdurb', 'gagr'],
     'nbgrader-gagr':[],
}
c.JupyterHub.services = [{'name':'gagr20181',
                          'url':'http://127.0.0.1:9993',
                          'api_token':'4d60feb9df9f480db1eb8b77a4ef81ad',
'command':['jupyterhub-singleuser',
           '--group=formgrade-gagr',
           '--debug',
          ],
        'user':'gagr',
        'cwd':'/home/gagr',
}]

Note: I only tested this on Jupyterhub 0.8.1 on Ubuntu 16.04, still have to test this without Jupyterhub.
But I think it is fairly safe to say that for those who need they can run this with Jupyterhub now.

@sivasquez
Copy link

Hey! With @FranLucchini we are also interested in solving the issue of multiple courses for multiple students, although our approach is different than yours.

I'd like to ask you some questions about your design, to see how we can continue:

  • How does your solution scale when the number of courses / students / teachers grow up? Using groups is an easy and fast way, but I fear that when you manage numbers in the hundreds it may be difficult to control.

  • Is there any kind of history of a given student's course? My solution is thought more for implementation in an University, so it'd be important to have records (like, in how many courses I've been TA)

Thanks in advance!

@sigurdurb
Copy link
Contributor Author

sigurdurb commented Nov 10, 2018

Answer to first question:
No problem scaling that I see.
No student needs to have a course_id in their nbgrader_config anymore. But the course needs to have one(or stored in the course database)
Groups need to be named nbgrader-<course_id> for students and formgrade-<course_id> for instructors. So I just use the course ids + year and semester(1,2,3) so even though the same course is setup next year or next semester I differentiate them that way.
Just set the formgrade- group in your config or add the group and users through the hubs api. No need to add the nbgrader- group because it is done automatically when you add the first student.
The students are automatically added to the Jupyterhub nbgrader-<course_id> group when you add them to a course database, such as nbgrader db student add <student id> so no need to have that in the config. aka, no need to manually add those couple hundred students, just write a short script that gets the students from your LMS system if you have one. Removing students from a jupyterhub group is a similar command, nbgrader db student remove <student_id>.

Answer to second question:
This PR does not do that; store any historical data. That is something that the Learning management system at my University already handles quite well, we use Canvas LMS. It shows who is a TA etc and keeps all that data handy if we need to request it. So I see no need to have that info in both places. But ofcourse the student ids and teachers ids are stored in the jupyterhub database and students also in the course database.

@sigurdurb
Copy link
Contributor Author

sigurdurb commented Nov 20, 2018

Forgot to add that we also need to(add to docs):

  • Add the system user that runs the course service to the formgrade-<course_id> group.

This is needed for the released assignments to be listed correctly in the formgrader. Because this line only checks if the user is in either group to retrieve the course_id names.

Copy link
Member

@jhamrick jhamrick left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks!

Two high-level comments:

  1. I think it might be nice to create a new file, jupyterhub_api.py, that includes all the JupyterHub-specific logic for querying the API, adding students to groups, removing students, etc. Currently it is a bit spread out across both api.py and utils.py and I think it would be a little more modular if it were all in the same place. This would mean moving methods from the gradebook like add_student_to_group into this new file.

  2. Then, rather than trying to access the JupyterHub API everytime the students in the database are modified (e.g. in add_student), I think the Gradebook should accept another constructor argument called use_jupyterhub or something like that. Then, you should only call methods like add_student_to_group if that variable is set to True. Otherwise, this code prints out a lot of warnings that people who don't use JupyterHub won't care about (but which would be useful debug statements if JupyterHub is indeed being used).

Also, rather than using print statements, ideally the code could use log statements.

return course_id_record

@property
def course_id(self):
Copy link
Member

Choose a reason for hiding this comment

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

Rather than returning an error, I might have this return None.

@@ -1069,6 +1086,47 @@ def close(self):
self.db.remove()
self.engine.dispose()

def set_course_id(self, course_id, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, so it seems like the current functionality is:

  1. If course_id is already set, silently fail to do anything.
  2. Otherwise, set the course_id

I think a better logic might be:

  1. If the course_id is already set, throw an error.
  2. Otherwise, set the course_id

And then any code calling this function (e.g. in the init) should check whether the course id has been set already by accessing self.course_id, and only setting it if necessary.

@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/jupyter-nbgrader-hackathon-topics/1040/1

@jhamrick jhamrick changed the base branch from master to multiple-classes-dev May 29, 2019 15:55
@jhamrick jhamrick changed the title [WIP] Multiple courses Restrict access for students to different courses May 29, 2019
@jhamrick
Copy link
Member

Thanks!!

@jhamrick
Copy link
Member

The tests are going to fail since I think this actually breaks things for the non-JupyterHub case, but I am going to go ahead and merge since we're using a development branch anyway. I think my next plugin PR will alleviate the problems and make things more compatible with and without JupyterHub 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants