-
Notifications
You must be signed in to change notification settings - Fork 84
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
Enhancement/wrobe0709/list unenrolled submissions #1128
base: master
Are you sure you want to change the base?
Enhancement/wrobe0709/list unenrolled submissions #1128
Conversation
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.
Nice. I have left some comments about performance - but this looks good otherwise!
(I haven't tried it locally but I will soon)
server/controllers/admin.py
Outdated
courses, current_course = get_courses(cid) | ||
|
||
submissions = set(b.submitter_id for b in (Backup.query.join(Backup.assignment).filter(Assignment.course_id == cid).all())) | ||
enrollment = set(e.user_id for e in (Enrollment.query.filter(Enrollment.course_id == cid).all())) |
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.
These two lines are a bit too long. I would suggest putting the queries on their own lines
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.
Maybe you can use course.get_students here? It also does a joined load with Users - so you can just do e.user without the cost of another query
server/controllers/admin.py
Outdated
|
||
for s in submissions: | ||
if s not in enrollment: | ||
unenrolled_submitters.append(User.query.get(s)) |
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.
See above - you should be able to do e.user if you use enrollment objects directly
{{ unenrolled_submitter.email }} | ||
</a> | ||
</td> | ||
<td class="sid">{{ unenrolled_submitter.id }}</td> |
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.
The ID isn't particularly useful (it's our database ID) Maybe their name could go here instead?
I'm pulling in staff and lab assistants because I was getting them in the list of un-enrolled submitters. I'm assuming we don't want them in that list and that we just want students? Because I can get the user objects from |
Fixes #1070
If a student is not enrolled in a class but submitting assignments for the class their submissions won't be seen by the staff.
This adds a view called "Un-Enrolled Submitters" which allows staff members to see any students who are submitting assignments to the course but are not enrolled.