fix: seals after changes

src/authoring/select.rs

@@ -479,6 +479,7 @@ pub fn to_record(
             credit_overrides: BTreeMap::new(),
             dropped: false,
             dropped_as: None,
+            dropped_before_printing: false,
         });
     }
 

src/data/decode.rs

@@ -81,6 +81,11 @@ pub enum Numbering {
     Recorded,
 }
 
+/// The form marker put on a row that could not be translated, so it can be
+/// removed after the borrow on `set.rows` ends. No real form id can collide
+/// with it: form ids come from the record and are short labels like `A`.
+const UNMAPPED: &str = "\u{1f}unmapped";
+
 /// One question's mapping on one form.
 #[derive(Debug, Clone)]
 pub struct QuestionMap {
@@ -354,6 +359,11 @@ impl FormDecoder {
         self.by_position.len()
     }
 
+    /// Every recorded question number this form carries.
+    pub fn numbers(&self) -> impl Iterator<Item = u32> + '_ {
+        self.by_position.values().map(|q| q.number)
+    }
+
     /// Whether the form prints nothing, which means the record is empty.
     pub fn is_empty(&self) -> bool {
         self.by_position.is_empty()
@@ -572,9 +582,17 @@ pub fn form_warning(claimed: &str, fits: &[FormFit]) -> Option<String> {
 pub fn apply(set: &mut ResponseSet, decoder: &FormDecoder, numbering: Numbering) -> Vec<String> {
     let mut warnings = Vec::new();
     let mut unmapped_positions: BTreeSet<u32> = BTreeSet::new();
+    let mut colliding_positions: BTreeSet<u32> = BTreeSet::new();
     let mut unmapped_letters: BTreeSet<String> = BTreeSet::new();
     let mut translated = 0usize;
 
+    // Numbers this form really uses. An untranslated row whose raw number is one
+    // of these would silently masquerade as that question, and two rows would
+    // then share a number: one the student's answer to it, one an answer to
+    // something else entirely. Nothing downstream can tell them apart, so the
+    // collision has to be caught here.
+    let recorded: BTreeSet<u32> = decoder.numbers().collect();
+
     for row in &mut set.rows {
         let belongs = row
             .form
@@ -587,6 +605,13 @@ pub fn apply(set: &mut ResponseSet, decoder: &FormDecoder, numbering: Numbering)
 
         let Some(map) = decoder.lookup(row.item_number, numbering) else {
             unmapped_positions.insert(row.item_number);
+            if recorded.contains(&row.item_number) {
+                colliding_positions.insert(row.item_number);
+                // Marked so the row can be discarded below. Attributing it to
+                // the question that legitimately holds this number would corrupt
+                // that question's statistics.
+                row.form = Some(UNMAPPED.to_string());
+            }
             continue;
         };
 
@@ -615,9 +640,28 @@ pub fn apply(set: &mut ResponseSet, decoder: &FormDecoder, numbering: Numbering)
     if !unmapped_positions.is_empty() {
         let list: Vec<String> = unmapped_positions.iter().map(|n| n.to_string()).collect();
         warnings.push(format!(
-            "form {}: question(s) {} are in the export but not on this form; they were left \
+            "form {}: question(s) {} are in the export but not on this form ({} printed). The \
-             untranslated",
+             export may have been taken before a question was dropped, or from a different \
+             form.",
             decoder.form,
+            list.join(", "),
+            decoder.len()
+        ));
+    }
+    if !colliding_positions.is_empty() {
+        let list: Vec<String> = colliding_positions.iter().map(|n| n.to_string()).collect();
+        let discarded = set.rows.len();
+        set.rows
+            .retain(|row| row.form.as_deref() != Some(UNMAPPED));
+        warnings.push(format!(
+            "form {}: {} response(s) at position(s) {} could not be translated, and their raw \
+             numbers are numbers this form does use. Keeping them would have given those \
+             questions two different answers each, so they were discarded. This is the shape of \
+             an export made before a question was dropped: re-export the responses from the \
+             administration you sealed, or re-run with --recorded-numbers if the export already \
+             carries recorded numbers.",
+            decoder.form,
+            discarded - set.rows.len(),
             list.join(", ")
         ));
     }

src/export/practice.rs

@@ -223,7 +223,7 @@ pub fn render(catalog: &Catalog, record: &AssessmentFile, opts: &Options) -> Res
     let mut printed_stimulus: Option<String> = None;
 
     let layout = select::layout(record, &opts.form);
-    for (position, placement) in layout.iter().filter(|p| !p.dropped).enumerate() {
+    for (position, placement) in layout.iter().filter(|p| p.was_printed()).enumerate() {
         let entry = catalog.require(&placement.item)?;
         let item = &entry.item;
         let number = position + 1;
@@ -602,6 +602,7 @@ items:
                     credit_overrides: Default::default(),
                     dropped: false,
                     dropped_as: None,
+                    dropped_before_printing: false,
                 },
                 Placement {
                     number: 2,
@@ -616,6 +617,7 @@ items:
                     credit_overrides: Default::default(),
                     dropped: false,
                     dropped_as: None,
+                    dropped_before_printing: false,
                 },
             ],
         }

src/export/qti.rs

@@ -1398,6 +1398,7 @@ items:
                     credit_overrides: Default::default(),
                     dropped: false,
                     dropped_as: None,
+                    dropped_before_printing: false,
                 },
                 Placement {
                     number: 2,
@@ -1412,6 +1413,7 @@ items:
                     credit_overrides: Default::default(),
                     dropped: false,
                     dropped_as: None,
+                    dropped_before_printing: false,
                 },
             ],
         }

src/export/site.rs

@@ -867,6 +867,7 @@ items:
                     credit_overrides: Default::default(),
                     dropped: false,
                     dropped_as: None,
+                    dropped_before_printing: false,
                 },
                 Placement {
                     number: 2,
@@ -881,6 +882,7 @@ items:
                     credit_overrides: Default::default(),
                     dropped: false,
                     dropped_as: None,
+                    dropped_before_printing: false,
                 },
             ],
         }

src/export/typst.rs

@@ -440,6 +440,7 @@ mod tests {
                     credit_overrides: Default::default(),
                     dropped: true,
                     dropped_as: None,
+                    dropped_before_printing: false,
                 },
                 Placement {
                     number: 2,
@@ -454,12 +455,13 @@ mod tests {
                     credit_overrides: Default::default(),
                     dropped: false,
                     dropped_as: None,
+                    dropped_before_printing: false,
                 },
             ],
         };
         let printable: Vec<u32> = select::layout(&record, &Options::default().form)
             .into_iter()
-            .filter(|p| !p.dropped)
+            .filter(|p| p.was_printed())
             .map(|p| p.number)
             .collect();
         assert_eq!(printable, vec![2]);

src/export/typst/templates/cohort-report.typ

@@ -1088,12 +1088,17 @@
 
   #explain[
     The full breakdown for every flagged question: what the flags mean, and who
-    chose what. The r column beside each option is the correlation between
+    chose what. In question order, so you can find one by its number; the lists
-    choosing that option and scoring well on the rest of the exam, which is how a
+    above are the same questions sorted by what they ask of you. The r column
-    defensible distractor announces itself.
+    beside each option is the correlation between choosing that option and
+    scoring well on the rest of the exam, which is how a defensible distractor
+    announces itself.
   ]
 
-  #for q in revise [
+  // Question order here, not triage order. The lists above are for deciding
+  // what to do; this section is for looking one question up while you do it,
+  // and a reader with a number in hand should not have to scan every entry.
+  #for q in revise.sorted(key: q => q.number) [
     #block(breakable: false, above: entry-gap, width: 100%)[
       #{
         let parts = (

src/model/assessment.rs

@@ -305,6 +305,28 @@ pub struct Placement {
     /// contribute.
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub dropped_as: Option<DropStyle>,
+    /// Set only when the item was pulled *before* the paper was printed.
+    ///
+    /// `dropped` on its own means what its own documentation says: the item was
+    /// printed, students answered it, and it was then taken out of scoring.
+    /// Such an item keeps its printed position, because it occupied one on the
+    /// page the students held, and the responses that come back are numbered
+    /// around it.
+    ///
+    /// An item pulled before printing never occupied a position, so every later
+    /// question moves up one. That case has to be distinguished, and it cannot
+    /// be inferred: both look identical in the record. Getting it wrong is not
+    /// a cosmetic error. Sealing a post-administration drop as if it had never
+    /// been printed renumbers every question after it, so each response is
+    /// attributed to the wrong item, the statistics for those items are
+    /// computed from answers to different questions, and nothing in the output
+    /// looks obviously wrong.
+    ///
+    /// Practically: leave this alone when you discover a bad question after the
+    /// exam, which is the common case. Set it when you cut a question from the
+    /// draft and reprinted.
+    #[serde(default, skip_serializing_if = "is_false")]
+    pub dropped_before_printing: bool,
 }
 
 /// How a dropped item was handled on the grading platform.
@@ -327,6 +349,19 @@ impl Placement {
     pub fn dropped_with_credit(&self) -> bool {
         self.dropped && self.dropped_as == Some(DropStyle::FullCredit)
     }
+
+    /// Whether this placement occupied a printed position on the paper.
+    ///
+    /// Everything that lays out a page or reads a page back goes through this,
+    /// so the printed form, the seal, and the decoder cannot disagree about
+    /// which question sat where.
+    ///
+    /// # Returns
+    ///
+    /// `true` unless the item was pulled before printing.
+    pub fn was_printed(&self) -> bool {
+        !(self.dropped && self.dropped_before_printing)
+    }
 }
 
 impl AssessmentFile {

src/model/seal.rs

@@ -518,12 +518,13 @@ fn sealed_item(
 fn sealed_form(catalog: &Catalog, record: &AssessmentFile, form: &Form) -> Result<SealedForm> {
     let mut questions = Vec::new();
 
-    // Dropped placements are not printed, so they take no printed position. A
+    // Only an item pulled before printing takes no printed position. An item
-    // drop recorded before sealing therefore shifts every later position, exactly
+    // dropped from scoring after the exam was on the page and keeps its place,
-    // as it shifts them on the page.
+    // so re-sealing after a drop describes the same paper the students held
+    // rather than renumbering everything after it.
     let printed: Vec<Placement> = select::layout(record, form)
         .into_iter()
-        .filter(|p| !p.dropped)
+        .filter(|p| p.was_printed())
         .collect();
 
     for (index, placement) in printed.iter().enumerate() {