From f5b9b86c16f880f21922d1d3b933637347ed1399 Mon Sep 17 00:00:00 2001
From: Julien Nahum <julien@nahum.net>
Date: Mon, 10 Feb 2025 21:48:21 +0100
Subject: [PATCH] Refactor checkbox condition logic with new operators

- Replace legacy 'equals' and 'does_not_equal' checkbox operators with 'is_checked' and 'is_not_checked'
- Update FormLogicConditionChecker in PHP and JavaScript to handle new operators
- Modify open_filters.json to reflect new checkbox comparator structure
- Add migration logic in ColumnCondition.vue to support legacy operator conversion
- Improve checkbox condition handling with explicit true/false checks
---
 .../Forms/FormLogicConditionChecker.php       |  16 +-
 api/resources/data/open_filters.json          |  20 +-
 .../Forms/FormLogicConditionCheckerTest.php   | 452 ++++++++++++++++++
 .../form-logic-components/ColumnCondition.vue |  38 +-
 client/data/open_filters.json                 |  20 +-
 client/lib/forms/FormLogicConditionChecker.js |   9 +-
 6 files changed, 499 insertions(+), 56 deletions(-)
 create mode 100644 api/tests/Unit/Service/Forms/FormLogicConditionCheckerTest.php

diff --git a/api/app/Service/Forms/FormLogicConditionChecker.php b/api/app/Service/Forms/FormLogicConditionChecker.php
index b2d6035f..b5a50a06 100644
--- a/api/app/Service/Forms/FormLogicConditionChecker.php
+++ b/api/app/Service/Forms/FormLogicConditionChecker.php
@@ -89,7 +89,7 @@ class FormLogicConditionChecker
         if (is_array($fieldValue)) {
             return in_array($condition['value'], $fieldValue);
         }
-        return \Str::contains($fieldValue, $condition['value']);
+        return \Illuminate\Support\Str::contains($fieldValue, $condition['value']);
     }
 
     private function checkMatrixContains($condition, $fieldValue): bool
@@ -362,11 +362,21 @@ class FormLogicConditionChecker
 
     private function checkboxConditionMet(array $propertyCondition, $value): bool
     {
+        // Treat null or missing values as false
+        if ($value === null || !isset($value)) {
+            $value = false;
+        }
+
         switch ($propertyCondition['operator']) {
+            case 'is_checked':
+                return $value === true;
+            case 'is_not_checked':
+                return $value === false;
+                // Legacy operators
             case 'equals':
-                return $this->checkEquals($propertyCondition, $value);
+                return $value === true;
             case 'does_not_equal':
-                return !$this->checkEquals($propertyCondition, $value);
+                return $value === false;
         }
 
         return false;
diff --git a/api/resources/data/open_filters.json b/api/resources/data/open_filters.json
index 55331bbb..5899a6f2 100644
--- a/api/resources/data/open_filters.json
+++ b/api/resources/data/open_filters.json
@@ -505,24 +505,8 @@
   },
   "checkbox": {
     "comparators": {
-      "equals": {
-        "expected_type": "boolean",
-        "format": {
-          "type": "enum",
-          "values": [
-            true
-          ]
-        }
-      },
-      "does_not_equal": {
-        "expected_type": "boolean",
-        "format": {
-          "type": "enum",
-          "values": [
-            true
-          ]
-        }
-      }
+      "is_checked": {},
+      "is_not_checked": {}
     }
   },
   "select": {
diff --git a/api/tests/Unit/Service/Forms/FormLogicConditionCheckerTest.php b/api/tests/Unit/Service/Forms/FormLogicConditionCheckerTest.php
new file mode 100644
index 00000000..e47ad5f5
--- /dev/null
+++ b/api/tests/Unit/Service/Forms/FormLogicConditionCheckerTest.php
@@ -0,0 +1,452 @@
+<?php
+
+use App\Service\Forms\FormLogicConditionChecker;
+
+describe('FormLogicConditionChecker', function () {
+    describe('checkbox conditions', function () {
+        it('handles is_checked operator correctly', function () {
+            $condition = [
+                'value' => [
+                    'property_meta' => [
+                        'id' => 'checkbox_field',
+                        'type' => 'checkbox'
+                    ],
+                    'operator' => 'is_checked',
+                    'value' => true
+                ]
+            ];
+
+            $formData = ['checkbox_field' => true];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $formData = ['checkbox_field' => false];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+        });
+
+        it('handles is_not_checked operator correctly', function () {
+            $condition = [
+                'value' => [
+                    'property_meta' => [
+                        'id' => 'checkbox_field',
+                        'type' => 'checkbox'
+                    ],
+                    'operator' => 'is_not_checked',
+                    'value' => true
+                ]
+            ];
+
+            $formData = ['checkbox_field' => false];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $formData = ['checkbox_field' => true];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+        });
+
+        it('handles legacy equals operator correctly', function () {
+            $condition = [
+                'value' => [
+                    'property_meta' => [
+                        'id' => 'checkbox_field',
+                        'type' => 'checkbox'
+                    ],
+                    'operator' => 'equals',
+                    'value' => true
+                ]
+            ];
+
+            $formData = ['checkbox_field' => true];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $formData = ['checkbox_field' => false];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+        });
+
+        it('handles legacy does_not_equal operator correctly', function () {
+            $condition = [
+                'value' => [
+                    'property_meta' => [
+                        'id' => 'checkbox_field',
+                        'type' => 'checkbox'
+                    ],
+                    'operator' => 'does_not_equal',
+                    'value' => true
+                ]
+            ];
+
+            $formData = ['checkbox_field' => false];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $formData = ['checkbox_field' => true];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+        });
+
+        it('handles null values correctly', function () {
+            $condition = [
+                'value' => [
+                    'property_meta' => [
+                        'id' => 'checkbox_field',
+                        'type' => 'checkbox'
+                    ],
+                    'operator' => 'is_checked',
+                    'value' => true
+                ]
+            ];
+
+            // Null should be treated as unchecked (false)
+            $formData = ['checkbox_field' => null];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+
+            $condition['value']['operator'] = 'is_not_checked';
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+        });
+
+        it('handles missing values correctly', function () {
+            $condition = [
+                'value' => [
+                    'property_meta' => [
+                        'id' => 'checkbox_field',
+                        'type' => 'checkbox'
+                    ],
+                    'operator' => 'is_checked',
+                    'value' => true
+                ]
+            ];
+
+            // Missing value should be treated as unchecked (false)
+            $formData = [];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+
+            $condition['value']['operator'] = 'is_not_checked';
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+        });
+    });
+
+    describe('number conditions', function () {
+        it('handles comparison operators correctly', function () {
+            $condition = [
+                'value' => [
+                    'property_meta' => [
+                        'id' => 'number_field',
+                        'type' => 'number'
+                    ],
+                    'operator' => 'equals',
+                    'value' => 42
+                ]
+            ];
+
+            $formData = ['number_field' => 42];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $formData = ['number_field' => 41];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+
+            $condition['value']['operator'] = 'greater_than';
+            $condition['value']['value'] = 40;
+            $formData = ['number_field' => 41];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $condition['value']['operator'] = 'less_than';
+            $condition['value']['value'] = 42;
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+        });
+
+        it('handles empty checks correctly', function () {
+            $condition = [
+                'value' => [
+                    'property_meta' => [
+                        'id' => 'number_field',
+                        'type' => 'number'
+                    ],
+                    'operator' => 'is_empty',
+                    'value' => true
+                ]
+            ];
+
+            $formData = ['number_field' => null];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $formData = ['number_field' => 42];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+
+            $condition['value']['operator'] = 'is_not_empty';
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+        });
+    });
+
+    describe('text conditions', function () {
+        it('handles string comparison operators correctly', function () {
+            $condition = [
+                'value' => [
+                    'property_meta' => [
+                        'id' => 'text_field',
+                        'type' => 'text'
+                    ],
+                    'operator' => 'equals',
+                    'value' => 'test'
+                ]
+            ];
+
+            $formData = ['text_field' => 'test'];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $formData = ['text_field' => 'other'];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+
+            $condition['value']['operator'] = 'contains';
+            $condition['value']['value'] = 'es';
+            $formData = ['text_field' => 'test'];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $condition['value']['operator'] = 'starts_with';
+            $condition['value']['value'] = 'te';
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $condition['value']['operator'] = 'ends_with';
+            $condition['value']['value'] = 'st';
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            // Test does_not_contain
+            $condition['value']['operator'] = 'does_not_contain';
+            $condition['value']['value'] = 'xyz';
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+        });
+
+        it('handles content length operators correctly', function () {
+            $condition = [
+                'value' => [
+                    'property_meta' => [
+                        'id' => 'text_field',
+                        'type' => 'text'
+                    ],
+                    'operator' => 'content_length_equals',
+                    'value' => 4
+                ]
+            ];
+
+            $formData = ['text_field' => 'test'];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $condition['value']['operator'] = 'content_length_greater_than';
+            $condition['value']['value'] = 3;
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $condition['value']['operator'] = 'content_length_less_than';
+            $condition['value']['value'] = 5;
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+        });
+
+        it('handles regex operators correctly', function () {
+            $condition = [
+                'value' => [
+                    'property_meta' => [
+                        'id' => 'text_field',
+                        'type' => 'text'
+                    ],
+                    'operator' => 'matches_regex',
+                    'value' => '^test[0-9]+$'
+                ]
+            ];
+
+            $formData = ['text_field' => 'test123'];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $formData = ['text_field' => 'invalid'];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+
+            // Test invalid regex pattern
+            $condition['value']['value'] = '['; // Invalid regex
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+        });
+    });
+
+    describe('date conditions', function () {
+        it('handles date comparison operators correctly', function () {
+            $condition = [
+                'value' => [
+                    'property_meta' => [
+                        'id' => 'date_field',
+                        'type' => 'date'
+                    ],
+                    'operator' => 'equals',
+                    'value' => '2024-01-01'
+                ]
+            ];
+
+            $formData = ['date_field' => '2024-01-01'];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $condition['value']['operator'] = 'before';
+            $condition['value']['value'] = '2024-01-02';
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $condition['value']['operator'] = 'after';
+            $condition['value']['value'] = '2023-12-31';
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+        });
+
+        it('handles relative date operators correctly', function () {
+            $condition = [
+                'value' => [
+                    'property_meta' => [
+                        'id' => 'date_field',
+                        'type' => 'date'
+                    ],
+                    'operator' => 'past_week',
+                    'value' => '{}'
+                ]
+            ];
+
+            $formData = ['date_field' => now()->subDays(3)->toDateString()];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $formData = ['date_field' => now()->subDays(10)->toDateString()];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+
+            $condition['value']['operator'] = 'next_week';
+            $formData = ['date_field' => now()->addDays(3)->toDateString()];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+        });
+    });
+
+    describe('multi_select conditions', function () {
+        it('handles contains operators correctly', function () {
+            $condition = [
+                'value' => [
+                    'property_meta' => [
+                        'id' => 'multi_select_field',
+                        'type' => 'multi_select'
+                    ],
+                    'operator' => 'contains',
+                    'value' => 'option1'
+                ]
+            ];
+
+            $formData = ['multi_select_field' => ['option1', 'option2']];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $formData = ['multi_select_field' => ['option2', 'option3']];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+
+            // Test with array of values
+            $condition['value']['value'] = ['option1', 'option2'];
+            $formData = ['multi_select_field' => ['option1', 'option2', 'option3']];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+        });
+    });
+
+    describe('matrix conditions', function () {
+        it('handles matrix comparison operators correctly', function () {
+            $condition = [
+                'value' => [
+                    'property_meta' => [
+                        'id' => 'matrix_field',
+                        'type' => 'matrix'
+                    ],
+                    'operator' => 'equals',
+                    'value' => ['row1' => 'col1', 'row2' => 'col2']
+                ]
+            ];
+
+            $formData = ['matrix_field' => ['row1' => 'col1', 'row2' => 'col2']];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            $formData = ['matrix_field' => ['row1' => 'col2', 'row2' => 'col2']];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+
+            $condition['value']['operator'] = 'contains';
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+        });
+    });
+
+    describe('group conditions', function () {
+        it('handles nested AND/OR conditions correctly', function () {
+            $condition = [
+                'operatorIdentifier' => 'and',
+                'children' => [
+                    [
+                        'operatorIdentifier' => 'or',
+                        'children' => [
+                            [
+                                'value' => [
+                                    'property_meta' => [
+                                        'id' => 'checkbox_field',
+                                        'type' => 'checkbox'
+                                    ],
+                                    'operator' => 'is_checked',
+                                    'value' => true
+                                ]
+                            ],
+                            [
+                                'value' => [
+                                    'property_meta' => [
+                                        'id' => 'number_field',
+                                        'type' => 'number'
+                                    ],
+                                    'operator' => 'greater_than',
+                                    'value' => 40
+                                ]
+                            ]
+                        ]
+                    ],
+                    [
+                        'value' => [
+                            'property_meta' => [
+                                'id' => 'text_field',
+                                'type' => 'text'
+                            ],
+                            'operator' => 'contains',
+                            'value' => 'test'
+                        ]
+                    ]
+                ]
+            ];
+
+            // Test case where OR condition is true (checkbox) and text contains 'test'
+            $formData = [
+                'checkbox_field' => true,
+                'number_field' => 30,
+                'text_field' => 'test123'
+            ];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            // Test case where OR condition is true (number) and text contains 'test'
+            $formData = [
+                'checkbox_field' => false,
+                'number_field' => 41,
+                'text_field' => 'test123'
+            ];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeTrue();
+
+            // Test case where OR condition is false and text contains 'test'
+            $formData = [
+                'checkbox_field' => false,
+                'number_field' => 30,
+                'text_field' => 'test123'
+            ];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+
+            // Test case where OR condition is true but text doesn't contain 'test'
+            $formData = [
+                'checkbox_field' => true,
+                'number_field' => 30,
+                'text_field' => 'other'
+            ];
+            expect(FormLogicConditionChecker::conditionsMet($condition, $formData))->toBeFalse();
+        });
+
+        it('handles invalid conditions gracefully', function () {
+            // Test with null conditions
+            expect(FormLogicConditionChecker::conditionsMet(null, []))->toBeFalse();
+
+            // Test with empty conditions
+            expect(FormLogicConditionChecker::conditionsMet([], []))->toBeFalse();
+
+            // Test with invalid operator
+            $condition = [
+                'operatorIdentifier' => 'invalid',
+                'children' => []
+            ];
+            expect(fn () => FormLogicConditionChecker::conditionsMet($condition, []))->toThrow(\Exception::class);
+        });
+    });
+});
diff --git a/client/components/open/forms/components/form-logic-components/ColumnCondition.vue b/client/components/open/forms/components/form-logic-components/ColumnCondition.vue
index a64f9d3f..56352265 100644
--- a/client/components/open/forms/components/form-logic-components/ColumnCondition.vue
+++ b/client/components/open/forms/components/form-logic-components/ColumnCondition.vue
@@ -107,7 +107,7 @@ export default {
       ).map((key) => {
         return {
           value: key,
-          name: this.optionFilterNames(key, this.property.type),
+          name: this.optionFilterNames(key),
         }
       })
     },
@@ -116,19 +116,22 @@ export default {
       if (!operator) {
         return false
       }
-      const operatorFormat = operator.format
-      if (!operatorFormat) return true
+      
+      // If operator has no format and no expected_type, it means it doesn't need input
+      if (!operator.format && !operator.expected_type) {
+        return false
+      }
 
       if (
         operator.expected_type === "boolean" &&
-        operatorFormat.type === "enum" &&
-        operatorFormat.values.length === 1
+        operator.format?.type === "enum" &&
+        operator.format.values.length === 1
       ) {
         return false
       } else if (
         operator.expected_type === "object" &&
-        operatorFormat.type === "empty" &&
-        operatorFormat.values === "{}"
+        operator.format?.type === "empty" &&
+        operator.format.values === "{}"
       ) {
         return false
       }
@@ -205,13 +208,7 @@ export default {
         this.content.operator
       ]
     },
-    optionFilterNames(key, propertyType) {
-      if (propertyType === "checkbox") {
-        return {
-          equals: "Is checked",
-          does_not_equal: "Is not checked",
-        }[key]
-      }
+    optionFilterNames(key) {
       return key
         .split("_")
         .map(function (item) {
@@ -223,9 +220,20 @@ export default {
       this.$emit("update:modelValue", this.castContent(this.content))
     },
     refreshContent() {
+      const modelValue = { ...this.modelValue }
+      
+      // Migrate legacy checkbox operators
+      if (this.property.type === 'checkbox') {
+        if (modelValue?.operator === 'equals') {
+          modelValue.operator = 'is_checked'
+        } else if (modelValue?.operator === 'does_not_equal') {
+          modelValue.operator = 'is_not_checked'
+        }
+      }
+
       this.content = {
         operator: this.operators[0].value,
-        ...this.modelValue,
+        ...modelValue,
         property_meta: {
           id: this.property.id,
           type: this.property.type,
diff --git a/client/data/open_filters.json b/client/data/open_filters.json
index 55331bbb..5899a6f2 100644
--- a/client/data/open_filters.json
+++ b/client/data/open_filters.json
@@ -505,24 +505,8 @@
   },
   "checkbox": {
     "comparators": {
-      "equals": {
-        "expected_type": "boolean",
-        "format": {
-          "type": "enum",
-          "values": [
-            true
-          ]
-        }
-      },
-      "does_not_equal": {
-        "expected_type": "boolean",
-        "format": {
-          "type": "enum",
-          "values": [
-            true
-          ]
-        }
-      }
+      "is_checked": {},
+      "is_not_checked": {}
     }
   },
   "select": {
diff --git a/client/lib/forms/FormLogicConditionChecker.js b/client/lib/forms/FormLogicConditionChecker.js
index 38368535..cd683945 100644
--- a/client/lib/forms/FormLogicConditionChecker.js
+++ b/client/lib/forms/FormLogicConditionChecker.js
@@ -332,10 +332,15 @@ function numberConditionMet(propertyCondition, value) {
 
 function checkboxConditionMet(propertyCondition, value) {
   switch (propertyCondition.operator) {
+    case "is_checked":
+      return value === true
+    case "is_not_checked":
+      return value === false
+    // Legacy operators
     case "equals":
-      return checkEquals(propertyCondition, value)
+      return value === true
     case "does_not_equal":
-      return !checkEquals(propertyCondition, value)
+      return value === false
   }
   return false
 }