From 8293b855fb7c157e680083257b89e0cc60060eb4 Mon Sep 17 00:00:00 2001 From: Steve Geinitz Date: Thu, 14 May 2026 12:42:24 -0600 Subject: [PATCH 1/2] added a --send-all option to avoid prompting for each message to be sent --- canvigator.py | 21 +++++-- canvigator_course.py | 15 ++++- canvigator_help.py | 17 +++++- canvigator_quiz.py | 134 +++++++++++++++++++++++++++++++++++------- test_canvigator.py | 137 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 291 insertions(+), 33 deletions(-) diff --git a/canvigator.py b/canvigator.py index 927eab0..af7814a 100755 --- a/canvigator.py +++ b/canvigator.py @@ -87,7 +87,7 @@ def _run_course_task(task, course, canv_config, n_days, cloud_questions): cd.prepClassDigest(course, days=n_days, cloud_questions=cloud_questions) -def _run_quiz_task(task, quiz, dry_run, tag, reply_window_days): +def _run_quiz_task(task, quiz, dry_run, tag, reply_window_days, send_all=False): """Dispatch a quiz-level task to the appropriate method.""" if task == 'get-quiz-questions': quiz.getQuizQuestions(tag=tag) @@ -98,11 +98,11 @@ def _run_quiz_task(task, quiz, dry_run, tag, reply_window_days): elif task == 'assess-replies': quiz.assessFollowUpReplies(reply_window_days=reply_window_days) elif task == 'send-quiz-reminder': - quiz.sendQuizReminders(dry_run=dry_run) + quiz.sendQuizReminders(dry_run=dry_run, send_all=send_all) elif task == 'send-follow-up-question': - quiz.sendFollowUpQuestions(dry_run=dry_run) + quiz.sendFollowUpQuestions(dry_run=dry_run, send_all=send_all) elif task == 'send-follow-up-assessments': - quiz.sendFollowUpAssessments(dry_run=dry_run) + quiz.sendFollowUpAssessments(dry_run=dry_run, send_all=send_all) elif task == 'create-pairs': quiz.openPresentCSV() quiz.generateDistanceMatrix(only_present=True) @@ -132,6 +132,7 @@ def _run_quiz_task(task, quiz, dry_run, tag, reply_window_days): '-g': '--auto-grade', '-n': '--days', '-q': '--cloud-questions', + '-s': '--send-all', '-h': '--help', } args = [_SHORT_TO_LONG.get(a, a) for a in sys.argv[1:]] @@ -148,6 +149,14 @@ def _run_quiz_task(task, quiz, dry_run, tag, reply_window_days): if dry_run: args.remove('--dry-run') +send_all = '--send-all' in args +if send_all: + args.remove('--send-all') + +if dry_run and send_all: + print("Error: --send-all/-s and --dry-run/-d are mutually exclusive.") + sys.exit(1) + tag = '--tag' in args if tag: args.remove('--tag') @@ -346,7 +355,7 @@ def _run_quiz_task(task, quiz, dry_run, tag, reply_window_days): course.getAllQuizzesAndSubmissions() elif task == 'send-quiz-reminder' and all_quizzes_flag: - course.sendAllQuizReminders(dry_run=dry_run) + course.sendAllQuizReminders(dry_run=dry_run, send_all=send_all) elif task in ('create-media-recording-assignment', 'get-media-recordings', 'analyze-media-recordings'): _run_assignment_task(task, course, canvas, canv_config, dry_run, auto_grade_flag) @@ -383,6 +392,6 @@ def _run_quiz_task(task, quiz, dry_run, tag, reply_window_days): print(f"\nSelected quiz: {quiz_choice.title}") skip = task in ('get-quiz-questions', 'assess-replies', 'send-follow-up-assessments') quiz = cq.CanvigatorQuiz(canvas, course, quiz_choice, canv_config, verbose=False, skip_student_data=skip) - _run_quiz_task(task, quiz, dry_run, tag, reply_window_days) + _run_quiz_task(task, quiz, dry_run, tag, reply_window_days, send_all=send_all) print("\n*** Done ***\n") diff --git a/canvigator_course.py b/canvigator_course.py index 1f4ed0e..022ec0e 100644 --- a/canvigator_course.py +++ b/canvigator_course.py @@ -59,15 +59,26 @@ def getAllQuizzesAndSubmissions(self): else: print(" Skipping (unpublished or insufficient submissions)") - def sendAllQuizReminders(self, dry_run=False): + def sendAllQuizReminders(self, dry_run=False, send_all=False): """Send one consolidated reminder per student covering every published, future-due quiz they're behind on. + With ``send_all=True``, the per-student ``[y/N]`` prompt is bypassed, + but the task refuses unless a ``--dry-run`` of the same task ran in + the last 10 minutes (verified via + ``canvigator_quiz._hasRecentDryRunManifest`` against the course-level + ``course_reminder_sent_dryrun_*.csv`` manifest). + Iterates every quiz passing ``cu.is_quiz_open_for_reminder`` (published with a ``due_at`` strictly in the future), classifies each enrolled student per quiz (no attempt / imperfect / page-blur on perfect), and composes a single message per student listing all eligible quizzes. Skips quizzes lacking a ``*_questions_w_tags_*.csv`` with a warning. """ + if send_all and not cq._hasRecentDryRunManifest(self.config.data_path, "course_reminder_sent"): + raise RuntimeError( + "--send-all requires a fresh --dry-run of 'send-quiz-reminder --all' " + "for this course within the last 10 minutes; none found." + ) all_quizzes = list(self.canvas_course.get_quizzes()) eligible = [q for q in all_quizzes if cu.is_quiz_open_for_reminder(q)] @@ -150,7 +161,7 @@ def sendAllQuizReminders(self, dry_run=False): cq._sendOrPreviewMessages(messages, subject_str, header_label) _saveCourseReminderManifest(messages, student_state, subject_str, dry_run, self.config.data_path) else: - sent_messages = cq._interactiveSend(self.canvas, messages, subject_str, header_label) + sent_messages = cq._interactiveSend(self.canvas, messages, subject_str, header_label, send_all=send_all) if sent_messages: _saveCourseReminderManifest(sent_messages, student_state, subject_str, dry_run, self.config.data_path) diff --git a/canvigator_help.py b/canvigator_help.py index 2ac7578..077d58f 100644 --- a/canvigator_help.py +++ b/canvigator_help.py @@ -62,6 +62,12 @@ "Route the discussion-question step to cloud Gemini 3 with a redacted " "prompt (tag names + integer counts only — no transcripts or themes).", ), + '--send-all': ( + '-s', None, + "Skip the per-student [y/N] confirmation and send every message. " + "Requires a matching --dry-run run on the same task within the last " + "10 minutes (otherwise refuses). Mutually exclusive with --dry-run.", + ), } @@ -480,10 +486,11 @@ "With --all: data//course_reminder_sent[_dryrun]_YYYYMMDD" ".csv (one row per student listing every quiz reminded about).", ], - 'flags': ['--dry-run', '--all'], + 'flags': ['--dry-run', '--all', '--send-all'], 'examples': [ "python canvigator.py --crn 12345 send-quiz-reminder", "python canvigator.py --crn 12345 --all --dry-run send-quiz-reminder", + "python canvigator.py --crn 12345 --send-all send-quiz-reminder", ], 'run_before': ['get-quiz-questions', 'get-quiz-submission-events'], 'run_after': ['generate-follow-up-questions', 'send-follow-up-question'], @@ -522,10 +529,11 @@ "(manifest with conversation_id, question_id, sent_at, " "question_mode columns).", ], - 'flags': ['--dry-run'], + 'flags': ['--dry-run', '--send-all'], 'examples': [ "python canvigator.py --crn 12345 --dry-run send-follow-up-question", "python canvigator.py --crn 12345 send-follow-up-question", + "python canvigator.py --crn 12345 --send-all send-follow-up-question", ], 'run_before': ['generate-follow-up-questions'], 'run_after': ['assess-replies', 'send-follow-up-assessments'], @@ -599,11 +607,13 @@ "data//quiz_followup_assessments.csv (rewritten in " "place with sent_assessment=1 and updated sent_at on success).", ], - 'flags': ['--dry-run'], + 'flags': ['--dry-run', '--send-all'], 'examples': [ "python canvigator.py --crn 12345 --dry-run " "send-follow-up-assessments", "python canvigator.py --crn 12345 send-follow-up-assessments", + "python canvigator.py --crn 12345 --send-all " + "send-follow-up-assessments", ], 'run_before': ['assess-replies'], 'run_after': [], @@ -871,6 +881,7 @@ def print_global_help(task_groups): for flag in ( '--dry-run', '--tag', '--all', '--crn', '--months', '--reply-window-days', '--auto-grade', '--days', '--cloud-questions', + '--send-all', ): label = _flag_label(flag) _, _, text = FLAG_DESCRIPTIONS[flag] diff --git a/canvigator_quiz.py b/canvigator_quiz.py index 959814e..e6e6b4a 100644 --- a/canvigator_quiz.py +++ b/canvigator_quiz.py @@ -42,6 +42,20 @@ def _parse_points_possible_from_student_analysis(csv_path): return points_possible +def _hasRecentDryRunManifest(data_path, pattern, max_age_minutes=10): + """Return True iff a ``*_dryrun_*.csv`` mtime is within ``max_age_minutes``. + + The recency anchor for ``--send-all``: forces the instructor to run + ``--dry-run`` of the same task within the window before bulk-sending, + which means they've just seen the previews on screen. + """ + latest = find_latest_csv(data_path, pattern + '_dryrun_') + if latest is None: + return False + cutoff_ts = (datetime.now() - timedelta(minutes=max_age_minutes)).timestamp() + return latest.stat().st_mtime >= cutoff_ts + + def _findRecentSubmissionCsvs(data_path, quiz_prefix, quiz_id, max_age_minutes=10): """Return paths to the three submission CSVs if all are recent, else None. @@ -401,13 +415,14 @@ def _sendOrPreviewMessages(messages, subject_str, header_label): logger.info(f"Quiz reminders (dry run) would be sent: {len(messages)}") -def _interactiveSend(canvas, messages, subject_str, header_label): +def _interactiveSend(canvas, messages, subject_str, header_label, send_all=False): """Interactively prompt the instructor to send or skip each message. Default behavior is SKIP — the message is sent if and only if the user types ``y`` (case-insensitive) and presses Enter. Any other input, - including a bare Enter, skips. Returns the list of messages that were - actually sent as 5-tuples (..., conversation_id). + including a bare Enter, skips. With ``send_all=True``, the per-student + prompt is bypassed and every message is sent. Returns the list of + messages actually sent as 5-tuples (..., conversation_id). """ if not messages: print("No messages to send!") @@ -415,7 +430,10 @@ def _interactiveSend(canvas, messages, subject_str, header_label): print(f"\n{header_label}") print(f"Messages to review: {len(messages)}") - print("For each student, enter 'y' + Enter to send, or just Enter to skip (default).\n") + if send_all: + print("--send-all set — sending every message without per-student confirmation.\n") + else: + print("For each student, enter 'y' + Enter to send, or just Enter to skip (default).\n") sent_messages = [] for i, (student_id, student_name, message_str, reason) in enumerate(messages, 1): @@ -424,9 +442,12 @@ def _interactiveSend(canvas, messages, subject_str, header_label): print(f" Subject: {subject_str}") print(f" Message: {message_str}\n") - # Strict 'y' check — N is the default; anything other than 'y' (case-insensitive) - # skips. This includes 'yes', 'send', 'Y', empty, etc. — only a single 'y' sends. - choice = input(f" Send to {student_name}? [y/N]: ").strip().lower() + if send_all: + choice = 'y' + else: + # Strict 'y' check — N is the default; anything other than 'y' (case-insensitive) + # skips. This includes 'yes', 'send', 'Y', empty, etc. — only a single 'y' sends. + choice = input(f" Send to {student_name}? [y/N]: ").strip().lower() if choice == 'y': try: convos = canvas.create_conversation( @@ -735,8 +756,20 @@ def _classifyStudentForQuiz(self, student_id, quiz_scores, points_possible, return ('page blur', blur_bullets) return ('perfect clean', None) - def sendQuizReminders(self, dry_run=False): - """Send reminder messages to students who haven't taken the quiz, haven't achieved a perfect score, or had page blur events.""" + def sendQuizReminders(self, dry_run=False, send_all=False): + """Send reminder messages to students who haven't taken the quiz, haven't achieved a perfect score, or had page blur events. + + With ``send_all=True``, the per-student ``[y/N]`` prompt is bypassed, + but the task refuses unless a ``--dry-run`` of the same task ran in + the last 10 minutes (verified via ``_hasRecentDryRunManifest``). + """ + if send_all: + pattern = f"{self.config.quiz_prefix}{self.canvas_quiz.id}_reminder_sent" + if not _hasRecentDryRunManifest(self.config.data_path, pattern): + raise RuntimeError( + "--send-all requires a fresh --dry-run of send-quiz-reminder " + "for this quiz within the last 10 minutes; none found." + ) quiz_name = self.canvas_quiz.title points_possible = self.canvas_quiz.points_possible @@ -834,7 +867,7 @@ def sendQuizReminders(self, dry_run=False): self._sendOrPreviewMessages(messages, subject_str, quiz_name, points_possible) self._saveReminderManifest(messages, subject_str, dry_run) else: - sent_messages = self._interactiveSend(messages, subject_str, quiz_name, points_possible) + sent_messages = self._interactiveSend(messages, subject_str, quiz_name, points_possible, send_all=send_all) if sent_messages: self._saveReminderManifest(sent_messages, subject_str, dry_run) @@ -847,9 +880,13 @@ def _sendOrPreviewMessages(self, messages, subject_str, quiz_name, points_possib header_label = f"Quiz: {quiz_name} ({points_possible} points possible)" _sendOrPreviewMessages(messages, subject_str, header_label) - def sendFollowUpQuestions(self, dry_run=False): + def sendFollowUpQuestions(self, dry_run=False, send_all=False): """Send the instructor-selected open-ended follow-up question to every student who attempted the quiz. + With ``send_all=True``, the per-student ``[y/N]`` prompt is bypassed, + but the task refuses unless a ``--dry-run`` of the same task ran in + the last 10 minutes (verified via ``_hasRecentDryRunManifest``). + Picks the first row in the *_open_ended_*.csv with `selected_question == 1` as the question to send (the instructor curates the choice offline). Every student who has at least one attempt receives the follow-up — the @@ -858,6 +895,13 @@ def sendFollowUpQuestions(self, dry_run=False): get-quiz-questions --tag and generate-follow-up-questions have both been run for this quiz. Auto-refreshes submission data to pick up recent attempts. """ + if send_all: + pattern = f"{self.config.quiz_prefix}{self.canvas_quiz.id}_followup_sent" + if not _hasRecentDryRunManifest(self.config.data_path, pattern): + raise RuntimeError( + "--send-all requires a fresh --dry-run of send-follow-up-question " + "for this quiz within the last 10 minutes; none found." + ) quiz_name = self.canvas_quiz.title # Load question metadata (from *_questions_w_tags_*.csv) @@ -968,18 +1012,18 @@ def sendFollowUpQuestions(self, dry_run=False): self._sendOrPreviewMessages(messages, subject_str, quiz_name, self.canvas_quiz.points_possible) self._saveFollowUpManifest(messages, selected_qid, question_mode, subject_str, dry_run) else: - sent_messages = self._interactiveSend(messages, subject_str, quiz_name, self.canvas_quiz.points_possible) + sent_messages = self._interactiveSend(messages, subject_str, quiz_name, self.canvas_quiz.points_possible, send_all=send_all) if sent_messages: self._saveFollowUpManifest(sent_messages, selected_qid, question_mode, subject_str, dry_run) - def _interactiveSend(self, messages, subject_str, quiz_name, points_possible): + def _interactiveSend(self, messages, subject_str, quiz_name, points_possible, send_all=False): """Interactively prompt for send/skip per message (single-quiz path). Thin wrapper around the module-level ``_interactiveSend`` that builds the per-quiz header label and threads ``self.canvas`` through. """ header_label = f"Quiz: {quiz_name} ({points_possible} points possible)" - return _interactiveSend(self.canvas, messages, subject_str, header_label) + return _interactiveSend(self.canvas, messages, subject_str, header_label, send_all=send_all) def _classifyStudentsByQuestionResult(self, question_id, subs_by_q_df, question_info): """Return ``{student_id: 'missed' | 'correct'}`` for every student who attempted the quiz. @@ -1566,9 +1610,13 @@ def _writeAssessmentsCsv(self, df): print(f" Assessments saved: {path.name}") logger.info(f"Follow-up assessments saved: {path}") - def sendFollowUpAssessments(self, dry_run=False): + def sendFollowUpAssessments(self, dry_run=False, send_all=False): """Send instructor-curated feedback back to students on their follow-up threads. + With ``send_all=True``, the per-student ``[y/N]`` prompt is bypassed, + but the task refuses unless a ``--dry-run`` of the same task ran in + the last 10 minutes (verified via ``_hasRecentDryRunManifest``). + Reads the persistent ``*_followup_assessments.csv`` for this quiz and, for every row with ``sent_assessment=0`` and a non-empty ``feedback`` value, posts the feedback text as a reply to the existing Canvas conversation @@ -1576,6 +1624,13 @@ def sendFollowUpAssessments(self, dry_run=False): flip ``sent_assessment`` to ``1`` and stamp ``sent_at``; the file is then rewritten in place. """ + if send_all: + pattern = f"{self.config.quiz_prefix}{self.canvas_quiz.id}_assessments_sent" + if not _hasRecentDryRunManifest(self.config.data_path, pattern): + raise RuntimeError( + "--send-all requires a fresh --dry-run of send-follow-up-assessments " + "for this quiz within the last 10 minutes; none found." + ) path = self._assessmentsPath() if not path.exists(): raise FileNotFoundError( @@ -1597,14 +1652,19 @@ def sendFollowUpAssessments(self, dry_run=False): return print(f"\n{'[DRY-RUN] ' if dry_run else ''}Sending feedback for {len(to_send)} student(s).") - if not dry_run: + if dry_run: + pass # no per-student prompt + elif send_all: + print("--send-all set — sending every feedback without per-student confirmation.") + else: print("For each student, enter 'y' + Enter to send, or just Enter to skip (default).") n_sent = 0 for idx, convo_id, row in to_send: - if self._sendOneAssessment(df, idx, convo_id, row, dry_run): + if self._sendOneAssessment(df, idx, convo_id, row, dry_run, send_all=send_all): n_sent += 1 if dry_run: + self._saveAssessmentsDryRunManifest(to_send) print(f"\n[DRY-RUN] Would have sent {len(to_send)} feedback message(s). No changes written.") return @@ -1612,6 +1672,32 @@ def sendFollowUpAssessments(self, dry_run=False): print(f"\nSent {n_sent} feedback message(s); updated {path.name}.") logger.info(f"Sent {n_sent} follow-up feedback messages; updated {path}") + def _saveAssessmentsDryRunManifest(self, to_send): + """Write a *_assessments_sent_dryrun_YYYYMMDD.csv listing rows that would be sent. + + Mirrors the dry-run manifest written by send-quiz-reminder and + send-follow-up-question, and serves as the recency anchor consulted + by ``--send-all`` on this task. + """ + rows = [] + for _, convo_id, row in to_send: + feedback = str(row.get('feedback', '')).strip() + preview = feedback if len(feedback) <= 200 else feedback[:200] + '...' + rows.append({ + 'student_id': row.get('student_id', ''), + 'student_name': row.get('student_name', ''), + 'question_id': row.get('question_id', ''), + 'conversation_id': convo_id, + 'feedback_preview': preview, + }) + out_df = pd.DataFrame(rows, columns=['student_id', 'student_name', 'question_id', 'conversation_id', 'feedback_preview']) + csv_name = self.config.data_path / ( + f"{self.config.quiz_prefix}{self.canvas_quiz.id}_assessments_sent_dryrun_{today_str()}.csv" + ) + out_df.to_csv(csv_name, index=False) + print(f" Dry-run manifest saved: {csv_name.name}") + logger.info(f"Assessments dry-run manifest saved: {csv_name}") + def _collectAssessmentsToSend(self, df, convo_lookup): """Filter the assessments DataFrame to rows that should be sent now. @@ -1640,13 +1726,14 @@ def _collectAssessmentsToSend(self, df, convo_lookup): to_send.append((idx, int(convo_id), row)) return to_send - def _sendOneAssessment(self, df, idx, convo_id, row, dry_run): + def _sendOneAssessment(self, df, idx, convo_id, row, dry_run, send_all=False): """Send (or preview) feedback for a single assessment row. Updates ``df`` in place on a successful real send and returns True; - returns False on dry-run, skip, or send failure. Real sends require - per-message [y/N] confirmation — default (bare Enter or anything - other than ``y``) is SKIP. + returns False on dry-run, skip, or send failure. Real sends normally + require per-message [y/N] confirmation — default (bare Enter or + anything other than ``y``) is SKIP. ``send_all=True`` bypasses the + per-message prompt. """ student_name = row.get('student_name', '?') feedback = str(row['feedback']).strip() @@ -1658,7 +1745,10 @@ def _sendOneAssessment(self, df, idx, convo_id, row, dry_run): print(f" {line}") if dry_run: return False - choice = input(f" Send to {student_name}? [y/N]: ").strip().lower() + if send_all: + choice = 'y' + else: + choice = input(f" Send to {student_name}? [y/N]: ").strip().lower() if choice != 'y': print(f" Skipped {student_name}.") return False diff --git a/test_canvigator.py b/test_canvigator.py index 8014ee2..4c14582 100644 --- a/test_canvigator.py +++ b/test_canvigator.py @@ -3675,3 +3675,140 @@ def test_cli_invalid_task_with_help(self): rc, stdout, _ = self._run_cli('bogus-task', '--help') assert rc == 1 assert 'Invalid task' in stdout + + +# --------------------------------------------------------------------------- +# --send-all flag tests +# --------------------------------------------------------------------------- + +class TestHasRecentDryRunManifest: + """Tests for the 10-minute mtime gate that anchors --send-all.""" + + def test_missing_file_returns_false(self, tmp_path): + """No matching manifest in data_path → False.""" + from canvigator_quiz import _hasRecentDryRunManifest + assert _hasRecentDryRunManifest(tmp_path, 'quiz1_5_reminder_sent') is False + + def test_recent_file_returns_true(self, tmp_path): + """Fresh manifest mtime (just touched) → True.""" + from canvigator_quiz import _hasRecentDryRunManifest + (tmp_path / 'quiz1_5_reminder_sent_dryrun_20260514.csv').write_text('a\n') + assert _hasRecentDryRunManifest(tmp_path, 'quiz1_5_reminder_sent') is True + + def test_stale_file_returns_false(self, tmp_path): + """Manifest mtime older than max_age_minutes → False.""" + import os + import time + from canvigator_quiz import _hasRecentDryRunManifest + f = tmp_path / 'quiz1_5_reminder_sent_dryrun_20260514.csv' + f.write_text('a\n') + stale = time.time() - 11 * 60 + os.utime(f, (stale, stale)) + assert _hasRecentDryRunManifest(tmp_path, 'quiz1_5_reminder_sent') is False + + def test_pattern_isolates_dryrun_only(self, tmp_path): + """A non-dryrun manifest with the same prefix does NOT satisfy the check.""" + from canvigator_quiz import _hasRecentDryRunManifest + (tmp_path / 'quiz1_5_reminder_sent_20260514.csv').write_text('a\n') + assert _hasRecentDryRunManifest(tmp_path, 'quiz1_5_reminder_sent') is False + + +class TestInteractiveSendWithSendAll: + """Module-level _interactiveSend should bypass input() when send_all=True.""" + + def test_send_all_skips_input(self, monkeypatch): + """input() must not be called when send_all=True; canvas.create_conversation fires per message.""" + from canvigator_quiz import _interactiveSend + + def _raise_input(prompt): # noqa: ARG001 + raise AssertionError("input() should not be called when send_all=True") + monkeypatch.setattr('builtins.input', _raise_input) + + fake_convo = SimpleNamespace(id=999) + canvas = MagicMock() + canvas.create_conversation.return_value = [fake_convo] + + messages = [(1, 'Alice', 'msg1', 'reason1'), (2, 'Bob', 'msg2', 'reason2')] + sent = _interactiveSend(canvas, messages, 'subj', 'hdr', send_all=True) + assert len(sent) == 2 + assert canvas.create_conversation.call_count == 2 + + def test_default_still_prompts(self, monkeypatch): + """Without send_all, input() drives the loop and 'y' sends, anything else skips.""" + from canvigator_quiz import _interactiveSend + + responses = iter(['y', '']) + monkeypatch.setattr('builtins.input', lambda prompt: next(responses)) + + fake_convo = SimpleNamespace(id=42) + canvas = MagicMock() + canvas.create_conversation.return_value = [fake_convo] + + messages = [(1, 'Alice', 'msg1', 'r'), (2, 'Bob', 'msg2', 'r')] + sent = _interactiveSend(canvas, messages, 'subj', 'hdr') + assert len(sent) == 1 + assert sent[0][1] == 'Alice' + + +class TestSendOneAssessmentWithSendAll: + """_sendOneAssessment should bypass input() and post when send_all=True.""" + + def test_send_all_skips_input(self, monkeypatch): + """With send_all=True, the per-row [y/N] is bypassed and add_message is called.""" + from canvigator_quiz import CanvigatorQuiz + + def _raise_input(prompt): # noqa: ARG001 + raise AssertionError("input() should not be called when send_all=True") + monkeypatch.setattr('builtins.input', _raise_input) + + convo = MagicMock() + canvas = MagicMock() + canvas.get_conversation.return_value = convo + + df = pd.DataFrame([{ + 'student_id': 1, 'student_name': 'Alice', 'question_id': 99, + 'feedback': 'Nice work', 'result': 'pass', + 'sent_assessment': 0, 'sent_at': '', + }]) + row = df.iloc[0] + + quiz = CanvigatorQuiz.__new__(CanvigatorQuiz) + quiz.canvas = canvas + + ok = CanvigatorQuiz._sendOneAssessment(quiz, df, 0, 555, row, dry_run=False, send_all=True) + assert ok is True + convo.add_message.assert_called_once() + assert df.at[0, 'sent_assessment'] == 1 + + +class TestSendAllFlagConflict: + """The CLI must reject --dry-run + --send-all together.""" + + def _run_cli(self, *args): + """Invoke canvigator.py as a subprocess; return (rc, stdout, stderr).""" + import os + import sys + env = {} + for k in ('PATH', 'HOME', 'PYTHONPATH', 'PYTHONHOME'): + if k in os.environ: + env[k] = os.environ[k] + env['CANVAS_URL'] = '' + env['CANVAS_TOKEN'] = '' + cwd = Path(__file__).parent + result = subprocess.run( + [sys.executable, 'canvigator.py', *args], + capture_output=True, text=True, cwd=cwd, env=env, timeout=15, + ) + return result.returncode, result.stdout, result.stderr + + def test_dry_run_and_send_all_rejected(self): + """`--dry-run --send-all` must exit non-zero and explain why.""" + rc, stdout, _ = self._run_cli('--dry-run', '--send-all', 'send-follow-up-assessments') + assert rc != 0 + assert 'mutually exclusive' in stdout + + def test_short_forms_also_rejected(self): + """`-d -s` (short forms) must also be rejected.""" + rc, stdout, _ = self._run_cli('-d', '-s', 'send-follow-up-assessments') + assert rc != 0 + assert 'mutually exclusive' in stdout From 63e78eaf12f51f16efc76a519a181958401108a5 Mon Sep 17 00:00:00 2001 From: Steve Geinitz Date: Thu, 14 May 2026 12:59:22 -0600 Subject: [PATCH 2/2] Harden _summarizeFollowupThemes against NaN/empty question_mode A hand-edited *_followup_assessments.csv row with an empty question_mode cell parses as NaN via pd.read_csv. NaN is truthy in Python, so `(rows[0].get('question_mode') or 'explain').lower()` would call `.lower()` on a float and raise AttributeError instead of falling back. Guard explicitly via pd.isna + a non-empty-string check, then default to 'explain'. Co-Authored-By: Claude Opus 4.7 --- canvigator_digest.py | 9 ++++- test_canvigator.py | 90 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/canvigator_digest.py b/canvigator_digest.py index fbcb464..365882c 100644 --- a/canvigator_digest.py +++ b/canvigator_digest.py @@ -370,7 +370,14 @@ def _summarizeFollowupThemes(quiz_blocks, client, model): out = {} for entry in quiz_blocks: for question_id, rows in entry['rows_by_question'].items(): - mode = (rows[0].get('question_mode') or 'explain').lower() + raw_mode = rows[0].get('question_mode') + # NaN is truthy in Python — guard explicitly so a hand-edited CSV + # row with an empty question_mode falls back to 'explain' instead + # of crashing on `.lower()`. + if raw_mode is None or pd.isna(raw_mode) or not str(raw_mode).strip(): + mode = 'explain' + else: + mode = str(raw_mode).strip().lower() prompt = _buildFollowupThemePrompt( entry['quiz_name'], question_id, rows, mode, ) diff --git a/test_canvigator.py b/test_canvigator.py index 4c14582..ad919c2 100644 --- a/test_canvigator.py +++ b/test_canvigator.py @@ -3289,6 +3289,96 @@ def test_criteria_evaluations_rendered_as_bullets(self): assert '[FATAL]' in prompt +class TestSummarizeFollowupThemesNanMode: + """_summarizeFollowupThemes must tolerate NaN/empty question_mode without crashing.""" + + def test_nan_mode_falls_back_to_explain(self, monkeypatch): + """A row with NaN question_mode must not crash on `.lower()` — fall back to 'explain'.""" + import canvigator_digest as cd + import math + + captured_modes = [] + + def _fake_chat(client, **kwargs): + user_msg = kwargs['messages'][-1]['content'] + for line in user_msg.splitlines(): + if line.startswith('Mode: '): + captured_modes.append(line.split('Mode: ', 1)[1]) + return {'message': {'content': '- theme bullet'}} + + monkeypatch.setattr(cd, '_chat_with_retry', _fake_chat) + + quiz_blocks = [{ + 'quiz_id': '5', 'quiz_name': 'quiz1', + 'rows_by_question': {7: [{ + 'result': 'fail', 'confidence': 'high', + 'transcript': 't', 'feedback': 'f', + 'criteria_evaluations': '', + 'question_mode': math.nan, + }]}, + 'n_dropped_nat': 0, + }] + out = cd._summarizeFollowupThemes(quiz_blocks, client=None, model='m') + assert captured_modes == ['explain'] + assert ('5', 7) in out # produced an entry, didn't crash + + def test_empty_string_mode_falls_back_to_explain(self, monkeypatch): + """An empty-string question_mode also falls back to 'explain' (no `.lower()` on ''→'').""" + import canvigator_digest as cd + + captured_modes = [] + + def _fake_chat(client, **kwargs): + user_msg = kwargs['messages'][-1]['content'] + for line in user_msg.splitlines(): + if line.startswith('Mode: '): + captured_modes.append(line.split('Mode: ', 1)[1]) + return {'message': {'content': ''}} + + monkeypatch.setattr(cd, '_chat_with_retry', _fake_chat) + + quiz_blocks = [{ + 'quiz_id': '5', 'quiz_name': 'quiz1', + 'rows_by_question': {7: [{ + 'result': 'fail', 'confidence': 'high', + 'transcript': 't', 'feedback': 'f', + 'criteria_evaluations': '', + 'question_mode': '', + }]}, + 'n_dropped_nat': 0, + }] + cd._summarizeFollowupThemes(quiz_blocks, client=None, model='m') + assert captured_modes == ['explain'] + + def test_real_mode_is_preserved(self, monkeypatch): + """A real 'draw' value passes through (lowercased) and isn't overridden by the fallback.""" + import canvigator_digest as cd + + captured_modes = [] + + def _fake_chat(client, **kwargs): + user_msg = kwargs['messages'][-1]['content'] + for line in user_msg.splitlines(): + if line.startswith('Mode: '): + captured_modes.append(line.split('Mode: ', 1)[1]) + return {'message': {'content': ''}} + + monkeypatch.setattr(cd, '_chat_with_retry', _fake_chat) + + quiz_blocks = [{ + 'quiz_id': '5', 'quiz_name': 'quiz1', + 'rows_by_question': {7: [{ + 'result': 'fail', 'confidence': 'high', + 'transcript': '', 'feedback': 'f', + 'criteria_evaluations': '', + 'question_mode': 'DRAW', + }]}, + 'n_dropped_nat': 0, + }] + cd._summarizeFollowupThemes(quiz_blocks, client=None, model='m') + assert captured_modes == ['draw'] + + class TestBuildDigestPriorities: """Tests for canvigator_digest._buildDigestPriorities."""