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#208: Renaming any "NamedElement" on the DMN Editor should update all references to the old name #2760

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

danielzhe
Copy link
Contributor

@danielzhe danielzhe commented Nov 22, 2024

Closes: apache/incubator-kie-issues#208

Some comments about the changes:

  • We are tracking now all changes performed in BEE reported to DMN Editor. For now, we only have "special treatment" for the Action.VariableChanged;
  • A big part of the changes are related to this. Don't panic. It is just an indentation level that can make the PR bigger than it really is;
  • In the lower levels of the code, I refactored from "variable" to "identifier", because those objects are not only variables but identifiers;
  • I added documentation to a few methods and classes.

Renaming Data Types

Data.Types.mp4

Renaming BKMs and parameters

Bkm.and.Parameters.mp4

General rename

Some.Renames.mp4

Renaming Included Models name
It does not show the confirmation dialog, because doesn't make sense to not update the references in this case, since this already was the default behavior before this PR (when we renamed a included model, we already updated all the references except the expressions, now we update the expressions).

Included.Models.mp4

@danielzhe danielzhe marked this pull request as ready for review November 22, 2024 18:41
@danielzhe danielzhe changed the title [WIP] kie-issues#208: Renaming any "NamedElement" on the DMN Editor should update all references to the old name kie-issues#208: Renaming any "NamedElement" on the DMN Editor should update all references to the old name Nov 22, 2024

const resetFormData = useCallback(() => {
setExpressionName(selectedExpressionName);
setDataType(selectedDataType);
}, [selectedExpressionName, selectedDataType]);

// onCancel doens't prevent the onHide call
// onCancel doesn't prevent the onHide call
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a typo.

@tiagobento
Copy link
Contributor

Thank you for the PR @danielzhe. It's going to take me a few days to go over everything with the care it deserves. Will keep you posted!

@porcelli porcelli self-assigned this Nov 23, 2024
@jomarko jomarko self-requested a review November 25, 2024 09:38
@tiagobento
Copy link
Contributor

@danielzhe Storybook is pointing to code using ../src because it is used during development. The changes you introduced will make the development experience much worse, requiring us to rebuild stuff after a change, if I'm not mistaken... can you please check?

@jomarko
Copy link
Contributor

jomarko commented Nov 26, 2024

01

image
Using the feature, came to my mind, maybe we could have a third action button 'Cancel', like do nothing? During my testing I was in situation oh, I want to double-check what I will do, so I just wanted to cancel the operation.

@jomarko
Copy link
Contributor

jomarko commented Nov 26, 2024

02

The feature seems to not work in decision tables.
image
In the attached table there is input node IDAnyStruct. This node has a data type set to a custom structure with three fields: A, B and C. Once I renamed A, B or C on the Data Types tab, the occurrences in the decision table were not updated.

I think that at least the row 2 should be updated. I slightly think also decision table header row should be, however that may be very tricky and if we decide to implement, maybe in a separate ticket? I am not sure about the second point.

The same seems to be reproducible if the user is renaming for example the some identifier while in or satisfies expression is a decision table and has an occurrence of the identifier some.

@jomarko
Copy link
Contributor

jomarko commented Nov 26, 2024

03

FYI, I didn't check this feature with the List expression because of apache/incubator-kie-issues#1635

@jomarko
Copy link
Contributor

jomarko commented Nov 26, 2024

04

Question, should this feature work for BKM node and Invocation expression. Imagine this model:
Screenshot 2024-11-26 104246
Where internally the Invocation says it invokes mybkm node.

However if user renames the mybkm to mybkm renamed the Invocation remains the same (notice mybkm in last screenshot):
Screenshot 2024-11-26 104416
Screenshot 2024-11-26 104422

@jomarko
Copy link
Contributor

jomarko commented Nov 26, 2024

05

We have an issue editor allows to define two function parameters with the same name, then renaming does not work.
Screenshot 2024-11-26 105859
Screenshot 2024-11-26 105912

@jomarko
Copy link
Contributor

jomarko commented Nov 26, 2024

06

Is there a way to disable this feature? Committing the rename action starts to be slower, so I expect some user may ask to disable this feature in their environment.

@jomarko
Copy link
Contributor

jomarko commented Nov 26, 2024

07

During included models review, I found this apache/incubator-kie-issues#1636, however I think I didn't find issue related to changes of #2760.

- Fixes includes to point to src
- Some refactor to reduce nesting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Renaming any "NamedElement" on the DMN Editor should update all references to the old name
4 participants