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: ensure questions that set a data field with no option data values are not automated #3903

Merged
merged 3 commits into from
Nov 4, 2024

Conversation

jessicamcinchak
Copy link
Member

@jessicamcinchak jessicamcinchak commented Nov 4, 2024

Fixes edge case flagged here: https://opensystemslab.slack.com/archives/C5Q59R3HB/p1730455691984869

Two changes:

  • autoAnswerableOptions condition is corrected so that existing nodes will behave as expected (put to user)
  • Formik validation added to editor modal for Questions & Checklists so that future nodes aren't allowed to do this at all

Screenshot from 2024-11-04 10-15-31

Testing:

@jessicamcinchak
Copy link
Member Author

Copy link

github-actions bot commented Nov 4, 2024

Removed vultr server and associated DNS entries

);
if (!visitedFns) return;
if (!visitedFns.length) return;
Copy link
Member Author

Choose a reason for hiding this comment

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

A filter is always going to return an [] so the previous condition incorrectly always returned true!

Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch 👍

@jessicamcinchak jessicamcinchak marked this pull request as ready for review November 4, 2024 09:53
@jessicamcinchak jessicamcinchak requested a review from a team November 4, 2024 09:53
Copy link
Contributor

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

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

Nice simple solution 👏

);
if (!visitedFns) return;
if (!visitedFns.length) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch 👍

Comment on lines +315 to +322
validate: ({ options, ...values }) => {
const errors: FormikErrors<FormikValues> = {};
if (values.fn && !options?.some((option) => option.data.val)) {
errors.fn =
"At least one option must set a data value when the checklist has a data field";
}
return errors;
},
Copy link
Contributor

@DafyddLlyr DafyddLlyr Nov 4, 2024

Choose a reason for hiding this comment

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

nit: It would be nice to make these validation schemas for ease of expansion in future, but this could well be a YAGNI thing - they've been without any validation for a very long time!

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.

2 participants