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

Quiz submission limit and Per Student Overrides #76

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
4 changes: 4 additions & 0 deletions tin/apps/assignments/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

from .models import (
Assignment,
AssignmentOverride,
CooldownPeriod,
FileAction,
Folder,
Expand Down Expand Up @@ -140,3 +141,6 @@ def match(self, obj):
elif obj.match_type == "C":
return f"*{obj.match_value}*"
return ""


admin.site.register(AssignmentOverride)
JasonGrace2282 marked this conversation as resolved.
Show resolved Hide resolved
25 changes: 24 additions & 1 deletion tin/apps/assignments/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from django.conf import settings

from ..submissions.models import Submission
from .models import Assignment, Folder, MossResult
from .models import Assignment, AssignmentOverride, Folder, MossResult

logger = getLogger(__name__)

Expand All @@ -30,6 +30,10 @@ def __init__(self, course, *args, **kwargs):
# prevent description from getting too big
self.fields["description"].widget.attrs.update({"id": "description"})

# these fields aren't required
for field in ("submission_cap", "submission_cap_after_due"):
self.fields[field].required = False

def get_sections(self) -> Iterable[dict[str, str | tuple[str, ...] | bool]]:
"""This is used in templates to find which fields should be in a dropdown div."""
for section in self.Meta.sections:
Expand Down Expand Up @@ -66,6 +70,9 @@ class Meta:
"grader_timeout",
"grader_has_network_access",
"has_network_access",
"use_submission_cap",
"submission_cap",
"submission_cap_after_due",
"submission_limit_count",
"submission_limit_interval",
"submission_limit_cooldown",
Expand All @@ -83,6 +90,7 @@ class Meta:
"grader_timeout": "Grader timeout (seconds):",
"grader_has_network_access": "Give the grader internet access?",
"has_network_access": "Give submissions internet access?",
"use_submission_cap": "Cap submissions?",
"submission_limit_count": "Rate limit count",
"submission_limit_interval": "Rate limit interval (minutes)",
"submission_limit_cooldown": "Rate limit cooldown period (minutes)",
Expand Down Expand Up @@ -136,6 +144,9 @@ class Meta:
"submission_limit_count",
"submission_limit_interval",
"submission_limit_cooldown",
"use_submission_cap",
"submission_cap",
"submission_cap_after_due",
),
"collapsed": True,
},
Expand All @@ -150,6 +161,9 @@ class Meta:
'internet access" below. If set, it increases the amount '
"of time it takes to start up the grader (to about 1.5 "
"seconds). This is not recommended unless necessary.",
"use_submission_cap": "This enables setting a limit on the number of submissions that can be made on assignments."
"It has no effect on quizzes",
"submission_cap": "The maximum number of submissions that can be made. It can be overridden on a per-student basis.",
"submission_limit_count": "",
"submission_limit_interval": "Tin sets rate limits on submissions. If a student tries "
"to submit too many submissions in a given interval, "
Expand Down Expand Up @@ -241,3 +255,12 @@ class Meta:
"name",
]
help_texts = {"name": "Note: Folders are ordered alphabetically."}


class AssignmentOverrideForm(forms.ModelForm):
class Meta:
model = AssignmentOverride
fields = [
"submission_cap",
]
help_texts = {"submission_cap": "The maximum number of submissions that can be made."}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# Generated by Django 4.2.14 on 2024-09-02 00:46

from django.conf import settings
import django.core.validators
from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):

dependencies = [
migrations.swappable_dependency(settings.AUTH_USER_MODEL),
('assignments', '0032_assignment_quiz_description_and_more'),
]

operations = [
migrations.AddField(
model_name='assignment',
name='submission_cap',
field=models.PositiveSmallIntegerField(null=True, validators=[django.core.validators.MinValueValidator(1)]),
),
migrations.AddField(
model_name='assignment',
name='submission_cap_after_due',
field=models.PositiveSmallIntegerField(null=True, validators=[django.core.validators.MinValueValidator(1)]),
),
migrations.AddField(
model_name='assignment',
name='use_submission_cap',
field=models.BooleanField(default=False),
),
migrations.CreateModel(
name='AssignmentOverride',
fields=[
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('submission_cap', models.PositiveSmallIntegerField(default=None, null=True)),
('assignment', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='student_overrides', to='assignments.assignment')),
('student', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)),
],
),
]
64 changes: 64 additions & 0 deletions tin/apps/assignments/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ class Assignment(models.Model):

has_network_access = models.BooleanField(default=False)

# WARNING: this is the rate limit
submission_limit_count = models.PositiveIntegerField(
default=90,
validators=[MinValueValidator(10)],
Expand All @@ -172,6 +173,16 @@ class Assignment(models.Model):
validators=[MinValueValidator(10)],
)

use_submission_cap = models.BooleanField(default=False)
JasonGrace2282 marked this conversation as resolved.
Show resolved Hide resolved
submission_cap = models.PositiveSmallIntegerField(
null=True,
validators=[MinValueValidator(1)],
)
submission_cap_after_due = models.PositiveSmallIntegerField(
null=True,
validators=[MinValueValidator(1)],
Copy link
Member

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...

)
Copy link
Member

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


last_action_output = models.CharField(max_length=16 * 1024, default="", null=False, blank=True)

is_quiz = models.BooleanField(default=False)
Expand All @@ -192,6 +203,39 @@ def get_absolute_url(self):
def __repr__(self):
return self.name

def within_submission_limit(self, student) -> bool:
"""Check if a student is within the submission limit for an assignment."""
# teachers should have infinite submissions
if not student.is_student or not self.use_submission_cap:
return True

# note that this doesn't care about killed/incomplete submissions
submission_count = self.submissions.filter(student=student).count()

cap = self.find_submission_cap(student)
return submission_count < cap

def find_submission_cap(self, student) -> float:
"""Given a student, find the submission cap.

This takes into account student overrides, and due dates.
"""
if student.is_superuser or student.is_teacher:
return float("inf")
data = self.find_student_override(student)
if data is not None:
return data.submission_cap
elif timezone.localtime() > self.due:
return self.submission_cap_after_due or float("inf")
return self.submission_cap or float("inf")

def find_student_override(self, student) -> AssignmentOverride | None:
"""Find an :class:`.AssignmentOverride` for a student.

Returns ``None`` if no override exists.
"""
return self.student_overrides.filter(student=student).first()

def make_assignment_dir(self) -> None:
"""Creates the directory where the assignment grader scripts go."""
assignment_path = os.path.join(settings.MEDIA_ROOT, f"assignment-{self.id}")
Expand Down Expand Up @@ -371,6 +415,26 @@ def quiz_issues_for_student(self, student) -> bool:
)


class AssignmentOverride(models.Model):
"""A collection of per-student overrides for each :class:`.Assignment`."""

assignment = models.ForeignKey(
Assignment,
on_delete=models.CASCADE,
related_name="student_overrides",
)

student = models.ForeignKey(
get_user_model(),
on_delete=models.CASCADE,
)

submission_cap = models.PositiveSmallIntegerField()
Copy link
Member

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)

Copy link
Member Author

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.


def __str__(self):
return f"Override for {self.student!r} @ Assignment {self.assignment!r}"


class CooldownPeriod(models.Model):
assignment = models.ForeignKey(
Assignment,
Expand Down
3 changes: 3 additions & 0 deletions tin/apps/assignments/tests/test_assignments.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ def test_create_assignment(client, course) -> None:
"submission_limit_cooldown": "30",
"is_quiz": False,
"quiz_action": "2",
"submission_cap": "100",
"submission_cap_after_due": "100",
"use_submission_cap": False,
}
response = client.post(
reverse("assignments:add", args=[course.id]),
Expand Down
153 changes: 153 additions & 0 deletions tin/apps/assignments/tests/test_submission_cap.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
from __future__ import annotations

from datetime import datetime, timedelta

import pytest
from django.urls import reverse

from tin.tests import is_redirect, login


@login("student")
def test_submission_cap(client, assignment):
assignment.use_submission_cap = True
assignment.submission_cap = 1
assignment.submission_cap_after_due = 1
assignment.due = datetime.today() + timedelta(days=1)
assignment.save()
code = "print('hello, world')"
assignment.save_grader_file(code)

def submit():
url = "assignments:submit"
return client.post(
reverse(url, args=[assignment.id]),
{"text": "print('I hate fun')"},
)

response = submit()
assert is_redirect(response), f"Expected redirect, got {response}"

# but now we've passed the cap
response = submit()
assert response.status_code == 403, f"Expected status code 403, got {response}"


@login("student")
def test_submission_cap_overdue(client, assignment):
assignment.use_submission_cap = True
assignment.submission_cap = 10000
assignment.submission_cap_after_due = 1
assignment.due = datetime.today() - timedelta(days=1)
assignment.save()
code = "print('hello, world')"
assignment.save_grader_file(code)

def submit():
url = "assignments:submit"
return client.post(
reverse(url, args=[assignment.id]),
{"text": "print('I hate fun')"},
)

response = submit()
assert is_redirect(response), f"Expected redirect, got {response}"

# but now we've passed the cap
response = submit()
assert response.status_code == 403, f"Expected status code 403, got {response}"


@login("student")
@pytest.mark.parametrize(
"due", (datetime.today() - timedelta(days=1), datetime.today() + timedelta(days=1))
)
def test_submission_cap_with_override(client, assignment, student, due):
"""Test the submission cap with a per student override.

The override should take place regardless of the due date of the :class:`.Assignment`.
"""
assignment.due = due
assignment.use_submission_cap = True
assignment.submission_cap = 1
assignment.submission_cap_after_due = 1
assignment.save()
code = "print('hello, world')"
assignment.save_grader_file(code)

# add student override
assignment.student_overrides.create(student=student, submission_cap=2)

def submit():
url = "assignments:submit"
return client.post(
reverse(url, args=[assignment.id]),
{"text": "print('I hate fun')"},
)

# first and second submission should be fine
response = submit()
assert is_redirect(response), f"Expected redirect, got {response}"
response = submit()
assert is_redirect(response), f"Expected redirect, got {response}"

# but now we've passed the cap for the student
response = submit()
assert response.status_code == 403, f"Expected status code 403, got {response}"


@login("student")
def test_submission_cap_on_quiz(client, quiz):
quiz.use_submission_cap = True
quiz.submission_cap = 1
quiz.submission_cap_after_due = 1
quiz.save()
code = "print('hello, world')"
quiz.save_grader_file(code)

def submit():
url = "assignments:quiz"
return client.post(
reverse(url, args=[quiz.id]),
{"text": "print('I hate fun')"},
)

response = submit()
assert is_redirect(response), f"Expected redirect, got {response}"
response = submit()
assert is_redirect(response), f"Expected submission cap to not affect quizzes (got {response})"


@login("student")
def test_submission_cap_not_used(client, assignment):
assignment.use_submission_cap = False
assignment.submission_cap = 1
assignment.submission_cap_after_due = 1
assignment.due = datetime.today() + timedelta(days=1)
assignment.save()

def submit():
url = "assignments:submit"
return client.post(
reverse(url, args=[assignment.id]),
{"text": "print('I hate fun')"},
)

# all submissions should be fine because use_submission_cap is False
response = submit()
assert is_redirect(response), f"Expected redirect, got {response}"
response = submit()
assert is_redirect(response), f"Expected redirect, got {response}"


@login("teacher")
def test_create_student_override(client, assignment, student):
response = client.post(
reverse("assignments:create_student_override", args=[assignment.id, student.id]),
{"submission_cap": 2},
)
assert is_redirect(response), f"Expected redirect, got {response}"

override = assignment.find_student_override(student)
assert override is not None
assert override.submission_cap == 2
6 changes: 6 additions & 0 deletions tin/apps/assignments/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@
path("<int:assignment_id>/delete", views.delete_view, name="delete"),
path("<int:assignment_id>/grader", views.manage_grader_view, name="manage_grader"),
path("<int:assignment_id>/grader/download", views.download_grader_view, name="download_grader"),
path("<int:assignment_id>/manage_students", views.manage_students_view, name="manage_students"),
path(
"<int:assignment_id>/manage_students/<int:student_id>",
views.manage_student,
name="create_student_override",
),
Copy link
Member

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/

path("<int:assignment_id>/files", views.manage_files_view, name="manage_files"),
path(
"<int:assignment_id>/files/download/<int:file_id>",
Expand Down
Loading
Loading