refactor: improve type option filter logic (#4652)

frontend/rust-lib/flowy-database2/src/entities/filter_entities/date_filter.rs

@@ -43,9 +43,8 @@ impl FromStr for DateFilterContentPB {
   }
 }
 
-#[derive(Debug, Clone, PartialEq, Eq, ProtoBuf_Enum)]
+#[derive(Debug, Clone, Default, PartialEq, Eq, ProtoBuf_Enum)]
 #[repr(u8)]
-#[derive(Default)]
 pub enum DateFilterConditionPB {
   #[default]
   DateIs = 0,

frontend/rust-lib/flowy-database2/src/entities/filter_entities/number_filter.rs

@@ -12,9 +12,8 @@ pub struct NumberFilterPB {
   pub content: String,
 }
 
-#[derive(Debug, Clone, PartialEq, Eq, ProtoBuf_Enum)]
+#[derive(Debug, Clone, Default, PartialEq, Eq, ProtoBuf_Enum)]
 #[repr(u8)]
-#[derive(Default)]
 pub enum NumberFilterConditionPB {
   #[default]
   Equal = 0,

frontend/rust-lib/flowy-database2/src/services/field/type_options/date_type_option/date_filter.rs

@@ -1,65 +1,108 @@
 use crate::entities::{DateFilterConditionPB, DateFilterPB};
-use chrono::NaiveDateTime;
+
+use chrono::{NaiveDate, NaiveDateTime};
+
+use super::DateCellData;
 
 impl DateFilterPB {
-  pub fn is_visible<T: Into<Option<i64>>>(&self, cell_timestamp: T) -> bool {
-    match cell_timestamp.into() {
-      None => DateFilterConditionPB::DateIsEmpty == self.condition,
-      Some(timestamp) => {
-        match self.condition {
-          DateFilterConditionPB::DateIsNotEmpty => {
-            return true;
+  /// Returns `None` if the DateFilterPB doesn't have the necessary data for
+  /// the condition. For example, `start` and `end` timestamps for
+  /// `DateFilterConditionPB::DateWithin`.
+  pub fn is_visible(&self, cell_data: &DateCellData) -> Option<bool> {
+    let strategy = match self.condition {
+      DateFilterConditionPB::DateIs => DateFilterStrategy::On(self.timestamp?),
+      DateFilterConditionPB::DateBefore => DateFilterStrategy::Before(self.timestamp?),
+      DateFilterConditionPB::DateAfter => DateFilterStrategy::After(self.timestamp?),
+      DateFilterConditionPB::DateOnOrBefore => DateFilterStrategy::OnOrBefore(self.timestamp?),
+      DateFilterConditionPB::DateOnOrAfter => DateFilterStrategy::OnOrAfter(self.timestamp?),
+      DateFilterConditionPB::DateWithIn => DateFilterStrategy::DateWithin {
+        start: self.start?,
+        end: self.end?,
       },
-          DateFilterConditionPB::DateIsEmpty => {
-            return false;
+      DateFilterConditionPB::DateIsEmpty => DateFilterStrategy::Empty,
+      DateFilterConditionPB::DateIsNotEmpty => DateFilterStrategy::NotEmpty,
+    };
+
+    Some(strategy.filter(cell_data))
+  }
+}
+
+#[inline]
+fn naive_date_from_timestamp(timestamp: i64) -> Option<NaiveDate> {
+  NaiveDateTime::from_timestamp_opt(timestamp, 0).map(|date_time: NaiveDateTime| date_time.date())
+}
+
+enum DateFilterStrategy {
+  On(i64),
+  Before(i64),
+  After(i64),
+  OnOrBefore(i64),
+  OnOrAfter(i64),
+  DateWithin { start: i64, end: i64 },
+  Empty,
+  NotEmpty,
+}
+
+impl DateFilterStrategy {
+  fn filter(self, cell_data: &DateCellData) -> bool {
+    match self {
+      DateFilterStrategy::On(expected_timestamp) => cell_data.timestamp.is_some_and(|timestamp| {
+        let cell_date = naive_date_from_timestamp(timestamp);
+        let expected_date = naive_date_from_timestamp(expected_timestamp);
+        cell_date == expected_date
+      }),
+      DateFilterStrategy::Before(expected_timestamp) => {
+        cell_data.timestamp.is_some_and(|timestamp| {
+          let cell_date = naive_date_from_timestamp(timestamp);
+          let expected_date = naive_date_from_timestamp(expected_timestamp);
+          cell_date < expected_date
+        })
       },
-          _ => {},
-        }
-
-        let cell_time = NaiveDateTime::from_timestamp_opt(timestamp, 0);
-        let cell_date = cell_time.map(|time| time.date());
-        match self.timestamp {
-          None => {
-            if self.start.is_none() {
-              return true;
-            }
-
-            if self.end.is_none() {
-              return true;
-            }
-
-            let start_time = NaiveDateTime::from_timestamp_opt(*self.start.as_ref().unwrap(), 0);
-            let start_date = start_time.map(|time| time.date());
-
-            let end_time = NaiveDateTime::from_timestamp_opt(*self.end.as_ref().unwrap(), 0);
-            let end_date = end_time.map(|time| time.date());
-
-            cell_date >= start_date && cell_date <= end_date
+      DateFilterStrategy::After(expected_timestamp) => {
+        cell_data.timestamp.is_some_and(|timestamp| {
+          let cell_date = naive_date_from_timestamp(timestamp);
+          let expected_date = naive_date_from_timestamp(expected_timestamp);
+          cell_date > expected_date
+        })
       },
-          Some(timestamp) => {
-            let expected_timestamp = NaiveDateTime::from_timestamp_opt(timestamp, 0);
-            let expected_date = expected_timestamp.map(|time| time.date());
-
-            // We assume that the cell_timestamp doesn't contain hours, just day.
-            match self.condition {
-              DateFilterConditionPB::DateIs => cell_date == expected_date,
-              DateFilterConditionPB::DateBefore => cell_date < expected_date,
-              DateFilterConditionPB::DateAfter => cell_date > expected_date,
-              DateFilterConditionPB::DateOnOrBefore => cell_date <= expected_date,
-              DateFilterConditionPB::DateOnOrAfter => cell_date >= expected_date,
-              _ => true,
-            }
+      DateFilterStrategy::OnOrBefore(expected_timestamp) => {
+        cell_data.timestamp.is_some_and(|timestamp| {
+          let cell_date = naive_date_from_timestamp(timestamp);
+          let expected_date = naive_date_from_timestamp(expected_timestamp);
+          cell_date <= expected_date
+        })
       },
-        }
+      DateFilterStrategy::OnOrAfter(expected_timestamp) => {
+        cell_data.timestamp.is_some_and(|timestamp| {
+          let cell_date = naive_date_from_timestamp(timestamp);
+          let expected_date = naive_date_from_timestamp(expected_timestamp);
+          cell_date >= expected_date
+        })
       },
+      DateFilterStrategy::DateWithin { start, end } => {
+        cell_data.timestamp.is_some_and(|timestamp| {
+          let cell_date = naive_date_from_timestamp(timestamp);
+          let expected_start_date = naive_date_from_timestamp(start);
+          let expected_end_date = naive_date_from_timestamp(end);
+          cell_date >= expected_start_date && cell_date <= expected_end_date
+        })
+      },
+      DateFilterStrategy::Empty => {
+        cell_data.timestamp.is_none() && cell_data.end_timestamp.is_none()
+      },
+      DateFilterStrategy::NotEmpty => cell_data.timestamp.is_some(),
     }
   }
 }
 
 #[cfg(test)]
 mod tests {
-  #![allow(clippy::all)]
   use crate::entities::{DateFilterConditionPB, DateFilterPB};
+  use crate::services::field::DateCellData;
+
+  fn to_cell_data(timestamp: i32) -> DateCellData {
+    DateCellData::new(timestamp as i64, false, false, "".to_string())
+  }
 
   #[test]
   fn date_filter_is_test() {
@@ -70,10 +113,11 @@ mod tests {
       start: None,
     };
 
-    for (val, visible) in vec![(1668387885, true), (1647251762, false)] {
-      assert_eq!(filter.is_visible(val as i64), visible);
+    for (val, visible) in [(1668387885, true), (1647251762, false)] {
+      assert_eq!(filter.is_visible(&to_cell_data(val)).unwrap(), visible);
     }
   }
+
   #[test]
   fn date_filter_before_test() {
     let filter = DateFilterPB {
@@ -83,8 +127,13 @@ mod tests {
       end: None,
     };
 
-    for (val, visible, msg) in vec![(1668387884, false, "1"), (1647251762, true, "2")] {
-      assert_eq!(filter.is_visible(val as i64), visible, "{}", msg);
+    for (val, visible, msg) in [(1668387884, false, "1"), (1647251762, true, "2")] {
+      assert_eq!(
+        filter.is_visible(&to_cell_data(val)).unwrap(),
+        visible,
+        "{}",
+        msg
+      );
     }
   }
 
@@ -97,8 +146,8 @@ mod tests {
       end: None,
     };
 
-    for (val, visible) in vec![(1668387884, true), (1668387885, true)] {
-      assert_eq!(filter.is_visible(val as i64), visible);
+    for (val, visible) in [(1668387884, true), (1668387885, true)] {
+      assert_eq!(filter.is_visible(&to_cell_data(val)).unwrap(), visible);
     }
   }
   #[test]
@@ -110,8 +159,8 @@ mod tests {
       end: None,
     };
 
-    for (val, visible) in vec![(1668387888, false), (1668531885, true), (0, false)] {
-      assert_eq!(filter.is_visible(val as i64), visible);
+    for (val, visible) in [(1668387888, false), (1668531885, true), (0, false)] {
+      assert_eq!(filter.is_visible(&to_cell_data(val)).unwrap(), visible);
     }
   }
 
@@ -124,12 +173,12 @@ mod tests {
       timestamp: None,
     };
 
-    for (val, visible, _msg) in vec![
+    for (val, visible, _msg) in [
       (1668272685, true, "11/13"),
       (1668359085, true, "11/14"),
       (1668704685, false, "11/18"),
     ] {
-      assert_eq!(filter.is_visible(val as i64), visible);
+      assert_eq!(filter.is_visible(&to_cell_data(val)).unwrap(), visible);
     }
   }
 
@@ -142,8 +191,16 @@ mod tests {
       timestamp: None,
     };
 
-    for (val, visible) in vec![(None, true), (Some(123), false)] {
-      assert_eq!(filter.is_visible(val), visible);
+    for (val, visible) in [(None, true), (Some(123), false)] {
+      assert_eq!(
+        filter
+          .is_visible(&DateCellData {
+            timestamp: val,
+            ..Default::default()
+          })
+          .unwrap(),
+        visible
+      );
     }
   }
 }

frontend/rust-lib/flowy-database2/src/services/field/type_options/date_type_option/date_type_option.rs

@@ -19,9 +19,6 @@ use crate::services::field::{
 };
 use crate::services::sort::SortCondition;
 
-/// The [DateTypeOption] is used by [FieldType::Date], [FieldType::LastEditedTime], and [FieldType::CreatedTime].
-/// So, storing the field type is necessary to distinguish the field type.
-/// Most of the cases, each [FieldType] has its own [TypeOption] implementation.
 #[derive(Clone, Debug, Serialize, Deserialize, Default)]
 pub struct DateTypeOption {
   pub date_format: DateFormat,
@@ -351,8 +348,7 @@ impl TypeOptionCellDataFilter for DateTypeOption {
     if !field_type.is_date() {
       return true;
     }
-
-    filter.is_visible(cell_data.timestamp)
+    filter.is_visible(cell_data).unwrap_or(true)
   }
 }
 

frontend/rust-lib/flowy-database2/src/services/field/type_options/number_type_option/number_filter.rs

@@ -5,32 +5,62 @@ use rust_decimal::Decimal;
 use std::str::FromStr;
 
 impl NumberFilterPB {
-  pub fn is_visible(&self, num_cell_data: &NumberCellFormat) -> bool {
-    if self.content.is_empty() {
-      match self.condition {
-        NumberFilterConditionPB::NumberIsEmpty => {
-          return num_cell_data.is_empty();
+  pub fn is_visible(&self, cell_data: &NumberCellFormat) -> Option<bool> {
+    let expected_decimal = Decimal::from_str(&self.content).unwrap_or_else(|_| Decimal::zero());
+
+    let strategy = match self.condition {
+      NumberFilterConditionPB::Equal => NumberFilterStrategy::Equal(expected_decimal),
+      NumberFilterConditionPB::NotEqual => NumberFilterStrategy::NotEqual(expected_decimal),
+      NumberFilterConditionPB::GreaterThan => NumberFilterStrategy::GreaterThan(expected_decimal),
+      NumberFilterConditionPB::LessThan => NumberFilterStrategy::LessThan(expected_decimal),
+      NumberFilterConditionPB::GreaterThanOrEqualTo => {
+        NumberFilterStrategy::GreaterThanOrEqualTo(expected_decimal)
       },
-        NumberFilterConditionPB::NumberIsNotEmpty => {
-          return !num_cell_data.is_empty();
+      NumberFilterConditionPB::LessThanOrEqualTo => {
+        NumberFilterStrategy::LessThanOrEqualTo(expected_decimal)
       },
-        _ => {},
+      NumberFilterConditionPB::NumberIsEmpty => NumberFilterStrategy::Empty,
+      NumberFilterConditionPB::NumberIsNotEmpty => NumberFilterStrategy::NotEmpty,
+    };
+
+    Some(strategy.filter(cell_data))
   }
-    }
-    match num_cell_data.decimal().as_ref() {
-      None => false,
-      Some(cell_decimal) => {
-        let decimal = Decimal::from_str(&self.content).unwrap_or_else(|_| Decimal::zero());
-        match self.condition {
-          NumberFilterConditionPB::Equal => cell_decimal == &decimal,
-          NumberFilterConditionPB::NotEqual => cell_decimal != &decimal,
-          NumberFilterConditionPB::GreaterThan => cell_decimal > &decimal,
-          NumberFilterConditionPB::LessThan => cell_decimal < &decimal,
-          NumberFilterConditionPB::GreaterThanOrEqualTo => cell_decimal >= &decimal,
-          NumberFilterConditionPB::LessThanOrEqualTo => cell_decimal <= &decimal,
-          _ => true,
-        }
-      },
+}
+
+enum NumberFilterStrategy {
+  Equal(Decimal),
+  NotEqual(Decimal),
+  GreaterThan(Decimal),
+  LessThan(Decimal),
+  GreaterThanOrEqualTo(Decimal),
+  LessThanOrEqualTo(Decimal),
+  Empty,
+  NotEmpty,
+}
+
+impl NumberFilterStrategy {
+  fn filter(self, cell_data: &NumberCellFormat) -> bool {
+    match self {
+      NumberFilterStrategy::Equal(expected_value) => cell_data
+        .decimal()
+        .is_some_and(|decimal| decimal == expected_value),
+      NumberFilterStrategy::NotEqual(expected_value) => cell_data
+        .decimal()
+        .is_some_and(|decimal| decimal != expected_value),
+      NumberFilterStrategy::GreaterThan(expected_value) => cell_data
+        .decimal()
+        .is_some_and(|decimal| decimal > expected_value),
+      NumberFilterStrategy::LessThan(expected_value) => cell_data
+        .decimal()
+        .is_some_and(|decimal| decimal < expected_value),
+      NumberFilterStrategy::GreaterThanOrEqualTo(expected_value) => cell_data
+        .decimal()
+        .is_some_and(|decimal| decimal >= expected_value),
+      NumberFilterStrategy::LessThanOrEqualTo(expected_value) => cell_data
+        .decimal()
+        .is_some_and(|decimal| decimal <= expected_value),
+      NumberFilterStrategy::Empty => cell_data.is_empty(),
+      NumberFilterStrategy::NotEmpty => !cell_data.is_empty(),
     }
   }
 }
@@ -39,6 +69,7 @@ impl NumberFilterPB {
 mod tests {
   use crate::entities::{NumberFilterConditionPB, NumberFilterPB};
   use crate::services::field::{NumberCellFormat, NumberFormat};
+
   #[test]
   fn number_filter_equal_test() {
     let number_filter = NumberFilterPB {
@@ -48,15 +79,16 @@ mod tests {
 
     for (num_str, visible) in [("123", true), ("1234", false), ("", false)] {
       let data = NumberCellFormat::from_format_str(num_str, &NumberFormat::Num).unwrap_or_default();
-      assert_eq!(number_filter.is_visible(&data), visible);
+      assert_eq!(number_filter.is_visible(&data), Some(visible));
     }
 
     let format = NumberFormat::USD;
     for (num_str, visible) in [("$123", true), ("1234", false), ("", false)] {
       let data = NumberCellFormat::from_format_str(num_str, &format).unwrap();
-      assert_eq!(number_filter.is_visible(&data), visible);
+      assert_eq!(number_filter.is_visible(&data), Some(visible));
     }
   }
+
   #[test]
   fn number_filter_greater_than_test() {
     let number_filter = NumberFilterPB {
@@ -65,7 +97,7 @@ mod tests {
     };
     for (num_str, visible) in [("123", true), ("10", false), ("30", true), ("", false)] {
       let data = NumberCellFormat::from_format_str(num_str, &NumberFormat::Num).unwrap_or_default();
-      assert_eq!(number_filter.is_visible(&data), visible);
+      assert_eq!(number_filter.is_visible(&data), Some(visible));
     }
   }
 
@@ -77,7 +109,7 @@ mod tests {
     };
     for (num_str, visible) in [("12", true), ("1234", false), ("30", true), ("", false)] {
       let data = NumberCellFormat::from_format_str(num_str, &NumberFormat::Num).unwrap_or_default();
-      assert_eq!(number_filter.is_visible(&data), visible);
+      assert_eq!(number_filter.is_visible(&data), Some(visible));
     }
   }
 }

frontend/rust-lib/flowy-database2/src/services/field/type_options/number_type_option/number_type_option.rs

@@ -120,10 +120,7 @@ impl NumberTypeOption {
     Self::default()
   }
 
-  pub(crate) fn format_cell_data(
-    &self,
-    num_cell_data: &NumberCellData,
-  ) -> FlowyResult<NumberCellFormat> {
+  fn format_cell_data(&self, num_cell_data: &NumberCellData) -> FlowyResult<NumberCellFormat> {
     match self.format {
       NumberFormat::Num => {
         if SCIENTIFIC_NOTATION_REGEX
@@ -252,7 +249,7 @@ impl TypeOptionCellDataFilter for NumberTypeOption {
       return true;
     }
     match self.format_cell_data(cell_data) {
-      Ok(cell_data) => filter.is_visible(&cell_data),
+      Ok(cell_data) => filter.is_visible(&cell_data).unwrap_or(true),
       Err(_) => true,
     }
   }

frontend/rust-lib/flowy-database2/src/services/field/type_options/timestamp_type_option/timestamp_type_option.rs

@@ -174,15 +174,14 @@ impl CellDataChangeset for TimestampTypeOption {
 impl TypeOptionCellDataFilter for TimestampTypeOption {
   fn apply_filter(
     &self,
-    filter: &<Self as TypeOption>::CellFilter,
+    _filter: &<Self as TypeOption>::CellFilter,
     field_type: &FieldType,
-    cell_data: &<Self as TypeOption>::CellData,
+    _cell_data: &<Self as TypeOption>::CellData,
   ) -> bool {
     if !field_type.is_last_edited_time() && !field_type.is_created_time() {
       return true;
     }
-
-    filter.is_visible(cell_data.timestamp)
+    false
   }
 }