feat: add course assignment system with submissions and grading#1040
Open
ayesha1145 wants to merge 1 commit intoalphaonelabs:mainfrom
Open
feat: add course assignment system with submissions and grading#1040ayesha1145 wants to merge 1 commit intoalphaonelabs:mainfrom
ayesha1145 wants to merge 1 commit intoalphaonelabs:mainfrom
Conversation
Contributor
👀 Peer Review RequiredHi @ayesha1145! This pull request does not yet have a peer review. Before this PR can be merged, please request a review from one of your peers:
Thank you for contributing! 🎉 |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Cohort / File(s) | Summary |
|---|---|
Models web/models.py |
Added Assignment and AssignmentSubmission models: fields (FKs, title/description, due_date, max_score, statuses, timestamps), properties (is_past_due, submission_count, percentage), ordering, unique constraint (assignment, student), and post-delete file cleanup signal. |
Views web/views.py |
Added views: course_assignments, create_assignment, assignment_detail, edit_assignment, delete_assignment, grade_submission — implement access checks (teacher/enrolled), parsing/validation (due_date, max_score), submission upsert, file type/size checks, grading logic, and redirects. |
URLs web/urls.py |
Added language-prefixed routes under courses/<slug:slug>/ for assignment list, create, detail, edit, delete, and grading (course_assignments, create_assignment, assignment_detail, edit_assignment, delete_assignment, grade_submission). |
Admin web/admin.py |
Imported and registered Assignment and AssignmentSubmission with AssignmentAdmin and AssignmentSubmissionAdmin (configured list_display, list_filter, search_fields). |
Templates — assignments & grading web/templates/courses/assignment_list.html, .../assignment_detail.html, .../assignment_form.html, .../assignment_confirm_delete.html, .../grade_submission.html |
Added templates for assignment list, detail (teacher controls and student submission view/form), create/edit form, delete confirmation, and grade submission UI. |
Course detail template web/templates/courses/detail.html |
Added an "Assignments" action button for the course teacher in the teacher actions grid. |
Sequence Diagram(s)
sequenceDiagram
participant Browser as Browser
participant View as Django View
participant DB as Database (Assignments/Submissions)
participant Storage as File Storage
Browser->>View: GET /courses/<slug>/assignments/ or /courses/<slug>/assignments/<id>/
View->>DB: query Assignments and AssignmentSubmissions
DB-->>View: assignments + submission_map
View-->>Browser: render list/detail
alt Student submits assignment
Browser->>View: POST submission (text + optional file)
View->>Storage: save uploaded file (if present)
Storage-->>View: file path/URL
View->>DB: create/update AssignmentSubmission
DB-->>View: created/updated submission
View-->>Browser: redirect to assignment detail
end
alt Teacher grades submission
Browser->>View: POST grade (score + feedback)
View->>DB: update AssignmentSubmission (score, feedback, graded_by, graded_at, status)
DB-->>View: updated submission
View-->>Browser: redirect to assignment detail
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately and concisely summarizes the main change: introducing a complete course assignment system with student submissions and teacher grading capabilities. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
Contributor
|
Hey @coderabbitai[bot]! You're now assigned to this issue. Please finish your PR within 1 day. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/admin.py`:
- Around line 617-619: Change the mutable list literals used for Django admin
option attributes to immutable tuples: replace list_display, list_filter, and
search_fields in the AssignmentAdmin and AssignmentSubmissionAdmin classes with
tuple literals (e.g., ("title", "course", ...)) so the admin configuration is
immutable and RUF012-compliant; ensure you update all occurrences across both
classes (list_display, list_filter, search_fields).
In `@web/models.py`:
- Around line 3202-3203: Add explicit return type annotations to the __str__
methods for Assignment and AssignmentSubmission: update the method signatures
Assignment.__str__ and AssignmentSubmission.__str__ to declare they return str
(e.g., def __str__(self) -> str:). Keep existing bodies unchanged but ensure the
return type annotation is present to satisfy static analysis and project style
guidelines.
- Around line 3217-3232: Add model-level validation to prevent empty submissions
by implementing AssignmentSubmission.clean to check the fields text_response and
file_submission; if both are empty/None raise
django.core.exceptions.ValidationError with a clear message. Update save() or
ensure full_clean() is called where necessary so the validation runs on save (or
document that views must call full_clean()), and reference AssignmentSubmission,
text_response, file_submission, clean, and ValidationError when making the
change.
- Around line 3251-3255: Add a return type hint and guard against
division-by-zero in the percentage property: annotate percentage (e.g., ->
Optional[int]) and change its logic in the percentage property to check that
self.score is not None and self.assignment.max_score is truthy and non-zero
before performing the division (return rounded int); otherwise return None.
Ensure references remain to the percentage property, self.score, and
self.assignment.max_score.
- Around line 3205-3210: Remove the redundant local import of
django.utils.timezone inside the is_past_due property: rely on the module
already imported at the top of the file and delete the inner "from django.utils
import timezone" line; also add a return type annotation to the property
signature so it reads as def is_past_due(self) -> bool:, and ensure the body
still returns a boolean by keeping the existing due_date check and comparison
using timezone.now().
In `@web/templates/courses/assignment_confirm_delete.html`:
- Around line 14-20: Replace the inline Tailwind classes on the destructive
submit button (the "Yes, Delete" <button> in this template) with the project's
danger button variant (using red-600 as the base) and replace the Cancel link
(the "Cancel" <a> pointing to the course_assignments URL) with the project's
secondary button variant (bg-gray-100 hover:bg-gray-200 dark:bg-gray-700
text-gray-700 dark:text-gray-300) so both actions use the shared button classes
and match the project's UX guidelines.
In `@web/templates/courses/assignment_detail.html`:
- Around line 65-69: The template currently links directly to
submission.file_submission.url which exposes the media URL; instead create a
protected endpoint (e.g., a view named download_submission or reuse a
download_submission view) that takes the submission id (or a secure token),
performs the same enrollment/teacher authorization checks as the assignment
detail view, and returns the file via Django's FileResponse (or Django's
streaming file response) with appropriate headers; then update
web/templates/courses/assignment_detail.html to link to that endpoint (e.g., {%
url 'download_submission' submission.id %}) rather than using
submission.file_submission.url so downloads go through the authorized view.
In `@web/templates/courses/assignment_form.html`:
- Around line 19-44: The form controls (elements with id/title, id/description
textarea, id/due_date, id/max_score, and id/status select) are using
inconsistent Tailwind classes; update each input, textarea and select to use the
repo's canonical form control class string "w-full px-4 py-2 border
border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2
focus:ring-blue-500" (preserving any additional semantic classes like text color
or rows only if required) so all controls follow the shared contract; ensure you
replace the existing class attribute values on the elements referenced above
(title, description, due_date, max_score, status) with the canonical class
string.
In `@web/templates/courses/assignment_list.html`:
- Around line 49-53: The template defines sub=submission_map|default:{} but
never uses it and checks membership using assignment.id|stringformat:"i" which
fails when submission_map keys are integers; update the block to use the local
variable and test membership against the integer key (e.g. replace the {% if
assignment.id|stringformat:"i" in submission_map %} check with {% if
assignment.id in sub %}) so the Submitted badge appears when submission_map is
keyed by integer assignment IDs.
In `@web/templates/courses/detail.html`:
- Around line 597-599: The Assignments CTA anchor (the <a> with href "{% url
'course_assignments' course.slug %}") uses non-standard button classes; update
its class list to match the project's primary button pattern by replacing the
current classes with: bg-teal-300 hover:bg-teal-400 text-white px-6 py-2
rounded-lg transition duration-200 so it conforms to the shared Tailwind primary
button style.
In `@web/templates/courses/grade_submission.html`:
- Around line 42-50: Replace the custom class sets on the score input
(id="score", name="score") and feedback textarea (id="feedback",
name="feedback") with the repository’s standard Tailwind form classes so they
match other forms; specifically use "w-full px-4 py-2 border border-gray-300
dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500" for both
controls and keep existing other attributes
(min/max/value/placeholder/rows/required) unchanged.
In `@web/views.py`:
- Around line 2044-2061: The current two-step check (using submission variable)
then calling AssignmentSubmission.objects.create can race; wrap the create in an
atomic operation and handle collisions by returning the "already submitted"
message instead of allowing duplicates or integrity errors. Replace the separate
existence-check + create with a single atomic create/lookup pattern: within a
transaction.atomic() block attempt to create the submission (or use
get_or_create with defaults for text_response and file_submission) and if a
duplicate is detected (handle IntegrityError or the get_or_create returned
existing flag), call messages.error(request, "You have already submitted this
assignment.") and redirect to assignment_detail; keep the existing validation
for past due / no late submissions and empty response checks before the atomic
create. Ensure you reference AssignmentSubmission.objects.create /
get_or_create, transaction.atomic, and IntegrityError when implementing the fix.
- Around line 1963-1985: Centralize and harden validation for assignment writes
by creating a shared helper (e.g., validate_assignment_data) and use it from the
create_assignment and edit_assignment view blocks; the helper should ensure
title and description are non-empty (reject or return errors), safely parse
due_date with django.utils.dateparse.parse_datetime, coerce max_score to int
with try/except and enforce sensible bounds/defaults, and validate status
against the Assignment model's allowed choices (e.g., Assignment.STATUS_CHOICES
or Assignment._meta.get_field("status").choices) so unrecognized values are
rejected or normalized; update the POST handling in the create_assignment and
edit_assignment views to call this helper, return messages.error and redirect
when validation fails, and only call Assignment.objects.create / save after
validation succeeds.
- Around line 2051-2061: The current handler reads file_submission from
request.FILES and immediately calls AssignmentSubmission.objects.create, so you
must validate the uploaded file before persisting: check file_submission.size
against a configured MAX_FILE_SIZE constant and verify
file_submission.content_type (and optionally the filename extension) is in an
allowed list (ALLOWED_CONTENT_TYPES/ALLOWED_EXTENSIONS); if validation fails
call messages.error(...) and return redirect("assignment_detail", slug=slug,
assignment_id=assignment_id) without calling
AssignmentSubmission.objects.create; only create the AssignmentSubmission after
the file passes these checks (symbols to change: text_response, file_submission,
AssignmentSubmission.objects.create, messages.error,
redirect("assignment_detail")).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 662b189b-bc65-433d-8736-19050f8c8dcb
⛔ Files ignored due to path filters (1)
web/migrations/0064_add_assignment_and_submission.pyis excluded by!**/migrations/**
📒 Files selected for processing (10)
web/admin.pyweb/models.pyweb/templates/courses/assignment_confirm_delete.htmlweb/templates/courses/assignment_detail.htmlweb/templates/courses/assignment_form.htmlweb/templates/courses/assignment_list.htmlweb/templates/courses/detail.htmlweb/templates/courses/grade_submission.htmlweb/urls.pyweb/views.py
ayesha1145
force-pushed
the
feat/course-assignment-system
branch
from
b821429 to
f81039d
Compare
Contributor
💬 Unresolved Review ConversationsHi @ayesha1145! 👋 This pull request currently has 5 unresolved review conversations. Please address all review feedback and push a new commit to resolve them before this PR can be merged. Steps to resolve:
Once all conversations are resolved, this notice will be removed automatically. Thank you! 🙏 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
web/admin.py (1)
615-627: 🧹 Nitpick | 🔵 TrivialConvert admin option attributes to tuples for immutability.
The admin classes are well-configured with appropriate
list_display,list_filter, andsearch_fields. However, these attributes use mutable list literals, which triggers Ruff's RUF012 warning. Using tuples ensures the configuration remains safely immutable.Convert lists to tuples
`@admin.register`(Assignment) class AssignmentAdmin(admin.ModelAdmin): - list_display = ["title", "course", "status", "due_date", "max_score", "created_at"] - list_filter = ["status", "course"] - search_fields = ["title", "course__title"] + list_display = ("title", "course", "status", "due_date", "max_score", "created_at") + list_filter = ("status", "course") + search_fields = ("title", "course__title") `@admin.register`(AssignmentSubmission) class AssignmentSubmissionAdmin(admin.ModelAdmin): - list_display = ["student", "assignment", "status", "score", "submitted_at"] - list_filter = ["status"] - search_fields = ["student__username", "assignment__title"] + list_display = ("student", "assignment", "status", "score", "submitted_at") + list_filter = ("status",) + search_fields = ("student__username", "assignment__title")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/admin.py` around lines 615 - 627, Change the mutable list literals on the admin classes to tuples to avoid RUF012: update AssignmentAdmin.list_display, AssignmentAdmin.list_filter, AssignmentAdmin.search_fields and AssignmentSubmissionAdmin.list_display, AssignmentSubmissionAdmin.list_filter, AssignmentSubmissionAdmin.search_fields to use tuple literals instead of list literals (e.g., ("title", "course", ...) rather than ["title", "course", ...]) so the admin option attributes are immutable.web/templates/courses/assignment_list.html (1)
49-53:⚠️ Potential issue | 🟠 MajorFix the submission status lookup to use the defined variable.
There's a logic issue here:
subis defined but never used, and the membership check usesstringformat:"i"which converts the ID to a string. Ifsubmission_mapis keyed by integers (which is typical in Django), this check will always fail, causing students to never see the "Submitted" badge.Fix the lookup logic
-{% with sub=submission_map|default:{} %} -{% if assignment.id|stringformat:"i" in submission_map %} +{% with sub=submission_map|default:{} %} +{% if assignment.id in sub %} <span class="text-green-600 dark:text-green-400"><i class="fas fa-check mr-1" aria-hidden="true"></i> Submitted</span> {% endif %} {% endwith %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_list.html` around lines 49 - 53, The template defines sub with "{% with sub=submission_map|default:{} %}" but then checks membership against submission_map using a string-converted key; change the membership check to use the defined sub variable and the actual assignment id (e.g., "if assignment.id in sub") so the lookup uses the same map and the correct key type; update the block around the "Submitted" badge to use sub and remove the unnecessary stringformat conversion.web/templates/courses/assignment_form.html (1)
19-44: 🧹 Nitpick | 🔵 TrivialOptional: Align form input classes with project guidelines.
The form is well-structured with proper validation attributes and CSRF protection. The input styling uses
px-3 py-2andfocus:ring-teal-300, while the project guidelines specifypx-4 py-2andfocus:ring-blue-500.If you want strict adherence to the shared form control contract, you can update the classes. However, this is a minor styling consistency matter—the form functions correctly as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_form.html` around lines 19 - 44, Update the form control classes on the inputs/textarea/select to match the project contract by replacing padding and focus ring values: change "px-3" to "px-4" for elements with ids title, description, due_date, max_score and the status select, and replace "focus:ring-teal-300" with "focus:ring-blue-500" on the same elements so the class sets are consistent with the shared form control guideline.web/templates/courses/assignment_detail.html (1)
65-69:⚠️ Potential issue | 🟡 MinorConsider protecting file downloads with a dedicated view.
Similar to other templates in this PR, the direct
{{ submission.file_submission.url }}exposes files as public media URLs. The project'sdownload_materialview demonstrates the preferred pattern—serving files through an authorized endpoint.For defense-in-depth, consider creating a
download_submissionview that checks enrollment/teacher status before serving the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_detail.html` around lines 65 - 69, Replace the direct public URL usage of submission.file_submission.url in assignment_detail.html with a link to a secured endpoint: create a download_submission view (analogous to download_material) that checks permissions (enrollment or teacher), streams the file, and returns a protected response; then update the template to link to that view (e.g., via a named URL like "download_submission" accepting submission.id) so files are served only after authorization by the new view.web/templates/courses/assignment_confirm_delete.html (1)
14-20: 🧹 Nitpick | 🔵 TrivialMinor: Consider aligning button styles with project conventions.
The delete action works correctly, but for visual consistency across the project:
- The danger button uses
bg-red-500while the project convention suggestsred-600as the danger color.- The "Cancel" link could use the secondary button styling for better visual balance.
These are small polish items and not blockers—the functionality is solid and the form includes proper CSRF protection.
Optional styling alignment
<button type="submit" - class="bg-red-500 hover:bg-red-600 dark:bg-red-700 dark:hover:bg-red-600 text-white font-semibold px-6 py-2 rounded-lg transition duration-200 focus:outline-none focus:ring-2 focus:ring-red-400"> + class="bg-red-600 hover:bg-red-700 text-white font-semibold px-6 py-2 rounded-lg transition duration-200 focus:outline-none focus:ring-2 focus:ring-red-400"> Yes, Delete </button> </form> <a href="{% url 'course_assignments' course.slug %}" - class="text-gray-500 hover:text-gray-700 dark:text-gray-400 text-sm">Cancel</a> + class="bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300 px-6 py-2 rounded-lg transition duration-200">Cancel</a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_confirm_delete.html` around lines 14 - 20, Update the submit button (the element with type="submit") to use the project danger color by changing the red utility from bg-red-500 to bg-red-600 (and adjust related hover/dark variants if needed to match project tokens), and change the Cancel link (the <a> with class="text-gray-500 hover:text-gray-700 dark:text-gray-400 text-sm") to use the project’s standard secondary button styling by replacing its current text/hover classes with the shared secondary button class set used elsewhere in the codebase.web/views.py (1)
2129-2134:⚠️ Potential issue | 🟠 MajorReuse the same validation rules in
edit_assignment.Line 2132 can still raise on a tampered
max_score, and Lines 2129-2134 writetitle,description, andstatusstraight from the request without the checks already present increate_assignment. Please route edits through the same validated helper so both write paths reject bad input consistently instead of returning a 500 or saving broken data. As per coding guidelines: Review Python code for PEP 8 compliance, proper type hints, and Django best practices where applicable. Ensure proper error handling and logging.Also applies to: 2136-2136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/views.py` around lines 2129 - 2134, The edit_assignment flow writes title, description, status and casts max_score directly from request which can raise on tampered input and bypasses the validation used in create_assignment; refactor edit_assignment to call the same validation/sanitization helper used by create_assignment (e.g., validate_assignment_data or sanitize_assignment_input) to parse and validate title, description, due_date, status, allow_late_submissions and max_score, handle invalid max_score with try/except (returning a proper error response or form error), and log validation failures; ensure types are explicit (int conversion only after validation), reuse the same error messages/field checks as create_assignment, and update edit_assignment to persist only validated fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/models.py`:
- Around line 3211-3213: The property submission_count lacks a return type
annotation; update its definition (the `@property` method named submission_count)
to include the appropriate typing (e.g., -> int) matching other properties like
is_past_due, so the signature indicates the return type while keeping the body
returning self.submissions.count().
- Line 3228: Move the file validation out of the view and into the model or form
by adding a validator for the FileField 'file_submission' (models.FileField)
that enforces allowed content types and the 10MB size limit; implement the logic
as a reusable function (e.g., validate_submission_file) and attach it to the
FileField via the validators parameter or implement a clean_file_submission
method on the corresponding ModelForm to centralize checks; then remove the
duplicate checks from the view (the current checks around file type and size) so
all uploads go through the model/form validator.
- Line 3230: Add model-level validation to prevent score > assignment.max_score
by implementing a clean() on the model that checks self.score against
self.assignment.max_score and raises django.core.exceptions.ValidationError when
score is greater; ensure save() calls full_clean() (or that callers call
full_clean()) so the check runs on save. Reference the score field and the
model's clean() and save() methods and use self.assignment.max_score to perform
the comparison.
In `@web/templates/courses/assignment_detail.html`:
- Around line 93-158: The assignment_detail.html template is corrupted: finish
the incomplete file input element (id="file_submission") by closing its class
attribute and input tag, remove the stray shell command line ("cat >
web/templates/courses/grade_submission.html << 'HTMLEOF'") and all embedded
grade_submission.html content that was accidentally pasted into this file, and
restore assignment_detail.html to only include the assignment detail markup
(ensuring all opened tags and Django blocks are properly closed); keep
grade_submission.html as a separate template file instead of embedded here.
In `@web/templates/courses/grade_submission.html`:
- Around line 25-29: The template currently links directly to
submission.file_submission.url which exposes a media URL; change the template in
grade_submission.html to point to a protected download_submission endpoint (e.g.
using {% url 'download_submission' submission.id %}) instead of
submission.file_submission.url, and implement a download_submission view that
performs the same access checks as the grade_submission view (teacher/course
enrollment/ownership as appropriate) and serves the file via Django's
FileResponse (or streams it) similar to download_material so sharing the raw
media URL cannot bypass authorization.
In `@web/views.py`:
- Around line 2135-2146: The assignment.due_date assignment is over-indented
causing a SyntaxError; unindent the line so it lines up with the following
statements (e.g., align "assignment.due_date = parse_datetime(due_date) if
due_date else None" with "assignment.save()" and "messages.success(...)") inside
the same view function block; ensure parse_datetime, assignment.save(),
messages.success(...) and the subsequent return redirect("course_assignments",
slug=slug) remain at the same indentation level so the function parses
correctly.
---
Duplicate comments:
In `@web/admin.py`:
- Around line 615-627: Change the mutable list literals on the admin classes to
tuples to avoid RUF012: update AssignmentAdmin.list_display,
AssignmentAdmin.list_filter, AssignmentAdmin.search_fields and
AssignmentSubmissionAdmin.list_display, AssignmentSubmissionAdmin.list_filter,
AssignmentSubmissionAdmin.search_fields to use tuple literals instead of list
literals (e.g., ("title", "course", ...) rather than ["title", "course", ...])
so the admin option attributes are immutable.
In `@web/templates/courses/assignment_confirm_delete.html`:
- Around line 14-20: Update the submit button (the element with type="submit")
to use the project danger color by changing the red utility from bg-red-500 to
bg-red-600 (and adjust related hover/dark variants if needed to match project
tokens), and change the Cancel link (the <a> with class="text-gray-500
hover:text-gray-700 dark:text-gray-400 text-sm") to use the project’s standard
secondary button styling by replacing its current text/hover classes with the
shared secondary button class set used elsewhere in the codebase.
In `@web/templates/courses/assignment_detail.html`:
- Around line 65-69: Replace the direct public URL usage of
submission.file_submission.url in assignment_detail.html with a link to a
secured endpoint: create a download_submission view (analogous to
download_material) that checks permissions (enrollment or teacher), streams the
file, and returns a protected response; then update the template to link to that
view (e.g., via a named URL like "download_submission" accepting submission.id)
so files are served only after authorization by the new view.
In `@web/templates/courses/assignment_form.html`:
- Around line 19-44: Update the form control classes on the
inputs/textarea/select to match the project contract by replacing padding and
focus ring values: change "px-3" to "px-4" for elements with ids title,
description, due_date, max_score and the status select, and replace
"focus:ring-teal-300" with "focus:ring-blue-500" on the same elements so the
class sets are consistent with the shared form control guideline.
In `@web/templates/courses/assignment_list.html`:
- Around line 49-53: The template defines sub with "{% with
sub=submission_map|default:{} %}" but then checks membership against
submission_map using a string-converted key; change the membership check to use
the defined sub variable and the actual assignment id (e.g., "if assignment.id
in sub") so the lookup uses the same map and the correct key type; update the
block around the "Submitted" badge to use sub and remove the unnecessary
stringformat conversion.
In `@web/views.py`:
- Around line 2129-2134: The edit_assignment flow writes title, description,
status and casts max_score directly from request which can raise on tampered
input and bypasses the validation used in create_assignment; refactor
edit_assignment to call the same validation/sanitization helper used by
create_assignment (e.g., validate_assignment_data or sanitize_assignment_input)
to parse and validate title, description, due_date, status,
allow_late_submissions and max_score, handle invalid max_score with try/except
(returning a proper error response or form error), and log validation failures;
ensure types are explicit (int conversion only after validation), reuse the same
error messages/field checks as create_assignment, and update edit_assignment to
persist only validated fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 274cf3f8-3c5e-450f-8cfe-d8ccd005a5ec
⛔ Files ignored due to path filters (1)
web/migrations/0064_add_assignment_and_submission.pyis excluded by!**/migrations/**
📒 Files selected for processing (10)
web/admin.pyweb/models.pyweb/templates/courses/assignment_confirm_delete.htmlweb/templates/courses/assignment_detail.htmlweb/templates/courses/assignment_form.htmlweb/templates/courses/assignment_list.htmlweb/templates/courses/detail.htmlweb/templates/courses/grade_submission.htmlweb/urls.pyweb/views.py
ayesha1145
force-pushed
the
feat/course-assignment-system
branch
from
f81039d to
b9aa130
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (12)
web/templates/courses/grade_submission.html (1)
42-50:⚠️ Potential issue | 🟡 MinorUse the shared form-control class contract for score and feedback fields.
These controls still use custom class sets instead of the repository-standard input styling.
As per coding guidelines: "Use Tailwind form input classes: w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500 for form inputs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/grade_submission.html` around lines 42 - 50, Replace the custom class lists on the score input (id="score", name="score") and feedback textarea (id="feedback", name="feedback") with the repository-standard form-control class contract used across the project (the Tailwind classes: w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500), preserving other attributes like id/name/value/placeholder/rows/required; ensure both elements use the identical shared class string so styling is consistent with other form inputs.web/templates/courses/assignment_detail.html (2)
93-158:⚠️ Potential issue | 🔴 CriticalRemove the embedded shell/template payload — this template is currently broken.
The file input is cut off, then a shell heredoc command and an entire different template are embedded. This will fail template parsing/rendering and must be removed before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_detail.html` around lines 93 - 158, The template contains an accidental shell/heredoc payload starting at the truncated file input element (id="file_submission") and an entire duplicate template "grade_submission.html" embedded inside assignment_detail.html; remove the shell heredoc and the duplicated template content, restore or complete the original file input element markup (the input with id="file_submission" and name="file_submission") so assignment_detail.html ends cleanly, and ensure only the intended assignment_detail.html template remains (no embedded "cat > ... << 'HTMLEOF'" or second template).
65-68:⚠️ Potential issue | 🟠 MajorServe submitted files through an authorized download endpoint, not direct media URLs.
Using
{{ submission.file_submission.url }}exposes a raw media path and bypasses view-level authorization once shared. Route this through a protecteddownload_submissionview (with teacher/enrollment checks) and link to that URL instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_detail.html` around lines 65 - 68, The template currently links directly to submission.file_submission.url which exposes raw media URLs and bypasses view authorization; change the anchor to point to a protected download endpoint (e.g., reverse/URL name "download_submission") that maps to a view like download_submission(request, submission_id) which performs teacher/enrollment/owner checks and streams the file via Django's FileResponse or a signed URL, and update the template to use that URL (pass submission.id or a secure token) instead of submission.file_submission.url so downloads always go through the authorization logic.web/templates/courses/assignment_confirm_delete.html (1)
14-20:⚠️ Potential issue | 🟡 MinorAlign delete/cancel actions with shared danger and secondary button variants.
The actions work, but they still use local styles instead of the shared destructive/secondary button patterns.
Suggested update
- <button type="submit" - class="bg-red-500 hover:bg-red-600 dark:bg-red-700 dark:hover:bg-red-600 text-white font-semibold px-6 py-2 rounded-lg transition duration-200 focus:outline-none focus:ring-2 focus:ring-red-400"> + <button type="submit" + class="bg-red-600 hover:bg-red-700 text-white px-6 py-2 rounded-lg transition duration-200 focus:outline-none focus:ring-2 focus:ring-red-400"> Yes, Delete </button> @@ - <a href="{% url 'course_assignments' course.slug %}" - class="text-gray-500 hover:text-gray-700 dark:text-gray-400 text-sm">Cancel</a> + <a href="{% url 'course_assignments' course.slug %}" + class="bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300 px-6 py-2 rounded-lg transition duration-200">Cancel</a>As per coding guidelines: "Follow the project's color scheme ... Danger: red-600" and "Use Tailwind secondary button classes: bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_confirm_delete.html` around lines 14 - 20, The delete and cancel actions use local Tailwind classes instead of the project's shared button variants; update the "Yes, Delete" submit button (the form submit with text "Yes, Delete") to use the shared destructive/danger variant (use red-600 as the base, proper hover/dark states per project convention) and update the cancel anchor (the link with text "Cancel" that points to the course_assignments URL) to use the shared secondary button classes (bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300) so both follow the project's color and component guidelines.web/templates/courses/assignment_form.html (1)
19-44:⚠️ Potential issue | 🟡 MinorStandardize form controls to the shared input/select/textarea class contract.
These controls are still using custom class sets instead of the repo’s canonical form-control classes.
As per coding guidelines: "Use Tailwind form input classes: w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500 for form inputs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_form.html` around lines 19 - 44, The form controls (inputs/textarea/select) for title, description, due_date, max_score and status are using a custom class set; replace their class attributes to use the repo canonical form-control Tailwind contract (e.g., w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500) so all controls (input#title, textarea#description, input#due_date, input#max_score, select#status) follow the shared styling; ensure you apply the same canonical class string for text inputs, textarea and select elements and remove the old custom class fragments.web/templates/courses/detail.html (1)
597-599:⚠️ Potential issue | 🟡 MinorUse the shared primary button spacing for the Assignments CTA.
This CTA still deviates from the shared primary button contract by using
px-4instead ofpx-6.Suggested class adjustment
- class="inline-flex items-center justify-center px-4 py-2 bg-teal-300 hover:bg-teal-400 dark:bg-teal-600 dark:hover:bg-teal-500 text-white rounded-lg transition duration-200 focus:outline-none focus:ring-2 focus:ring-teal-300"> + class="inline-flex items-center justify-center bg-teal-300 hover:bg-teal-400 text-white px-6 py-2 rounded-lg transition duration-200 dark:bg-teal-600 dark:hover:bg-teal-500 focus:outline-none focus:ring-2 focus:ring-teal-300">As per coding guidelines: "Use Tailwind primary button classes: bg-teal-300 hover:bg-teal-400 text-white px-6 py-2 rounded-lg transition duration-200."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/detail.html` around lines 597 - 599, The Assignments CTA anchor (the <a> with href "{% url 'course_assignments' course.slug %}") uses px-4 and therefore doesn't match the shared primary button contract; update its class list to replace px-4 with px-6 so it matches the canonical primary button classes (bg-teal-300 hover:bg-teal-400 text-white px-6 py-2 rounded-lg transition duration-200) while keeping the existing dark: and focus: classes intact.web/templates/courses/assignment_list.html (1)
49-53:⚠️ Potential issue | 🟠 MajorFix submitted-status lookup to use the local map and consistent key type.
The current condition can miss real submissions when
submission_mapkeys are integers.Suggested fix
-{% with sub=submission_map|default:{} %} -{% if assignment.id|stringformat:"i" in submission_map %} +{% with sub=submission_map|default:{} %} +{% if assignment.id in sub %} <span class="text-green-600 dark:text-green-400"><i class="fas fa-check mr-1" aria-hidden="true"></i> Submitted</span> {% endif %} {% endwith %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_list.html` around lines 49 - 53, The template uses submission_map directly and stringifies assignment.id, which can miss matches when submission_map keys are integers; change the membership check to use the local 'sub' variable created by the with block and a consistent key type: replace the condition that reads {% if assignment.id|stringformat:"i" in submission_map %} with a check against sub (e.g. {% if assignment.id in sub %} or, if your map uses string keys, {% if assignment.id|stringformat:"i" in sub %}) so the lookup uses the local variable 'sub' and a matching key type.web/models.py (2)
3212-3213:⚠️ Potential issue | 🟡 MinorAdd a return type annotation to
submission_count.On Line [3212], please annotate the property return type for consistency with nearby typed properties.
💡 Proposed fix
`@property` - def submission_count(self): + def submission_count(self) -> int: return self.submissions.count()As per coding guidelines: "Use type hints in Python where appropriate".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/models.py` around lines 3212 - 3213, Add a return type annotation to the submission_count method: change the signature of def submission_count(self) to include the appropriate return type (e.g., -> int) so it reads def submission_count(self) -> int:, and if this is a `@property` keep the annotation on the property method; update any related type hints or imports if necessary to satisfy linters/type-checkers.
3225-3233:⚠️ Potential issue | 🟠 MajorAdd model-level validation for submission content and score bounds.
Currently, a submission can be saved with neither text nor file (Lines [3227]-[3228]), and
scorecan exceedassignment.max_score(Line [3230]). Enforce both constraints inclean()and run validation on save.💡 Proposed fix
class AssignmentSubmission(models.Model): @@ class Meta: unique_together = ["assignment", "student"] ordering = ["-submitted_at"] def __str__(self) -> str: return f"{self.student.username} - {self.assignment.title}" + + def clean(self) -> None: + errors = {} + if not self.text_response and not self.file_submission: + errors["text_response"] = "Provide a text response or upload a file." + if self.score is not None and self.score > self.assignment.max_score: + errors["score"] = f"Score cannot exceed {self.assignment.max_score}." + if errors: + raise ValidationError(errors) + + def save(self, *args, **kwargs) -> None: + self.full_clean() + super().save(*args, **kwargs)Also applies to: 3243-3245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/models.py` around lines 3225 - 3233, Add model-level validation in the Submission model by implementing a clean(self) method that raises ValidationError if both text_response and file_submission are empty and if score is not None but is < 0 or > self.assignment.max_score; then override save(self, *args, **kwargs) to call self.full_clean() before super().save(...) so validation runs on save. Apply the same clean/save pattern to the other submission-like model nearby to enforce identical content and score bounds checks (reference fields: text_response, file_submission, score and attribute assignment.max_score).web/views.py (3)
2135-2146:⚠️ Potential issue | 🔴 CriticalFix the indentation blocker before anything else.
The over-indented
assignment.due_dateline makesweb/views.pyinvalid Python, so Django cannot import this module and every view in it stops loading.🩹 Suggested fix
- assignment.due_date = parse_datetime(due_date) if due_date else None + assignment.due_date = parse_datetime(due_date) if due_date else NoneThis parser check should currently fail around Line 2135; after the indentation fix it should print
AST parse OK.#!/bin/bash set -euo pipefail python - <<'PY' from pathlib import Path import ast path = Path("web/views.py") source = path.read_text() ast.parse(source) print("AST parse OK") PYAs per coding guidelines, "
**/*.py: Fix linting errors in code.`"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/views.py` around lines 2135 - 2146, The file has a mis-indented line: "assignment.due_date = parse_datetime(due_date) if due_date else None" is over-indented and breaks Python parsing; adjust its indentation to match the surrounding block (i.e., align it with the following "assignment.save()" and other statements within the same view function) so the assignment update block is syntactically correct; check the function that processes the form (the block that uses assignment, parse_datetime, assignment.save, messages.success and return redirect) and ensure all statements inside that conditional/handler have consistent indentation, then re-run the AST parse check.
2129-2134:⚠️ Potential issue | 🟠 MajorPlease run the same validation on edit that you do on create.
This branch still writes
title,description,status, andmax_scoredirectly from the request. A tampered POST can save blank fields or invalid statuses, andint(request.POST.get("max_score", 100))can still raise or persist a negative score before the user sees an error. Reuse the same validated path ascreate_assignmentbefore mutatingassignment.As per coding guidelines, "
**/*.py: Review Python code for PEP 8 compliance, proper type hints, and Django best practices where applicable. Check for security vulnerabilities such as SQL injection, XSS, and insecure use of eval(). Ensure proper error handling and logging. Verify tests cover the key logic paths.`"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/views.py` around lines 2129 - 2134, The edit handler is directly assigning request.POST values to assignment (title, description, status, max_score, allow_late_submissions) without the same validation used on create; update the edit flow to reuse the create_assignment validation path (e.g., call the same form or validator used by create_assignment) instead of assigning raw POST values: validate/clean title, description, status and max_score first (handle int conversion and non-negative constraint) and only then mutate assignment and save; reference the existing create_assignment validation routine or AssignmentForm.clean() so edits and creates share identical checks.
2061-2070:⚠️ Potential issue | 🟠 MajorDon't trust
content_typeas the only upload gate.
file_submission.content_typeis client-controlled, so a crafted multipart request can label arbitrary bytes asapplication/pdforimage/pngand still pass this check. Please validate the actual file signature or a server-side MIME detection result before saving, with extension checks only as a secondary filter.Does Django's UploadedFile.content_type come from the client request, and is it safe to use as the sole server-side file-type validation check in a Django upload handler?As per coding guidelines, "
**/*.py: Review Python code for PEP 8 compliance, proper type hints, and Django best practices where applicable. Check for security vulnerabilities such as SQL injection, XSS, and insecure use of eval(). Ensure proper error handling and logging. Verify tests cover the key logic paths.`"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/views.py` around lines 2061 - 2070, The current check uses client-controlled file_submission.content_type; replace this with server-side MIME/signature validation: use a library like python-magic (libmagic) or Django's built-in file inspection to detect the real MIME from the file bytes for the variable file_submission, compare that detected MIME against allowed_types, and only then allow the upload; additionally verify the file extension matches expected types as a secondary check, ensure you read a limited prefix of file_submission (e.g., first few KB) and seek back to start before saving, and keep the existing size check for file_submission.size.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/models.py`:
- Line 3193: The max_score PositiveIntegerField currently allows 0; add a
minimum value validator to enforce >=1 by updating the max_score field
declaration (symbol: max_score, type: models.PositiveIntegerField) to include
validators=[MinValueValidator(1)] and ensure
django.core.validators.MinValueValidator is imported at the top of the module;
keep the existing default=100 as-is.
In `@web/views.py`:
- Around line 1959-2159: Edit the indentation bug in edit_assignment: fix the
over-indented line assigning assignment.due_date so it aligns with the other
assignment.* assignments inside the POST block (look for function
edit_assignment and the assignment.due_date line). Add tests covering key paths
for the assignment views: create_assignment, assignment_detail (student
submission flows including duplicate submission, file type and size validation,
missing text/file), course_assignments permission split (teacher vs enrolled
student), and grade_submission (score bounds, grading state changes,
graded_by/graded_at). Ensure tests assert redirects/messages and model state
changes (e.g., AssignmentSubmission created, status/score set,
Assignment.max_score enforcement) and run formatter/PEP8 checks after the fix.
---
Duplicate comments:
In `@web/models.py`:
- Around line 3212-3213: Add a return type annotation to the submission_count
method: change the signature of def submission_count(self) to include the
appropriate return type (e.g., -> int) so it reads def submission_count(self) ->
int:, and if this is a `@property` keep the annotation on the property method;
update any related type hints or imports if necessary to satisfy
linters/type-checkers.
- Around line 3225-3233: Add model-level validation in the Submission model by
implementing a clean(self) method that raises ValidationError if both
text_response and file_submission are empty and if score is not None but is < 0
or > self.assignment.max_score; then override save(self, *args, **kwargs) to
call self.full_clean() before super().save(...) so validation runs on save.
Apply the same clean/save pattern to the other submission-like model nearby to
enforce identical content and score bounds checks (reference fields:
text_response, file_submission, score and attribute assignment.max_score).
In `@web/templates/courses/assignment_confirm_delete.html`:
- Around line 14-20: The delete and cancel actions use local Tailwind classes
instead of the project's shared button variants; update the "Yes, Delete" submit
button (the form submit with text "Yes, Delete") to use the shared
destructive/danger variant (use red-600 as the base, proper hover/dark states
per project convention) and update the cancel anchor (the link with text
"Cancel" that points to the course_assignments URL) to use the shared secondary
button classes (bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700
dark:text-gray-300) so both follow the project's color and component guidelines.
In `@web/templates/courses/assignment_detail.html`:
- Around line 93-158: The template contains an accidental shell/heredoc payload
starting at the truncated file input element (id="file_submission") and an
entire duplicate template "grade_submission.html" embedded inside
assignment_detail.html; remove the shell heredoc and the duplicated template
content, restore or complete the original file input element markup (the input
with id="file_submission" and name="file_submission") so assignment_detail.html
ends cleanly, and ensure only the intended assignment_detail.html template
remains (no embedded "cat > ... << 'HTMLEOF'" or second template).
- Around line 65-68: The template currently links directly to
submission.file_submission.url which exposes raw media URLs and bypasses view
authorization; change the anchor to point to a protected download endpoint
(e.g., reverse/URL name "download_submission") that maps to a view like
download_submission(request, submission_id) which performs
teacher/enrollment/owner checks and streams the file via Django's FileResponse
or a signed URL, and update the template to use that URL (pass submission.id or
a secure token) instead of submission.file_submission.url so downloads always go
through the authorization logic.
In `@web/templates/courses/assignment_form.html`:
- Around line 19-44: The form controls (inputs/textarea/select) for title,
description, due_date, max_score and status are using a custom class set;
replace their class attributes to use the repo canonical form-control Tailwind
contract (e.g., w-full px-4 py-2 border border-gray-300 dark:border-gray-600
rounded-lg focus:ring-2 focus:ring-blue-500) so all controls (input#title,
textarea#description, input#due_date, input#max_score, select#status) follow the
shared styling; ensure you apply the same canonical class string for text
inputs, textarea and select elements and remove the old custom class fragments.
In `@web/templates/courses/assignment_list.html`:
- Around line 49-53: The template uses submission_map directly and stringifies
assignment.id, which can miss matches when submission_map keys are integers;
change the membership check to use the local 'sub' variable created by the with
block and a consistent key type: replace the condition that reads {% if
assignment.id|stringformat:"i" in submission_map %} with a check against sub
(e.g. {% if assignment.id in sub %} or, if your map uses string keys, {% if
assignment.id|stringformat:"i" in sub %}) so the lookup uses the local variable
'sub' and a matching key type.
In `@web/templates/courses/detail.html`:
- Around line 597-599: The Assignments CTA anchor (the <a> with href "{% url
'course_assignments' course.slug %}") uses px-4 and therefore doesn't match the
shared primary button contract; update its class list to replace px-4 with px-6
so it matches the canonical primary button classes (bg-teal-300
hover:bg-teal-400 text-white px-6 py-2 rounded-lg transition duration-200) while
keeping the existing dark: and focus: classes intact.
In `@web/templates/courses/grade_submission.html`:
- Around line 42-50: Replace the custom class lists on the score input
(id="score", name="score") and feedback textarea (id="feedback",
name="feedback") with the repository-standard form-control class contract used
across the project (the Tailwind classes: w-full px-4 py-2 border
border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2
focus:ring-blue-500), preserving other attributes like
id/name/value/placeholder/rows/required; ensure both elements use the identical
shared class string so styling is consistent with other form inputs.
In `@web/views.py`:
- Around line 2135-2146: The file has a mis-indented line: "assignment.due_date
= parse_datetime(due_date) if due_date else None" is over-indented and breaks
Python parsing; adjust its indentation to match the surrounding block (i.e.,
align it with the following "assignment.save()" and other statements within the
same view function) so the assignment update block is syntactically correct;
check the function that processes the form (the block that uses assignment,
parse_datetime, assignment.save, messages.success and return redirect) and
ensure all statements inside that conditional/handler have consistent
indentation, then re-run the AST parse check.
- Around line 2129-2134: The edit handler is directly assigning request.POST
values to assignment (title, description, status, max_score,
allow_late_submissions) without the same validation used on create; update the
edit flow to reuse the create_assignment validation path (e.g., call the same
form or validator used by create_assignment) instead of assigning raw POST
values: validate/clean title, description, status and max_score first (handle
int conversion and non-negative constraint) and only then mutate assignment and
save; reference the existing create_assignment validation routine or
AssignmentForm.clean() so edits and creates share identical checks.
- Around line 2061-2070: The current check uses client-controlled
file_submission.content_type; replace this with server-side MIME/signature
validation: use a library like python-magic (libmagic) or Django's built-in file
inspection to detect the real MIME from the file bytes for the variable
file_submission, compare that detected MIME against allowed_types, and only then
allow the upload; additionally verify the file extension matches expected types
as a secondary check, ensure you read a limited prefix of file_submission (e.g.,
first few KB) and seek back to start before saving, and keep the existing size
check for file_submission.size.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f0a1926-e419-4815-b113-8b79cbe101f4
⛔ Files ignored due to path filters (1)
web/migrations/0064_add_assignment_and_submission.pyis excluded by!**/migrations/**
📒 Files selected for processing (10)
web/admin.pyweb/models.pyweb/templates/courses/assignment_confirm_delete.htmlweb/templates/courses/assignment_detail.htmlweb/templates/courses/assignment_form.htmlweb/templates/courses/assignment_list.htmlweb/templates/courses/detail.htmlweb/templates/courses/grade_submission.htmlweb/urls.pyweb/views.py
ayesha1145
force-pushed
the
feat/course-assignment-system
branch
2 times, most recently
from
42152b1 to
173f915
Compare
Author
|
Addressed remaining feedback:
|
3 tasks
Contributor
|
⏳ @coderabbitai[bot], you have been unassigned due to 24+ hours of inactivity. This task is now available for reassignment. |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
web/urls.py (1)
189-219:⚠️ Potential issue | 🟠 MajorAdd a protected submission-download route to keep file access behind authorization checks.
This URL block introduces assignment flows but still lacks a
download_submissionendpoint. As a result, templates currently rely on rawsubmission.file_submission.urllinks, which can bypass view-level permission checks once the media URL is known. Please add a protected download route and switch templates to that route.🔧 Suggested URL addition
path( "courses/<slug:slug>/submissions/<int:submission_id>/grade/", views.grade_submission, name="grade_submission", ), + path( + "courses/<slug:slug>/submissions/<int:submission_id>/download/", + views.download_submission, + name="download_submission", + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/urls.py` around lines 189 - 219, Add a protected download route for submission files: define a new URL pattern similar to the other assignment routes (e.g., path("courses/<slug:slug>/submissions/<int:submission_id>/download/", views.download_submission, name="download_submission")) and implement a corresponding view function download_submission that performs authorization checks before streaming/serving the file (rather than exposing submission.file_submission.url). Update templates to use the "download_submission" named route instead of linking directly to submission.file_submission.url so file access is always mediated by the view.web/templates/courses/assignment_list.html (1)
49-53:⚠️ Potential issue | 🟠 MajorFix submitted-status lookup to use the local map and native key type.
subis declared on Line 49 but not used, and the string-cast key check on Line 50 can miss real submissions when keys are integers.✅ Suggested fix
- {% with sub=submission_map|default:{} %} - {% if assignment.id|stringformat:"i" in submission_map %} + {% with sub=submission_map|default:{} %} + {% if assignment.id in sub %} <span class="text-green-600 dark:text-green-400"><i class="fas fa-check mr-1" aria-hidden="true"></i> Submitted</span> {% endif %} {% endwith %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_list.html` around lines 49 - 53, The template declares a local dictionary as "sub" but then ignores it and checks membership against "submission_map" using a string-cast key (assignment.id|stringformat:"i"), which can miss integer-keyed entries; change the membership test to use the local "sub" and the native assignment.id value (i.e., replace the string-cast check with a plain {% if assignment.id in sub %}) so the template uses the defaulted local map and integer keys correctly.web/views.py (2)
1964-1982:⚠️ Potential issue | 🟠 MajorKeep create/edit validation in one place.
These two write paths have already drifted:
edit_assignmentcan still 500 on malformedmax_score, accepts anystatus, and both views silently turn an unparseabledue_dateintoNone. Please move the POST parsing into one shared helper or form so create/edit enforce the same title/description/status/max-score/due-date rules before saving. As per coding guidelines:Review Python code for PEP 8 compliance, proper type hints, and Django best practices where applicable. Ensure proper error handling and logging.Also applies to: 2128-2136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/views.py` around lines 1964 - 1982, Extract the POST parsing/validation logic from create_assignment and edit_assignment into a single helper (e.g., validate_assignment_payload) or a Django Form, centralizing checks for title/description presence, integer max_score range (1–1000) with proper ValueError/TypeError handling, whitelisting status to ("draft","published"), deterministic parsing of due_date using parse_datetime with explicit error handling (do not silently drop invalid dates), and allow_late_submissions handling; have both create_assignment and edit_assignment call this helper and surface validation errors via messages.error and redirects (or form errors) so both endpoints enforce identical rules and logging.
2061-2068:⚠️ Potential issue | 🟠 MajorDon’t rely on
content_typealone for upload safety.
file_submission.content_typeis client-controlled. A student can upload an allowed MIME with a dangerous filename or actual payload, and with direct media URLs still in use that can turn into stored active content later. Please add an extension allowlist at minimum, and preferably verify the file signature server-side before saving. As per coding guidelines:Check for security vulnerabilities such as SQL injection, XSS, and insecure use of eval().Suggested hardening
if file_submission: allowed_types = ["application/pdf", "image/jpeg", "image/png", "text/plain", "application/msword", "application/vnd.openxmlformats-officedocument.wordprocessingml.document"] - if file_submission.content_type not in allowed_types: + allowed_extensions = {".pdf", ".jpg", ".jpeg", ".png", ".txt", ".doc", ".docx"} + file_ext = os.path.splitext(file_submission.name)[1].lower() + if file_submission.content_type not in allowed_types or file_ext not in allowed_extensions: messages.error(request, "Invalid file type. Allowed: PDF, JPEG, PNG, TXT, DOC, DOCX.") return redirect("assignment_detail", slug=slug, assignment_id=assignment_id)Run this read-only check to confirm whether uploads are still exposed directly and whether any stronger validation already exists:
#!/bin/bash set -euo pipefail echo "Validation around assignment uploads:" rg -n -C2 --glob '!**/migrations/**' \ 'file_submission|content_type|allowed_extensions|splitext|suffix' . echo echo "How uploaded files are exposed:" rg -n -C2 --glob '!**/migrations/**' \ 'file_submission\.url|MEDIA_URL|MEDIA_ROOT|FileResponse|Content-Disposition' .Expected result: either uploads are only exposed through protected download flows and already have stronger validation, or the MIME-only check here is still the main gate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/views.py` around lines 2061 - 2068, The current upload check uses only file_submission.content_type (allowed_types) which is client-controlled; update the validation in the assignment upload path to (1) enforce an extension allowlist (e.g., check os.path.splitext on file_submission.name against a set of allowed extensions like .pdf,.jpg,.png,.txt,.doc,.docx) and reject mismatches before redirecting to assignment_detail, (2) perform server-side content verification of the file signature/magic bytes (using python-magic or a manual magic-bytes check) to ensure the payload matches the claimed type, (3) sanitize the filename and ensure files are saved only via a protected download flow (not directly exposed by MEDIA_URL) and (4) keep the existing size check; update any error messages to reflect extension/signature failures and use the symbols file_submission, allowed_types, request, slug, assignment_id to locate and modify the code paths.web/templates/courses/assignment_confirm_delete.html (1)
14-20: 🧹 Nitpick | 🔵 TrivialUse shared danger/secondary action button styles for delete/cancel.
Please update the submit and cancel controls to the project’s standard danger (
red-600) and secondary button variants for consistency and clearer UX.As per coding guidelines: "Follow the project's color scheme ... Danger: red-600" and "Use Tailwind secondary button classes: bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_confirm_delete.html` around lines 14 - 20, Update the delete confirmation controls to use the project's shared danger and secondary button variants: change the submit button (the form's "Yes, Delete" button) to use the danger style (use red-600 as the base color with appropriate hover/dark variants) and change the cancel link (the anchor pointing to {% url 'course_assignments' course.slug %}) to use the project's Tailwind secondary button classes (bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300) so both controls match the standard styles.web/templates/courses/detail.html (1)
597-599: 🧹 Nitpick | 🔵 TrivialAlign the Assignments CTA with the shared primary button class contract.
Nice addition overall. Please switch this button to the exact shared primary variant to avoid UI drift (
px-6etc.) across teacher actions.As per coding guidelines: "Use Tailwind primary button classes: bg-teal-300 hover:bg-teal-400 text-white px-6 py-2 rounded-lg transition duration-200."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/detail.html` around lines 597 - 599, The Assignments CTA anchor using {% url 'course_assignments' course.slug %} should use the shared Tailwind primary button class contract; update its class attribute to exactly the primary variant (e.g., "inline-flex items-center justify-center bg-teal-300 hover:bg-teal-400 text-white px-6 py-2 rounded-lg transition duration-200") so spacing and styles match other teacher actions and avoid UI drift.web/templates/courses/assignment_form.html (1)
19-44: 🛠️ Refactor suggestion | 🟠 MajorStandardize form controls to the project’s canonical input class contract.
Please replace the per-field custom class variants with the shared input/select/textarea class string so this form matches the rest of the app.
As per coding guidelines: "Use Tailwind form input classes: w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500 for form inputs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_form.html` around lines 19 - 44, The form inputs (elements with ids title, description, due_date, max_score, and status) use per-field Tailwind class variants; replace each element's class attribute with the project's canonical shared input class string ("w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500") so inputs/select/textarea follow the app-wide contract and match other forms.web/models.py (2)
3228-3228:⚠️ Potential issue | 🟠 MajorMove submission file checks to model/form validators for consistent enforcement.
file_submissioncurrently has no attached validators, so file type/size rules can be bypassed outside the specific view flow. Add a reusable validator at model/form level and keep view checks as defense-in-depth if needed.Suggested fix
+def validate_submission_file(file_obj): + max_size = 10 * 1024 * 1024 + allowed_exts = {".pdf", ".doc", ".docx", ".txt", ".png", ".jpg", ".jpeg"} + ext = os.path.splitext(file_obj.name)[1].lower() + if ext not in allowed_exts: + raise ValidationError("Unsupported file type for assignment submission.") + if file_obj.size > max_size: + raise ValidationError("File size must not exceed 10 MB.") + class AssignmentSubmission(models.Model): @@ - file_submission = models.FileField(upload_to="assignment_submissions/", blank=True, null=True) + file_submission = models.FileField( + upload_to="assignment_submissions/", + blank=True, + null=True, + validators=[validate_submission_file], + )As per coding guidelines: "Review Python code for PEP 8 compliance, proper type hints, and Django best practices where applicable. Check for security vulnerabilities..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/models.py` at line 3228, The FileField file_submission lacks model/form-level validators so file type/size rules can be bypassed; create a reusable validator function (e.g., validate_submission_file) that enforces allowed MIME/extensions and max size, register it on the models.FileField via validators=[validate_submission_file] in the model and also add a corresponding clean_file_submission (or use the validator) in the ModelForm to ensure consistent enforcement across flows; keep existing view checks as defense-in-depth but move the authoritative validation into the model/form so uploads are always validated regardless of call path.
3227-3231:⚠️ Potential issue | 🟠 MajorAdd model-level
clean()validation for empty submissions and score overflow.Right now, a submission can be saved with no content, and
scorecan exceedassignment.max_scoreif data bypasses the grading view (e.g., admin/import/script). Please enforce both inAssignmentSubmission.clean().Suggested fix
class AssignmentSubmission(models.Model): @@ class Meta: unique_together = ["assignment", "student"] ordering = ["-submitted_at"] @@ + def clean(self): + errors = {} + + if not (self.text_response or "").strip() and not self.file_submission: + errors["text_response"] = "Provide a text response or upload a file." + + if self.score is not None and self.score > self.assignment.max_score: + errors["score"] = f"Score cannot exceed max score ({self.assignment.max_score})." + + if errors: + raise ValidationError(errors) + + def save(self, *args, **kwargs): + self.full_clean() + super().save(*args, **kwargs)As per coding guidelines: "Review Python code for PEP 8 compliance, proper type hints, and Django best practices where applicable."
Also applies to: 3243-3254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/models.py` around lines 3227 - 3231, Add a model-level clean() on AssignmentSubmission that calls super().clean(), raises django.core.exceptions.ValidationError if both text_response and file_submission are empty (require at least one), and validates score is not negative and does not exceed self.assignment.max_score (if assignment is present and max_score is not None); include field-specific errors keyed to "text_response" / "file_submission" or "score" and ensure the validation runs on save by leaving Django's standard clean() hook in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/templates/courses/assignment_form.html`:
- Around line 55-62: Replace the Cancel text link (the <a> with href "{% url
'course_assignments' course.slug %}") with the shared secondary button styling:
add Tailwind classes bg-gray-100 hover:bg-gray-200 dark:bg-gray-700
text-gray-700 dark:text-gray-300 plus matching spacing/shape classes (e.g., px-6
py-2 rounded-lg text-sm transition duration-200 focus:outline-none focus:ring-2
focus:ring-gray-200) so it visually matches the submit button variant while
keeping the same href and element semantics.
In `@web/templates/courses/assignment_list.html`:
- Around line 76-79: The empty-state CTA anchor that links to the
'create_assignment' URL lacks keyboard focus styling; update the anchor (the <a>
element with href="{% url 'create_assignment' course.slug %}") to include
explicit focus classes (e.g., focus:outline-none and a visible focus ring like
focus:ring-2 focus:ring-offset-2 focus:ring-teal-500 or similar) so keyboard
users see a clear focus state while preserving existing hover styles and
dark-mode variants.
In `@web/views.py`:
- Around line 2152-2155: Deleting an Assignment via assignment.delete() cascades
and removes AssignmentSubmission rows but leaves underlying files in storage
because Django FileField does not auto-delete files; update the model to clean
up files by either (a) implementing AssignmentSubmission.delete(self, *args,
**kwargs) to call self.file_submission.delete(save=False) before
super().delete(...) or (b) registering a post_delete signal handler for
AssignmentSubmission that deletes instance.file_submission; ensure you reference
the AssignmentSubmission model and its file_submission field so all submission
blobs are removed when an assignment is deleted.
---
Duplicate comments:
In `@web/models.py`:
- Line 3228: The FileField file_submission lacks model/form-level validators so
file type/size rules can be bypassed; create a reusable validator function
(e.g., validate_submission_file) that enforces allowed MIME/extensions and max
size, register it on the models.FileField via
validators=[validate_submission_file] in the model and also add a corresponding
clean_file_submission (or use the validator) in the ModelForm to ensure
consistent enforcement across flows; keep existing view checks as
defense-in-depth but move the authoritative validation into the model/form so
uploads are always validated regardless of call path.
- Around line 3227-3231: Add a model-level clean() on AssignmentSubmission that
calls super().clean(), raises django.core.exceptions.ValidationError if both
text_response and file_submission are empty (require at least one), and
validates score is not negative and does not exceed self.assignment.max_score
(if assignment is present and max_score is not None); include field-specific
errors keyed to "text_response" / "file_submission" or "score" and ensure the
validation runs on save by leaving Django's standard clean() hook in place.
In `@web/templates/courses/assignment_confirm_delete.html`:
- Around line 14-20: Update the delete confirmation controls to use the
project's shared danger and secondary button variants: change the submit button
(the form's "Yes, Delete" button) to use the danger style (use red-600 as the
base color with appropriate hover/dark variants) and change the cancel link (the
anchor pointing to {% url 'course_assignments' course.slug %}) to use the
project's Tailwind secondary button classes (bg-gray-100 hover:bg-gray-200
dark:bg-gray-700 text-gray-700 dark:text-gray-300) so both controls match the
standard styles.
In `@web/templates/courses/assignment_form.html`:
- Around line 19-44: The form inputs (elements with ids title, description,
due_date, max_score, and status) use per-field Tailwind class variants; replace
each element's class attribute with the project's canonical shared input class
string ("w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg
focus:ring-2 focus:ring-blue-500") so inputs/select/textarea follow the app-wide
contract and match other forms.
In `@web/templates/courses/assignment_list.html`:
- Around line 49-53: The template declares a local dictionary as "sub" but then
ignores it and checks membership against "submission_map" using a string-cast
key (assignment.id|stringformat:"i"), which can miss integer-keyed entries;
change the membership test to use the local "sub" and the native assignment.id
value (i.e., replace the string-cast check with a plain {% if assignment.id in
sub %}) so the template uses the defaulted local map and integer keys correctly.
In `@web/templates/courses/detail.html`:
- Around line 597-599: The Assignments CTA anchor using {% url
'course_assignments' course.slug %} should use the shared Tailwind primary
button class contract; update its class attribute to exactly the primary variant
(e.g., "inline-flex items-center justify-center bg-teal-300 hover:bg-teal-400
text-white px-6 py-2 rounded-lg transition duration-200") so spacing and styles
match other teacher actions and avoid UI drift.
In `@web/urls.py`:
- Around line 189-219: Add a protected download route for submission files:
define a new URL pattern similar to the other assignment routes (e.g.,
path("courses/<slug:slug>/submissions/<int:submission_id>/download/",
views.download_submission, name="download_submission")) and implement a
corresponding view function download_submission that performs authorization
checks before streaming/serving the file (rather than exposing
submission.file_submission.url). Update templates to use the
"download_submission" named route instead of linking directly to
submission.file_submission.url so file access is always mediated by the view.
In `@web/views.py`:
- Around line 1964-1982: Extract the POST parsing/validation logic from
create_assignment and edit_assignment into a single helper (e.g.,
validate_assignment_payload) or a Django Form, centralizing checks for
title/description presence, integer max_score range (1–1000) with proper
ValueError/TypeError handling, whitelisting status to ("draft","published"),
deterministic parsing of due_date using parse_datetime with explicit error
handling (do not silently drop invalid dates), and allow_late_submissions
handling; have both create_assignment and edit_assignment call this helper and
surface validation errors via messages.error and redirects (or form errors) so
both endpoints enforce identical rules and logging.
- Around line 2061-2068: The current upload check uses only
file_submission.content_type (allowed_types) which is client-controlled; update
the validation in the assignment upload path to (1) enforce an extension
allowlist (e.g., check os.path.splitext on file_submission.name against a set of
allowed extensions like .pdf,.jpg,.png,.txt,.doc,.docx) and reject mismatches
before redirecting to assignment_detail, (2) perform server-side content
verification of the file signature/magic bytes (using python-magic or a manual
magic-bytes check) to ensure the payload matches the claimed type, (3) sanitize
the filename and ensure files are saved only via a protected download flow (not
directly exposed by MEDIA_URL) and (4) keep the existing size check; update any
error messages to reflect extension/signature failures and use the symbols
file_submission, allowed_types, request, slug, assignment_id to locate and
modify the code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dec175ed-f280-431c-9c6d-4af07faad621
⛔ Files ignored due to path filters (1)
web/migrations/0064_add_assignment_and_submission.pyis excluded by!**/migrations/**
📒 Files selected for processing (10)
web/admin.pyweb/models.pyweb/templates/courses/assignment_confirm_delete.htmlweb/templates/courses/assignment_detail.htmlweb/templates/courses/assignment_form.htmlweb/templates/courses/assignment_list.htmlweb/templates/courses/detail.htmlweb/templates/courses/grade_submission.htmlweb/urls.pyweb/views.py
ayesha1145
force-pushed
the
feat/course-assignment-system
branch
from
173f915 to
723dc08
Compare
Author
|
Addressed latest CodeRabbit feedback:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
web/models.py (1)
3227-3230:⚠️ Potential issue | 🟠 MajorEnforce submission invariants in the model layer.
AssignmentSubmissioncan still be saved with no content at all, andscorecan exceedassignment.max_score. That is easy to bypass through admin or any future non-view write path, and it letspercentagego over 100. Put both checks inclean()and runfull_clean()before save.Possible shape
class AssignmentSubmission(models.Model): """A student submission for an assignment.""" @@ def __str__(self) -> str: return f"{self.student.username} - {self.assignment.title}" + def clean(self): + errors = {} + if not self.text_response and not self.file_submission: + errors["__all__"] = "Provide a text response or upload a file." + if self.score is not None and self.score > self.assignment.max_score: + errors["score"] = f"Score cannot exceed {self.assignment.max_score}." + if errors: + raise ValidationError(errors) + + def save(self, *args, **kwargs): + self.full_clean() + super().save(*args, **kwargs) + `@property` def percentage(self) -> "int | None": if self.score is not None and self.assignment.max_score > 0: return round((self.score / self.assignment.max_score) * 100) return NoneAlso applies to: 3250-3254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/models.py` around lines 3227 - 3230, Add model-level validation to enforce submission invariants: implement a clean(self) on the AssignmentSubmission model that (1) raises ValidationError if both text_response and file_submission are empty/None and (2) validates score is not negative and does not exceed self.assignment.max_score (use self.assignment.max_score to compare); then override save(self, *args, **kwargs) to call self.full_clean() before super().save(...) so all writes (admin or otherwise) are validated. Apply the same pattern to the other similar model referenced around the 3250–3254 region.web/templates/courses/assignment_confirm_delete.html (1)
14-20: 🧹 Nitpick | 🔵 TrivialMatch the destructive and cancel actions to the shared button variants.
The flow is correct, but these classes still diverge from the repo's standard danger/secondary treatments.
As per coding guidelines: "Follow the project's color scheme using Tailwind's color classes (Primary: teal-300, Secondary: gray-600, Success: green-600, Warning: yellow-600, Danger: red-600, Dark mode variants using dark: prefix)" and "Use Tailwind secondary button classes: bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300."Suggested update
<button type="submit" - class="bg-red-500 hover:bg-red-600 dark:bg-red-700 dark:hover:bg-red-600 text-white font-semibold px-6 py-2 rounded-lg transition duration-200 focus:outline-none focus:ring-2 focus:ring-red-400"> + class="bg-red-600 hover:bg-red-700 dark:bg-red-700 dark:hover:bg-red-600 text-white px-6 py-2 rounded-lg transition duration-200 focus:outline-none focus:ring-2 focus:ring-red-400"> Yes, Delete </button> @@ <a href="{% url 'course_assignments' course.slug %}" - class="text-gray-500 hover:text-gray-700 dark:text-gray-400 text-sm">Cancel</a> + class="bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300 px-6 py-2 rounded-lg transition duration-200 focus:outline-none focus:ring-2 focus:ring-gray-200">Cancel</a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_confirm_delete.html` around lines 14 - 20, The delete confirmation uses nonstandard Tailwind classes for the destructive and cancel actions; update the "Yes, Delete" submit button and the "Cancel" anchor (the elements containing the text "Yes, Delete" and "Cancel") to use the project's shared button variants: set the destructive/danger button to use the danger variant (red-600 with dark: red-600 hover behaviors per project convention) and change the cancel anchor to the secondary button classes (bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300) so they match the repo's standard styling.web/templates/courses/assignment_form.html (2)
55-61: 🧹 Nitpick | 🔵 TrivialStyle Cancel as the shared secondary action.
The submit action reads like a button, but Cancel still falls back to a lighter one-off style. Using the shared secondary variant keeps the action hierarchy consistent with the rest of the course forms.
As per coding guidelines: "Use Tailwind secondary button classes: bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300."Suggested update
<a href="{% url 'course_assignments' course.slug %}" - class="bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 dark:hover:bg-gray-600 text-gray-700 dark:text-gray-300 font-semibold px-6 py-2 rounded-lg transition duration-200">Cancel</a> + class="bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300 px-6 py-2 rounded-lg transition duration-200 focus:outline-none focus:ring-2 focus:ring-gray-200">Cancel</a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_form.html` around lines 55 - 61, The Cancel link (the <a> element that points to the {% url 'course_assignments' course.slug %}) should use the shared secondary Tailwind button variant so it matches the other form secondary actions; update its class list to the standard secondary classes (bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300 font-semibold px-6 py-2 rounded-lg transition duration-200) replacing the current classes so the Cancel action visually matches the shared secondary style used elsewhere.
19-20: 🧹 Nitpick | 🔵 TrivialStandardize these controls on the shared form-control class contract.
These fields are still using a one-off class stack instead of the repo's canonical input/select/textarea styling. Keeping them on the shared class string avoids Tailwind drift across forms.
As per coding guidelines: "Use Tailwind form input classes: w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500 for form inputs."Representative update
- class="w-full px-3 py-2 border border-gray-300 dark:border-gray-600 rounded-lg bg-white dark:bg-gray-700 text-gray-900 dark:text-white text-sm focus:outline-none focus:ring-2 focus:ring-teal-300" + class="w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500"Also applies to: 24-25, 30-32, 36-38, 43-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_form.html` around lines 19 - 20, The input with id="title" (name="title") is using a one-off Tailwind class stack; update its class attribute to the repo's canonical form input classes (either replace the current class value with "w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500" or use the shared utility class that maps to that string, e.g., form-control) and apply the same change to the other inputs/selects/textareas in this template that currently use custom stacks (the other form controls referenced in the review) so all form controls conform to the standardized contract.web/templates/courses/assignment_list.html (1)
49-53:⚠️ Potential issue | 🟠 MajorFix the "Submitted" status lookup logic—string formatting prevents badge display.
The template converts
assignment.idto a string withstringformat:"i", butsubmission_mapuses integer keys (fromassignment_idin the view). In Python,"1" in {1: ...}returnsFalse, so the membership check fails and the "Submitted" badge won't appear even when students have submitted.Additionally, the template defines
sub=submission_mapbut then checks againstsubmission_mapinstead ofsub, making the context variable unused.🔧 Suggested fix
-{% with sub=submission_map|default:{} %} -{% if assignment.id|stringformat:"i" in submission_map %} +{% with sub=submission_map|default:{} %} +{% if assignment.id in sub %} <span class="text-green-600 dark:text-green-400"><i class="fas fa-check mr-1" aria-hidden="true"></i> Submitted</span> {% endif %} {% endwith %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/courses/assignment_list.html` around lines 49 - 53, The template's membership check converts assignment.id to a string and checks submission_map (not the local alias), causing false negatives; update the condition to use the aliased dict and the native integer id by replacing the membership test to check assignment.id in sub (keeping the existing "{% with sub=submission_map %}" block) so the "Submitted" badge displays correctly for integer keys.web/views.py (1)
1964-1982:⚠️ Potential issue | 🟠 MajorKeep assignment create/edit on one validated write path.
These two branches have already drifted.
create_assignmentnow validatesmax_score, required fields, and status, but it still turns an unparseable non-emptydue_dateintoNone;edit_assignmentis further behind and still writes raw POST values straight onto the model, so a badmax_scorecan 500 and blank/invalid fields can be saved. Please move this parsing into one shared validator/helper and reuse it in both views. An arbitrary status saved fromedit_assignmentalso interacts badly with the detail view’s draft-only guard.Also applies to: 2128-2135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/views.py` around lines 1964 - 1982, create_assignment and edit_assignment have duplicated, inconsistent validation (max_score, title/description, status, due_date parsing) causing crashes and invalid saves; extract the validation/parsing into a single helper (e.g., validate_assignment_data or parse_assignment_form) and call it from both create_assignment and edit_assignment, moving logic that currently lives in create_assignment (max_score range check, required fields, canonical status enforcement, converting invalid non-empty due_date to a validation error or consistent None) into that helper; ensure the helper returns sanitized values (title, description, parsed_due or error, max_score int, allow_late boolean, status limited to "draft"|"published") and have both views use its return/raise behavior to show messages.error and redirect rather than writing raw POST values, preserving the detail view’s draft-only assumptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/admin.py`:
- Around line 615-626: The AssignmentSubmissionAdmin is missing relation widgets
and will render huge dropdowns; update AssignmentSubmissionAdmin (and optionally
AssignmentAdmin) to use relation widgets by adding either raw_id_fields or
autocomplete_fields for the foreign keys (e.g., add raw_id_fields = ("student",
"assignment", "graded_by") or configure autocomplete_fields = ("student",
"assignment", "graded_by") and ensure corresponding ModelAdmin/search_fields
exist on the related models); modify AssignmentAdmin similarly if its foreign
keys (like course) need relation widgets (e.g., raw_id_fields = ("course",)) so
look for the classes AssignmentAdmin and AssignmentSubmissionAdmin and add the
appropriate raw_id_fields/autocomplete_fields entries.
In `@web/models.py`:
- Line 3228: The FileField file_submission on the model uses the default
(public) media storage; update it to use a private storage backend or a secured
upload flow so submitted files aren't publicly accessible. Locate the
file_submission declaration (models.FileField) and change it to reference your
private storage backend (e.g., pass storage=PrivateStorageInstance or use a
custom storage class like PrivateGCSStorage) or switch to storing submissions
behind an authenticated download view; ensure any existing references to
file_submission and its URL generation are updated to use the authenticated
download endpoint or storage API so access always enforces course/teacher
permissions.
- Around line 3243-3245: The Submission model (the model with Meta
unique_together = ["assignment","student"] that has the file_submission field)
currently only deletes files when rows are deleted; add logic to remove the old
stored file when a row is updated and file_submission changes or is cleared:
implement either a pre_save signal handler or override Submission.save() to load
the existing instance from the DB, compare its file_submission to the incoming
instance, and if different (and the old file exists) call
old_file.delete(save=False) (or the storage delete equivalent) before saving the
new value; apply the same pattern to the other related model referenced around
lines 3258-3267 that also uses file_submission.
In `@web/templates/courses/assignment_detail.html`:
- Around line 78-83: The Text Response field (textarea with id/name
"text_response") is ambiguous about whether it's required; either add the
required attribute to the textarea (id="text_response") and ensure server-side
validation accepts/returns errors for missing text, or explicitly mark the label
("Text Response (optional)") to indicate it's not required; update the label
element text and any client/server validation logic (e.g., form handling code
that checks text_response) to match the chosen behavior so UI and backend are
consistent.
In `@web/views.py`:
- Around line 2060-2078: This code stores student file submissions but exposes
them via direct media URLs; add a permission-checked download/view endpoint and
stop linking to raw file URLs. Implement a view like
submission_file_download(request, submission_id) that fetches
AssignmentSubmission, verifies access (e.g., request.user == submission.student
or user is course instructor/staff/enrolled via your enrollment check), and
returns the file using Django FileResponse or your sendfile helper; register a
URL pattern for it and update templates to link to this view instead of
file_submission.url. Also ensure uploaded files are kept in non-public storage
or protected media (or set file_submission storage to private) so direct media
links aren’t publicly accessible if your deployment supports private storage.
Ensure the permission check is used anywhere files are served (not just
assignment_detail).
---
Duplicate comments:
In `@web/models.py`:
- Around line 3227-3230: Add model-level validation to enforce submission
invariants: implement a clean(self) on the AssignmentSubmission model that (1)
raises ValidationError if both text_response and file_submission are empty/None
and (2) validates score is not negative and does not exceed
self.assignment.max_score (use self.assignment.max_score to compare); then
override save(self, *args, **kwargs) to call self.full_clean() before
super().save(...) so all writes (admin or otherwise) are validated. Apply the
same pattern to the other similar model referenced around the 3250–3254 region.
In `@web/templates/courses/assignment_confirm_delete.html`:
- Around line 14-20: The delete confirmation uses nonstandard Tailwind classes
for the destructive and cancel actions; update the "Yes, Delete" submit button
and the "Cancel" anchor (the elements containing the text "Yes, Delete" and
"Cancel") to use the project's shared button variants: set the
destructive/danger button to use the danger variant (red-600 with dark: red-600
hover behaviors per project convention) and change the cancel anchor to the
secondary button classes (bg-gray-100 hover:bg-gray-200 dark:bg-gray-700
text-gray-700 dark:text-gray-300) so they match the repo's standard styling.
In `@web/templates/courses/assignment_form.html`:
- Around line 55-61: The Cancel link (the <a> element that points to the {% url
'course_assignments' course.slug %}) should use the shared secondary Tailwind
button variant so it matches the other form secondary actions; update its class
list to the standard secondary classes (bg-gray-100 hover:bg-gray-200
dark:bg-gray-700 text-gray-700 dark:text-gray-300 font-semibold px-6 py-2
rounded-lg transition duration-200) replacing the current classes so the Cancel
action visually matches the shared secondary style used elsewhere.
- Around line 19-20: The input with id="title" (name="title") is using a one-off
Tailwind class stack; update its class attribute to the repo's canonical form
input classes (either replace the current class value with "w-full px-4 py-2
border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2
focus:ring-blue-500" or use the shared utility class that maps to that string,
e.g., form-control) and apply the same change to the other
inputs/selects/textareas in this template that currently use custom stacks (the
other form controls referenced in the review) so all form controls conform to
the standardized contract.
In `@web/templates/courses/assignment_list.html`:
- Around line 49-53: The template's membership check converts assignment.id to a
string and checks submission_map (not the local alias), causing false negatives;
update the condition to use the aliased dict and the native integer id by
replacing the membership test to check assignment.id in sub (keeping the
existing "{% with sub=submission_map %}" block) so the "Submitted" badge
displays correctly for integer keys.
In `@web/views.py`:
- Around line 1964-1982: create_assignment and edit_assignment have duplicated,
inconsistent validation (max_score, title/description, status, due_date parsing)
causing crashes and invalid saves; extract the validation/parsing into a single
helper (e.g., validate_assignment_data or parse_assignment_form) and call it
from both create_assignment and edit_assignment, moving logic that currently
lives in create_assignment (max_score range check, required fields, canonical
status enforcement, converting invalid non-empty due_date to a validation error
or consistent None) into that helper; ensure the helper returns sanitized values
(title, description, parsed_due or error, max_score int, allow_late boolean,
status limited to "draft"|"published") and have both views use its return/raise
behavior to show messages.error and redirect rather than writing raw POST
values, preserving the detail view’s draft-only assumptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8739267f-e908-4d7f-8dbb-b0245c914749
⛔ Files ignored due to path filters (1)
web/migrations/0064_add_assignment_and_submission.pyis excluded by!**/migrations/**
📒 Files selected for processing (10)
web/admin.pyweb/models.pyweb/templates/courses/assignment_confirm_delete.htmlweb/templates/courses/assignment_detail.htmlweb/templates/courses/assignment_form.htmlweb/templates/courses/assignment_list.htmlweb/templates/courses/detail.htmlweb/templates/courses/grade_submission.htmlweb/urls.pyweb/views.py
ayesha1145
force-pushed
the
feat/course-assignment-system
branch
from
723dc08 to
7938ee6
Compare
Author
|
Addressed latest CodeRabbit feedback:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a complete course assignment system allowing teachers to create assignments and students to submit work for grading.
Changes:
Features:
Overview
Adds a full course assignment system: teachers can create/publish/edit/delete assignments, view and grade student submissions; students can view published assignments, submit text and/or a file, and view scores/feedback. Changes include new models + migration, admin registrations, views/URL routes, templates, and permission guards. UI includes status/due indicators and dark-mode styling.
Key changes
Impact & reviewer focus