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

feat: default user tasks with zeebe:UserTask extension element #86

Merged
merged 5 commits into from
Dec 10, 2024

Conversation

Skaiir
Copy link
Contributor

@Skaiir Skaiir commented Nov 5, 2024

@Skaiir Skaiir requested a review from philippfromme November 5, 2024 11:28
@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Nov 5, 2024
@Skaiir Skaiir force-pushed the 4648-zeebe-user-task-defaulting branch from aa2d261 to 4e28fbd Compare November 5, 2024 11:31
@philippfromme
Copy link
Collaborator

What is the state of this pull request?

@Skaiir Skaiir force-pushed the 4648-zeebe-user-task-defaulting branch from 4e28fbd to 11d8c72 Compare November 28, 2024 08:19
@Skaiir
Copy link
Contributor Author

Skaiir commented Nov 28, 2024

This is currently in draft because there are some failing tests around userTaskForm. Getting some help from @philippfromme to get this unblocked.

@barmac
Copy link
Contributor

barmac commented Dec 2, 2024

I will look into this now.

@barmac barmac requested review from barmac and removed request for philippfromme December 2, 2024 13:33
@barmac barmac force-pushed the 4648-zeebe-user-task-defaulting branch from 11d8c72 to 6dbef38 Compare December 2, 2024 15:45
@barmac
Copy link
Contributor

barmac commented Dec 2, 2024

Since @philippfromme's test update is merged now, let's run it on CI again.

The test case requires manual XML editing.
@barmac
Copy link
Contributor

barmac commented Dec 2, 2024

Regarding the failing CleanUpTaskListenersBehaviorSpec test case, we could remove it as it cannot be reproduced in the UI (task listener on job worker user task).

@barmac
Copy link
Contributor

barmac commented Dec 2, 2024

Done via 44dd8bd

@barmac
Copy link
Contributor

barmac commented Dec 2, 2024

I suspect that the remaining test cases might be failing because we change from job worker to zeebe user task, so specifically related to the content of the PR.

What about if we consider changing the editor's behaviour via a popup menu / context pad extension? So that we don't need to add the extension after the element is created, but rather create a proper user task from the beginning? This could have a down (or up?) side of being able to copy and paste a job-worker user task. What do you think @Skaiir ?

@philippfromme
Copy link
Collaborator

I think using a behavior is the right approach here. We are using the same pattern for similar cases already. We just need to make sure the behaviors aren't conflicting with each other.

@barmac
Copy link
Contributor

barmac commented Dec 3, 2024

Whatever solution we take, let's make sure these requirements are met:

  • When I create a user task from scratch, its implementation is set to Zeebe user task
  • When I replace a task with user task, its implementation is set to Zeebe user task
  • When I copy a job worker user task, the implementation type is kept

@Skaiir
Copy link
Contributor Author

Skaiir commented Dec 7, 2024

I have adjusted the behavior to act on replace, and not do so on copy. Struggled with that second part but GPT found the hint in the context regarding how to differentiate a regular creation from a copy creation.

Then I looked it up in the bpmn-js codebase and it seems to be the proper way: bpmn-io/bpmn-js@bf18032#diff-2f0de25761fb7459e88071f83fd845c5R22.

===

Regarding the leftover failing tests. I don't know this form behavior and I'm not sure what the expectations are, but it seems that it is only applicable for non-zeebe userTasks. The simple fix I could find to clear the last couple tests to green was to bypass the zeebe:UserTask behavior in those tests by setting the "copy" hint directly. It basically emulates what would happen in a copy scenario.

@Skaiir Skaiir marked this pull request as ready for review December 7, 2024 08:40
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Dec 7, 2024
@barmac
Copy link
Contributor

barmac commented Dec 9, 2024

I'll have a look into this tomorrow.

@barmac
Copy link
Contributor

barmac commented Dec 10, 2024

Thanks @Skaiir for a great review session together. I will implement the changes where we marked it with comments and release it today afternoon.

@barmac
Copy link
Contributor

barmac commented Dec 10, 2024

I've applied the changes ideated in the review session, cf. dc78601

@barmac barmac merged commit 612fc43 into main Dec 10, 2024
5 checks passed
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Dec 10, 2024
@barmac barmac deleted the 4648-zeebe-user-task-defaulting branch December 10, 2024 17:03
@barmac
Copy link
Contributor

barmac commented Dec 10, 2024

Thanks @Skaiir for providing this

@barmac
Copy link
Contributor

barmac commented Dec 10, 2024

Released as v1.8.0

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

Successfully merging this pull request may close these issues.

3 participants