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

Keep original IDs, if unused in target diagram #1410

Closed
1 task done
nikku opened this issue Jun 19, 2019 · 9 comments · Fixed by #2514
Closed
1 task done

Keep original IDs, if unused in target diagram #1410

nikku opened this issue Jun 19, 2019 · 9 comments · Fixed by #2514
Assignees
Labels
BPMN channel:support enhancement New feature or request modeling spring cleaning Could be cleaned up one day
Milestone

Comments

@nikku
Copy link
Member

nikku commented Jun 19, 2019

Is your feature request related to a problem? Please describe.

As a user I'd like to paste contents from another diagram without loosing the original IDs.

Use case: Merge diagrams by pasting conflicting parts from one diagram to the other.

Describe the solution you'd like

  • Keep IDs when inserting copied contents, if the IDs are not taken yet in the target diagram

Describe alternatives you've considered

None.


Related to SUPPORT-4777

@nikku nikku added backlog Queued in backlog BPMN enhancement New feature or request modeling channel:support labels Jun 19, 2019
@MaxTru MaxTru added the spring cleaning Could be cleaned up one day label Jun 7, 2021
@MaxTru MaxTru added this to the M50 milestone Sep 6, 2021
@barmac
Copy link
Contributor

barmac commented Sep 6, 2021

@MaxTru
Copy link
Contributor

MaxTru commented Sep 17, 2021

We will not be able to deliver this for milestone M50 - I will get in touch with the CSM responsible to inform the customer

@MaxTru MaxTru modified the milestones: M50, M51 Sep 20, 2021
@barmac barmac self-assigned this Oct 14, 2021
barmac added a commit to bpmn-io/bpmn-js that referenced this issue Oct 15, 2021
barmac added a commit to bpmn-io/bpmn-js that referenced this issue Oct 15, 2021
barmac added a commit to bpmn-io/bpmn-js that referenced this issue Oct 15, 2021
barmac added a commit to bpmn-io/bpmn-js that referenced this issue Oct 15, 2021
barmac added a commit to bpmn-io/bpmn-js that referenced this issue Oct 15, 2021
@barmac barmac added fixed upstream Requires integration of upstream change and removed backlog Queued in backlog labels Oct 15, 2021
@MaxTru
Copy link
Contributor

MaxTru commented Oct 17, 2021

Great job @barmac! <3 that we have this

@nikku nikku added ready Ready to be worked on and removed fixed upstream Requires integration of upstream change labels Oct 19, 2021
@nikku
Copy link
Member Author

nikku commented Oct 19, 2021

There exists an issue with the way we implemented this bug fix via bpmn-io/bpmn-js@1253326.

In the Camunda Modeler paste into another diagram only works for the first time. The second time I do it, it blows up:

capture jZTEBY_optimized

@barmac
Copy link
Contributor

barmac commented Oct 20, 2021

That's confusing :/ Thanks for reporting!

@barmac
Copy link
Contributor

barmac commented Oct 20, 2021

I was able to reproduce it. It seems that the ID of a copied element is not reserved in the moddle.ids. The ID appears then as unassigned.

@nikku
Copy link
Member Author

nikku commented Oct 20, 2021

I suggest that we create an end-to-end integration test in ModelerSpec (testing with two modeler instances) to assert this is actually fixed for good.

@nikku
Copy link
Member Author

nikku commented Oct 20, 2021

As discussed I created a failing end-to-end test case here: bpmn-js#copy-paste-failing.

@barmac
Copy link
Contributor

barmac commented Oct 20, 2021

OK I was able to find the root cause of the problem. Let me try to explain it.

In the BpmnFactory, we _ensureId when an element is created. This assigns a new id if it's not already provided in the properties passed to BpmnFactory#create. Such ID is at the same time claimed in the moddle.ids object.

However, if the properties passed to BpmnFactory#create already include ID, it will never be claimed in the moddle.ids object unless the caller assigns it themself. Nevertheless, the element added on a diagram is also added to the ElementRegistry, and this is where the error thrown in the test case originates from.

I just debugged it and it seems a bit more complicated.

barmac added a commit to bpmn-io/bpmn-js that referenced this issue Oct 20, 2021
barmac added a commit to bpmn-io/bpmn-js that referenced this issue Oct 20, 2021
barmac added a commit to bpmn-io/bpmn-js that referenced this issue Oct 20, 2021
barmac added a commit to bpmn-io/bpmn-js that referenced this issue Oct 20, 2021
nikku pushed a commit to bpmn-io/bpmn-js that referenced this issue Oct 20, 2021
barmac added a commit to bpmn-io/bpmn-js that referenced this issue Oct 20, 2021
@barmac barmac added fixed upstream Requires integration of upstream change and removed ready Ready to be worked on labels Oct 20, 2021
nikku added a commit that referenced this issue Oct 20, 2021
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed fixed upstream Requires integration of upstream change labels Oct 20, 2021
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BPMN channel:support enhancement New feature or request modeling spring cleaning Could be cleaned up one day
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants