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

kie-issues#970: Test Scenario Editor: Integrate the @kie-tools/scesim-editor component with the DMN Marshaller #2887

Merged
merged 77 commits into from
Feb 21, 2025

Conversation

yesamer
Copy link
Contributor

@yesamer yesamer commented Jan 30, 2025

Closes apache/incubator-kie-issues#970

This PR integrates the new Scesim Editor with the DMN Marshaller, making it possible to retrieve required information from an External DMN Model.

To whoever will review and test this PR, please consider some KI have already reported here

In addition, some use cases are still not fully implemented (eg. collections, expressions, nested data objects)
Those will be managed with apache/incubator-kie-issues#1514

Some examples:

Opening an existing Scesim file:

Screen.Recording.2025-02-05.at.15.24.17.mov

Creating a new DMN-based Scesim file with auto-population

Screen.Recording.2025-02-05.at.15.17.19.mov

Creating a new DMN-based Scesim file without auto-population

Screen.Recording.2025-02-05.at.15.19.16.mov

Opening an existing DMN-based Scesim file with the referenced DMN file removed

Screen.Recording.2025-02-05.at.15.27.45.mov

Opening an existing DMN-based Scesim file with the referenced DMN file moved to another location

Screen.Recording.2025-02-05.at.16.43.28.mov

Blocked by #2866. Required to update the tests.

@yesamer yesamer added pr: DO NOT MERGE Draft PR, not ready for merging pr: wip PR is still under development area:scesim labels Jan 30, 2025
@yesamer yesamer requested a review from tiagobento as a code owner January 30, 2025 16:19
@jomarko
Copy link
Contributor

jomarko commented Jan 31, 2025

🥳 🍾 🚀

Copy link
Contributor

@tiagobento tiagobento left a comment

Choose a reason for hiding this comment

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

Please consider my comment inline. Thank you!

if (callBackError) {
throw callBackError;
}
}, [callBackError]);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're letting error propagete, why do we need the callBackError state? Why not let them explode when they happen? Mapping errors as state is usually for a try/catch-like mechanism, when we want the screen to show a different UI for when errors occur.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good observation, so it's enough to rethrow the error inside the catch block, right?

Copy link
Contributor

@tiagobento tiagobento Feb 20, 2025

Choose a reason for hiding this comment

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

What try/catch block? Sorry it seems there are multiple ones. If an error happens and we do something on the screen to "treat it", I think we shouldn't re-throw it. Now, if it's really an "end of the world" type of error, I think we should just throw and let the first ErrorBoundary catch it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mapping errors as state is usually for a try/catch-like mechanism, when we want the screen to show a different UI for when errors occur.

I guess that is the case. The error is retrieved and set in the state in the catch blocks at lines 92 and 124.
Without that useEffect(), the error is NOT propagated to the error boundary, at least with the tests I did.

@jomarko
Copy link
Contributor

jomarko commented Feb 18, 2025

Postponing my review after code changes required by @tiagobento are in.

@yesamer
Copy link
Contributor Author

yesamer commented Feb 21, 2025

@jomarko @tiagobento Hi, can you please give priority to this PR review? It would be great to have this merged EOD. Thank you

@yesamer yesamer requested a review from jomarko February 21, 2025 06:55
@tiagobento
Copy link
Contributor

Expediting my approval here by your request @yesamer. We can address any outstanding issues on separate PRs. Thanks!

@tiagobento
Copy link
Contributor

Merging by @yesamer's request. @jomarko and other reviewers, please let's go to #2919 for any issues you find here. Thank you.

@tiagobento tiagobento merged commit b0a5754 into apache:main Feb 21, 2025
15 checks passed
@yesamer yesamer deleted the kie-issues#970-3 branch February 22, 2025 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test Scenario Editor: Integrate the @kie-tools/scesim-editor component with the DMN Marshaller
4 participants