Skip to content
This repository has been archived by the owner on Dec 9, 2024. It is now read-only.

fix(DataMapper): Delete DataMapper metadata entry when the step is de… #94

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

igarashitm
Copy link
Collaborator

@igarashitm igarashitm commented Oct 2, 2024

…leted

Fixes: KaotoIO/kaoto#1757

Here's something that works as expected. Now I have 2 questions:

  1. @lordrip is there any design rule that I have to follow? Although the name RenderingAnchor doesn't fit for this purpose, registering onDelete along with DataMapperLauncher component looks good for maintainability.
  2. @lordrip @apupier IMO we need to remove the associated XSLT file as well, hence we'd need to add deleteResourceContent to VS Code extension side. Does it sound right?

TODO

  1. Introduce dedicated provider for node interaction addons
  2. Check if prev and next step also need to be looked at when hooking onDelete for the children - https://github.com/KaotoIO/kaoto-datamapper-integration/pull/94/files#r1786507148
    • Checked, just looking at children is sufficient.
  3. Removing choice or when uses ItemDeleteStep but not ItemDeleteGroup, so both ItemDeleteStep and ItemReplaceStep would have to process recursively
    • ItemDeleteGroup is used when an entire route is deleted.
  4. Name collision on the word NodeInteraction, the NodeInteractionProvider in this PR might be better to rename to something else - https://github.com/KaotoIO/kaoto/blob/main/packages/ui/src/models/visualization/base-visual-entity.ts#L165
    • renamed to NodeInteractionAddonProvider
  5. Add unit tests
  6. Split the PR, submit NodeInteractionXXProvider and Item*.tsx change to the upstream first, then rebase this PR with just DataMapper specific
    • turns out RegisterComponents and RenderingAnchor are also not yet merged to upstream main. Since NodeInteractionAddonProvider shares activationFn with it, keep it here until we merge them together.

Update - splitted following to a separate issue - KaotoIO/kaoto#1749

  1. Show a confirmation dialog and ask if associated XSLT file could be removed - confirmation dialog should offer 3 options, (1) Remove DataMapper step and associated XSLT file, (2) Remove DataMapper step but keep associated XSLT file, (3) Cancel
  2. Remove XSLT file if the previous answer from user is No.1, remove associated XSLT file as well

@igarashitm igarashitm requested a review from lordrip October 2, 2024 20:53
@apupier
Copy link
Collaborator

apupier commented Oct 3, 2024

2. @lordrip @apupier IMO we need to remove the associated XSLT file as well, hence we'd need to add deleteResourceContent to VS Code extension side. Does it sound right?

reported KaotoIO/vscode-kaoto#681

Copy link
Collaborator

@apupier apupier left a comment

Choose a reason for hiding this comment

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

PR implementing API on VS Code Kaoto side KaotoIO/vscode-kaoto#682
based on this (internal) conversation https://redhat-internal.slack.com/archives/C06NYTRDMSR/p1727940000320609?thread_ts=1727725969.056549&cid=C06NYTRDMSR we should ask end-user if he wants to delete the associated xslt file or not

@igarashitm
Copy link
Collaborator Author

I have 2 questions about the confirmation dialog

  1. Currently removing DataMapper step doesn't show any confirmation dialog, can we add the confirmation dialog only for the DataMapper step without affecting other steps which don't have children?
  2. Also the confirmation dialog will be very DataMapper step specific - probably offer 3 options(buttons)? do we register the confirmation dialog component along with the DataMapperLauncher component just like onDelete this PR adds?
    a. Remove DataMapper step and XSLT file
    b. Remove DataMapper, but keep XSLT file
    c. Cancel

Copy link
Collaborator

@lordrip lordrip left a comment

Choose a reason for hiding this comment

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

While the approach is working, as you mentioned in the description, the place of the implementation might not be ideal.

For example, when deleting the datamapper step, we iterate over the registered components in the CanvasForm anchor. This could be expected if the operation were in the canvas form itself, but it's not the case.

There's also the situation when multiple registered components might pass the activationFn callback when deleting the datamapper step.

That being said, I see 2 ways to move forward:

  1. We consider the current approach, although not ideal, enough for what we need to do at this exact moment and merge it.

  2. Update this PR a bit to create a similar provider to the RenderingProvider, perhaps named InteractionProviders to register interaction handlers with step names.

The idea behind is the same, we register kaoto-datamapper step with some callbacks, for starters, we could implement the onDelete, this way, we could extend it in the future to have other lifecycle events, like onAdd, onReplace, etc...

@lordrip
Copy link
Collaborator

lordrip commented Oct 4, 2024

I have 2 questions about the confirmation dialog

1. Currently removing DataMapper step doesn't show any confirmation dialog, can we add the confirmation dialog only for the DataMapper step without affecting other steps which don't have children?

2. Also the confirmation dialog will be very DataMapper step specific - probably offer 3 options(buttons)? do we register the confirmation dialog component along with the `DataMapperLauncher` component just like `onDelete` this PR adds?
   a. Remove DataMapper step and XSLT file
   b. Remove DataMapper, but keep XSLT file
   c. Cancel

As you commented in another thread, I think we can expand the existing confirmation modal to include different buttons and return the ID of the clicked one, instead of the current boolean.

@igarashitm igarashitm force-pushed the 80 branch 2 times, most recently from b295d48 to 65e738d Compare October 7, 2024 12:35
@igarashitm igarashitm changed the title [WIP] fix(DataMapper): Delete DataMapper metadata entry when the step is de… fix(DataMapper): Delete DataMapper metadata entry when the step is de… Oct 7, 2024
@igarashitm igarashitm requested a review from lordrip October 7, 2024 19:38
Comment on lines +22 to +33
const onDeleteRecursively = useCallback(
(parentVizNode: IVisualizationNode) => {
parentVizNode.getChildren()?.forEach((child) => {
onDeleteRecursively(child);
});
const addons = getRegisteredInteractionAddons(IInteractionAddonType.ON_DELETE, parentVizNode);
addons.forEach((addon) => {
addon.callback(parentVizNode);
});
},
[getRegisteredInteractionAddons],
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function looks very similar to the ItemDeleteStep and the ItemReplace components, we should extract them to a single place so we can also benefit from testing the function on isolation.

If you don't mind, let's create an issue for this and move forward 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think once we extract the function, we could use it like this: (we probably should pick a more appropriate name than onDelete since the function will live somewhere else now)

onDelete(IVizNode, (vizNode: IVisualizationNode => getRegisteredInteractionAddons(IInteractionAddonType.ON_DELETE, parentVizNode)

Notice how we pass the current visualizationNode to the function as the first argument, and for the second argument, we pass a function that tells how and what type of interactions we are looking for, this way we bridge the React world (providers) with the outside, using regular Javascript.

Alternatively, since we're just traversing objects and we're not relying on any particular class instance, we might even simplify it as:

onDelete(IVizNode, getRegisteredInteractionAddons);

Then the onDelete function body will be in charge of passing the right VizNode and the right interaction type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete DataMapper metadata entry when the step is deleted
3 participants