Fix/select option filter (#1530)

* fix: multi select filter bugs

* fix: single select bugs

Co-authored-by: nathan <[email protected]>

frontend/app_flowy/lib/plugins/board/application/card/board_text_cell_bloc.dart

@@ -1,5 +1,4 @@
 import 'package:app_flowy/plugins/grid/application/cell/cell_service/cell_service.dart';
-import 'package:flutter/foundation.dart';
 import 'package:flutter_bloc/flutter_bloc.dart';
 import 'package:freezed_annotation/freezed_annotation.dart';
 import 'dart:async';

frontend/app_flowy/lib/plugins/grid/application/field/field_service.dart

@@ -3,7 +3,6 @@ import 'package:flowy_sdk/dispatch/dispatch.dart';
 import 'package:flowy_sdk/protobuf/flowy-error/errors.pb.dart';
 import 'package:flowy_sdk/protobuf/flowy-grid/field_entities.pb.dart';
 import 'package:flowy_sdk/protobuf/flowy-grid/grid_entities.pb.dart';
-import 'package:flutter/foundation.dart';
 import 'package:freezed_annotation/freezed_annotation.dart';
 
 part 'field_service.freezed.dart';

frontend/app_flowy/lib/plugins/grid/presentation/widgets/filter/choicechip/select_option/select_option.dart

@@ -61,7 +61,7 @@ class _SelectOptionFilterChoicechipState
         builder: (blocContext, state) {
           return AppFlowyPopover(
             controller: PopoverController(),
-            constraints: BoxConstraints.loose(const Size(200, 160)),
+            constraints: BoxConstraints.loose(const Size(240, 160)),
             direction: PopoverDirection.bottomWithCenterAligned,
             popupBuilder: (BuildContext context) {
               return SelectOptionFilterEditor(bloc: bloc);

frontend/app_flowy/lib/plugins/grid/presentation/widgets/toolbar/filter_button.dart

@@ -3,7 +3,6 @@ import 'package:app_flowy/plugins/grid/application/filter/filter_menu_bloc.dart'
 import 'package:appflowy_popover/appflowy_popover.dart';
 import 'package:easy_localization/easy_localization.dart';
 import 'package:flowy_infra/color_extension.dart';
-import 'package:flowy_infra/size.dart';
 import 'package:flowy_infra_ui/flowy_infra_ui.dart';
 import 'package:flowy_infra_ui/style_widget/button.dart';
 import 'package:flutter/material.dart';
@@ -35,7 +34,7 @@ class _FilterButtonState extends State<FilterButton> {
             height: 26,
             child: FlowyTextButton(
               LocaleKeys.grid_settings_filter.tr(),
-              fontSize: FontSizes.s14,
+              fontSize: 13,
               fontColor: textColor,
               fillColor: Colors.transparent,
               hoverColor: AFThemeExtension.of(context).lightGreyHover,

frontend/app_flowy/lib/plugins/grid/presentation/widgets/toolbar/setting_button.dart

@@ -4,7 +4,6 @@ import 'package:app_flowy/plugins/grid/application/setting/setting_bloc.dart';
 import 'package:appflowy_popover/appflowy_popover.dart';
 import 'package:easy_localization/easy_localization.dart';
 import 'package:flowy_infra/color_extension.dart';
-import 'package:flowy_infra/size.dart';
 import 'package:flowy_infra_ui/flowy_infra_ui.dart';
 import 'package:flowy_infra_ui/style_widget/button.dart';
 import 'package:flutter/material.dart';
@@ -50,7 +49,7 @@ class _SettingButtonState extends State<SettingButton> {
           triggerActions: PopoverTriggerFlags.none,
           child: FlowyTextButton(
             LocaleKeys.settings_title.tr(),
-            fontSize: FontSizes.s14,
+            fontSize: 13,
             fillColor: Colors.transparent,
             hoverColor: AFThemeExtension.of(context).lightGreyHover,
             padding: const EdgeInsets.symmetric(vertical: 2, horizontal: 6),

frontend/rust-lib/flowy-grid/src/services/field/type_options/selection_type_option/checklist_filter.rs

@@ -14,7 +14,7 @@ impl ChecklistFilterPB {
             .map(|option| option.id.as_str())
             .collect::<Vec<&str>>();
 
-        return match self.condition {
+        match self.condition {
             ChecklistFilterCondition::IsComplete => {
                 if selected_option_ids.is_empty() {
                     return false;
@@ -31,6 +31,6 @@ impl ChecklistFilterPB {
                 all_option_ids.retain(|option_id| !selected_option_ids.contains(option_id));
                 !all_option_ids.is_empty()
             }
-        };
+        }
     }
 }

frontend/rust-lib/flowy-grid/src/services/field/type_options/selection_type_option/select_filter.rs

@@ -1,39 +1,77 @@
 #![allow(clippy::needless_collect)]
 
-use crate::entities::{ChecklistFilterPB, SelectOptionCondition, SelectOptionFilterPB};
+use crate::entities::{ChecklistFilterPB, FieldType, SelectOptionCondition, SelectOptionFilterPB};
 use crate::services::cell::{CellFilterOperation, TypeCellData};
 use crate::services::field::{ChecklistTypeOptionPB, MultiSelectTypeOptionPB, SingleSelectTypeOptionPB};
 use crate::services::field::{SelectTypeOptionSharedAction, SelectedSelectOptions};
 use flowy_error::FlowyResult;
 
 impl SelectOptionFilterPB {
-    pub fn is_visible(&self, selected_options: &SelectedSelectOptions) -> bool {
+    pub fn is_visible(&self, selected_options: &SelectedSelectOptions, field_type: FieldType) -> bool {
         let selected_option_ids: Vec<&String> = selected_options.options.iter().map(|option| &option.id).collect();
         match self.condition {
-            SelectOptionCondition::OptionIs => {
-                if self.option_ids.len() != selected_option_ids.len() {
-                    return false;
+            SelectOptionCondition::OptionIs => match field_type {
+                FieldType::SingleSelect => {
+                    if self.option_ids.is_empty() {
+                        return true;
+                    }
+
+                    if selected_options.options.is_empty() {
+                        return false;
+                    }
+
+                    let required_options = self
+                        .option_ids
+                        .iter()
+                        .filter(|id| selected_option_ids.contains(id))
+                        .collect::<Vec<_>>();
+
+                    !required_options.is_empty()
+                }
+                FieldType::MultiSelect => {
+                    if self.option_ids.is_empty() {
+                        return true;
+                    }
+
+                    let required_options = self
+                        .option_ids
+                        .iter()
+                        .filter(|id| selected_option_ids.contains(id))
+                        .collect::<Vec<_>>();
+
+                    !required_options.is_empty()
                 }
-                // if selected options equal to filter's options, then the required_options will be empty.
-                let required_options = self
-                    .option_ids
-                    .iter()
-                    .filter(|id| selected_option_ids.contains(id))
-                    .collect::<Vec<_>>();
-                required_options.len() == selected_option_ids.len()
-            }
-            SelectOptionCondition::OptionIsNot => {
-                if self.option_ids.len() != selected_option_ids.len() {
-                    return true;
+                _ => false,
+            },
+            SelectOptionCondition::OptionIsNot => match field_type {
+                FieldType::SingleSelect => {
+                    if self.option_ids.is_empty() {
+                        return true;
+                    }
+
+                    if selected_options.options.is_empty() {
+                        return false;
+                    }
+
+                    let required_options = self
+                        .option_ids
+                        .iter()
+                        .filter(|id| selected_option_ids.contains(id))
+                        .collect::<Vec<_>>();
+
+                    required_options.is_empty()
+                }
+                FieldType::MultiSelect => {
+                    let required_options = self
+                        .option_ids
+                        .iter()
+                        .filter(|id| selected_option_ids.contains(id))
+                        .collect::<Vec<_>>();
+
+                    required_options.is_empty()
                 }
-                let required_options = self
-                    .option_ids
-                    .iter()
-                    .filter(|id| selected_option_ids.contains(id))
-                    .collect::<Vec<_>>();
-
-                required_options.len() != selected_option_ids.len()
-            }
+                _ => false,
+            },
             SelectOptionCondition::OptionIsEmpty => selected_option_ids.is_empty(),
             SelectOptionCondition::OptionIsNotEmpty => !selected_option_ids.is_empty(),
         }
@@ -47,7 +85,7 @@ impl CellFilterOperation<SelectOptionFilterPB> for MultiSelectTypeOptionPB {
         }
 
         let selected_options = SelectedSelectOptions::from(self.get_selected_options(any_cell_data.into()));
-        Ok(filter.is_visible(&selected_options))
+        Ok(filter.is_visible(&selected_options, FieldType::MultiSelect))
     }
 }
 
@@ -57,7 +95,7 @@ impl CellFilterOperation<SelectOptionFilterPB> for SingleSelectTypeOptionPB {
             return Ok(true);
         }
         let selected_options = SelectedSelectOptions::from(self.get_selected_options(any_cell_data.into()));
-        Ok(filter.is_visible(&selected_options))
+        Ok(filter.is_visible(&selected_options, FieldType::SingleSelect))
     }
 }
 
@@ -74,7 +112,7 @@ impl CellFilterOperation<ChecklistFilterPB> for ChecklistTypeOptionPB {
 #[cfg(test)]
 mod tests {
     #![allow(clippy::all)]
-    use crate::entities::{SelectOptionCondition, SelectOptionFilterPB};
+    use crate::entities::{FieldType, SelectOptionCondition, SelectOptionFilterPB};
     use crate::services::field::selection_type_option::{SelectOptionPB, SelectedSelectOptions};
 
     #[test]
@@ -85,10 +123,26 @@ mod tests {
             option_ids: vec![],
         };
 
-        assert_eq!(filter.is_visible(&SelectedSelectOptions { options: vec![] }), true);
+        assert_eq!(
+            filter.is_visible(&SelectedSelectOptions { options: vec![] }, FieldType::SingleSelect),
+            true
+        );
+        assert_eq!(
+            filter.is_visible(
+                &SelectedSelectOptions {
+                    options: vec![option.clone()]
+                },
+                FieldType::SingleSelect
+            ),
+            false,
+        );
 
         assert_eq!(
-            filter.is_visible(&SelectedSelectOptions { options: vec![option] }),
+            filter.is_visible(&SelectedSelectOptions { options: vec![] }, FieldType::MultiSelect),
+            true
+        );
+        assert_eq!(
+            filter.is_visible(&SelectedSelectOptions { options: vec![option] }, FieldType::MultiSelect),
             false,
         );
     }
@@ -103,82 +157,162 @@ mod tests {
         };
 
         assert_eq!(
-            filter.is_visible(&SelectedSelectOptions {
-                options: vec![option_1]
-            }),
+            filter.is_visible(
+                &SelectedSelectOptions {
+                    options: vec![option_1.clone()]
+                },
+                FieldType::SingleSelect
+            ),
             true
         );
+        assert_eq!(
+            filter.is_visible(&SelectedSelectOptions { options: vec![] }, FieldType::SingleSelect),
+            false,
+        );
 
-        assert_eq!(filter.is_visible(&SelectedSelectOptions { options: vec![] }), false,);
+        assert_eq!(
+            filter.is_visible(
+                &SelectedSelectOptions {
+                    options: vec![option_1.clone()]
+                },
+                FieldType::MultiSelect
+            ),
+            true
+        );
+        assert_eq!(
+            filter.is_visible(&SelectedSelectOptions { options: vec![] }, FieldType::MultiSelect),
+            false,
+        );
     }
 
     #[test]
-    fn select_option_filter_is_not_test() {
+    fn single_select_option_filter_is_not_test() {
         let option_1 = SelectOptionPB::new("A");
         let option_2 = SelectOptionPB::new("B");
         let option_3 = SelectOptionPB::new("C");
-
         let filter = SelectOptionFilterPB {
             condition: SelectOptionCondition::OptionIsNot,
             option_ids: vec![option_1.id.clone(), option_2.id.clone()],
         };
 
-        assert_eq!(
-            filter.is_visible(&SelectedSelectOptions {
-                options: vec![option_1.clone(), option_2.clone()],
-            }),
-            false
-        );
-
-        assert_eq!(
-            filter.is_visible(&SelectedSelectOptions {
-                options: vec![option_1.clone(), option_2.clone(), option_3.clone()],
-            }),
-            true
-        );
+        for (options, is_visible) in vec![
+            (vec![option_2.clone()], false),
+            (vec![option_1.clone()], false),
+            (vec![option_3.clone()], true),
+            (vec![option_1.clone(), option_2.clone()], false),
+        ] {
+            assert_eq!(
+                filter.is_visible(&SelectedSelectOptions { options }, FieldType::SingleSelect),
+                is_visible
+            );
+        }
+    }
 
-        assert_eq!(
-            filter.is_visible(&SelectedSelectOptions {
-                options: vec![option_1.clone(), option_3.clone()],
-            }),
-            true
-        );
+    #[test]
+    fn single_select_option_filter_is_test() {
+        let option_1 = SelectOptionPB::new("A");
+        let option_2 = SelectOptionPB::new("B");
+        let option_3 = SelectOptionPB::new("c");
 
-        assert_eq!(filter.is_visible(&SelectedSelectOptions { options: vec![] }), true);
+        let filter = SelectOptionFilterPB {
+            condition: SelectOptionCondition::OptionIs,
+            option_ids: vec![option_1.id.clone()],
+        };
+        for (options, is_visible) in vec![
+            (vec![option_1.clone()], true),
+            (vec![option_2.clone()], false),
+            (vec![option_3.clone()], false),
+            (vec![option_1.clone(), option_2.clone()], true),
+        ] {
+            assert_eq!(
+                filter.is_visible(&SelectedSelectOptions { options }, FieldType::SingleSelect),
+                is_visible
+            );
+        }
     }
 
     #[test]
-    fn select_option_filter_is_test() {
+    fn single_select_option_filter_is_test2() {
         let option_1 = SelectOptionPB::new("A");
         let option_2 = SelectOptionPB::new("B");
-        let option_3 = SelectOptionPB::new("C");
 
         let filter = SelectOptionFilterPB {
             condition: SelectOptionCondition::OptionIs,
+            option_ids: vec![],
+        };
+        for (options, is_visible) in vec![
+            (vec![option_1.clone()], true),
+            (vec![option_2.clone()], true),
+            (vec![option_1.clone(), option_2.clone()], true),
+        ] {
+            assert_eq!(
+                filter.is_visible(&SelectedSelectOptions { options }, FieldType::SingleSelect),
+                is_visible
+            );
+        }
+    }
+
+    #[test]
+    fn multi_select_option_filter_not_contains_test() {
+        let option_1 = SelectOptionPB::new("A");
+        let option_2 = SelectOptionPB::new("B");
+        let option_3 = SelectOptionPB::new("C");
+        let filter = SelectOptionFilterPB {
+            condition: SelectOptionCondition::OptionIsNot,
             option_ids: vec![option_1.id.clone(), option_2.id.clone()],
         };
 
-        assert_eq!(
-            filter.is_visible(&SelectedSelectOptions {
-                options: vec![option_1.clone(), option_2.clone()],
-            }),
-            true
-        );
+        for (options, is_visible) in vec![
+            (vec![option_1.clone(), option_2.clone()], false),
+            (vec![option_1.clone()], false),
+            (vec![option_2.clone()], false),
+            (vec![option_3.clone()], true),
+            (vec![option_1.clone(), option_2.clone(), option_3.clone()], false),
+            (vec![], true),
+        ] {
+            assert_eq!(
+                filter.is_visible(&SelectedSelectOptions { options }, FieldType::MultiSelect),
+                is_visible
+            );
+        }
+    }
+    #[test]
+    fn multi_select_option_filter_contains_test() {
+        let option_1 = SelectOptionPB::new("A");
+        let option_2 = SelectOptionPB::new("B");
+        let option_3 = SelectOptionPB::new("C");
 
-        assert_eq!(
-            filter.is_visible(&SelectedSelectOptions {
-                options: vec![option_1.clone(), option_2.clone(), option_3.clone()],
-            }),
-            false
-        );
+        let filter = SelectOptionFilterPB {
+            condition: SelectOptionCondition::OptionIs,
+            option_ids: vec![option_1.id.clone(), option_2.id.clone()],
+        };
+        for (options, is_visible) in vec![
+            (vec![option_1.clone(), option_2.clone(), option_3.clone()], true),
+            (vec![option_2.clone(), option_1.clone()], true),
+            (vec![option_2.clone()], true),
+            (vec![option_1.clone(), option_3.clone()], true),
+            (vec![option_3.clone()], false),
+        ] {
+            assert_eq!(
+                filter.is_visible(&SelectedSelectOptions { options }, FieldType::MultiSelect),
+                is_visible
+            );
+        }
+    }
 
-        assert_eq!(
-            filter.is_visible(&SelectedSelectOptions {
-                options: vec![option_1.clone(), option_3.clone()],
-            }),
-            false
-        );
+    #[test]
+    fn multi_select_option_filter_contains_test2() {
+        let option_1 = SelectOptionPB::new("A");
 
-        assert_eq!(filter.is_visible(&SelectedSelectOptions { options: vec![] }), false);
+        let filter = SelectOptionFilterPB {
+            condition: SelectOptionCondition::OptionIs,
+            option_ids: vec![],
+        };
+        for (options, is_visible) in vec![(vec![option_1.clone()], true), (vec![], true)] {
+            assert_eq!(
+                filter.is_visible(&SelectedSelectOptions { options }, FieldType::MultiSelect),
+                is_visible
+            );
+        }
     }
 }

frontend/rust-lib/flowy-grid/tests/grid/filter_test/select_option_filter_test.rs

@@ -38,7 +38,7 @@ async fn grid_filter_multi_select_is_test() {
             condition: SelectOptionCondition::OptionIs,
             option_ids: vec![options.remove(0).id, options.remove(0).id],
         },
-        AssertNumberOfVisibleRows { expected: 2 },
+        AssertNumberOfVisibleRows { expected: 3 },
     ];
     test.run_scripts(scripts).await;
 }
@@ -53,7 +53,7 @@ async fn grid_filter_multi_select_is_test2() {
             condition: SelectOptionCondition::OptionIs,
             option_ids: vec![options.remove(1).id],
         },
-        AssertNumberOfVisibleRows { expected: 1 },
+        AssertNumberOfVisibleRows { expected: 3 },
     ];
     test.run_scripts(scripts).await;
 }