diff --git a/canvigator_course.py b/canvigator_course.py index 74030a6..1f4ed0e 100644 --- a/canvigator_course.py +++ b/canvigator_course.py @@ -96,8 +96,9 @@ def sendAllQuizReminders(self, dry_run=False): logger.warning(msg) continue - print(" Fetching latest submissions...") - quiz.getAllSubmissionsAndEvents() + if not quiz._tryLoadRecentSubmissions(): + print(" Fetching latest submissions...") + quiz.getAllSubmissionsAndEvents() subs_by_q_df = quiz.all_subs_by_question events_df = quiz.all_subs_and_events quiz_scores = quiz._buildQuizScores() diff --git a/canvigator_help.py b/canvigator_help.py index 35766da..37c5e9d 100644 --- a/canvigator_help.py +++ b/canvigator_help.py @@ -466,7 +466,10 @@ "message; imperfect-score students get a bulleted list of the " "questions they missed on their most recent attempt. With --all, " "sends ONE consolidated message per student covering every " - "published, future-due quiz." + "published, future-due quiz. If the quiz's submission CSVs were " + "written within the last 10 minutes, they are reused instead of " + "refetching from Canvas — making the typical dry-run-then-real " + "workflow fast on the second run." ), 'prerequisites': [ "get-quiz-questions --tag must have produced " @@ -499,10 +502,16 @@ 'description': ( "Send the instructor-selected open-ended follow-up question (the " "row marked selected_question=1 in *_open_ended_*.csv) via Canvas " - "message to each student who missed the corresponding original " - "quiz question on their latest attempt. Mode-aware instructions: " - "'explain' asks for a voice recording, 'draw' asks for an attached " - "photo. Uses force_new=True so the thread is dedicated." + "message to every student who has attempted the quiz. The intro " + "sentence is tailored to whether the student got the corresponding " + "original question right on their latest attempt ('nice job, here " + "is a more challenging follow-up to confirm mastery') or wrong " + "('here is a follow-up to reinforce your understanding'). " + "Mode-aware instructions: 'explain' asks for a voice recording, " + "'draw' asks for an attached photo. Uses force_new=True so the " + "thread is dedicated. Reuses the quiz's submission CSVs if they " + "were written within the last 10 minutes (e.g. just after a " + "send-quiz-reminder run) instead of refetching from Canvas." ), 'prerequisites': [ "get-quiz-questions --tag must have produced " @@ -575,12 +584,14 @@ 'send-follow-up-assessments': { 'description': ( "For every row in the persistent *_followup_assessments.csv with " - "sent_assessment=0 and a non-empty 'feedback' value, post the " - "feedback as a reply on the existing follow-up conversation using " - "the row's conversation_id. On success, sets sent_assessment=1 and " - "stamps sent_at. The instructor edits the 'feedback' column " - "between assess-replies and this task — feedback already sent is " - "never overwritten." + "sent_assessment=0 and a non-empty 'feedback' value, preview the " + "feedback and prompt the instructor [y/N] before posting it as a " + "reply on the existing follow-up conversation (using the row's " + "conversation_id). Default is SKIP — only an exact 'y' sends. " + "On a confirmed send, sets sent_assessment=1 and stamps sent_at. " + "Skipped rows stay at sent_assessment=0 so they can be revisited. " + "The instructor edits the 'feedback' column between assess-replies " + "and this task — feedback already sent is never overwritten." ), 'prerequisites': [ "assess-replies must have produced " diff --git a/canvigator_quiz.py b/canvigator_quiz.py index 0d2e5a9..9427c84 100644 --- a/canvigator_quiz.py +++ b/canvigator_quiz.py @@ -42,6 +42,31 @@ def _parse_points_possible_from_student_analysis(csv_path): return points_possible +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. + + Looks up the latest dated ``*_all_submissions_*.csv``, + ``*_all_subs_by_question_*.csv``, and ``*_all_subs_and_events_*.csv`` for + the given quiz. Returns the tuple ``(submissions, by_q, events)`` only when + all three exist AND each file's mtime is within ``max_age_minutes``. Pure + disk inspection — no DataFrame loading or side effects. Lets the + dry-run-then-real ``send-quiz-reminder`` workflow skip a Canvas refetch. + """ + prefix_id = f"{quiz_prefix}{quiz_id}" + paths = ( + find_latest_csv(data_path, f"{prefix_id}_all_submissions_"), + find_latest_csv(data_path, f"{prefix_id}_all_subs_by_question_"), + find_latest_csv(data_path, f"{prefix_id}_all_subs_and_events_"), + ) + if any(p is None for p in paths): + return None + + cutoff_ts = (datetime.now() - timedelta(minutes=max_age_minutes)).timestamp() + if not all(p.stat().st_mtime >= cutoff_ts for p in paths): + return None + return paths + + def _annotate_stats(ax, mean, std): """Render a small mean/sd text box in the upper-left of a matplotlib axis.""" text = f'mean: {_fmt_stat(mean)}\nsd: {_fmt_stat(std)}' @@ -719,9 +744,11 @@ def sendQuizReminders(self, dry_run=False): # Fail fast with a clear instruction if not. question_info = self._loadQuestionInfo() - # Refresh submission data — may be minutes old, so always re-fetch. - print("\nFetching latest submissions for missed-question details...") - self.getAllSubmissionsAndEvents() + # Refresh submission data, reusing CSVs from a recent run when present + # (the dry-run-then-real workflow re-runs minutes apart). + if not self._tryLoadRecentSubmissions(): + print("\nFetching latest submissions for missed-question details...") + self.getAllSubmissionsAndEvents() subs_by_q_df = self.all_subs_by_question events_df = self.all_subs_and_events @@ -821,14 +848,15 @@ def _sendOrPreviewMessages(self, messages, subject_str, quiz_name, points_possib _sendOrPreviewMessages(messages, subject_str, header_label) def sendFollowUpQuestions(self, dry_run=False): - """Send the instructor-selected open-ended follow-up question to students who missed it. + """Send the instructor-selected open-ended follow-up question to every student who attempted the quiz. Picks the first row in the *_open_ended_*.csv with `selected_question == 1` as the question to send (the instructor curates the choice offline). - Recipients are still students who missed the corresponding original quiz - question on their latest attempt. Requires that 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. + Every student who has at least one attempt receives the follow-up — the + message wording differs based on whether their latest attempt got the + corresponding original question right or wrong. Requires that + 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. """ quiz_name = self.canvas_quiz.title @@ -860,22 +888,28 @@ def sendFollowUpQuestions(self, dry_run=False): print(f" Mode: {question_mode}") print(f" Follow-up: {open_ended_text[:120]}{'...' if len(open_ended_text) > 120 else ''}") - # Refresh submission data so the recipient list reflects the latest attempts - print("\nFetching latest submissions to identify students who missed this question...") - self.getAllSubmissionsAndEvents() + # Refresh submission data so the recipient list reflects the latest attempts, + # reusing CSVs from a recent run when present (e.g. send-quiz-reminder a few minutes ago). + if not self._tryLoadRecentSubmissions(): + print("\nFetching latest submissions to identify students who missed this question...") + self.getAllSubmissionsAndEvents() subs_by_q_df = self.all_subs_by_question if subs_by_q_df is None or subs_by_q_df.empty: print("No submission data available — cannot determine recipients.") return - # Build the list of students who missed this question on their latest attempt - students_who_missed = self._findStudentsWhoMissed(selected_qid, subs_by_q_df, question_info) + # Classify every attempter as 'missed' or 'correct' on this question + classification = self._classifyStudentsByQuestionResult(selected_qid, subs_by_q_df, question_info) - if not students_who_missed: - print("No students missed this question on their latest attempt — no follow-up needed!") + if not classification: + print("No students have attempted this quiz yet — no follow-up to send.") return + n_missed = sum(1 for v in classification.values() if v == 'missed') + n_correct = sum(1 for v in classification.values() if v == 'correct') + print(f" Sending follow-up to {len(classification)} students ({n_correct} correct, {n_missed} missed).") + # Compose messages if question_mode == 'draw': instructions = ( @@ -899,24 +933,36 @@ def sendFollowUpQuestions(self, dry_run=False): enrolled = self.canvas_course.students enrolled_map = {s['id']: s['name'] for s in enrolled} + tail = ( + f"{open_ended_text}\n\n" + f"{instructions}\n\n" + f"You can reply as many times as you'd like — only your most recent " + f"response will be assessed.\n\n" + f"NOTE: This is an auto-generated message, please let me know if you " + f"have any questions/concerns/suggestions about it." + ) + messages = [] - for student_id in students_who_missed: + for student_id, status in classification.items(): student_name = enrolled_map.get(student_id) if student_name is None: continue first_name = student_name.split()[0] - message_str = ( - f"Hello {first_name}, based on your recent attempt on {quiz_name}, " - f"here is a follow-up question to help reinforce your understanding " - f"of the topic ({keywords}):\n\n" - f"{open_ended_text}\n\n" - f"{instructions}\n\n" - f"You can reply as many times as you'd like — only your most recent " - f"response will be assessed.\n\n" - f"NOTE: This is an auto-generated message, please let me know if you " - f"have any questions/concerns/suggestions about it." - ) - messages.append((student_id, student_name, message_str, f"missed Q{position}")) + if status == 'correct': + intro = ( + f"Hello {first_name}, nice job on {quiz_name}! Here is a slightly " + f"more challenging follow-up question corresponding to one that " + f"you answered correctly — this will be good practice to ensure " + f"you have mastered the topic ({keywords})" + ) + else: + intro = ( + f"Hello {first_name}, based on your recent attempt on {quiz_name}, " + f"here is a follow-up question to help reinforce your understanding " + f"of the topic ({keywords})" + ) + message_str = f"{intro}:\n\n{tail}" + messages.append((student_id, student_name, message_str, f"{status} Q{position}")) if dry_run: self._sendOrPreviewMessages(messages, subject_str, quiz_name, self.canvas_quiz.points_possible) @@ -935,22 +981,29 @@ def _interactiveSend(self, messages, subject_str, quiz_name, points_possible): header_label = f"Quiz: {quiz_name} ({points_possible} points possible)" return _interactiveSend(self.canvas, messages, subject_str, header_label) - def _findStudentsWhoMissed(self, question_id, subs_by_q_df, question_info): - """Return a list of student IDs who scored below points_possible on the given question in their latest attempt.""" + def _classifyStudentsByQuestionResult(self, question_id, subs_by_q_df, question_info): + """Return ``{student_id: 'missed' | 'correct'}`` for every student who attempted the quiz. + + Classification is by the student's **latest** attempt on the given question: + ``'missed'`` if ``points < points_possible``, ``'correct'`` otherwise. + Students with no attempts are absent from the result entirely (the + follow-up question presupposes they saw the original). + """ pp = question_info.get(question_id, {}).get('points_possible') if pp is None: - return [] + return {} max_attempt_per_student = subs_by_q_df.groupby('id')['attempt'].transform('max') latest_attempts = subs_by_q_df[subs_by_q_df['attempt'] == max_attempt_per_student] q_rows = latest_attempts[latest_attempts['question_id'] == question_id] - missed_ids = [] + result = {} for _, row in q_rows.iterrows(): points = row.get('points') - if points is not None and pd.notna(points) and float(points) < float(pp): - missed_ids.append(row['id']) - return missed_ids + if points is None or pd.isna(points): + continue + result[row['id']] = 'missed' if float(points) < float(pp) else 'correct' + return result def _loadOpenEndedQuestions(self): """Load the latest *_open_ended_*.csv and return a dict keyed by question_id. @@ -1544,6 +1597,8 @@ 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: + 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): @@ -1589,7 +1644,9 @@ def _sendOneAssessment(self, df, idx, convo_id, row, dry_run): """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 or send failure. + 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. """ student_name = row.get('student_name', '?') feedback = str(row['feedback']).strip() @@ -1601,6 +1658,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 choice != 'y': + print(f" Skipped {student_name}.") + return False try: convo = self.canvas.get_conversation(convo_id, auto_mark_as_read=False) convo.add_message(message_body) @@ -2292,6 +2353,32 @@ def _buildSubByQuestionRow(self, user_id, attempt_num, q_idx, qdata): 'correct': qdata['correct'], } + def _tryLoadRecentSubmissions(self, max_age_minutes=10): + """Load cached submission CSVs if all three are recent; return ``True`` on hit. + + On a cache hit, populates ``self.all_subs_by_question`` and + ``self.all_subs_and_events`` from disk so the caller can skip + ``getAllSubmissionsAndEvents``. The third CSV (``all_submissions``) + is checked for freshness but not loaded — it has no in-memory + consumer in the reminder flow. + """ + paths = _findRecentSubmissionCsvs( + self.config.data_path, + self.config.quiz_prefix, + self.canvas_quiz.id, + max_age_minutes=max_age_minutes, + ) + if paths is None: + return False + _submissions_csv, by_q_csv, events_csv = paths + age_secs = int(datetime.now().timestamp() - min(p.stat().st_mtime for p in paths)) + msg = f"Reusing submissions fetched {age_secs}s ago (within {max_age_minutes}-min cache)." + print(msg) + logger.info(msg) + self.all_subs_by_question = pd.read_csv(by_q_csv) + self.all_subs_and_events = pd.read_csv(events_csv) + return True + def getAllSubmissionsAndEvents(self): """Collect per-attempt submission history and events into three CSVs.""" quiz_takers = self.quiz_df[['name', 'id']].copy() diff --git a/test_canvigator.py b/test_canvigator.py index 465b277..8014ee2 100644 --- a/test_canvigator.py +++ b/test_canvigator.py @@ -1080,17 +1080,17 @@ def _make_quiz_stub(): return CanvigatorQuiz -class TestFindStudentsWhoMissed: - """Tests for CanvigatorQuiz._findStudentsWhoMissed.""" +class TestClassifyStudentsByQuestionResult: + """Tests for CanvigatorQuiz._classifyStudentsByQuestionResult.""" def _call(self, question_id, subs_rows, question_info): - """Helper to call _findStudentsWhoMissed without a full quiz instance.""" + """Helper to call the classifier without a full quiz instance.""" cls = _make_quiz_stub() df = _make_subs_by_question_df(subs_rows) - return cls._findStudentsWhoMissed(None, question_id, df, question_info) + return cls._classifyStudentsByQuestionResult(None, question_id, df, question_info) - def test_returns_students_who_missed(self): - """Only students who scored below points_possible are returned.""" + def test_classifies_correct_and_missed(self): + """Each attempter is labeled 'missed' (points < pp) or 'correct' (points == pp).""" question_info = {100: {'position': 1, 'keywords': 'topic a', 'points_possible': 1.0}} rows = [ ('A', 1, 1, 1, 100, 0.0, 1.0, False), @@ -1098,26 +1098,39 @@ def test_returns_students_who_missed(self): ('C', 3, 1, 1, 100, 0.5, 1.0, False), ] result = self._call(100, rows, question_info) - assert sorted(result) == [1, 3] + assert result == {1: 'missed', 2: 'correct', 3: 'missed'} def test_uses_latest_attempt(self): - """Student who fixed the question on a later attempt is excluded.""" + """A later attempt overrides the earlier one (fixed on retry → 'correct').""" question_info = {100: {'position': 1, 'keywords': 'topic a', 'points_possible': 1.0}} rows = [ ('A', 1, 1, 1, 100, 0.0, 1.0, False), ('A', 1, 2, 1, 100, 1.0, 1.0, True), ] result = self._call(100, rows, question_info) - assert result == [] + assert result == {1: 'correct'} - def test_empty_when_no_misses(self): - """Returns empty list when all students scored perfectly.""" + def test_all_correct(self): + """When every latest attempt scored perfectly, every student is 'correct'.""" question_info = {100: {'position': 1, 'keywords': 'topic a', 'points_possible': 1.0}} rows = [ ('A', 1, 1, 1, 100, 1.0, 1.0, True), + ('B', 2, 1, 1, 100, 1.0, 1.0, True), ] result = self._call(100, rows, question_info) - assert result == [] + assert result == {1: 'correct', 2: 'correct'} + + def test_question_missing_from_info_returns_empty(self): + """Without a points_possible entry the classifier can't label anyone.""" + rows = [('A', 1, 1, 1, 100, 1.0, 1.0, True)] + result = self._call(100, rows, question_info={}) + assert result == {} + + def test_no_attempts_returns_empty(self): + """Students who never attempted the quiz are absent from the result.""" + question_info = {100: {'position': 1, 'keywords': 'topic a', 'points_possible': 1.0}} + result = self._call(100, [], question_info) + assert result == {} # --------------------------------------------------------------------------- @@ -3016,6 +3029,81 @@ def test_results_sorted_ascending_by_date(self, tmp_path): assert [p.name for p in result] == ['foo_20260424.csv', 'foo_20260427.csv', 'foo_20260501.csv'] +class TestFindRecentSubmissionCsvs: + """Tests for canvigator_quiz._findRecentSubmissionCsvs.""" + + SUFFIXES = ('all_submissions', 'all_subs_by_question', 'all_subs_and_events') + + def _write_csv(self, tmp_path, name, age_seconds=0): + """Write a CSV in ``tmp_path``; backdate its mtime by ``age_seconds`` if positive.""" + import os + import time + path = tmp_path / name + path.write_text('id\n1\n') + if age_seconds > 0: + ts = time.time() - age_seconds + os.utime(path, (ts, ts)) + + def _write_all_three(self, tmp_path, prefix='quiz', quiz_id=123, date_str='20260510', age_seconds=0): + """Write all three submission CSVs for ``prefix{quiz_id}`` with the given mtime offset.""" + for suffix in self.SUFFIXES: + self._write_csv(tmp_path, f"{prefix}{quiz_id}_{suffix}_{date_str}.csv", age_seconds=age_seconds) + + def test_all_three_fresh_returns_paths(self, tmp_path): + """When all three CSVs exist with current mtimes, the helper returns a 3-tuple of Paths.""" + from canvigator_quiz import _findRecentSubmissionCsvs + self._write_all_three(tmp_path) + result = _findRecentSubmissionCsvs(tmp_path, 'quiz', 123, max_age_minutes=10) + assert result is not None + assert len(result) == 3 + for p, suffix in zip(result, self.SUFFIXES): + assert suffix in p.name + + def test_missing_one_returns_none(self, tmp_path): + """If any of the three CSVs is absent, the helper returns None.""" + from canvigator_quiz import _findRecentSubmissionCsvs + self._write_csv(tmp_path, 'quiz123_all_submissions_20260510.csv') + self._write_csv(tmp_path, 'quiz123_all_subs_by_question_20260510.csv') + # all_subs_and_events deliberately missing + assert _findRecentSubmissionCsvs(tmp_path, 'quiz', 123, max_age_minutes=10) is None + + def test_one_stale_returns_none(self, tmp_path): + """A single file outside the freshness window invalidates the cache.""" + from canvigator_quiz import _findRecentSubmissionCsvs + self._write_csv(tmp_path, 'quiz123_all_submissions_20260510.csv') + self._write_csv(tmp_path, 'quiz123_all_subs_by_question_20260510.csv') + # 15 minutes is past the 10-minute window + self._write_csv(tmp_path, 'quiz123_all_subs_and_events_20260510.csv', age_seconds=15 * 60) + assert _findRecentSubmissionCsvs(tmp_path, 'quiz', 123, max_age_minutes=10) is None + + def test_all_stale_returns_none(self, tmp_path): + """All-stale CSVs return None.""" + from canvigator_quiz import _findRecentSubmissionCsvs + self._write_all_three(tmp_path, age_seconds=20 * 60) + assert _findRecentSubmissionCsvs(tmp_path, 'quiz', 123, max_age_minutes=10) is None + + def test_respects_quiz_prefix(self, tmp_path): + """A non-default quiz_prefix is honored when looking up files.""" + from canvigator_quiz import _findRecentSubmissionCsvs + self._write_all_three(tmp_path, prefix='midterm') + # Default prefix should miss + assert _findRecentSubmissionCsvs(tmp_path, 'quiz', 123, max_age_minutes=10) is None + # Matching prefix should hit + result = _findRecentSubmissionCsvs(tmp_path, 'midterm', 123, max_age_minutes=10) + assert result is not None + + def test_picks_latest_dated_per_pattern(self, tmp_path): + """When multiple dated copies exist, the helper inherits ``find_latest_csv``'s newest-wins semantics.""" + from canvigator_quiz import _findRecentSubmissionCsvs + # Older copies are stale; newer copies are fresh — newest dated should win and pass freshness. + self._write_all_three(tmp_path, date_str='20260401', age_seconds=60 * 60) + self._write_all_three(tmp_path, date_str='20260510') + result = _findRecentSubmissionCsvs(tmp_path, 'quiz', 123, max_age_minutes=10) + assert result is not None + for p in result: + assert '20260510' in p.name + + class TestLoadQuizMisses: """Tests for canvigator_digest._loadQuizMisses."""