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

feat(cloud): don't apply length and pattern validation to FEEL #115

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

philippfromme
Copy link
Contributor

@philippfromme philippfromme commented Aug 5, 2024

brave_Ii9FYknxbI

Try out via npm start.

Related to camunda/camunda-modeler#4140

Proposed Changes

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Aug 5, 2024
@philippfromme philippfromme marked this pull request as ready for review August 5, 2024 12:20
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Aug 5, 2024
Comment on lines 922 to 925
if (isFeel(value)) {
return;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

We do have access to the property, so we could theoretically add stricter validation with

Suggested change
if (isFeel(value)) {
return;
}
if (property.feel && isFeel(value)) {
return;
}

With this, this would still apply patterns to properties that are normal string type without feel expressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the schema the feel property can also be static. Would that mean the property is not FEEL? In that case we'd need to check the value of the property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Answered. static still means it's a FEEL expression. So a simple check is enough.

@philippfromme
Copy link
Contributor Author

One UX issue I found with this is that the error message stays until there is an actual value. There is some internal state of the FEEL entry that decides whether it's FEEL and doesn't actually commit the initial = until you enter something else like =foo. That internal state is not accessable outside of the FEEL entry. @marstamm Any idea if we can do something about that?

brave_8JC0So0ScH

@marstamm
Copy link
Collaborator

As we have validation only when a value is persisted in other places as well (e.g. when you are still typing), I think this is fine for now. In these cases, it is basically "Should not be empty" but with another error message. I don't think this will be to confusing for the user, as the message disappears once an expression is present

@philippfromme
Copy link
Contributor Author

@marstamm In that case it's ready for review.

@philippfromme philippfromme requested a review from marstamm August 14, 2024 07:34
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Great change, great addition to the existing test coverage!

@nikku nikku merged commit faaa81d into main Aug 16, 2024
7 of 8 checks passed
@nikku nikku deleted the feel-validation branch August 16, 2024 20:30
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Aug 16, 2024
@nikku
Copy link
Member

nikku commented Aug 16, 2024

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