From ce71e0241bf57d861428533d13a1fae361e52a5f Mon Sep 17 00:00:00 2001 From: Simone Mosciatti Date: Sun, 31 May 2026 21:21:34 -0700 Subject: [PATCH] Allow teachers/TAs to try conversations safely - Fix broken conversation links in section_detail template (was href='#') - Add 'Try Conversation' button for teachers on homework detail page - Add 'Test Conversation' button for TAs on homework detail page - List old test conversations below the button for resuming - Guard all student_profile accesses against teacher/TA test conversations (get_all_homework_matrix, get_course_homework_matrix, Submission.student, get_submission_data) - Add 4 tests covering teacher test conversations in matrix, submission, and model edge cases --- src/conversations/models.py | 2 +- src/conversations/services.py | 7 ++-- src/conversations/tests/test_models.py | 8 +++++ .../tests/test_submission_service.py | 13 ++++++++ src/homeworks/services.py | 8 +++++ src/homeworks/templates/homeworks/detail.html | 30 +++++++++++++++++ .../templates/homeworks/section_detail.html | 2 +- src/homeworks/tests/test_matrix_view.py | 26 +++++++++++++++ src/homeworks/views.py | 33 +++++++++++++++++++ 9 files changed, 124 insertions(+), 5 deletions(-) diff --git a/src/conversations/models.py b/src/conversations/models.py index 17fb524..d002a38 100644 --- a/src/conversations/models.py +++ b/src/conversations/models.py @@ -126,7 +126,7 @@ def section(self): @property def student(self): """Get the student through the conversation.""" - return self.conversation.user.student_profile + return getattr(self.conversation.user, "student_profile", None) def clean(self): """Ensure only one submission per student per section.""" diff --git a/src/conversations/services.py b/src/conversations/services.py index f33ad16..64b9a13 100644 --- a/src/conversations/services.py +++ b/src/conversations/services.py @@ -819,7 +819,7 @@ class SubmissionData: conversation_id: UUID section_id: UUID section_title: str - student_id: UUID + student_id: UUID | None student_name: str submitted_at: datetime @@ -904,8 +904,9 @@ def get_submission_data( "conversation__user__student_profile", "conversation__section" ).get(id=submission_id) - # Get student's name + # Get student's name and profile user = submission.conversation.user + student_profile = getattr(user, "student_profile", None) student_name = f"{user.first_name} {user.last_name}" if not student_name.strip(): student_name = user.username @@ -916,7 +917,7 @@ def get_submission_data( conversation_id=submission.conversation.id, section_id=submission.conversation.section.id, section_title=submission.conversation.section.title, - student_id=submission.conversation.user.student_profile.id, + student_id=student_profile.id if student_profile else None, student_name=student_name, submitted_at=submission.submitted_at, ) diff --git a/src/conversations/tests/test_models.py b/src/conversations/tests/test_models.py index 0a8566e..2b167cd 100644 --- a/src/conversations/tests/test_models.py +++ b/src/conversations/tests/test_models.py @@ -405,6 +405,14 @@ def test_submission_student_property(self): submission = Submission.objects.create(**self.submission_data) self.assertEqual(submission.student, self.student) + def test_submission_student_property_with_teacher_conversation(self): + """Submission.student should return None for teacher test conversations.""" + teacher_conv = Conversation.objects.create( + user=self.teacher_user, section=self.section + ) + submission = Submission.objects.create(conversation=teacher_conv) + self.assertIsNone(submission.student) + def test_submission_clean_method_validation(self): """Test submission clean method validation.""" # Create first submission diff --git a/src/conversations/tests/test_submission_service.py b/src/conversations/tests/test_submission_service.py index 7be26cb..8c98122 100644 --- a/src/conversations/tests/test_submission_service.py +++ b/src/conversations/tests/test_submission_service.py @@ -125,6 +125,19 @@ def test_get_submission_data(self): self.assertEqual(data.student_id, self.student.id) self.assertIn(self.student_user.username, data.student_name) + def test_get_submission_data_with_teacher_conversation(self): + """get_submission_data should handle teacher-owned conversations.""" + teacher_conv = Conversation.objects.create( + user=self.teacher_user, section=self.section + ) + submission = Submission.objects.create(conversation=teacher_conv) + + data = SubmissionService.get_submission_data(submission.id) + + self.assertIsNotNone(data) + self.assertEqual(data.id, submission.id) + self.assertEqual(data.conversation_id, teacher_conv.id) + def test_get_student_submissions(self): """Test retrieving all submissions for a student.""" # Create two submissions for different sections diff --git a/src/homeworks/services.py b/src/homeworks/services.py index 11640d0..2896bf3 100644 --- a/src/homeworks/services.py +++ b/src/homeworks/services.py @@ -90,6 +90,8 @@ class SectionData: status: SectionStatus | None = None conversation_id: UUID | None = None answer_count: int = 0 + # Teacher test conversations (populated in view for teachers/TAs) + conversations: list[dict] | None = None @property def has_solution(self) -> bool: @@ -678,6 +680,8 @@ def get_all_homework_matrix(teacher_id: UUID) -> HomeworkMatrixData | None: # Populate conversation map for conv in conversations: + if not hasattr(conv.user, "student_profile"): + continue student_id = conv.user.student_profile.id homework_id = conv.section.homework.id key = (student_id, homework_id) @@ -883,6 +887,8 @@ def get_course_homework_matrix( submission_map = {sub.conversation.id: sub for sub in submissions} for conv in conversations: + if not hasattr(conv.user, "student_profile"): + continue student_id = conv.user.student_profile.id homework_id = conv.section.homework.id key = (student_id, homework_id) @@ -897,6 +903,8 @@ def get_course_homework_matrix( tuple[UUID, UUID], datetime ] = {} for answer in section_answers: + if not hasattr(answer.user, "student_profile"): + continue student_id = answer.user.student_profile.id homework_id = answer.section.homework.id key = (student_id, homework_id) diff --git a/src/homeworks/templates/homeworks/detail.html b/src/homeworks/templates/homeworks/detail.html index 7684a7b..e23626b 100644 --- a/src/homeworks/templates/homeworks/detail.html +++ b/src/homeworks/templates/homeworks/detail.html @@ -158,6 +158,36 @@

+ {% if 'teacher' in data.user_roles and section.section_type != 'non_interactive' %} +
+ + Try Conversation + +
+ {% endif %} + + {% if 'teacher_assistant' in data.user_roles and section.section_type != 'non_interactive' %} +
+ + Test Conversation + +
+ {% endif %} + + {% if section.conversations %} +
+
Previous Conversations
+
+ {% for conv in section.conversations %} + + {{ conv.label }} + {{ conv.message_count }} messages + + {% endfor %} +
+
+ {% endif %} + {% if 'student' in data.user_roles %}
{% if data.widget_progress and not data.widget_progress.all_pre_answered %} diff --git a/src/homeworks/templates/homeworks/section_detail.html b/src/homeworks/templates/homeworks/section_detail.html index 5cbfc18..c9248dd 100644 --- a/src/homeworks/templates/homeworks/section_detail.html +++ b/src/homeworks/templates/homeworks/section_detail.html @@ -83,7 +83,7 @@

Conversations

{% if data.conversations %}
{% for conversation in data.conversations %} - +
{{ conversation.label }} diff --git a/src/homeworks/tests/test_matrix_view.py b/src/homeworks/tests/test_matrix_view.py index 441e977..514d4f8 100644 --- a/src/homeworks/tests/test_matrix_view.py +++ b/src/homeworks/tests/test_matrix_view.py @@ -650,3 +650,29 @@ def test_ta_not_shown_in_matrix(self): "TA with student profile should not appear in matrix", ) self.assertEqual(matrix_data.total_students, 2) + + def test_matrix_with_teacher_test_conversations_ignored(self): + """Teacher test conversations should not crash get_course_homework_matrix.""" + Conversation.objects.create(user=self.teacher_user, section=self.section1_1) + + matrix_data = HomeworkService.get_course_homework_matrix(self.course.id) + + self.assertIsNotNone(matrix_data) + self.assertEqual(matrix_data.total_students, 2) + for student_row in matrix_data.student_rows: + for cell in student_row.homework_cells: + self.assertEqual(cell.submitted_sections, 0) + self.assertEqual(cell.total_conversations, 0) + + def test_all_homework_matrix_with_teacher_conversations(self): + """Teacher test conversations should not crash get_all_homework_matrix.""" + Conversation.objects.create(user=self.teacher_user, section=self.section1_1) + + matrix_data = HomeworkService.get_all_homework_matrix(self.teacher.id) + + self.assertIsNotNone(matrix_data) + self.assertEqual(matrix_data.total_students, 2) + for student_row in matrix_data.student_rows: + for cell in student_row.homework_cells: + self.assertEqual(cell.submitted_sections, 0) + self.assertEqual(cell.total_conversations, 0) diff --git a/src/homeworks/views.py b/src/homeworks/views.py index 50a2ce5..fe9e68c 100644 --- a/src/homeworks/views.py +++ b/src/homeworks/views.py @@ -994,10 +994,42 @@ def _get_view_data(self, user, homework_id: UUID) -> HomeworkDetailData | None: pass if homework_detail.sections: + # Gather teacher/TA test conversations per section + teacher_conversations_map: dict[UUID, list[dict]] = {} + if "teacher" in user_roles or "teacher_assistant" in user_roles: + from conversations.models import Conversation as ConvModel + + section_ids = [s.id for s in homework_detail.sections] + test_convs = ( + ConvModel.objects.filter( + user=user, + section_id__in=section_ids, + is_deleted=False, + ) + .select_related("section") + .prefetch_related("messages") + ) + for conv in test_convs: + section_id = conv.section.id + if section_id not in teacher_conversations_map: + teacher_conversations_map[section_id] = [] + teacher_conversations_map[section_id].append( + { + "id": conv.id, + "created_at": conv.created_at, + "updated_at": conv.updated_at, + "message_count": conv.message_count, + "label": f"Test conversation {conv.created_at.strftime('%Y-%m-%d %H:%M')}", + } + ) + for section_data in homework_detail.sections: # Get progress data for this section if available progress: SectionData | None = section_progress_map.get(section_data.id) + # Get teacher test conversations for this section + convs = teacher_conversations_map.get(section_data.id) + sections.append( SectionData( id=section_data.id, @@ -1011,6 +1043,7 @@ def _get_view_data(self, user, homework_id: UUID) -> HomeworkDetailData | None: status=progress.status if progress else None, conversation_id=progress.conversation_id if progress else None, answer_count=progress.answer_count if progress else 0, + conversations=convs, ) )