Sort out problem with hasCandidateGroup about camunda-bpm-assert HOT 14 CLOSED

I do confirm that the fix from Thorben works. Do you already know when you will fix this, or shall I take over?

from camunda-bpm-assert.

Many thx for this notice. You are of course welcome to complete the job!

from camunda-bpm-assert.

The fix "includeAssignedTasks" is not available in the 7.1 taskService API. What do you think: is the moment where we switch to 7.2 (and push this project to 2.0) ?

from camunda-bpm-assert.

Mmh. I understand. But then it's even questionable whether it's a bug from that point of view... I would rather not propose to change the behaviour of an existing assertion for a higher Camunda BPM version, but rather introduce another one, probably overloading the existing method with one that has a boolean flag parameter "includeAssignedTasks" (and delegate the old implementation to the new one). What do you think?

When it comes to 7.2., feel free to just adapt the version for that fix, but please provide it as pull request then. I will then merge it before a coming release.

from camunda-bpm-assert.

I found a quite elegant solution that even allows better assertionErrors than before: instead of using the taskQuery() API, I do a native SQL query on ACT_RU_IDENTITYLINK. This not even fixes the problem with assigned Tasks but also allows hasCandidateGroups and hasCandidateUsers as a side aspect. Check it out, I am quite confident with it.

from camunda-bpm-assert.

@jangalinski Sorry for reverting this - I do not want to have things that need to be discussed in the master branch, because people are forking this. No offense, but please provide this as a pull request, as I asked you in my comment above.

from camunda-bpm-assert.

Furthermore, can you please make sure that you just change what's necessary and do not reformat code. I agree that the formatting could be improved in this project, but it makes the work really hard when this kind of improvement is not done in a separate commit.

from camunda-bpm-assert.

@martinschimak I would not call this issue "improve". This is clearly a bug, the assertions give the wrong answer.

  assertThat(pi).hasCandidateGroup("group"); // test ok
  claim(task(),"user");
  assertThat(pi).hasCandidateGroup("group"); // test not ok though nothing changed

from camunda-bpm-assert.

@zambrovski @jangalinski I want to sort this out now, also why I changed this from bug to improvement in the first place, so let me go through the whole issue:

1. My understanding is that Camunda did not change the behaviour behind its API:

   taskQuery.taskId(taskId).taskCandidateGroup(candidateGroupId).singleResult();

   returns the same result across all versions of Camunda BPM since 7.0: it returns unassigned tasks associated to a candidate group.

2. Because this is somewhat unfortunate, but acceptable if documented, Camunda introduced since 7.2 the possibility to include assigned tasks without changing the known default behaviour:

   taskQuery.taskId(taskId).taskCandidateGroup(candidateGroupId)
     .includeAssignedTasks().singleResult();

   The javadoc for TaskQuery#taskCandidateGroup says since then: "Per default it only selects tasks which are not already assigned to a user. To also include assigned tasks in the result specify includeAssignedTasks() in your query."

3. The assertion hasCandidateGroup(String candidateGroupId) was implemented against the 7.0/7.1 API and always used the simple query shown above in 1. - iow does not include assigned tasks - and sticks to that behaviour consistently across all Camunda BPM versions.

   The javadoc for TaskAssert#hasCandidateGroup always said: "Verifies the expectation that the Task is currently waiting to be assigned to a user of the specified candidate group." In my mind this makes also very clear that assigned tasks are not included.

4. Having said that, this behaviour of TaskAssert#hasCandidateGroup is probably equally unfortunate as Camunda's default behaviour of TaskQuery#taskCandidateGroup. But I also did not want to change the behaviour of an existing method. Therefore I wrote above:

   "I would rather not propose to change the behaviour of an existing assertion for a higher Camunda BPM version, but rather introduce another one, probably overloading the existing method with one that has a boolean flag parameter "includeAssignedTasks" (and delegate the old implementation to the new one)."

   What I meant is a new signature like this

   public TaskAssert hasCandidateGroup(String candidateGroupId, boolean includeAssignedTasks)
   

   Even though I was in the meantime inclined to follow @jangalinski to look at this to be a bug (because the default behaviour is so unfortunate) I do believe now, it's probably better to revert to my original position, leave the old assertion as it is, and introduce this new one. Because the intuitive understanding of whether assigned tasks are included or not could continue to differ, it's maybe even better to deprecate the old hasCandidateGroup(String candidateGroupId)assertion. Then the user would always need to conciously decide whether to include assigned tasks or not by setting the boolean flag.

from camunda-bpm-assert.

I like this. Good summary @martinschimak!

from camunda-bpm-assert.

I still believe that the answer to the question "hasCandidateGroup" should not be different if the task is claimed or not, regardless of what the default behaviour of the taskQuery is. So for me, that always was a bug.
And I am not a fan of boolean switches in signatures.

from camunda-bpm-assert.

As I told you by phone, Camunda defines the term "candidate user" as something that is not assigned. I can actually follow this argumentation. There is an artifical and a little technical usage of a the task API when you set both, the candidate user (so a user which can potentially claim a task) and assign it to the same time - for me a task at runtime is not just a POJO where you can expect getter/setters to be unrelated, but a piece of art with behaviour in it.

from camunda-bpm-assert.

@jangalinski said:

"I still believe that the answer to the question "hasCandidateGroup" should not be different if the task is claimed or not, regardless of what the default behaviour of the taskQuery is."

I understand that, but I do believe now that we should accept that the intuitive understanding can differ from person to person... and even worse the current assertion was always documented to check whether the task is "waiting to be assigned to a user of the specified candidate group". So I do not want to just change the behaviour behind that assertion.

@jangalinski also said:

"And I am not a fan of boolean switches in signatures."

Me neither. Especially not in an API like this - which should by design contribute to clearly readable code. On top of that, my proposed parameter name includeAssignedTasks makes sense in the context of narrowing down a query, but much less sense in the context of the hasCandidateGroup assertion... what do you think about

assertThat(task).isAssociatedToGroup("muppets");

for assigned as well as unassigned tasks from 7.2 upwards so that we can deprecate hasCandidateGroup but leave it as is for the moment without changing its known (and documented) behaviour. The assertions isAssignedTo("kermit") as well as isNotAssigned() already exist anyway... when being interested in unassigned tasks for a specific candidateGroup, one could therefore write in future:

assertThat(task).isAssociatedToGroup("muppets").isNotAssigned();

What do you think? If you like it, we could brainstorm names for the new assertion

• isAssociatedToGroup
• isCandidateForGroup (sounds nice, but suggests that the task is a candidate for a user, whereas actually a user is a candidate for a task in the existing terminology...)
• isAssignableToGroup (sounds also nice, but maybe suggests, that an assignee outside of the group is forbidden?)
• ... ?

from camunda-bpm-assert.

Hi Martin

I must admit that my comment "camunda changed the behaviour" was misleading. My point is, that, besides what the default taskQuery returns, the assertion should be true when an identityLink for that candidateGroup exists, independent of the assignment state of the task. From that point of view, the behaviour of hasCandidateGroup was buggy from the beginning.
I can see that you do not want to change the behaviour of that method because it has been public api for a while now and changing could break tests of projects using this.

What we do quite often is something we call "auto assign", where the taskListener#create gets a candidateGroup but is also assigned to a user in one step. This is useful for use cases like "first task of manually started process is assigned to the user who started the instance" or the ui mediator where the next task is automatically claimed. In those cases, to verify the correct behaviour of the listener, we have to assert "hasCandidateGroup" and "isAssignedTo" at once. And then the assert framework is not helpful in its current implementation.

I do not want to keep a "religious" discussion going on forever, so I would agree to add an additional method hasCandidateGroup(String groupId, boolean incluseAssigned). That would then be the only method I'd use then in future.

Looking at the implementation details: if we'd use the taskService.getIdentityLinks() to determine all candidateGroups of a task, we could do a simple SetAssert.contains and even get a more meaningful error message where we can directly see the actual candidateGroups instead of just "expected to have but doesn't".

Cu
Jan

from camunda-bpm-assert.