-
Notifications
You must be signed in to change notification settings - Fork 5
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
Section override fix #178
base: prefect-by-assignment
Are you sure you want to change the base?
Section override fix #178
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.
approving -- I didn't see anything obviously wrong, but you'll definitely need to monitor what this does the first few runs to make sure it didn't break any corner cases
@@ -92,6 +92,7 @@ def _canvas_get_people_by_type(config, course_id, typ): | |||
return [ { 'id' : str(p['user']['id']), | |||
'name' : p['user']['name'], | |||
'sortable_name' : p['user']['sortable_name'], | |||
'course_section_id' : str(p['course_section_id']), |
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.
does this always exist? I imagine for instructors it might not? You should probably check existence and fill with None
otherwise
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.
also try probing the rest API directly to find out. Easy way to do that is to put the URL that I send via requests
to get lists of people directly into the URL bar in your browser instead. You'll see raw JSON response in the browser, just look through that to check.
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.
I checked the API docs and it appears to be a required field https://canvas.instructure.com/doc/api/enrollments.html. Also checked list of enrollments, looked like it was there for everyone.
snaps.append( {'due_at' : asgn['due_at'], | ||
'name' : _get_snap_name(config.course_names[course_id], asgn, None), | ||
'student_id' : None}) | ||
if asgn['due_at'] is not None: |
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.
this change might be a bit tricky. Consider an assignment with no overrides but the instructor forgot to set the deadline -- we don't want to silently ignore that problem.
But maybe that would have thrown an exception elsewhere anyway?
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.
Might be worth raising here if there is an assignment that doesn't have any overrides and doesn't have due_at/etc. Then from here on out we can assume if due_at is missing it's because their are overrides
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.
I made it so it throws an error here if due_at is missing and there are no overrides in commit 465bc4d.
for student_id in override['student_ids']: | ||
snaps.append({'due_at': override['due_at'], | ||
'name' : _get_snap_name(config.course_names[course_id], asgn, override), | ||
'student_id' : student_id}) |
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.
I think this should all work OK.
# Add check for SD student, save for later removal. | ||
if student['id'] not in subm_info[assignment['id']]: | ||
subms_to_remove.append(subm) | ||
continue |
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.
This is an interesting change. I hadn't considered SD students -- but basically in the past rudaux just treated them as regular students and gave them 0 for missing work (but we ignore those grades at the end of the semester anyway so it doesnt matter)
So I'm not sure this change is needed...and I do worry a bit that
if student['id'] not in subm_info[assignment['id']]:
might accidentally remove a regular student. What is in subm_info
? I forget
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.
(If you're certain this won't remove any regular students i'm happy w it tho)
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.
I think sumb_info is a list of dictionaries, one for each assignment with one key assignment id whose value is another dict with submissions for each student by student id.
The issue was that the SD students weren't assigned anything (probably because they aren't put into a section) which caused a KeyError with student id when trying to access subm_info[assignment['id']][student['id']]. I think any regular student should pass the check if they are properly assigned to worksheets/tutorials.
…ssion." This reverts commit c0e43c9.
Changes to rudaux autoextension and snapshot flows such that they take into account course section overrides.
Fix grading flow to take into account different course directories for R/python sections.