10000 Enhancement/wrobe0709/list unenrolled submissions by wrobe0709 · Pull Request #1128 · okpy/ok · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Enhancement/wrobe0709/list unenrolled submissions #1128

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Enhancement/wrobe0709/list unenrolled submissions #1128

wants to merge 5 commits into from

Conversation

wrobe0709
Copy link
Contributor
@wrobe0709 wrobe0709 commented Mar 25, 2017 •
edited
Loading

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.

Sumukh
Sumukh reviewed Mar 25, 2017
View reviewed changes
Copy link
Member
@Sumukh Sumukh left a 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()))
Copy link
Member

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

Copy link
Member

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))
Copy link
Member

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

server/templates/staff/course/enrollment/enrollment.unenrolled.html Outdated
{{ unenrolled_submitter.email }}
</a>
</td>
<td class="sid">{{ unenrolled_submitter.id }}</td>
Copy link
Member

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?

@wrobe0709
Copy link
Contributor Author

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 get_students(), get_staff(), and get_participants([LAB_ASSISTANT_ROLE])) I made submissions a set of user objects. Does that look like the best way to do it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sumukh Sumukh Sumukh left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wrobe0709 @Sumukh @colinschoen
0