-
Notifications
You must be signed in to change notification settings - Fork 8
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
Quiz submission limit and Per Student Overrides #76
base: master
Are you sure you want to change the base?
Conversation
d34fe99
to
bb81025
Compare
First draft of allowing two submission caps for before and after a due date
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 feature! Here are some initial "gut reaction" questions/comments for you to consider:
tin/apps/assignments/models.py
Outdated
) | ||
submission_cap_after_due = models.PositiveSmallIntegerField( | ||
null=True, | ||
validators=[MinValueValidator(1)], |
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.
Why can't the after-due-date-cap be zero? Seems like a logical use of the feature...
tin/apps/assignments/models.py
Outdated
submission_cap_after_due = models.PositiveSmallIntegerField( | ||
null=True, | ||
validators=[MinValueValidator(1)], | ||
) |
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.
Would be nice to have default values (e.g. 15 and 0) for both caps at the assignment level - this would also improve validation logic and make it consistent with validation for stuff like quiz action and grader timeout
tin/apps/assignments/models.py
Outdated
on_delete=models.CASCADE, | ||
) | ||
|
||
submission_cap = models.PositiveSmallIntegerField() |
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.
Why can only one of the caps be overridden at the student level? Seems like it could be useful to change the post-due-date-cap on a per-student basis as well (to let someone submit late, for example)
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 it would be better to have a SubmissionCap
model, and two foreignkeys to that model 🤔 that would solve most limitations.
tin/apps/assignments/urls.py
Outdated
"<int:assignment_id>/manage_students/<int:student_id>", | ||
views.manage_student, | ||
name="create_student_override", | ||
), |
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 assignments/X/students/
would be better than assignments/X/manage_students/
@@ -535,6 +585,8 @@ def submit_view(request, assignment_id): | |||
raise http.Http404 | |||
|
|||
student = request.user | |||
if not assignment.within_submission_limit(student): | |||
return http.HttpResponseForbidden("Submission limit exceeded") |
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 403 for exceeding the limit seems harsh - maybe an error message would be nicer?
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.
In theory, they shouldn't be able to even get to that screen without attempting to POST that URL or manualy entering the url.
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.
Yeah, but I figured there's a possibility that someone has Tin open on two devices (e.g. phone, pc). So, they would submit until they reach the cap on pc, forget about it, then open the page and submit on phone.
Very obscure/rare scenario and it's probably too much effort to fix the way I described, but probably replacing it with a redirect to the assignment page would be a better solution.
@@ -150,23 +150,26 @@ ul.no-list { | |||
ul#course-list, | |||
ul#assignment-list, | |||
ul#venv-list, | |||
ul#archive-list { | |||
ul#archive-list, | |||
ul.linebreak-list { |
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.
You should use #linebreak-list
instead, I don't see why a class is necessary
{% extends "base.html" %} | ||
|
||
{% block main %} | ||
<h2>Managing Student {{ user.username }}</h2> |
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, consider showing the default (assignment level) caps on this page, styled in a way that it helps teachers compare
tin/templates/assignments/show.html
Outdated
@@ -36,6 +36,7 @@ <h2 class="left">{% if assignment.is_quiz %}[QUIZ] {% endif %}{{ assignment.name | |||
<a class="right tin-btn" href="{% url 'assignments:manage_grader' assignment.id %}" style="color:#EF1010;">Upload grader</a> | |||
{% endif %} | |||
<a class="right tin-btn" href="{% url 'assignments:manage_files' assignment.id %}">Manage files</a> | |||
<a class="right tin-btn" href="{% url 'assignments:manage_students' assignment.id %}">Manage Students</a> |
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.
<a class="right tin-btn" href="{% url 'assignments:manage_students' assignment.id %}">Manage Students</a> | |
<a class="right tin-btn" href="{% url 'assignments:manage_students' assignment.id %}">Manage students</a> |
This PR
Assignment
modelIt adds two new views:
Manage Students
in the assignments show view.AssignmentOverride
.TODO