Fix(automation): Handles undefined values in conditions #1933

DelBiss · 2026-01-02T22:23:08Z

Addresses an issue where conditions with the 'not_equals' operator incorrectly evaluated undefined values. This change ensures that empty string comparisons correctly identify missing values in automation rules.

Fix #1932

Addresses an issue where conditions with the 'not_equals' operator incorrectly evaluated undefined values. This change ensures that empty string comparisons correctly identify missing values in automation rules.

coderabbitai · 2026-01-02T22:23:13Z

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

DelBiss · 2026-01-03T18:02:01Z

apps/server/src/api-data/automation/automation.service.ts

        return fieldValue == value;
      case 'not_equals':
+        // overload the edge case where we use empty string to check if a value does not exist
+        if (value === '' && fieldValue === undefined) {


Maybe it would be better to just do the inverse of equal?

I've implemented it in the 2nd commit.

Simplifies the 'not_equals' condition evaluation in automations by reusing the 'equals' condition, improving code readability and consistency. Fixes cpvalente#1932

Expanding test cases to cover various data types and edge case for each operators. Unexpected behavior have been mark with a TO_DO.

DelBiss · 2026-01-04T01:47:23Z

apps/server/src/api-data/automation/__tests__/automation.service.test.ts

+      expect(runTestCondition('null/undefined', null, 'equals', 'not-empty')).toBe(false);
+      expect(runTestCondition('boolean', false, 'equals', '')).toBe(false);
+
+      expect(runTestCondition('number', 0, 'equals', '')).toBe(true); // TO_DO: is this the desired behavior?


This case is probably wrong, especially if false != <empty/no value>

yes, this seems incorrect. I believe my suggestion for comparing values could improve this

DelBiss · 2026-01-04T01:49:17Z

apps/server/src/api-data/automation/__tests__/automation.service.test.ts

+    it('should handle case sensitivity', () => {
+      // TO_DO: is this the desired behavior?
+      expect(runTestCondition('string', 'Title', 'contains', 'title')).toBe(false);
+    });


contains only work on string, yet, it's case sensitive, contrary to equals.

this is a good observation, contains and equals should have similar implementations.
I would suggest that contains and not_contains should also be case insensitive. what do you think?

DelBiss · 2026-01-04T02:00:43Z

apps/server/src/api-data/automation/__tests__/automation.service.test.ts

+      expect(runTestCondition('boolean', true, 'contains', 'true')).toBe(false);
+      expect(runTestCondition('boolean', false, 'contains', 'false')).toBe(false);
+    });
+


This is a case that could go both way, especially if, on the client side, operation depends of field type.

But, my first instinct was that other type should match if theirs string representation match.
But again, that would be weird for boolean value that would alway contain a e.

In the current implementation, contains only deals with string types. I think this is the correct way to do it

semantically boolean contains "rue" doesnt make sense

- Ensures that their is a default scenario. - Fixing Deepsource issue "No default cases in switch statements JS-0047"

alex-Arc

thanks for working on this.
the fix itself seams good, and you bring up some good questions in the tests
maybe we could start by merging the fix and take a bit more time to clean up the test

cpvalente

Thank you for these observations.

The code changes were slightly hard to parse since they were mixed with a refactoring of the test scenarios.

I suggest that we create a single PR for now.

add tests for the edge cases with equals and contains you have brought up using the existing abstractions
add fixes for equals as suggested
add changes for casing in contains

we can then review separately whether the tests benefit from an abstraction. In general we avoid abstractions in testing. At this level we dont want to do any "hiding" and prefer all the changes to be explicit, also abstractions could introduce issues at abstraction level which make things hard to debug

cpvalente · 2026-01-06T10:24:33Z

apps/server/src/api-data/automation/automation.service.ts

        return fieldValue == value;
      case 'not_equals':
-        return fieldValue != value;
+        return !evaluateCondition({ field, operator: 'equals', value });


I believe the previous code attempted a loose comparison to allow matching "12" to 12.
This is to simplify automations

However, it has the side effects of matching all the falsy values 0, undefined, null, ""

how about creating a new helper isEquivalentValue which resolves this

given two values of the same type, make a strict comparison ===

given two values of different types:

a) if the types are string a number, make a loose comparison ==

b) for all other cases return false

This could also help with the logic in the equals condition

cpvalente · 2026-01-06T10:32:43Z

apps/server/src/api-data/automation/__tests__/automation.service.test.ts

+    it('should handle case sensitivity', () => {
+      // TO_DO: is this the desired behavior?
+      expect(runTestCondition('string', 'Title', 'contains', 'title')).toBe(false);
+    });


this is a good observation, contains and equals should have similar implementations.
I would suggest that contains and not_contains should also be case insensitive. what do you think?

cpvalente · 2026-01-06T10:34:20Z

apps/server/src/api-data/automation/__tests__/automation.service.test.ts

+      expect(runTestCondition('boolean', true, 'contains', 'true')).toBe(false);
+      expect(runTestCondition('boolean', false, 'contains', 'false')).toBe(false);
+    });
+


In the current implementation, contains only deals with string types. I think this is the correct way to do it

semantically boolean contains "rue" doesnt make sense

cpvalente · 2026-01-06T10:36:00Z

apps/server/src/api-data/automation/__tests__/automation.service.test.ts

+      expect(runTestCondition('null/undefined', null, 'equals', 'not-empty')).toBe(false);
+      expect(runTestCondition('boolean', false, 'equals', '')).toBe(false);
+
+      expect(runTestCondition('number', 0, 'equals', '')).toBe(true); // TO_DO: is this the desired behavior?


yes, this seems incorrect. I believe my suggestion for comparing values could improve this

DelBiss · 2026-01-06T15:23:58Z

I've removed the abstraction in the last commit.

But the Idea behind the abstraction was to have a better view of all the test and expected results.

Compare how those file look (look at line 104):

At first, I wanted even more abstraction with a matrix to generate all the test, but settle on this middle ground.

I will come back to this after my day job....

Fix(automation): Handles undefined values in conditions

206135e

Addresses an issue where conditions with the 'not_equals' operator incorrectly evaluated undefined values. This change ensures that empty string comparisons correctly identify missing values in automation rules.

DelBiss commented Jan 3, 2026

View reviewed changes

DelBiss added 2 commits January 3, 2026 13:12

Fixes automation "not equals" logic

6100ac9

Simplifies the 'not_equals' condition evaluation in automations by reusing the 'equals' condition, improving code readability and consistency. Fixes cpvalente#1932

Test(automation): Expanding filter condition testing

917f5de

Expanding test cases to cover various data types and edge case for each operators. Unexpected behavior have been mark with a TO_DO.

DelBiss commented Jan 4, 2026

View reviewed changes

Fix(automation): Handles default filter case

11314a0

- Ensures that their is a default scenario. - Fixing Deepsource issue "No default cases in switch statements JS-0047"

alex-Arc reviewed Jan 5, 2026

View reviewed changes

cpvalente reviewed Jan 6, 2026

View reviewed changes

Test(automation): Remove abstraction in test

47629c2

Uh oh!

Fix(automation): Handles undefined values in conditions #1933

Are you sure you want to change the base?

Fix(automation): Handles undefined values in conditions #1933

Conversation

DelBiss commented Jan 2, 2026

Uh oh!

coderabbitai bot commented Jan 2, 2026

Review skipped

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-Arc left a comment

Choose a reason for hiding this comment

Uh oh!

cpvalente left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DelBiss commented Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DelBiss commented Jan 6, 2026 •

edited

Loading