Q&A Feature#1249
Open
Nathan-Kimm wants to merge 61 commits into
Open
Conversation
make q-a up to date with dev
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds the question_detail view, URL pattern, tests, and a minimal placeholder template so the right panel can render question + answers via an AJAX GET request to /qa/question/<id>/. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rove tests - Add hidden=False filter to get_instructors_for_course queryset - Move TrigramSimilarity and Instructor imports to module level (remove local imports) - Add assertIsInstance check to test_course_search_returns_json - Rename test_empty_query_returns_empty to test_short_query_returns_empty with updated docstring Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nswer Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces the placeholder with the complete right-panel partial that renders the question content, per-answer voting controls, edit/delete dropdowns for owners, and the answer submission form. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…AX setup Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace minimal modal-only JS with full Q&A dashboard implementation covering question selection/AJAX detail loading, search, new-post modal with course autocomplete and instructor fetch, voting, answer form with duplicate check, and question/answer edit+delete actions - Append CSS for course-search dropdown, loading state, empty states, voted button highlight, login prompt, and required-field asterisk - Fix DeleteQuestion and DeleteAnswer success URLs to redirect to /qa/ instead of the now-incorrect course_instructor URL Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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:
📝 WalkthroughWalkthroughAdds a full Q&A feature: DB migrations to support threaded answers and nullable question targets; model changes for soft-delete and nested replies; new QA views, APIs and URL routes; server-rendered templates and modals; frontend JS/CSS for dashboard interaction; icons; and comprehensive tests covering create/search/detail/replies and voting. ChangesQ&A Feature (single cohesive change DAG)
Sequence Diagram(s)sequenceDiagram
participant Browser as Client (Browser)
participant Server as Django (Views)
participant DB as Database
Browser->>Server: GET /qa/ (q / department / course)
Server->>DB: Query questions, annotate votes, filter
DB-->>Server: Questions + annotations
Server-->>Browser: Rendered list or JSON (posts_html, detail_html)
Browser->>Server: GET /qa/question/<id>/ (XHR)
Server->>DB: Fetch question, answers, build nested replies
DB-->>Server: Question + threaded answers
Server-->>Browser: Rendered question detail HTML
Browser->>Server: POST /qa/new_reply/ (parent_answer, text, semester)
Server->>DB: Insert Answer with parent_answer FK
DB-->>Server: Created reply
Server-->>Browser: Redirect or refreshed detail (XHR)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (9)
tcf_website/static/qa/qa_dashboard.css (1)
988-998: Consolidate duplicate voted thumbs-up styling.
.vote-btn.voted .fa-thumbs-upand.post-stats.voted .fa-thumbs-updeclare identical rules. Combine into a single selector list to keep them in sync going forward.♻️ Proposed refactor
-.vote-btn.voted .fa-thumbs-up { - background-image: url("../icons/img/fa-thumbs-up-solid.svg"); - filter: brightness(0) saturate(100%) invert(51%) sepia(98%) saturate(1828%) hue-rotate(0deg) brightness(102%) contrast(103%); - transform: scale(1.2); -} - -.post-stats.voted .fa-thumbs-up { - background-image: url("../icons/img/fa-thumbs-up-solid.svg"); - filter: brightness(0) saturate(100%) invert(51%) sepia(98%) saturate(1828%) hue-rotate(0deg) brightness(102%) contrast(103%); - transform: scale(1.2); -} +.vote-btn.voted .fa-thumbs-up, +.post-stats.voted .fa-thumbs-up { + background-image: url("../icons/img/fa-thumbs-up-solid.svg"); + filter: brightness(0) saturate(100%) invert(51%) sepia(98%) saturate(1828%) hue-rotate(0deg) brightness(102%) contrast(103%); + transform: scale(1.2); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/static/qa/qa_dashboard.css` around lines 988 - 998, Combine the duplicate CSS rules for the voted thumbs-up state by merging the selectors .vote-btn.voted .fa-thumbs-up and .post-stats.voted .fa-thumbs-up into a single selector list (comma-separated) and remove the redundant block so the background-image, filter, and transform declarations are maintained in one place for both cases.tcf_website/migrations/0025_question_instructor_optional.py (1)
14-24: Consideron_delete=SET_NULLnow thatinstructoris optional.With
null=True/blank=True/default=None, a question can exist without an instructor (e.g., department- or course-level question per the PR description). Keepingon_delete=CASCADEmeans deleting an instructor will also wipe every otherwise-valid question that merely tagged that instructor — even though the question's course/department context is still meaningful.SET_NULLwould preserve those questions and matches the new "instructor is optional" semantics. Update bothtcf_website/models/models.pyand add a follow-upAlterFieldmigration if you agree.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/migrations/0025_question_instructor_optional.py` around lines 14 - 24, The migration currently changes Question.instructor to allow nulls but leaves on_delete=CASCADE; update the Question model in tcf_website/models/models.py to use on_delete=models.SET_NULL (or django.db.models.deletion.SET_NULL) for the instructor ForeignKey and then add a follow-up migration that AlterField on the Question.instructor to set on_delete to SET_NULL (so existing DB schema is migrated); reference the Question model and the instructor ForeignKey when making these two edits.tcf_website/templates/qa/qa_dashboard.html (1)
316-324: Fragile URL construction via placeholder stripping.Building base URLs by reversing with
id=0and then using.replace('/0/', '/')or.replace('0/edit/', '')is brittle:
editQuestion/editAnswerdrop the trailing segment entirely and rely on the JS to know the full shape (e.g. appending${id}/edit/), which duplicates URL structure knowledge in JS.- If any legitimate URL segment ever contains the sentinel substring (e.g.
/0/), the replacement will silently corrupt the URL.- If the route changes (e.g. renamed to
/qa/questions/<id>/edit), the hardcoded.replace()pattern will produce a wrong URL at runtime with no failure signal.Consider passing the full reversed URL with a safer sentinel and replacing it in JS, for example:
♻️ Proposed fix
const QA_URLS = { - questionDetail: '{% url "qa_question_detail" question_id=0 %}'.replace('/0/', '/'), + questionDetail: '{% url "qa_question_detail" question_id=0 %}', // replace '/0/' with '/<id>/' createQuestion: '{% url "create_question" %}', searchCourses: '{% url "qa_search_courses" %}', - getInstructors: '{% url "qa_get_instructors" course_id=0 %}'.replace('/0/instructors/', '/'), - editQuestion: '{% url "edit_question" question_id=0 %}'.replace('0/edit/', ''), - editAnswer: '{% url "edit_answer" answer_id=0 %}'.replace('0/edit/', ''), + getInstructors: '{% url "qa_get_instructors" course_id=0 %}', + editQuestion: '{% url "edit_question" question_id=0 %}', + editAnswer: '{% url "edit_answer" answer_id=0 %}', dashboard: '{% url "qa" %}', };Then in JS, substitute the literal
/0/(or use a clearly marked sentinel like__ID__) with the actual id at call sites. This keeps the route shape opaque to JS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/templates/qa/qa_dashboard.html` around lines 316 - 324, The QA_URLS object currently builds IDs by reversing URLs with id=0 and then string-replacing substrings like '/0/' or '0/edit/', which is brittle; instead change the template-reversed URLs to include a clear sentinel token (e.g. '__ID__') for all parameterized routes (keys: questionDetail, getInstructors, editQuestion, editAnswer) and update the JS call sites to replace '__ID__' with the real id when constructing links; keep non-parameterized entries (createQuestion, searchCourses, dashboard) unchanged and ensure all occurrences that append/format IDs use the sentinel-replacement approach so the route shape remains opaque to JS.tcf_website/tests/test_qa.py (1)
31-46: Consider asserting the creation response before dereferencingQuestion.objects.get(...).If the form silently fails (e.g., schema or validation drift), the test fails on
Question.DoesNotExistwith a cryptic traceback rather than pointing at the actual failing response code/redirect. A quickself.assertRedirects(response, reverse("qa"), fetch_redirect_response=False)here would localise failures. Minor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/tests/test_qa.py` around lines 31 - 46, Add an assertion that the POST returned the expected redirect before fetching the created Question so failures surface clearly: in test_create_question_creates_record, capture the response from self.client.post(reverse("create_question"), ...) and call self.assertRedirects(response, reverse("qa"), fetch_redirect_response=False) (or the appropriate expected redirect) before calling Question.objects.get(user=self.user1); this ensures form/validation errors show on the response instead of raising Question.DoesNotExist.tcf_website/views/qa.py (4)
119-128: Redundantquestion__course__isnull=Falsefilter.
Course.objects.filter(question__isnull=False, ...)already joins on the reverse FKQuestion.course, so every row in the join implicitly hasquestion.course_id = course.id(i.e., non-null). The extraquestion__course__isnull=Falsecondition is a no-op and just adds SQL noise. Safe to drop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/views/qa.py` around lines 119 - 128, The filter contains a redundant condition: remove the unnecessary "question__course__isnull=False" from the Course.objects.filter(...) call that constructs courses_with_questions since "question__isnull=False" already guarantees a joined Question.course; update the filter to only include question__isnull=False and keep the select_related/distinct/order_by unchanged (refer to courses_with_questions and the Course.objects.filter usage).
256-276: Hardcodedyear__gte=2022will silently rot.The "recent courses" cutoff is baked in as 2022. In a year or two, "recent" will span 4+ years and degrade search relevance; worse, it'll get forgotten. Compute from the current semester/year (e.g.,
timezone.now().year - 3) or pull the latest fewSemesterrows and filter by that set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/views/qa.py` around lines 256 - 276, The code hardcodes recent_courses = Course.objects.filter(semester_last_taught__year__gte=2022) which will rot; change it to compute a dynamic cutoff or derive recent semesters instead: either calculate cutoff_year = timezone.now().year - 3 (importing django.utils.timezone) and use semester_last_taught__year__gte=cutoff_year, or query the latest Semester rows (e.g., fetch latest N Semester objects) and filter Course.objects.filter(semester_last_taught__in=recent_semesters). Update the reference where recent_courses is used so TrigramSimilarity fallback logic (the annotate/filter/select_related block) continues to operate on the new recent_courses queryset.
325-347: Consider a cleaner "department-wins" UX instead of silently dropping stale course/instructor.
clean()currently accepts a POST that includes bothdepartmentandcourse+instructorand silently blanks the latter two. The existing testtest_department_question_ignores_stale_course_and_instructorpins this behavior, but from a user standpoint a conflicting payload is usually an integrity bug (e.g., the modal didn't reset state), not a valid request. If keeping the current behavior, a short comment noting it is intentional would help; otherwise, consider raising aValidationErrorso the client reveals the bug. Leaving as-is is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/views/qa.py` around lines 325 - 347, The form clean() currently silently clears course and instructor when department is present; either document this with a clear inline comment in the clean() method (to mark "department wins" as intentional) or change validation to surface the conflict: in clean() (the form's clean method) detect when department is set AND (course or instructor) is also set and raise forms.ValidationError with a message like "Conflicting target: select either a department or a course/instructor" (if you change behavior, update or remove the test named test_department_question_ignores_stale_course_and_instructor to reflect the new validation).
100-107:questions.get(id=...)on the filtered queryset will 500 if the selected question is filtered out.If a user lands on
?question=<id>&course=<unrelated_course>,questions.get(id=...)raisesQuestion.DoesNotExist, which is caught — fine. But the server then silently falls back toquestions.first()with no indication, so the URL's?question=...becomes meaningless. This is a UX nit, not a crash, but consider logging / surfacing a 404 partial when an explicit?question=is unresolvable, so the client can reconcile its URL state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/views/qa.py` around lines 100 - 107, The code silently ignores a mismatched ?question=<id> by catching Question.DoesNotExist on questions.get(id=selected_question_id) and falling back to questions.first(); update the handler around questions.get/question selection (references: questions.get, selected_question_id, selected_question, questions.first) to surface the mismatch: when the requested question id is not found in the filtered queryset, log a warning and return a clear indicator to the client (e.g., raise a 404/HttpResponseNotFound or include a flag in the response/context) so the frontend can reconcile the URL state instead of silently using questions.first().tcf_website/static/qa/qa_dashboard.js (1)
671-678: UseQA_URLS/ Django URL reversing instead of hardcoding/questions/...and/answers/...paths.
QA_URLS(defined inqa_dashboard.html) already centralises reversed URLs for this exact reason, yet the delete and vote endpoints are hardcoded here. Ifurls.pyever changes the URL shape (e.g., nests under/qa/...), these silently break. Please adddeleteQuestion,deleteAnswer,upvoteQuestion,upvoteAnswerentries toQA_URLSand reference them from here.Also applies to: 709-716, 756-767
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/static/qa/qa_dashboard.js` around lines 671 - 678, The fetch calls in qa_dashboard.js currently hardcode endpoints like `/questions/${questionIdToDelete}/delete/` (and similar hardcoded paths used around the vote and answer delete blocks), which will break if URL patterns change; update the code to use the centralized QA_URLS mapping from the template instead: add keys deleteQuestion, deleteAnswer, upvoteQuestion, upvoteAnswer to QA_URLS in qa_dashboard.html (or the template context) and replace hardcoded string constructions in the deleteQuestion/deleteAnswer and upvote handlers in qa_dashboard.js to use QA_URLS.deleteQuestion, QA_URLS.deleteAnswer, QA_URLS.upvoteQuestion, QA_URLS.upvoteAnswer (interpolating the ID where needed) so all requests reference reversed URLs. Ensure the headers/body usage remains the same and adjust any string formatting to match how QA_URLS provides placeholders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tcf_website/migrations/0024_forumcategory_forumpost_forumresponse_question_title_and_more.py`:
- Around line 1-18: The migration file name implies multiple model changes but
the Migration only adds a Question.title CharField via the AddField operation
(model_name="question", name="title"); if this migration has not been applied
anywhere, rename the file to reflect the single change (e.g.,
0024_question_title.py) to avoid confusion; if the migration has already been
applied in any environment, do not rename—leave the filename and instead add a
new, clearly named migration for any further model additions (or revert and
re-create migrations consistently) and verify the Migration class still depends
on ("tcf_website", "0023_remove_sectionenrollment_section_and_more") and
contains the AddField operation unchanged.
In `@tcf_website/models/models.py`:
- Around line 1583-1605: The current tree-building loses replies whose parent
was excluded by .exclude(text="") because children_by_parent is keyed on
parent_answer_id but root_answers is built from answers_list; update the
queryset (the one providing answers used to build answers_list) to include
parent rows even if their text == "" (or explicitly union in parents of replies)
so every parent referenced by parent_answer_id appears in answers_list; then the
existing attach_children logic (attach_children, children_by_parent,
root_answers) will attach replies correctly—optionally render empty-text parents
as "[deleted]" at display time rather than removing them.
- Around line 1431-1439: The Question model uses ForeignKey fields course,
department, and instructor with on_delete=models.CASCADE which will delete user
Q&A when their related Course/Department/Instructor are removed; change those
ForeignKey definitions in models.py to use on_delete=models.SET_NULL while
keeping null=True, blank=True, default=None so deletions null out the FK instead
of cascading deletes, then regenerate and commit the corresponding migrations
(update tcf_website/migrations/0028_question_department_and_optional_course.py
to reflect SET_NULL and add/ensure an AlterField migration for instructor
consistent with 0025_question_instructor_optional), and run
makemigrations/migrate so the DB schema matches.
In `@tcf_website/static/qa/qa_dashboard.js`:
- Around line 741-792: The current initVoting function uses cloneNode(true) and
replaceChild on elements with class 'vote-btn', which removes any other event
listeners (e.g., tooltips); instead make initVoting idempotent by skipping
already-bound buttons: check/set a data attribute like data-voting-bound on each
.vote-btn and only attach the click handler if not set (avoid
cloneNode/replaceChild), or if you must replace nodes, immediately call
initTooltips() after replacing to re-bind tooltip handlers; update references in
initVoting to remove calls to cloneNode/replaceChild and use the guard
(data-voting-bound) around the addEventListener logic so tooltips and other
listeners are preserved.
- Around line 770-785: The click handler currently assumes
this.closest('.post-actions') returns a container and immediately uses
container.querySelector('[data-action="up"]'), which can throw if the vote
button sits outside a .post-actions wrapper; update the handler (the block that
sets upBtn, counterEl, and updates sidebar when type === 'question') to guard
against a null container by either returning early (if container is null) or by
resolving the up button directly from this (the clicked element) instead of
using container.querySelector; ensure you still update counterId and the sidebar
element (`#sidebar-vote-count-`{id} and its .sidebar-vote-num) only after
confirming the relevant DOM elements exist.
- Around line 672-689: The fetch success path currently treats any HTTP response
as success; update the fetch handlers (e.g., the delete question fetch to
`/questions/${questionIdToDelete}/delete/` that uses CSRF_TOKEN and deleteData
and hides `#deleteQuestionModal` then navigates to QA_URLS.dashboard) to check
response.ok, throwing an error on non-ok responses (e.g., .then(r => { if
(!r.ok) throw new Error(r.status); return r; })). Move navigation/reload/DOM
updates into the true-success branch and in the catch show a user-facing error
(toast, modal error text, or alert) rather than only console.error so failures
in the delete-question, delete-answer, and post-answer flows (also at the other
fetch blocks around lines 710-730 and 828-848) do not falsely proceed.
- Around line 872-913: initQuestionActions() re-attaches the same click handlers
to page-level modal controls (closeEditBtn, cancelEditBtn, editModal) every time
it's called, causing duplicated event firings; to fix, move the one-time wiring
for closeEditBtn, cancelEditBtn and editModal out of initQuestionActions (and
similarly for initAnswerActions) into a DOMContentLoaded-only initialization, or
add a guard so listeners are only added once (e.g., check a data-flag on
closeEditBtn/cancelEditBtn before addEventListener or remove existing listeners
first); keep per-refresh logic limited to elements inside the replaced detail
pane (the edit button handlers) and follow initVoting()’s pattern if you prefer
cloning/replacing modal controls instead.
- Around line 517-524: The course result HTML is built with raw interpolations
of c.id, c.code, c.type, and c.title into courseResults.innerHTML which allows
XSS; update the rendering in the block that builds course-result-item (the code
handling data.results and courseResults) to escape these values instead of
injecting raw HTML: either use the existing escapeHtml helper (used for
instructors earlier) to sanitize c.id/c.code/c.type/c.title before
interpolation, or construct DOM nodes via createElement/setAttribute and set
text nodes via textContent for the code/title and setAttribute for id/type to
ensure values are escaped; ensure both attributes (data-id, data-code,
data-type) and visible text (course-result-code, course-result-title) are
sanitized.
In `@tcf_website/templates/qa/_question_content.html`:
- Line 5: The icon class fa-hand-o-left used on the <i> element is not defined
in the project's icons.css and won't render; update that <i class="fa
fa-hand-o-left fa-3x mb-3"></i> to use a defined icon class (for example replace
fa-hand-o-left with fa-comments, fa-arrow-right, or fa-list) while keeping the
existing fa, fa-3x and mb-3 classes so styling/spacing remain consistent.
In `@tcf_website/templates/qa/qa_dashboard.html`:
- Around line 327-331: The SEMESTERS JS array currently injects semester labels
unsafely via {"id": {{ semester.id }}, "label": "{{ semester }}"}; update the
template to escape semester string output (use the Django escapejs filter:
"label": "{{ semester|escapejs }}") or switch to rendering the whole structure
with json_script/json.dumps to safely serialize the array; ensure you change
occurrences that build SEMESTERS so any quotes, backslashes or newlines in
Semester.__str__ are properly escaped before embedding in JS.
In `@tcf_website/views/qa.py`:
- Around line 418-444: Replace direct model lookups that raise 500 on missing
IDs with Django's get_object_or_404 in all vote endpoints: use
get_object_or_404(Question, pk=question_id) in upvote_question and
downvote_question and the equivalent get_object_or_404(Answer, pk=answer_id) in
upvote_answer/downvote_answer; ensure you import get_object_or_404 from
django.shortcuts if not already imported so stale/forged ids return a 404
instead of raising DoesNotExist.
- Around line 599-604: Remove the duplicated unreachable return statements: in
tcf_website/views/qa.py inside the function containing the multiple sequential
"return JsonResponse({\"ok\": False})" lines (the block around the existing
return at line ~598), collapse them so only a single trailing "return
JsonResponse({\"ok\": False})" remains; delete the extra duplicate returns and
ensure no other logic is accidentally removed so the function returns exactly
once at the end.
- Around line 194-204: create_question currently discards form validation
failures; add an error path so failures surface to the dashboard modal: inside
create_question where you check form.is_valid(), add an else branch that calls
messages.error(request, form.errors) and redirects back to "qa" (or detect AJAX
and return a JsonResponse with {'success': False, 'errors': form.errors} and
appropriate HTTP status). Reference: the create_question view, the form variable
(QuestionForm instance), and use Django's messages.error or JsonResponse to
return errors so the frontend can show validation feedback; also ensure to
import django.contrib.messages or JsonResponse if not already imported.
- Around line 531-547: ReplyForm currently accepts both question and
parent_answer from POST without checking they match; add validation in
ReplyForm.clean() to fetch cleaned_data['parent_answer'] and
cleaned_data['question'] and raise a ValidationError if
parent_answer.question_id != question.id (include a user-friendly error message
attached to parent_answer or non_field_errors), ensuring the form is invalidated
before new_reply view saves the instance.
- Around line 408-409: Replace naive datetime usage when setting model
timestamps: change the assignments to question.created and answer.created to use
django.utils.timezone.now() instead of datetime.datetime.now(). Import timezone
from django.utils.timezone at the top if missing and update both occurrences
(the question.created assignment and the answer.created assignment) so
DateTimeField values are timezone-aware under USE_TZ=True.
---
Nitpick comments:
In `@tcf_website/migrations/0025_question_instructor_optional.py`:
- Around line 14-24: The migration currently changes Question.instructor to
allow nulls but leaves on_delete=CASCADE; update the Question model in
tcf_website/models/models.py to use on_delete=models.SET_NULL (or
django.db.models.deletion.SET_NULL) for the instructor ForeignKey and then add a
follow-up migration that AlterField on the Question.instructor to set on_delete
to SET_NULL (so existing DB schema is migrated); reference the Question model
and the instructor ForeignKey when making these two edits.
In `@tcf_website/static/qa/qa_dashboard.css`:
- Around line 988-998: Combine the duplicate CSS rules for the voted thumbs-up
state by merging the selectors .vote-btn.voted .fa-thumbs-up and
.post-stats.voted .fa-thumbs-up into a single selector list (comma-separated)
and remove the redundant block so the background-image, filter, and transform
declarations are maintained in one place for both cases.
In `@tcf_website/static/qa/qa_dashboard.js`:
- Around line 671-678: The fetch calls in qa_dashboard.js currently hardcode
endpoints like `/questions/${questionIdToDelete}/delete/` (and similar hardcoded
paths used around the vote and answer delete blocks), which will break if URL
patterns change; update the code to use the centralized QA_URLS mapping from the
template instead: add keys deleteQuestion, deleteAnswer, upvoteQuestion,
upvoteAnswer to QA_URLS in qa_dashboard.html (or the template context) and
replace hardcoded string constructions in the deleteQuestion/deleteAnswer and
upvote handlers in qa_dashboard.js to use QA_URLS.deleteQuestion,
QA_URLS.deleteAnswer, QA_URLS.upvoteQuestion, QA_URLS.upvoteAnswer
(interpolating the ID where needed) so all requests reference reversed URLs.
Ensure the headers/body usage remains the same and adjust any string formatting
to match how QA_URLS provides placeholders.
In `@tcf_website/templates/qa/qa_dashboard.html`:
- Around line 316-324: The QA_URLS object currently builds IDs by reversing URLs
with id=0 and then string-replacing substrings like '/0/' or '0/edit/', which is
brittle; instead change the template-reversed URLs to include a clear sentinel
token (e.g. '__ID__') for all parameterized routes (keys: questionDetail,
getInstructors, editQuestion, editAnswer) and update the JS call sites to
replace '__ID__' with the real id when constructing links; keep
non-parameterized entries (createQuestion, searchCourses, dashboard) unchanged
and ensure all occurrences that append/format IDs use the sentinel-replacement
approach so the route shape remains opaque to JS.
In `@tcf_website/tests/test_qa.py`:
- Around line 31-46: Add an assertion that the POST returned the expected
redirect before fetching the created Question so failures surface clearly: in
test_create_question_creates_record, capture the response from
self.client.post(reverse("create_question"), ...) and call
self.assertRedirects(response, reverse("qa"), fetch_redirect_response=False) (or
the appropriate expected redirect) before calling
Question.objects.get(user=self.user1); this ensures form/validation errors show
on the response instead of raising Question.DoesNotExist.
In `@tcf_website/views/qa.py`:
- Around line 119-128: The filter contains a redundant condition: remove the
unnecessary "question__course__isnull=False" from the Course.objects.filter(...)
call that constructs courses_with_questions since "question__isnull=False"
already guarantees a joined Question.course; update the filter to only include
question__isnull=False and keep the select_related/distinct/order_by unchanged
(refer to courses_with_questions and the Course.objects.filter usage).
- Around line 256-276: The code hardcodes recent_courses =
Course.objects.filter(semester_last_taught__year__gte=2022) which will rot;
change it to compute a dynamic cutoff or derive recent semesters instead: either
calculate cutoff_year = timezone.now().year - 3 (importing
django.utils.timezone) and use semester_last_taught__year__gte=cutoff_year, or
query the latest Semester rows (e.g., fetch latest N Semester objects) and
filter Course.objects.filter(semester_last_taught__in=recent_semesters). Update
the reference where recent_courses is used so TrigramSimilarity fallback logic
(the annotate/filter/select_related block) continues to operate on the new
recent_courses queryset.
- Around line 325-347: The form clean() currently silently clears course and
instructor when department is present; either document this with a clear inline
comment in the clean() method (to mark "department wins" as intentional) or
change validation to surface the conflict: in clean() (the form's clean method)
detect when department is set AND (course or instructor) is also set and raise
forms.ValidationError with a message like "Conflicting target: select either a
department or a course/instructor" (if you change behavior, update or remove the
test named test_department_question_ignores_stale_course_and_instructor to
reflect the new validation).
- Around line 100-107: The code silently ignores a mismatched ?question=<id> by
catching Question.DoesNotExist on questions.get(id=selected_question_id) and
falling back to questions.first(); update the handler around
questions.get/question selection (references: questions.get,
selected_question_id, selected_question, questions.first) to surface the
mismatch: when the requested question id is not found in the filtered queryset,
log a warning and return a clear indicator to the client (e.g., raise a
404/HttpResponseNotFound or include a flag in the response/context) so the
frontend can reconcile the URL state instead of silently using
questions.first().
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75548752-c143-4959-9a25-2518e1abe55c
⛔ Files ignored due to path filters (3)
tcf_website/static/icons/img/fa-comments.svgis excluded by!**/*.svgtcf_website/static/icons/img/fa-reply.svgis excluded by!**/*.svgtcf_website/static/icons/img/fa-thumbs-up-solid.svgis excluded by!**/*.svg
📒 Files selected for processing (19)
tcf_website/migrations/0024_forumcategory_forumpost_forumresponse_question_title_and_more.pytcf_website/migrations/0025_question_instructor_optional.pytcf_website/migrations/0026_answer_parent_answer.pytcf_website/migrations/0027_remove_answer_unique_answer_per_user_and_question_and_more.pytcf_website/migrations/0028_question_department_and_optional_course.pytcf_website/models/models.pytcf_website/static/icons/icons.csstcf_website/static/qa/qa_dashboard.csstcf_website/static/qa/qa_dashboard.jstcf_website/templates/base/sidebar.htmltcf_website/templates/qa/_answer_item.htmltcf_website/templates/qa/_question_content.htmltcf_website/templates/qa/_question_detail.htmltcf_website/templates/qa/_question_list.htmltcf_website/templates/qa/qa_dashboard.htmltcf_website/tests/test_qa.pytcf_website/urls.pytcf_website/views/__init__.pytcf_website/views/qa.py
Comment on lines
+1
to
+18
| # Generated by Django 4.2.28 on 2026-02-04 05:13 | ||
|
|
||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ("tcf_website", "0023_remove_sectionenrollment_section_and_more"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name="question", | ||
| name="title", | ||
| field=models.CharField(blank=True, max_length=200), | ||
| ), | ||
| ] |
Contributor
There was a problem hiding this comment.
Migration filename doesn't match its operations.
The filename 0024_forumcategory_forumpost_forumresponse_question_title_and_more.py implies new ForumCategory, ForumPost, and ForumResponse models plus a Question.title field, but the file only contains a single AddField for Question.title. This suggests either leftover scaffolding from an abandoned approach or a partially-squashed migration. Renaming to something like 0024_question_title.py would prevent future confusion (rename only if the migration hasn't been applied to any deployed environment yet — otherwise leave it).
🧰 Tools
🪛 Ruff (0.15.11)
[warning] 8-10: Mutable default value for class attribute
(RUF012)
[warning] 12-18: Mutable default value for class attribute
(RUF012)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tcf_website/migrations/0024_forumcategory_forumpost_forumresponse_question_title_and_more.py`
around lines 1 - 18, The migration file name implies multiple model changes but
the Migration only adds a Question.title CharField via the AddField operation
(model_name="question", name="title"); if this migration has not been applied
anywhere, rename the file to reflect the single change (e.g.,
0024_question_title.py) to avoid confusion; if the migration has already been
applied in any environment, do not rename—leave the filename and instead add a
new, clearly named migration for any further model additions (or revert and
re-create migrations consistently) and verify the Migration class still depends
on ("tcf_website", "0023_remove_sectionenrollment_section_and_more") and
contains the AddField operation unchanged.
Contributor
There was a problem hiding this comment.
@brandonistfan, thanks for the endorsement! To summarize the recommendation for the PR author:
The file tcf_website/migrations/0024_forumcategory_forumpost_forumresponse_question_title_and_more.py only contains a single AddField operation for Question.title, but its name implies the creation of ForumCategory, ForumPost, and ForumResponse models — none of which exist in this file. This is likely scaffolding left over from an earlier, broader plan.
Recommended action:
- If this migration has not yet been applied to any shared/deployed environment (staging, production, teammates' local DBs): rename the file to something like
0024_question_title.pyand update thedependenciesreference in0025_question_instructor_optional.pyto match the new name. - If it has already been applied: leave the filename as-is (renaming would break Django's migration history) and simply move on — the misleading name is cosmetic at that point.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Cannot run autofix: This PR has merge conflicts. Please resolve the conflicts with the base branch and try again. Alternatively, use |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tcf_website/templates/qa/_answer_item.html (1)
56-82: Reply form included for every answer — inflates DOM size and duplicatessemestersloop.Because this partial is recursively included per answer, each answer (including deeply nested replies) embeds its own full hidden
<form>with the complete semester<option>list. On a question with N answers and S semesters, you're rendering N×S<option>elements up front, most of which are never used. Consider rendering a single shared reply form that is re-parented/populated on demand (e.g., when the user clicks Reply), or lazy-loading the form via AJAX. Non-blocking, but worth noting given the dashboard p95 latency bottleneck mentioned in the PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/templates/qa/_answer_item.html` around lines 56 - 82, The per-answer reply form (div id="reply-form-{{ answer.id }}" with class "reply-form-container" containing the form named "reply-form", the semester <select> with class "semester-select" and options loop over "semesters") is being rendered for every answer and should be collapsed into a single shared form to avoid N×S duplicate <option> elements; move the entire form markup out of the recursive partial into a single shared container (e.g., a top-level hidden div) and update client-side logic to reparent/populate that shared "reply-form" when a user clicks a reply button (set the hidden inputs "question" and "parent_answer" and show the container), or alternatively lazy-load the form via AJAX when first used—ensure the form still posts to the "new_reply" URL and that the semester-select is populated once from the single "semesters" context rather than inside each answer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tcf_website/templates/qa/_answer_item.html`:
- Line 58: The reply form in templates/qa/_answer_item.html currently performs a
full-page POST to the view new_reply (tcf_website/views/qa.py) which redirects
to HTTP_REFERER and breaks the AJAX dashboard flow; change the UX to submit via
fetch/AJAX instead: add a JS handler bound to the .reply-form that prevents
default submission, gathers form data and CSRF token, calls the new_reply
endpoint, and on success updates/re-renders the answers fragment in-place (or
appends the new reply HTML returned). Update new_reply to detect
X-Requested-With or Accept: application/json and return JSON (or a rendered
partial HTML snippet) with success/error and message text instead of always
redirecting to request.META.get("HTTP_REFERER", "/"), ensuring errors are
surfaced to the SPA and no full-page navigation occurs.
- Around line 28-34: The outer <p> around the answer block causes nested
paragraphs because the template uses the Django filter "linebreaks" on
"answer.text" (and "linebreaks" wraps content in <p>), so either replace the
filter with "linebreaksbr" (use answer.text|linebreaksbr) to keep the outer <p>
and avoid inner <p> tags, or remove the outer <p> and render the deleted state
and the "answer.text|linebreaks" variant separately (e.g., if
answer.is_deleted_placeholder render an <em> block, else render
answer.text|linebreaks) so no nested <p> elements occur.
---
Nitpick comments:
In `@tcf_website/templates/qa/_answer_item.html`:
- Around line 56-82: The per-answer reply form (div id="reply-form-{{ answer.id
}}" with class "reply-form-container" containing the form named "reply-form",
the semester <select> with class "semester-select" and options loop over
"semesters") is being rendered for every answer and should be collapsed into a
single shared form to avoid N×S duplicate <option> elements; move the entire
form markup out of the recursive partial into a single shared container (e.g., a
top-level hidden div) and update client-side logic to reparent/populate that
shared "reply-form" when a user clicks a reply button (set the hidden inputs
"question" and "parent_answer" and show the container), or alternatively
lazy-load the form via AJAX when first used—ensure the form still posts to the
"new_reply" URL and that the semester-select is populated once from the single
"semesters" context rather than inside each answer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05d9f926-5aca-4587-8467-9ac5284a54b7
📒 Files selected for processing (4)
tcf_website/migrations/0025_question_instructor_optional.pytcf_website/migrations/0028_question_department_and_optional_course.pytcf_website/models/models.pytcf_website/templates/qa/_answer_item.html
🚧 Files skipped from review as they are similar to previous changes (1)
- tcf_website/models/models.py
|
|
||
| {% if user.is_authenticated %} | ||
| <div class="reply-form-container thread-depth-{{ answer.render_depth }}" id="reply-form-{{ answer.id }}" style="display: none;"> | ||
| <form class="reply-form" method="POST" action="{% url 'new_reply' %}"> |
Contributor
There was a problem hiding this comment.
Reply submission uses a full-page POST + HTTP_REFERER redirect despite the AJAX dashboard.
The dashboard loads question detail via AJAX, but new_reply (see tcf_website/views/qa.py:532-546) redirects to request.META.get("HTTP_REFERER", "/") on success/failure. If HTTP_REFERER is missing or points to a non-dashboard URL (e.g., after deep-linking), the user is sent to /, losing their context, and success/error messages won't surface in the single-page dashboard flow.
Consider submitting the reply via fetch/AJAX and re-rendering the answers section in place, consistent with the rest of the dashboard interactions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tcf_website/templates/qa/_answer_item.html` at line 58, The reply form in
templates/qa/_answer_item.html currently performs a full-page POST to the view
new_reply (tcf_website/views/qa.py) which redirects to HTTP_REFERER and breaks
the AJAX dashboard flow; change the UX to submit via fetch/AJAX instead: add a
JS handler bound to the .reply-form that prevents default submission, gathers
form data and CSRF token, calls the new_reply endpoint, and on success
updates/re-renders the answers fragment in-place (or appends the new reply HTML
returned). Update new_reply to detect X-Requested-With or Accept:
application/json and return JSON (or a rendered partial HTML snippet) with
success/error and message text instead of always redirecting to
request.META.get("HTTP_REFERER", "/"), ensuring errors are surfaced to the SPA
and no full-page navigation occurs.
Comment on lines
+1
to
+18
| # Generated by Django 4.2.28 on 2026-02-04 05:13 | ||
|
|
||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ("tcf_website", "0023_remove_sectionenrollment_section_and_more"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name="question", | ||
| name="title", | ||
| field=models.CharField(blank=True, max_length=200), | ||
| ), | ||
| ] |
| const submitBtn = newForm.querySelector('.btn-submit-reply'); | ||
|
|
||
| // Check for duplicates first | ||
| fetch('/answers/check_duplicate/', { |
Contributor
There was a problem hiding this comment.
Can we use a QA_URL rather than hardcode it?
|
|
||
| let questionIdToDelete = null; | ||
|
|
||
| function initDeleteQuestionModal() { |
Contributor
There was a problem hiding this comment.
When deleting a question, does all related data get deleted?
| function initDeleteAnswerModal() { | ||
| const confirmDeleteBtn = document.getElementById('confirmDeleteAnswer'); | ||
|
|
||
| if (confirmDeleteBtn) { |
Contributor
There was a problem hiding this comment.
What happens for replies to the answers?
| """Downvote a question.""" | ||
| if request.method == "POST": | ||
| question = get_object_or_404(Question, pk=question_id) | ||
| question.downvote(request.user) |
Contributor
There was a problem hiding this comment.
Didn't downvote get removed in models?
| """Downvote an answer.""" | ||
| if request.method == "POST": | ||
| answer = get_object_or_404(Answer, pk=answer_id) | ||
| answer.downvote(request.user) |
Contributor
There was a problem hiding this comment.
Didn't downvote get removed in models?
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tcf_website/static/qa/qa_dashboard.js (2)
768-768: Hardcoded/login/and/answers/check_duplicate/paths defeat theQA_URLSindirection.
window.location.href = '/login/'(here) andfetch('/answers/check_duplicate/', …)at Line 840 bypassQA_URLS(and the Django URL reverser used to populate it intcf_website/templates/qa/qa_dashboard.htmlLines 316-328). If the route prefix orLOGIN_URLever changes, the auth-required vote flow and the answer-duplicate check will silently break in this view. Consider addingloginandcheckDuplicateAnswer(or similar) toQA_URLS.♻️ Sketch
QA_URLS = { questionDetail: '...', ... dashboard: '{% url "qa" %}', + login: '{% url "login" %}', + checkDuplicateAnswer: '/answers/check_duplicate/', // or wire a name in urls.py };- window.location.href = '/login/'; + window.location.href = QA_URLS.login;- fetch('/answers/check_duplicate/', { + fetch(QA_URLS.checkDuplicateAnswer, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/static/qa/qa_dashboard.js` at line 768, The code uses hardcoded paths (window.location.href = '/login/' and the fetch call to '/answers/check_duplicate/') which bypass the QA_URLS indirection; add new keys (e.g., login and checkDuplicateAnswer) to the QA_URLS object populated by the Django template and replace the hardcoded usages: change the window.location.href assignment to use QA_URLS.login and change the fetch URL in the duplicate-checking logic to QA_URLS.checkDuplicateAnswer (or your chosen key names) so the view respects the template-provided URL reverser and will follow any route prefix or LOGIN_URL changes.
25-27:window.alertfor transient request failures is jarring.Every fetch error path (vote failures, duplicate-answer checks, delete confirmations, post-answer) routes through
showRequestError→alert(...), which blocks the page and yields a generic dialog with no stylistic affordance. Since the rest of the QA dashboard already uses Bootstrap modals/messages, a non-blocking toast or an inline error banner would be a much better fit. Not blocking, but worth a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/static/qa/qa_dashboard.js` around lines 25 - 27, Replace the blocking window.alert call in showRequestError with a non-blocking Bootstrap-style notification: update the showRequestError(message) implementation to render and show a toast (or reveal an existing hidden inline banner) using Bootstrap's Toast API or by toggling a styled error banner element, set the message text into that element, ensure it is announced for accessibility (aria-live) and auto-dismisses after a short timeout while remaining dismissible by the user; reference showRequestError to locate where to update and ensure all callers (vote, duplicate-answer check, delete, post-answer) continue to pass the message string unchanged.tcf_website/views/qa.py (2)
408-429:edit_questionGET branch is dead code; consider removing.Lines 428-429 build a
QuestionForm(instance=question)and immediately discard it, returning a redirect to the referer regardless. There's no template render, so a GET to/questions/<id>/edit/round-trips back to the previous page (or"/"if the referer is missing) without ever usingform. Either drop the form construction (the function would simplify to a POST-only view, since the dashboard handles edit via modal anyway) or, if you intend to keep a GET fallback, render an actual template.♻️ Suggested cleanup
messages.error(request, form.errors) return HttpResponseRedirect(request.META.get("HTTP_REFERER", "/")) - form = QuestionForm(instance=question) return HttpResponseRedirect(request.META.get("HTTP_REFERER", "/"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/views/qa.py` around lines 408 - 429, The GET branch in edit_question is dead: remove the unused QuestionForm(instance=question) and the unconditional redirect on GET, and make the view explicitly POST-only (either decorate edit_question with `@require_POST` or return HttpResponseNotAllowed for non-POST requests); keep the existing POST handling (QuestionForm(request.POST, instance=question), form.is_valid(), messages, question.save()) and import any needed symbols (`@require_POST` or HttpResponseNotAllowed) accordingly.
35-203:qa_dashboarddoes heavy work on every page load – consider deferring/caching the filter metadata.Each request to the dashboard (including AJAX refreshes for search keystrokes, every department/course filter click, and
pushStatenavigation) re-executes:
courses_with_questions(Lines 129-138): aCourse.objects.filter(question__isnull=False).distinct().order_by(...)join across courses, subdepartments, and questions.departments_list(Lines 141-148): anORbetweenquestion__isnull=Falseandsubdepartment__course__question__isnull=False, which forces a multi-join andDISTINCT.- The auto-selection fallback at Line 117 (
questions.first()), which then triggersAnswer.display_activity+ ananswer_countcount query (Lines 120-126) even when the user only changed a search filter and didn't ask for a specific question.This is consistent with the p95 bottleneck called out in the PR description. Two cheap wins that don't require a new architecture:
- Skip the auto-
first()selection for AJAX refreshes when noquestion=was supplied – the JS already drives selection clicks, so the server-rendered detail is wasted bandwidth on every keystroke.- Cache
courses_with_questionsanddepartments_listincache.get_or_set(...)with a short TTL (e.g., 60s) keyed on nothing — they only change when a new question is posted.Happy to send a follow-up patch if useful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/views/qa.py` around lines 35 - 203, The qa_dashboard view is doing expensive DB work on every request; modify qa_dashboard to 1) avoid automatically calling questions.first() and fetching answers/answer_count (and thus Answer.display_activity and Answer.objects.filter(...).count()) when the request is an AJAX refresh and no explicit selected_question_id was provided (check request.headers.get("X-Requested-With") == "XMLHttpRequest" and selected_question_id is None) so the detail pane isn't rebuilt on every keystroke, and 2) wrap the courses_with_questions and departments_list queries in cache.get_or_set(...) with a short TTL (e.g., 60 seconds) so Course.objects.filter(question__isnull=False)... and Department.objects.filter(...) are cached; keep the cache keys global/simple since they only change when questions change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tcf_website/views/qa.py`:
- Around line 447-459: Remove the broken downvote views: delete the
downvote_question and downvote_answer view functions (they call nonexistent
Question.downvote()/Answer.downvote()) and remove their URL patterns so no route
points to them; if you need to support removing a vote, wire clients to existing
upvote/unvote logic or implement proper model methods (e.g., add downvote() to
Question/Answer or use VoteQuestion/VoteAnswer handlers) and update any
references to vote handling to use those existing methods/handlers instead of
calling downvote_question/downvote_answer.
- Line 423: The view is overwriting auto_now_add timestamps by setting
question.created = timezone.now() (in edit_question) and answer.created =
timezone.now() (in edit_answer); remove those assignments so the created field
is not mutated, and if you need "last-edited" semantics add an updated
DateTimeField (auto_now=True) to the Question/Answer models and update that (or
order/display by it) instead of touching question.created/answer.created.
---
Nitpick comments:
In `@tcf_website/static/qa/qa_dashboard.js`:
- Line 768: The code uses hardcoded paths (window.location.href = '/login/' and
the fetch call to '/answers/check_duplicate/') which bypass the QA_URLS
indirection; add new keys (e.g., login and checkDuplicateAnswer) to the QA_URLS
object populated by the Django template and replace the hardcoded usages: change
the window.location.href assignment to use QA_URLS.login and change the fetch
URL in the duplicate-checking logic to QA_URLS.checkDuplicateAnswer (or your
chosen key names) so the view respects the template-provided URL reverser and
will follow any route prefix or LOGIN_URL changes.
- Around line 25-27: Replace the blocking window.alert call in showRequestError
with a non-blocking Bootstrap-style notification: update the
showRequestError(message) implementation to render and show a toast (or reveal
an existing hidden inline banner) using Bootstrap's Toast API or by toggling a
styled error banner element, set the message text into that element, ensure it
is announced for accessibility (aria-live) and auto-dismisses after a short
timeout while remaining dismissible by the user; reference showRequestError to
locate where to update and ensure all callers (vote, duplicate-answer check,
delete, post-answer) continue to pass the message string unchanged.
In `@tcf_website/views/qa.py`:
- Around line 408-429: The GET branch in edit_question is dead: remove the
unused QuestionForm(instance=question) and the unconditional redirect on GET,
and make the view explicitly POST-only (either decorate edit_question with
`@require_POST` or return HttpResponseNotAllowed for non-POST requests); keep the
existing POST handling (QuestionForm(request.POST, instance=question),
form.is_valid(), messages, question.save()) and import any needed symbols
(`@require_POST` or HttpResponseNotAllowed) accordingly.
- Around line 35-203: The qa_dashboard view is doing expensive DB work on every
request; modify qa_dashboard to 1) avoid automatically calling questions.first()
and fetching answers/answer_count (and thus Answer.display_activity and
Answer.objects.filter(...).count()) when the request is an AJAX refresh and no
explicit selected_question_id was provided (check
request.headers.get("X-Requested-With") == "XMLHttpRequest" and
selected_question_id is None) so the detail pane isn't rebuilt on every
keystroke, and 2) wrap the courses_with_questions and departments_list queries
in cache.get_or_set(...) with a short TTL (e.g., 60 seconds) so
Course.objects.filter(question__isnull=False)... and
Department.objects.filter(...) are cached; keep the cache keys global/simple
since they only change when questions change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1fa6870b-fcce-4553-96bb-4f634b311112
📒 Files selected for processing (9)
tcf_website/migrations/0024_question_title.pytcf_website/migrations/0025_question_instructor_optional.pytcf_website/static/qa/qa_dashboard.csstcf_website/static/qa/qa_dashboard.jstcf_website/templates/qa/_question_content.htmltcf_website/templates/qa/qa_dashboard.htmltcf_website/tests/test_qa.pytcf_website/urls.pytcf_website/views/qa.py
✅ Files skipped from review due to trivial changes (1)
- tcf_website/static/qa/qa_dashboard.css
| f"Successfully updated your question for {_question_target_label(form.instance)}!", | ||
| ) | ||
| question.created = datetime.datetime.now() | ||
| question.created = timezone.now() |
Contributor
There was a problem hiding this comment.
Edit silently overwrites the original created timestamp.
question.created = timezone.now() (and answer.created = timezone.now() at Line 555) mutates a field declared with auto_now_add=True on every successful edit. This has two user-visible effects:
- The dashboard ordering uses
order_by("-created")(Line 100), so any edit – even a typo fix – jumps the question to the top of the list, masking the true post date and pushing genuinely newer posts down. - The
post-dateshown in the sidebar ({{ question.created|date:"n/j/y" }}) and the "X minutes ago" label in the detail pane will both lie about when the post was made.
If the goal is to surface "last-edited" information, please add a separate updated field (e.g., auto_now=True) and order/display from that, and leave created alone.
🛡️ Suggested fix (remove the mutation)
form.save()
messages.success(
request,
f"Successfully updated your question for {_question_target_label(form.instance)}!",
)
- question.created = timezone.now()
- question.save()
return HttpResponseRedirect(request.META.get("HTTP_REFERER", "/"))Apply the analogous deletion at Lines 555-556 in edit_answer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tcf_website/views/qa.py` at line 423, The view is overwriting auto_now_add
timestamps by setting question.created = timezone.now() (in edit_question) and
answer.created = timezone.now() (in edit_answer); remove those assignments so
the created field is not mutated, and if you need "last-edited" semantics add an
updated DateTimeField (auto_now=True) to the Question/Answer models and update
that (or order/display by it) instead of touching
question.created/answer.created.
Comment on lines
447
to
459
| @login_required | ||
| def downvote_question(request, question_id): | ||
| """Downvote a view.""" | ||
| """Downvote a question.""" | ||
| if request.method == "POST": | ||
| question = Question.objects.get(pk=question_id) | ||
| question = get_object_or_404(Question, pk=question_id) | ||
| question.downvote(request.user) | ||
| return JsonResponse({"ok": True}) | ||
| net = ( | ||
| question.votequestion_set.aggregate(total=models.Sum("value"))["total"] or 0 | ||
| ) | ||
| vote_obj = question.votequestion_set.filter(user=request.user).first() | ||
| user_vote = vote_obj.value if vote_obj else 0 | ||
| return JsonResponse({"ok": True, "votes": net, "user_vote": user_vote}) | ||
| return JsonResponse({"ok": False}) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for upvote/downvote methods on Question and Answer models.
ast-grep --pattern $'class Question(models.Model):
$$$
def $METHOD($$$):
$$$
}'
echo "---- direct method-name search ----"
rg -nP '\bdef\s+(up|down)vote\s*\(' --type=py
echo "---- references to .downvote(...) ----"
rg -nP '\.downvote\s*\(' --type=pyRepository: thecourseforum/theCourseForum2
Length of output: 967
🏁 Script executed:
# Get context around the downvote definition and upvote definitions
echo "=== Around line 1246 (downvote) ==="
sed -n '1200,1260p' tcf_website/models/models.py | head -70
echo -e "\n=== Around line 1457 (upvote) ==="
sed -n '1440,1480p' tcf_website/models/models.py
echo -e "\n=== Around line 1534 (upvote) ==="
sed -n '1510,1560p' tcf_website/models/models.py
echo -e "\n=== Checking for class definitions near these lines ==="
rg -n 'class (Question|Answer|Review)' tcf_website/models/models.pyRepository: thecourseforum/theCourseForum2
Length of output: 4685
🏁 Script executed:
# Find where Question class ends (look for next class definition after line 1423)
rg -n '^class ' tcf_website/models/models.py | grep -A1 'Question'
# Get Question class definition (from line 1423 to before next class)
echo "=== Question class ==="
sed -n '1423,1507p' tcf_website/models/models.py | tail -30
# Get Answer class definition
echo -e "\n=== Answer class (looking for downvote) ==="
sed -n '1508,1650p' tcf_website/models/models.pyRepository: thecourseforum/theCourseForum2
Length of output: 5838
Remove downvote_question and downvote_answer views—the corresponding model methods do not exist.
The Question and Answer models have upvote() methods but lack downvote() methods (only Review has downvote()). Calls to question.downvote() at line 452 and answer.downvote() at line 626 will raise AttributeError on every POST request. Remove both views and the corresponding URL routes at tcf_website/urls.py (lines 126, 142).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tcf_website/views/qa.py` around lines 447 - 459, Remove the broken downvote
views: delete the downvote_question and downvote_answer view functions (they
call nonexistent Question.downvote()/Answer.downvote()) and remove their URL
patterns so no route points to them; if you need to support removing a vote,
wire clients to existing upvote/unvote logic or implement proper model methods
(e.g., add downvote() to Question/Answer or use VoteQuestion/VoteAnswer
handlers) and update any references to vote handling to use those existing
methods/handlers instead of calling downvote_question/downvote_answer.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
tcf_website/templates/qa/_answer_item.html (2)
30-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
linebreaksinside<p>still creates invalid nested paragraphs.This issue was raised in a previous review. The
linebreaksfilter emits<p>tags, nesting them inside the outer<p>at line 30, which produces invalid HTML.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/templates/qa/_answer_item.html` around lines 30 - 36, The template nests <p> tags because {{ answer.text|linebreaks }} emits <p> elements; change the rendering to avoid nested paragraphs by replacing the linebreaks filter with a non-paragraph-producing one (e.g., use {{ answer.text|linebreaksbr }} or explicitly convert newlines to <br>), keeping the surrounding <p> and preserving HTML-escaping as needed; update the code that references answer.is_deleted_placeholder / answer.text accordingly so deleted placeholders still render <em>[deleted]</em>.
58-84:⚠️ Potential issue | 🟠 MajorReply form still uses full-page POST +
HTTP_REFERERredirect on an AJAX dashboard.This issue was raised in a previous review. The reply form at line 60 POSTs to
{% url 'new_reply' %}, which redirects torequest.META.get("HTTP_REFERER", "/")on success/failure. In the AJAX dashboard context, if the referrer is absent or stale (e.g., deep-link, browser back), the user is silently sent to/, losing question context. The error path is also invisible to the single-page flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/templates/qa/_answer_item.html` around lines 58 - 84, The reply form currently does a full-page POST to the view at {% url 'new_reply' %} and relies on HTTP_REFERER for redirection, which breaks the AJAX single-page dashboard; change the template to submit the form via AJAX by intercepting the form with the "reply-form" class (and using the container id pattern "reply-form-{{ answer.id }}") and use fetch/XHR to POST to the same action endpoint, sending form data (including "question" and "parent_answer") and handling JSON success/error responses to update the UI in-place; also hook the "btn-cancel-reply" and "btn-submit-reply" buttons to close/reset the inline reply UI on cancel or show form validation/errors on failure instead of relying on server-side HTTP_REFERER redirects.tcf_website/views/qa.py (4)
394-398:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
edit_questioncorrupts thecreatedtimestamp on every edit.After
form.save()already persists the title/text changes, lines 396–397 overwritequestion.createdwith the current time and callquestion.save()a second time. This has two concrete effects:
- Every edited question floats to the top of the dashboard's
-createdordering, masking the true post date and pushing newer posts down.- The "X ago" timestamp shown in the detail pane reflects the edit time, not the creation time.
If surfacing "last edited" information is desired, add a separate
updated = models.DateTimeField(auto_now=True)field and display that instead.🛡️ Proposed fix
if form.is_valid(): form.save() - question.created = timezone.now() - question.save() return HttpResponseRedirect(request.META.get("HTTP_REFERER", "/"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/views/qa.py` around lines 394 - 398, The edit_question view is incorrectly resetting question.created after form.save(); remove the lines that set question.created = timezone.now() and the subsequent question.save() so edits do not alter the original creation timestamp, and if you need to show edit time add an updated = models.DateTimeField(auto_now=True) to the Question model and use that field (or set it via the form) instead of mutating question.created in edit_question.
579-589:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
downvote_answercalls non-existentAnswer.downvote()→AttributeErrorat runtime.Identical issue to
downvote_question:Answerhas onlyupvote(), nodownvote(). Line 584 will always raiseAttributeError. Remove this view and its URL route.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/views/qa.py` around lines 579 - 589, The downvote_answer view calls a non-existent Answer.downvote() (in function downvote_answer) which will raise AttributeError at runtime; remove the entire downvote_answer view and also delete its corresponding URL route (the route that maps to downvote_answer) so no code path invokes Answer.downvote(), and update any templates or JavaScript that post to that route to avoid broken links.
420-432:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
downvote_questioncalls non-existentQuestion.downvote()→AttributeErrorat runtime.
Questionhas only anupvote()method (line 1457 ofmodels.py); there is nodownvote(). Every POST to this endpoint raisesAttributeError: 'Question' object has no attribute 'downvote'. The UI also never calls this endpoint (QA_URLSinqa_dashboard.htmlhas nodownvoteQuestionentry and there is no downvote button rendered). This view is both broken and dead code and should be removed along with its URL route.#!/bin/bash # Confirm Question and Answer lack a downvote() method echo "=== Question.downvote ===" ast-grep --pattern $'class Question(models.Model): $$$ def downvote($$$) { $$$ } $$$' echo "=== Answer.downvote ===" ast-grep --pattern $'class Answer(models.Model): $$$ def downvote($$$) { $$$ } $$$' # Also check via plain text search echo "=== rg for downvote definitions in models ===" rg -n "def downvote" tcf_website/models/models.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/views/qa.py` around lines 420 - 432, The downvote_question view calls a non-existent Question.downvote() and is dead code (no template or QA_URLS reference); remove the broken view function downvote_question from tcf_website/views/qa.py and delete its corresponding URL route from the URLconf so the attribute error cannot occur, then run tests and search for any remaining references to downvoteQuestion/Downvote endpoints (e.g., QA_URLS in qa_dashboard.html and any JS that might call a downvote route) to ensure nothing else expects that endpoint.
512-516:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
edit_answercorrupts thecreatedtimestamp on every edit.Same issue as
edit_question: lines 514–515 overwriteanswer.createdwith the current time afterform.save(), producing a misleading "X ago" timestamp on every edited answer.🛡️ Proposed fix
if form.is_valid(): form.save() - answer.created = timezone.now() - answer.save() return HttpResponseRedirect(request.META.get("HTTP_REFERER", "/"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/views/qa.py` around lines 512 - 516, In edit_answer, do not overwrite the original creation time after form.save(): remove or stop the assignment answer.created = timezone.now() (and the subsequent answer.save()) so edits do not reset created; if you need to record edit time instead, set a dedicated timestamp (e.g., answer.updated or answer.modified = timezone.now()) before saving, or only set created when the object is new (guard with if not answer.created) to preserve the original created value.
🧹 Nitpick comments (1)
tcf_website/tests/test_qa.py (1)
1-10: ⚡ Quick winAdd test coverage for downvote endpoints, edit/delete flows, and ownership enforcement.
The suite is thorough for the new dashboard and detail paths. The following critical/ownership paths are not yet covered:
downvote_question/downvote_answer(adding a test here would immediately surface theAttributeErrordescribed in theqa.pyreview below)edit_question/edit_answer— including the timestamp mutation bugdelete_question/delete_answer— including the cascade-delete of replies- Attempting to edit or delete another user's question/answer (permission check)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/tests/test_qa.py` around lines 1 - 10, Add unit tests that cover the missing Q&A paths: create tests that call the downvote endpoints (downvote_question, downvote_answer) to reproduce the AttributeError and assert correct vote toggling and response codes; add tests for edit_question and edit_answer that verify edits update content but do not mutate created_at timestamps (catch the timestamp mutation bug), plus negative tests that editing someone else's question/answer is forbidden; add tests for delete_question and delete_answer ensuring cascade-delete of replies and that deletion by non-owners is denied; use existing helpers (setup, suppress_request_warnings) to create users/questions/answers and assert permissions, status codes, and database state changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tcf_website/migrations/0029_alter_answer_semester.py`:
- Around line 12-22: The migration AlterField for model "answer" and the
corresponding ForeignKey in models.py use on_delete=CASCADE which will delete
Answer rows when a Semester is removed; change both the migration
(0029_alter_answer_semester.py) and the Answer model's semester field to use
on_delete=models.SET_NULL (and keep null=True, blank=True, default=None) so
deleting a Semester sets semester to NULL instead of cascading deletions; also
ensure the migration imports models (or django.db.models.deletion) and
references models.SET_NULL consistently and update the models.py import if
necessary.
In `@tcf_website/models/models.py`:
- Around line 1522-1524: The model's parent_answer FK currently CASCADEs and the
DeleteAnswer view is performing a hard delete which removes entire reply
threads; instead, modify the DeleteAnswer view (the view/class/function named
DeleteAnswer) to perform a soft-delete by setting the Answer instance's text =
"" (and any existing is_deleted flag if present) and saving the instance rather
than calling .delete(), so display_activity's is_deleted_placeholder and
attach_children logic preserve child replies; alternatively, if you prefer
orphan promotion, change the Answer.parent_answer field's on_delete to SET_NULL
and ensure display_activity treats null parent_answer as root answers.
- Around line 1519-1521: Change the Answer.semester ForeignKey to use
on_delete=models.SET_NULL instead of CASCADE and update migration 0029 to
reflect the same behavior; specifically, in models.py modify the Answer.semester
field definition (symbol: Answer.semester) to on_delete=models.SET_NULL with
null=True, blank=True, default=None, and edit migration 0029 to use
migrations.AlterField (or the corresponding AddField/AlterField entry) that sets
on_delete=django.db.models.deletion.SET_NULL so the database migration records
the nulling behavior rather than cascade deletion.
In `@tcf_website/templates/qa/_question_detail.html`:
- Line 62: Remove the outer paragraph wrapper around the linebreaks filter to
avoid nested <p> tags: locate the template line that renders question.text with
the linebreaks filter (the expression question.text|linebreaks) and either
output that filter directly (no surrounding <p>) or switch to a single-line-safe
filter such as linebreaksbr if you must keep a surrounding <p>; update the
template where question.text|linebreaks is used (same pattern as fixed in
_answer_item.html) so the rendered HTML does not produce nested <p> elements.
In `@tcf_website/templates/qa/qa_dashboard.html`:
- Line 65: The hrefs that append the query string currently interpolate
search_query raw, which can produce invalid URLs for characters like #, %, or +;
update each occurrence that builds a query string (the template anchor that uses
"{% url 'qa' %}{% if search_query %}?q={{ search_query }}{% endif %}" and the
two other similar occurrences for the course filter and the other QA link) to
percent-encode the value by replacing {{ search_query }} with {{
search_query|urlencode }} so the links safely encode special characters.
---
Duplicate comments:
In `@tcf_website/templates/qa/_answer_item.html`:
- Around line 30-36: The template nests <p> tags because {{
answer.text|linebreaks }} emits <p> elements; change the rendering to avoid
nested paragraphs by replacing the linebreaks filter with a
non-paragraph-producing one (e.g., use {{ answer.text|linebreaksbr }} or
explicitly convert newlines to <br>), keeping the surrounding <p> and preserving
HTML-escaping as needed; update the code that references
answer.is_deleted_placeholder / answer.text accordingly so deleted placeholders
still render <em>[deleted]</em>.
- Around line 58-84: The reply form currently does a full-page POST to the view
at {% url 'new_reply' %} and relies on HTTP_REFERER for redirection, which
breaks the AJAX single-page dashboard; change the template to submit the form
via AJAX by intercepting the form with the "reply-form" class (and using the
container id pattern "reply-form-{{ answer.id }}") and use fetch/XHR to POST to
the same action endpoint, sending form data (including "question" and
"parent_answer") and handling JSON success/error responses to update the UI
in-place; also hook the "btn-cancel-reply" and "btn-submit-reply" buttons to
close/reset the inline reply UI on cancel or show form validation/errors on
failure instead of relying on server-side HTTP_REFERER redirects.
In `@tcf_website/views/qa.py`:
- Around line 394-398: The edit_question view is incorrectly resetting
question.created after form.save(); remove the lines that set question.created =
timezone.now() and the subsequent question.save() so edits do not alter the
original creation timestamp, and if you need to show edit time add an updated =
models.DateTimeField(auto_now=True) to the Question model and use that field (or
set it via the form) instead of mutating question.created in edit_question.
- Around line 579-589: The downvote_answer view calls a non-existent
Answer.downvote() (in function downvote_answer) which will raise AttributeError
at runtime; remove the entire downvote_answer view and also delete its
corresponding URL route (the route that maps to downvote_answer) so no code path
invokes Answer.downvote(), and update any templates or JavaScript that post to
that route to avoid broken links.
- Around line 420-432: The downvote_question view calls a non-existent
Question.downvote() and is dead code (no template or QA_URLS reference); remove
the broken view function downvote_question from tcf_website/views/qa.py and
delete its corresponding URL route from the URLconf so the attribute error
cannot occur, then run tests and search for any remaining references to
downvoteQuestion/Downvote endpoints (e.g., QA_URLS in qa_dashboard.html and any
JS that might call a downvote route) to ensure nothing else expects that
endpoint.
- Around line 512-516: In edit_answer, do not overwrite the original creation
time after form.save(): remove or stop the assignment answer.created =
timezone.now() (and the subsequent answer.save()) so edits do not reset created;
if you need to record edit time instead, set a dedicated timestamp (e.g.,
answer.updated or answer.modified = timezone.now()) before saving, or only set
created when the object is new (guard with if not answer.created) to preserve
the original created value.
---
Nitpick comments:
In `@tcf_website/tests/test_qa.py`:
- Around line 1-10: Add unit tests that cover the missing Q&A paths: create
tests that call the downvote endpoints (downvote_question, downvote_answer) to
reproduce the AttributeError and assert correct vote toggling and response
codes; add tests for edit_question and edit_answer that verify edits update
content but do not mutate created_at timestamps (catch the timestamp mutation
bug), plus negative tests that editing someone else's question/answer is
forbidden; add tests for delete_question and delete_answer ensuring
cascade-delete of replies and that deletion by non-owners is denied; use
existing helpers (setup, suppress_request_warnings) to create
users/questions/answers and assert permissions, status codes, and database state
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e95b72e5-1379-40e9-abce-691ed3f11e03
📒 Files selected for processing (8)
tcf_website/migrations/0029_alter_answer_semester.pytcf_website/models/models.pytcf_website/static/qa/qa_dashboard.jstcf_website/templates/qa/_answer_item.htmltcf_website/templates/qa/_question_detail.htmltcf_website/templates/qa/qa_dashboard.htmltcf_website/tests/test_qa.pytcf_website/views/qa.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tcf_website/static/qa/qa_dashboard.js
Comment on lines
+12
to
+22
| migrations.AlterField( | ||
| model_name="answer", | ||
| name="semester", | ||
| field=models.ForeignKey( | ||
| blank=True, | ||
| default=None, | ||
| null=True, | ||
| on_delete=django.db.models.deletion.CASCADE, | ||
| to="tcf_website.semester", | ||
| ), | ||
| ), |
Contributor
There was a problem hiding this comment.
on_delete=CASCADE on the now-nullable semester FK risks silent data loss — see models.py.
The Django docs explicitly show SET_NULL as the correct on_delete choice for a nullable ForeignKey. With CASCADE, deleting any Semester row will also delete all Answer rows that reference it — including answers that merely tagged a semester for context. Since the intent of making the field nullable is to represent "Not Taken" (no semester), SET_NULL is the correct semantic.
Fix both this migration and the field definition in models.py:
🛡️ Proposed fix
- on_delete=django.db.models.deletion.CASCADE,
+ on_delete=django.db.models.deletion.SET_NULL,Update models.py correspondingly (see detailed comment there).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| migrations.AlterField( | |
| model_name="answer", | |
| name="semester", | |
| field=models.ForeignKey( | |
| blank=True, | |
| default=None, | |
| null=True, | |
| on_delete=django.db.models.deletion.CASCADE, | |
| to="tcf_website.semester", | |
| ), | |
| ), | |
| migrations.AlterField( | |
| model_name="answer", | |
| name="semester", | |
| field=models.ForeignKey( | |
| blank=True, | |
| default=None, | |
| null=True, | |
| on_delete=django.db.models.deletion.SET_NULL, | |
| to="tcf_website.semester", | |
| ), | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tcf_website/migrations/0029_alter_answer_semester.py` around lines 12 - 22,
The migration AlterField for model "answer" and the corresponding ForeignKey in
models.py use on_delete=CASCADE which will delete Answer rows when a Semester is
removed; change both the migration (0029_alter_answer_semester.py) and the
Answer model's semester field to use on_delete=models.SET_NULL (and keep
null=True, blank=True, default=None) so deleting a Semester sets semester to
NULL instead of cascading deletions; also ensure the migration imports models
(or django.db.models.deletion) and references models.SET_NULL consistently and
update the models.py import if necessary.
| <div class="dropdown-items-list" id="departmentFilterList"> | ||
| <a class="dropdown-item {% if not selected_department %}active{% endif %}" | ||
| data-department-id="" | ||
| href="{% url 'qa' %}{% if search_query %}?q={{ search_query }}{% endif %}"> |
Contributor
There was a problem hiding this comment.
search_query must be URL-encoded before embedding in href attributes.
Django's auto-escaping only converts HTML entities (& → &), but does not percent-encode URL-special characters. A query containing #, %, or + will silently corrupt the URL: # truncates the query string into a fragment identifier, % can cause mis-decoding, and + is interpreted as a space in some contexts. Use {{ search_query|urlencode }} at all three occurrences:
🛡️ Proposed fix
-href="{% url 'qa' %}{% if search_query %}?q={{ search_query }}{% endif %}"
+href="{% url 'qa' %}{% if search_query %}?q={{ search_query|urlencode }}{% endif %}"Apply the same change to the course filter link at line 119:
-href="{% url 'qa' %}?course={{ course.id }}{% if search_query %}&q={{ search_query }}{% endif %}"
+href="{% url 'qa' %}?course={{ course.id }}{% if search_query %}&q={{ search_query|urlencode }}{% endif %}"Also applies to: 105-105, 119-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tcf_website/templates/qa/qa_dashboard.html` at line 65, The hrefs that append
the query string currently interpolate search_query raw, which can produce
invalid URLs for characters like #, %, or +; update each occurrence that builds
a query string (the template anchor that uses "{% url 'qa' %}{% if search_query
%}?q={{ search_query }}{% endif %}" and the two other similar occurrences for
the course filter and the other QA link) to percent-encode the value by
replacing {{ search_query }} with {{ search_query|urlencode }} so the links
safely encode special characters.
…de deleted replies not delete subsequent replies
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tcf_website/templates/qa/_question_detail.html (1)
67-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid nesting
linebreaksinside<p>.
linebreaksalready wraps paragraph blocks, so this can render invalid<p><p>…</p></p>markup. Use a<div>wrapper or switch tolinebreaksbr.Proposed fix
- <p>{{ question.text|linebreaks }}</p> + <div>{{ question.text|linebreaks }}</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/templates/qa/_question_detail.html` around lines 67 - 72, The template nests the Django linebreaks filter inside a <p> which can produce invalid <p><p>…</p></p> HTML; update the rendering of question.text in the question_detail template (the block using question.is_deleted and question.text with the linebreaks filter) to avoid wrapping linebreaks inside a <p> — either replace the outer <p> with a <div> when using {{ question.text|linebreaks }} or switch the filter to {{ question.text|linebreaksbr }} so you can keep the <p>; choose one approach and apply it to the section that renders question.text.tcf_website/models/models.py (1)
1529-1531:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
SET_NULLfor the optionalsemesterFK.With
null=True, deleting aSemestershould not wipe theAnswerrow. The currentCASCADEstill deletes user Q&A content and can tear down entire reply threads during routine semester cleanup. Please switch this toSET_NULLand updatetcf_website/migrations/0029_alter_answer_semester.pyto match.Proposed fix
semester = models.ForeignKey( - Semester, on_delete=models.CASCADE, null=True, blank=True, default=None + Semester, on_delete=models.SET_NULL, null=True, blank=True, default=None )#!/bin/bash set -euo pipefail echo "Model definition:" rg -n -C2 'semester = models\.ForeignKey\(' tcf_website/models/models.py echo echo "Migration 0029 definition:" rg -n -C4 'model_name="answer"|name="semester"|on_delete=' tcf_website/migrations/0029_alter_answer_semester.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/models/models.py` around lines 1529 - 1531, The Answer model's semester ForeignKey currently uses on_delete=models.CASCADE which removes Answer rows when a Semester is deleted; change the field in models.py (the semester = models.ForeignKey(...) on the Answer model) to on_delete=models.SET_NULL while keeping null=True, blank=True, default=None, and then update the existing migration tcf_website/migrations/0029_alter_answer_semester.py to reflect the new on_delete behavior (use models.SET_NULL in the AlterField operation) so migrations and model state remain consistent; after updating, run makemigrations/check migration validity to ensure no further migration changes are needed.tcf_website/templates/qa/_answer_item.html (1)
29-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the outer
<p>aroundlinebreaks.
linebreaksalready emits<p>blocks, so this markup nests paragraphs and can break layout/selector behavior. Either switch tolinebreaksbror render thelinebreaksoutput in a non-<p>container.Proposed fix
- <p> - {% if answer.is_deleted_placeholder %} - <em>[deleted]</em> - {% else %} - {{ answer.text|linebreaks }} - {% endif %} - </p> + <div> + {% if answer.is_deleted_placeholder %} + <em>[deleted]</em> + {% else %} + {{ answer.text|linebreaks }} + {% endif %} + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/templates/qa/_answer_item.html` around lines 29 - 36, The template currently wraps {{ answer.text|linebreaks }} inside an outer <p>, which causes nested <p> tags when linebreaks emits paragraph blocks; update the qa/_answer_item.html block that checks answer.is_deleted_placeholder so that you remove the surrounding <p> and instead render the answer content in a non-<p> container (e.g., a <div> or nothing) or switch the filter to linebreaksbr ({{ answer.text|linebreaksbr }}) to preserve line breaks without producing paragraph tags; ensure the deleted placeholder branch (<em>[deleted]</em>) remains correctly wrapped for styling.
🧹 Nitpick comments (1)
tcf_website/static/qa/qa_dashboard.js (1)
1007-1045: 💤 Low valueConsider adding idempotent guard for consistency.
Unlike other init functions,
initReplyFormsdoesn't check adata-*Boundattribute before adding listeners. While this works because the DOM is replaced on each call, adding the guard would be consistent with the pattern used elsewhere (e.g.,initVoting,initQuestionActions).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/static/qa/qa_dashboard.js` around lines 1007 - 1045, The initReplyForms function lacks the idempotent data-bound guard; update initReplyForms to skip attaching listeners if a marker is present and to set that marker after binding: check a data-bound (e.g., data-reply-bound) attribute on a stable container or on each element (referencing initReplyForms, '.reply-btn' and '.btn-cancel-reply') before adding event listeners, and set data-reply-bound="true" once listeners are attached so subsequent calls do not rebind.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tcf_website/static/qa/new_answer.js`:
- Around line 53-55: Remove the fixed 3s re-enable in the "#answerForm" submit
handler (delete the setTimeout(enableButton, 3000) call) and instead only call
enableButton on a real client-side failure path (e.g., in your AJAX/fetch .catch
or error callback) so the submit button remains disabled while the POST is in
flight; keep the initial $("#submitBtn").prop("disabled", true) in the submit
handler and ensure success paths do NOT re-enable the button so duplicate
submissions cannot occur.
In `@tcf_website/templates/qa/_question_list.html`:
- Around line 2-3: The sidebar question item is a non-interactive div (the
element with class "post-item" and attribute data-question-id="{{ question.id
}}") and must be made keyboard-accessible; replace that div with a semantic
interactive element (preferably a <button> or an <a> if it navigates) preserving
the existing class and data-question-id and the active conditional, and add
appropriate ARIA/attributes (e.g., aria-pressed or aria-current) and event
handlers so Enter/Space activate the selection; if you cannot swap the element,
at minimum add tabindex="0", role="button", and keydown handlers that trigger
the same selection logic as click on the element_id post-item/data-question-id
control to ensure keyboard activation and focusability.
---
Duplicate comments:
In `@tcf_website/models/models.py`:
- Around line 1529-1531: The Answer model's semester ForeignKey currently uses
on_delete=models.CASCADE which removes Answer rows when a Semester is deleted;
change the field in models.py (the semester = models.ForeignKey(...) on the
Answer model) to on_delete=models.SET_NULL while keeping null=True, blank=True,
default=None, and then update the existing migration
tcf_website/migrations/0029_alter_answer_semester.py to reflect the new
on_delete behavior (use models.SET_NULL in the AlterField operation) so
migrations and model state remain consistent; after updating, run
makemigrations/check migration validity to ensure no further migration changes
are needed.
In `@tcf_website/templates/qa/_answer_item.html`:
- Around line 29-36: The template currently wraps {{ answer.text|linebreaks }}
inside an outer <p>, which causes nested <p> tags when linebreaks emits
paragraph blocks; update the qa/_answer_item.html block that checks
answer.is_deleted_placeholder so that you remove the surrounding <p> and instead
render the answer content in a non-<p> container (e.g., a <div> or nothing) or
switch the filter to linebreaksbr ({{ answer.text|linebreaksbr }}) to preserve
line breaks without producing paragraph tags; ensure the deleted placeholder
branch (<em>[deleted]</em>) remains correctly wrapped for styling.
In `@tcf_website/templates/qa/_question_detail.html`:
- Around line 67-72: The template nests the Django linebreaks filter inside a
<p> which can produce invalid <p><p>…</p></p> HTML; update the rendering of
question.text in the question_detail template (the block using
question.is_deleted and question.text with the linebreaks filter) to avoid
wrapping linebreaks inside a <p> — either replace the outer <p> with a <div>
when using {{ question.text|linebreaks }} or switch the filter to {{
question.text|linebreaksbr }} so you can keep the <p>; choose one approach and
apply it to the section that renders question.text.
---
Nitpick comments:
In `@tcf_website/static/qa/qa_dashboard.js`:
- Around line 1007-1045: The initReplyForms function lacks the idempotent
data-bound guard; update initReplyForms to skip attaching listeners if a marker
is present and to set that marker after binding: check a data-bound (e.g.,
data-reply-bound) attribute on a stable container or on each element
(referencing initReplyForms, '.reply-btn' and '.btn-cancel-reply') before adding
event listeners, and set data-reply-bound="true" once listeners are attached so
subsequent calls do not rebind.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0898e5b9-0933-4cd1-a27a-18e44dda4008
📒 Files selected for processing (11)
tcf_website/migrations/0030_remove_answer_unique_answer_per_user_and_question.pytcf_website/models/models.pytcf_website/static/qa/new_answer.jstcf_website/static/qa/qa_dashboard.jstcf_website/templates/qa/_answer_item.htmltcf_website/templates/qa/_question_detail.htmltcf_website/templates/qa/_question_list.htmltcf_website/templates/qa/qa.htmltcf_website/tests/test_qa.pytcf_website/urls.pytcf_website/views/qa.py
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
4 participants
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.
About
The Q&A feature is designed to act like a Piazza for theCourseForum. Users can post questions and answer questions all on a dashboard. Questions can be posted about specific courses or broader department labels.
Features
Posting Questions
Users can create new questions by providing:
Browsing & Filtering (Left Pane)
Questions & Discussion View (Right Pane)
Replying
Editing
Files Modified
tcf_website/views/qa.py
tcf_website/models/models.py
tcf_website/templates/qa/qa_dashboard.html
tcf_website/templates/qa/_question_list.html
tcf_website/templates/qa/_question_content.html
tcf_website/templates/qa/_question_detail.html
tcf_website/templates/qa/_answer_item.html
tcf_website/templates/qa/qa.html
tcf_website/static/qa/qa_dashboard.js
tcf_website/static/qa/qa_dashboard.css
tcf_website/tests/test_qa.py
Screenshots
Q&A Dashboard
Answers and Replies
Adding Question
User Asked Question
Edit Question Modal
Delete Question Modal
Deleted Question with Replies
Testing
Added/updated automated tests in tcf_website/tests/test_qa.py covering:
Performed load testing for the Q&A dashboard using k6 script:
Load test results:
22,000 Total Requests
75 Virtual Users
Request Failure Rate: 0.00%
Endpoint Latency:
Load test summary:
Future Changes
Summary by CodeRabbit
New Features
Style
Tests