Fix Cards with Missing Last Review Time During Database Check (#4237)

* Fix Cards with Missing Last Review Time During Database Check

* clippy

* Apply suggestions from code review

Co-authored-by: Luc Mcgrady <lucmcgrady@gmail.com>

* Apply suggestions from code review

Co-authored-by: user1823 <92206575+user1823@users.noreply.github.com>

* Add is_reset method to RevlogEntry and update scheduling logic

This commit introduces the `is_reset` method to the `RevlogEntry` struct, which identifies entries representing reset operations. Additionally, the scheduling logic in `memory_state.rs` and `params.rs` has been updated to utilize this new method, ensuring that reset entries are handled correctly during review scheduling.

* Implement is_cramming method in RevlogEntry and update scheduling logic

This commit adds the `is_cramming` method to the `RevlogEntry` struct, which identifies entries representing cramming operations. The scheduling logic in `params.rs` has been updated to utilize this new method, improving the clarity and maintainability of the code.

* Refactor rating logic in RevlogEntry and update related scheduling functions

This commit introduces a new `has_rating` method in the `RevlogEntry` struct to encapsulate the logic for checking if an entry has a rating. The scheduling logic in `params.rs` and the calculation of normal answer counts in `card.rs` have been updated to use this new method, enhancing code clarity and maintainability.

* update revlog test helper function to assign button_chosen correctly

* Refactor card property fixing logic to use CardFixStats struct

* Add one-way sync trigger for last review time updates in dbcheck

* Update documentation for is_reset method in RevlogEntry to clarify ease_factor condition

* Apply suggestions from code review

Co-authored-by: user1823 <92206575+user1823@users.noreply.github.com>

* Minor wording tweak

---------

Co-authored-by: Luc Mcgrady <lucmcgrady@gmail.com>
Co-authored-by: user1823 <92206575+user1823@users.noreply.github.com>

ftl/core/database-check.ftl

@@ -5,6 +5,11 @@ database-check-card-properties =
         [one] Fixed { $count } invalid card property.
        *[other] Fixed { $count } invalid card properties.
     }
+database-check-card-last-review-time-empty =
+    { $count ->
+        [one] Added last review time to { $count } card.
+       *[other] Added last review time to { $count } cards.
+    }
 database-check-missing-templates =
     { $count ->
         [one] Deleted { $count } card with missing template.

rslib/src/dbcheck.rs

@@ -24,6 +24,7 @@ use crate::notetype::NotetypeId;
 use crate::notetype::NotetypeKind;
 use crate::prelude::*;
 use crate::progress::ThrottlingProgressHandler;
+use crate::storage::card::CardFixStats;
 use crate::timestamp::TimestampMillis;
 use crate::timestamp::TimestampSecs;
 
@@ -40,6 +41,7 @@ pub struct CheckDatabaseOutput {
     notetypes_recovered: usize,
     invalid_utf8: usize,
     invalid_ids: usize,
+    card_last_review_time_empty: usize,
 }
 
 #[derive(Debug, Clone, Copy, Default)]
@@ -69,6 +71,11 @@ impl CheckDatabaseOutput {
         if self.card_properties_invalid > 0 {
             probs.push(tr.database_check_card_properties(self.card_properties_invalid));
         }
+        if self.card_last_review_time_empty > 0 {
+            probs.push(
+                tr.database_check_card_last_review_time_empty(self.card_last_review_time_empty),
+            );
+        }
         if self.cards_missing_note > 0 {
             probs.push(tr.database_check_card_missing_note(self.cards_missing_note));
         }
@@ -158,14 +165,25 @@ impl Collection {
 
     fn check_card_properties(&mut self, out: &mut CheckDatabaseOutput) -> Result<()> {
         let timing = self.timing_today()?;
-        let (new_cnt, other_cnt) = self.storage.fix_card_properties(
+        let CardFixStats {
+            new_cards_fixed,
+            other_cards_fixed,
+            last_review_time_fixed,
+        } = self.storage.fix_card_properties(
             timing.days_elapsed,
             TimestampSecs::now(),
             self.usn()?,
             self.scheduler_version() == SchedulerVersion::V1,
         )?;
-        out.card_position_too_high = new_cnt;
-        out.card_properties_invalid += other_cnt;
+        out.card_position_too_high = new_cards_fixed;
+        out.card_properties_invalid += other_cards_fixed;
+        out.card_last_review_time_empty = last_review_time_fixed;
+
+        // Trigger one-way sync if last_review_time was updated to avoid conflicts
+        if last_review_time_fixed > 0 {
+            self.set_schema_modified()?;
+        }
+
         Ok(())
     }
 

rslib/src/revlog/mod.rs

@@ -84,6 +84,42 @@ impl RevlogEntry {
         })
         .unwrap()
     }
+
+    /// Returns true if this entry represents a reset operation.
+    /// These entries are created when a card is reset using
+    /// [`Collection::reschedule_cards_as_new`].
+    /// The 0 value of `ease_factor` differentiates it
+    /// from entry created by [`Collection::set_due_date`] that has
+    /// `RevlogReviewKind::Manual` but non-zero `ease_factor`.
+    pub(crate) fn is_reset(&self) -> bool {
+        self.review_kind == RevlogReviewKind::Manual && self.ease_factor == 0
+    }
+
+    /// Returns true if this entry represents a cramming operation.
+    /// These entries are created when a card is reviewed in a
+    /// filtered deck with "Reschedule cards based on my answers
+    /// in this deck" disabled.
+    /// [`crate::scheduler::answering::CardStateUpdater::apply_preview_state`].
+    /// The 0 value of `ease_factor` distinguishes it from the entry
+    /// created when a card is reviewed before its due date in a
+    /// filtered deck with reschedule enabled or using Grade Now.
+    pub(crate) fn is_cramming(&self) -> bool {
+        self.review_kind == RevlogReviewKind::Filtered && self.ease_factor == 0
+    }
+
+    pub(crate) fn has_rating(&self) -> bool {
+        self.button_chosen > 0
+    }
+
+    /// Returns true if the review entry is not manually rescheduled and not
+    /// cramming. Used to filter out entries that shouldn't be considered
+    /// for statistics and scheduling.
+    pub(crate) fn has_rating_and_affects_scheduling(&self) -> bool {
+        // not rescheduled/set due date/reset
+        self.has_rating()
+            // not cramming
+            && !self.is_cramming()
+    }
 }
 
 impl Collection {

rslib/src/scheduler/fsrs/memory_state.rs

@@ -306,15 +306,15 @@ pub(crate) fn fsrs_items_for_memory_states(
         .collect()
 }
 
-struct LastRevlogInfo {
+pub(crate) struct LastRevlogInfo {
     /// Used to determine the actual elapsed time between the last time the user
     /// reviewed the card and now, so that we can determine an accurate period
     /// when the card has subsequently been rescheduled to a different day.
-    last_reviewed_at: Option<TimestampSecs>,
+    pub(crate) last_reviewed_at: Option<TimestampSecs>,
 }
 
-/// Return a map of cards to info about last review/reschedule.
-fn get_last_revlog_info(revlogs: &[RevlogEntry]) -> HashMap<CardId, LastRevlogInfo> {
+/// Return a map of cards to info about last review.
+pub(crate) fn get_last_revlog_info(revlogs: &[RevlogEntry]) -> HashMap<CardId, LastRevlogInfo> {
     let mut out = HashMap::new();
     revlogs
         .iter()
@@ -323,8 +323,10 @@ fn get_last_revlog_info(revlogs: &[RevlogEntry]) -> HashMap<CardId, LastRevlogIn
         .for_each(|(card_id, group)| {
             let mut last_reviewed_at = None;
             for e in group.into_iter() {
-                if e.button_chosen >= 1 {
+                if e.has_rating_and_affects_scheduling() {
                     last_reviewed_at = Some(e.id.as_secs());
+                } else if e.is_reset() {
+                    last_reviewed_at = None;
                 }
             }
             out.insert(card_id, LastRevlogInfo { last_reviewed_at });

rslib/src/scheduler/fsrs/params.rs

@@ -394,13 +394,13 @@ pub(crate) fn reviews_for_fsrs(
     let mut revlogs_complete = false;
     // Working backwards from the latest review...
     for (index, entry) in entries.iter().enumerate().rev() {
-        if entry.review_kind == RevlogReviewKind::Filtered && entry.ease_factor == 0 {
+        if entry.is_cramming() {
             continue;
         }
         // For incomplete review histories, initial memory state is based on the first
         // user-graded review after the cutoff date with interval >= 1d.
         let within_cutoff = entry.id.0 > ignore_revlogs_before.0;
-        let user_graded = matches!(entry.button_chosen, 1..=4);
+        let user_graded = entry.has_rating();
         let interday = entry.interval >= 1 || entry.interval <= -86400;
         if user_graded && within_cutoff && interday {
             first_user_grade_idx = Some(index);
@@ -409,10 +409,7 @@ pub(crate) fn reviews_for_fsrs(
         if user_graded && entry.review_kind == RevlogReviewKind::Learning {
             first_of_last_learn_entries = Some(index);
             revlogs_complete = true;
-        } else if matches!(
-            (entry.review_kind, entry.ease_factor),
-            (RevlogReviewKind::Manual, 0)
-        ) {
+        } else if entry.is_reset() {
             // Ignore entries prior to a `Reset` if a learning step has come after,
             // but consider revlogs complete.
             if first_of_last_learn_entries.is_some() {
@@ -472,16 +469,7 @@ pub(crate) fn reviews_for_fsrs(
     }
 
     // Filter out unwanted entries
-    entries.retain(|entry| {
-        !(
-            // set due date, reset or rescheduled
-            (entry.review_kind == RevlogReviewKind::Manual || entry.button_chosen == 0)
-            || // cram
-            (entry.review_kind == RevlogReviewKind::Filtered && entry.ease_factor == 0)
-            || // rescheduled
-            (entry.review_kind == RevlogReviewKind::Rescheduled)
-        )
-    });
+    entries.retain(|entry| entry.has_rating_and_affects_scheduling());
 
     // Compute delta_t for each entry
     let delta_ts = iter::once(0)
@@ -560,10 +548,14 @@ pub(crate) mod tests {
     }
 
     pub(crate) fn revlog(review_kind: RevlogReviewKind, days_ago: i64) -> RevlogEntry {
+        let button_chosen = match review_kind {
+            RevlogReviewKind::Manual | RevlogReviewKind::Rescheduled => 0,
+            _ => 3,
+        };
         RevlogEntry {
             review_kind,
             id: days_ago_ms(days_ago).into(),
-            button_chosen: 3,
+            button_chosen,
             interval: 1,
             ..Default::default()
         }

rslib/src/stats/card.rs

@@ -197,7 +197,7 @@ impl Collection {
 }
 
 fn average_and_total_secs_strings(revlog: &[RevlogEntry]) -> (f32, f32) {
-    let normal_answer_count = revlog.iter().filter(|r| r.button_chosen > 0).count();
+    let normal_answer_count = revlog.iter().filter(|r| r.has_rating()).count();
     let total_secs: f32 = revlog
         .iter()
         .map(|entry| (entry.taken_millis as f32) / 1000.0)

rslib/src/stats/graphs/retention.rs

@@ -53,10 +53,7 @@ impl GraphsContext {
         self.revlog
             .iter()
             .filter(|review| {
-                // not rescheduled/set due date/reset
-                review.button_chosen > 0
-                    // not cramming
-                    && (review.review_kind != RevlogReviewKind::Filtered || review.ease_factor != 0)
+                review.has_rating_and_affects_scheduling()
                     // cards with an interval ≥ 1 day
                     && (review.review_kind == RevlogReviewKind::Review
                         || review.last_interval <= -86400

rslib/src/storage/card/mod.rs

@@ -33,6 +33,7 @@ use crate::decks::DeckKind;
 use crate::error::Result;
 use crate::notes::NoteId;
 use crate::scheduler::congrats::CongratsInfo;
+use crate::scheduler::fsrs::memory_state::get_last_revlog_info;
 use crate::scheduler::queue::BuryMode;
 use crate::scheduler::queue::DueCard;
 use crate::scheduler::queue::DueCardKind;
@@ -42,6 +43,13 @@ use crate::timestamp::TimestampMillis;
 use crate::timestamp::TimestampSecs;
 use crate::types::Usn;
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) struct CardFixStats {
+    pub new_cards_fixed: usize,
+    pub other_cards_fixed: usize,
+    pub last_review_time_fixed: usize,
+}
+
 impl FromSql for CardType {
     fn column_result(value: ValueRef<'_>) -> result::Result<Self, FromSqlError> {
         if let ValueRef::Integer(i) = value {
@@ -365,7 +373,7 @@ impl super::SqliteStorage {
         mtime: TimestampSecs,
         usn: Usn,
         v1_sched: bool,
-    ) -> Result<(usize, usize)> {
+    ) -> Result<CardFixStats> {
         let new_cnt = self
             .db
             .prepare(include_str!("fix_due_new.sql"))?
@@ -390,7 +398,24 @@ impl super::SqliteStorage {
             .db
             .prepare(include_str!("fix_ordinal.sql"))?
             .execute(params![mtime, usn])?;
-        Ok((new_cnt, other_cnt))
+        let mut last_review_time_cnt = 0;
+        let revlog = self.get_all_revlog_entries_in_card_order()?;
+        let last_revlog_info = get_last_revlog_info(&revlog);
+        for (card_id, last_revlog_info) in last_revlog_info {
+            let card = self.get_card(card_id)?;
+            if let Some(mut card) = card {
+                if card.ctype != CardType::New && card.last_review_time.is_none() {
+                    card.last_review_time = last_revlog_info.last_reviewed_at;
+                    self.update_card(&card)?;
+                    last_review_time_cnt += 1;
+                }
+            }
+        }
+        Ok(CardFixStats {
+            new_cards_fixed: new_cnt,
+            other_cards_fixed: other_cnt,
+            last_review_time_fixed: last_review_time_cnt,
+        })
     }
 
     pub(crate) fn delete_orphaned_cards(&self) -> Result<usize> {