Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix evaluation of Block exits when inbound message is a boolean-ish or numeric-ish string value #180

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

fedme
Copy link
Contributor

@fedme fedme commented Mar 26, 2024

Right now the FlowRunner fails to match the correct Block exit when the inbound message is a string that contains a value resembling a boolean (e.g. "True") or a number (e.g. "123").

This is because the expression package parses those strings values to proper booleans and numbers, which is not what we want when evaluating Blocks outgoing links.

turnhub/expression#189 introduced a flag that we can use to disable the automatic parsing of booleans/integers and this PR introduces the usage of that flag in the FlowRunner to fix the evaluation of outgoing links in SelectOneResponse and Case Blocks.

Copy link

@fedme fedme requested a review from smn March 26, 2024 17:54
Copy link
Contributor

@smn smn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this change but we'll want to revisit this when we upgrade the Expression lib (which is where this problem originates).

@santiagocardo could you weigh in as well as you've dealt with this stuff before as well.

Copy link
Contributor

@santiagocardo santiagocardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Perhaps we can add something like score == 0 as another option in the then() routhing test cases. To make sure we are also covering plain integers in these expression evaluations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants