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.

Sign up for GitHub

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

Jump to bottom

kie-issues#1463: Test Scenario Editor: Manage @kie-tools/scesim-editor's state with Zustand + Immer #2628

Merged
merged 57 commits into from
Oct 19, 2024
Merged

kie-issues#1463: Test Scenario Editor: Manage @kie-tools/scesim-editor's state with Zustand + Immer #2628

merged 57 commits into from
Oct 19, 2024

Conversation

yesamer
Copy link
Contributor

@yesamer yesamer commented Sep 27, 2024 •
edited
Loading

Closes apache/incubator-kie-issues#1463

Screen.Recording.2024-10-01.at.16.06.47.mov

In this PR, the whole test scenario editor state management has been refactored to rely on Zustand + Immer framework, following the same methodology applied in DMN and BPMN editors.
That enabled the Undo / Redo mechanism.

@yesamer yesamer added pr: DO NOT MERGE Draft PR, not ready for merging pr: wip PR is still under development labels Sep 27, 2024
@yesamer yesamer added pr: DO NOT MERGE Draft PR, not ready for merging and removed pr: DO NOT MERGE Draft PR, not ready for merging pr: wip PR is still under development labels Oct 10, 2024
@yesamer yesamer removed the pr: DO NOT MERGE Draft PR, not ready for merging label Oct 10, 2024
@yesamer
Copy link
Contributor Author

yesamer commented Oct 10, 2024

@tiagobento Ready for another round :)

jomarko
jomarko reviewed Oct 11, 2024
View reviewed changes
Copy link
Contributor

@jomarko jomarko left a comment

Choose a reason for hiding this comment

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

@yesamer hi, is there a way how to figure out what is currently and what is currently not supported by the scesim-editor?

I was about to do some manual checks, but I think it is not ready for such checks?

Screenshot 2024-10-11 083112
Screenshot 2024-10-11 083104

packages/scesim-editor/src/creation/TestScenarioCreationPanel.tsx Outdated Show resolved Hide resolved
@yesamer
Copy link
Contributor Author

yesamer commented Oct 14, 2024

@jomarko The table UX should be considered completed (and well covered by e2e test provided by @kbowers-ibm). There are some KI identified here apache/incubator-kie-issues#453.
The setting dock is completed too. The selector of data object works with simple cases, this still need to be extended with more complex cases.

jomarko
jomarko reviewed Oct 14, 2024
View reviewed changes
Copy link
Contributor

@jomarko jomarko left a comment

Choose a reason for hiding this comment

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

thank you for the changes @Yeser. I put some comments inline.

packages/scesim-editor/src/mutations/addColumn.ts
import { InsertRowColumnsDirection } from "@kie-tools/boxed-expression-component/dist/api/BeeTable";

export function addColumn({
beforeIndex,
Copy link
Contributor

Choose a reason for hiding this comment

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

beforeIndex sounds to me we as we will always add a column after the selected one, however as we have also insertDirection it is probably not true. could you please explain it little bit more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.
The BEE table UI allows you to add a column both on the right or on the left.
Screenshot 2024-10-14 at 16 14 10
insertDirection gives you exactly that information, in detail, the possible values are AboveOrRight,
BelowOrLeft
The addColumn mutation function requires both information to correctly persist the new column in the scesim file.

packages/scesim-editor/src/mutations/deleteColumn.ts Outdated Show resolved Hide resolved
packages/scesim-editor/src/mutations/updateCell.ts Outdated Show resolved Hide resolved
packages/scesim-editor/src/mutations/updateColumn.ts Outdated Show resolved Hide resolved
packages/scesim-editor/src/mutations/updateColumnWidth.ts Outdated Show resolved Hide resolved
packages/scesim-editor/src/store/computed/computeTestScenarioDataObjects.ts Outdated Show resolved Hide resolved
packages/scesim-editor/src/table/TestScenarioTable.tsx Outdated Show resolved Hide resolved
packages/scesim-editor/src/TestScenarioEditor.tsx Show resolved Hide resolved
packages/scesim-marshaller/src/schemas/scesim-1_8/SceSim.xsd Show resolved Hide resolved
jomarko
jomarko reviewed Oct 14, 2024
View reviewed changes
Copy link
Contributor

@jomarko jomarko left a comment

Choose a reason for hiding this comment

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

last comment to code

packages/scesim-editor/src/creation/TestScenarioCreationPanel.tsx
const settings = state.scesim.model.ScenarioSimulationModel.settings;
settings.dmnFilePath = assetType === "DMN" ? { __$$text: "./MockedDMNName.dmn" } : undefined;
settings.dmnName = assetType === "DMN" ? { __$$text: "MockedDMNName.dmn" } : undefined;
settings.dmnNamespace = assetType === "DMN" ? { __$$text: "https:\\kiegroup" } : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this is not correct, once I create new DMN, I see namespace value as "https:\kiegroup"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jomarko It's a temporary mockup, in a PR will be correctly managed (like all the above parameters)

@jomarko
Copy link
Contributor

jomarko commented Oct 14, 2024

During manual check, once I deleted all GIVEN instances, the right side panel did not display all objects, one was missing

Screenshot 2024-10-14 165827

@yesamer
Copy link
Contributor Author

yesamer commented Oct 14, 2024 •
edited
Loading

@jomarko Yeah, let me explain.
In DMN-based scesim, a type can be assigned one time only. So, if you assign a type in a column, that type will not more available in the Data Object list.
In RULE-based, this restriction shouldn't be present, which means, the same column can be present in both GIVEN and EXPECT groups. So, you've raised a proper issue.
The point is, at this moment the focus is on the DMN-based scenario only, so we can postpone it. Just mention it in this epic apache/incubator-kie-issues#1363, (IIRC, Kennedy already reported that)

@tiagobento
Copy link
Contributor

I still see some occurrences of JSON.parse(JSON.stringify( but I guess since this component is WIP in general, we can address those in separate PRs. Approving for us to move forward and not increase this PR's size.

@yesamer yesamer mentioned this pull request Oct 15, 2024
Closed
@yesamer
Copy link
Contributor Author

yesamer commented Oct 19, 2024

Finally green!!

@yesamer yesamer merged commit f8af243 into apache:main Oct 19, 2024
14 checks passed
@yesamer yesamer deleted the kie-issues#1463 branch October 19, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jomarko jomarko jomarko approved these changes

@tiagobento tiagobento tiagobento approved these changes

@kbowers-ibm kbowers-ibm Awaiting requested review from kbowers-ibm

Assignees
No one assigned
Labels
area:scesim pr: order: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test Scenario Editor: Manage @kie-tools/scesim-editor's state with Zustand + Immer
4 participants
@yesamer @jomarko @tiagobento @kbowers-ibm