Skip to content

Commit 91f9080

Browse files
committed
fix(FOUR-32830): evaluate assignment rules per rule when resolving assignees
Replace getAssignees plus in_array second pass with a single loop that evaluates each rule via isAssignmentRuleMatch. This prevents unmatched group rules from expanding when they share an assignee id with a matched user rule, and avoids losing group members when manager_id collides with sequential indexes from getConsolidatedUsers. - Merge group users into userIds keyed by user id - Reuse one ExpressionLanguage instance per loop - Add unit tests for assignee id collision and manager id collision cases https://processmaker.atlassian.net/browse/FOUR-32830
1 parent a84c9e5 commit 91f9080

2 files changed

Lines changed: 124 additions & 19 deletions

File tree

‎ProcessMaker/Models/ProcessRequestToken.php‎

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1074,21 +1074,7 @@ public function getAssignees(array $assignments, array $variables): array
10741074
$language = new ExpressionLanguage();
10751075

10761076
foreach ($assignments as $assignment) {
1077-
$isTrue = false;
1078-
1079-
if (!empty($assignment['expression'])) {
1080-
try {
1081-
$isTrue = $language->evaluate($assignment['expression'], $variables);
1082-
} catch (Throwable $e) {
1083-
$isTrue = false;
1084-
}
1085-
}
1086-
1087-
if ($isTrue) {
1088-
$result[] = $assignment['assignee'];
1089-
}
1090-
1091-
if (isset($assignment['default']) && $assignment['default'] === true) {
1077+
if ($this->isAssignmentRuleMatch($assignment, $variables, $language)) {
10921078
$result[] = $assignment['assignee'];
10931079
}
10941080
}
@@ -1110,16 +1096,21 @@ public function getAssigneesFromExpression(string|array $form_data): array
11101096
$assignmentRules = $activity->getProperty('assignmentRules', null);
11111097
$assignments = json_decode($assignmentRules, true) ?? [];
11121098

1113-
$assigneeIds = $this->getAssignees($assignments, $formData);
11141099
$userIds = [];
1115-
1100+
$language = new ExpressionLanguage();
11161101
foreach ($assignments as $assignment) {
1117-
if (!in_array($assignment['assignee'], $assigneeIds, true)) {
1102+
if (!$this->isAssignmentRuleMatch($assignment, $formData, $language)) {
11181103
continue;
11191104
}
11201105

11211106
if (($assignment['type'] ?? 'user') === 'group') {
1122-
$this->process->getConsolidatedUsers($assignment['assignee'], $userIds);
1107+
$groupUsers = [];
1108+
$this->process->getConsolidatedUsers($assignment['assignee'], $groupUsers);
1109+
foreach ($groupUsers as $userId) {
1110+
if (!empty($userId) && is_numeric($userId)) {
1111+
$userIds[$userId] = $userId;
1112+
}
1113+
}
11231114
} else {
11241115
$userIds[$assignment['assignee']] = $assignment['assignee'];
11251116
}
@@ -1134,6 +1125,23 @@ public function getAssigneesFromExpression(string|array $form_data): array
11341125
return array_values($userIds);
11351126
}
11361127

1128+
private function isAssignmentRuleMatch(array $assignment, array $variables, ExpressionLanguage $language): bool
1129+
{
1130+
if (isset($assignment['default']) && $assignment['default'] === true) {
1131+
return true;
1132+
}
1133+
1134+
if (empty($assignment['expression'])) {
1135+
return false;
1136+
}
1137+
1138+
try {
1139+
return $language->evaluate($assignment['expression'], $variables);
1140+
} catch (Throwable $e) {
1141+
return false;
1142+
}
1143+
}
1144+
11371145
/**
11381146
* Returns if the token has the self service option activated
11391147
*/

‎tests/Model/ProcessRequestTokenTest.php‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -471,4 +471,101 @@ public function testGetAssigneesFromExpressionAcceptsArrayFormData()
471471
$resultFromString = $token->getAssigneesFromExpression(json_encode($formData));
472472
$this->assertEquals($result, $resultFromString);
473473
}
474+
475+
public function testGetAssigneesFromExpressionDoesNotExpandUnmatchedGroupWithSameAssigneeId()
476+
{
477+
$assignableUser = User::factory()->create(['status' => 'ACTIVE']);
478+
$sharedAssigneeId = $assignableUser->id;
479+
$rules = [
480+
['type' => 'user', 'assignee' => $sharedAssigneeId, 'expression' => 'TestVar<10'],
481+
['type' => 'group', 'assignee' => $sharedAssigneeId, 'expression' => 'TestVar>10'],
482+
];
483+
484+
$activity = $this->createMock(\ProcessMaker\Nayra\Contracts\Bpmn\ActivityInterface::class);
485+
$activity->method('getProperty')
486+
->willReturnCallback(function ($key, $default) use ($rules) {
487+
if ($key === 'assignmentRules') {
488+
return json_encode($rules);
489+
}
490+
491+
return $default;
492+
});
493+
494+
$bpmnDefinition = $this->createMock(\ProcessMaker\Nayra\Storage\BpmnElement::class);
495+
$bpmnDefinition->method('getBpmnElementInstance')
496+
->willReturn($activity);
497+
498+
$process = $this->createMock(Process::class);
499+
$process->expects($this->never())->method('getConsolidatedUsers');
500+
501+
$request = ProcessRequest::factory()->create();
502+
503+
$token = $this->getMockBuilder(ProcessRequestToken::class)
504+
->onlyMethods(['getBpmnDefinition'])
505+
->getMock();
506+
507+
$token->process_id = $request->process_id;
508+
$token->process_request_id = $request->id;
509+
$token->process = $process;
510+
511+
$token->expects($this->atLeastOnce())
512+
->method('getBpmnDefinition')
513+
->willReturn($bpmnDefinition);
514+
515+
$result = $token->getAssigneesFromExpression(['TestVar' => 5]);
516+
517+
$this->assertEquals([$sharedAssigneeId], $result);
518+
}
519+
520+
public function testGetAssigneesFromExpressionPreservesGroupMembersWhenManagerIdCollides()
521+
{
522+
$groupUsers = User::factory()->count(3)->create(['status' => 'ACTIVE']);
523+
$group = Group::factory()->create();
524+
foreach ($groupUsers as $groupUser) {
525+
GroupMember::factory()->create([
526+
'group_id' => $group->id,
527+
'member_id' => $groupUser->id,
528+
'member_type' => User::class,
529+
]);
530+
}
531+
532+
$process = Process::factory()->create(['manager_id' => $groupUsers[1]->id]);
533+
$request = ProcessRequest::factory()->create(['process_id' => $process->id]);
534+
535+
$rules = [
536+
['type' => 'group', 'assignee' => $group->id, 'expression' => 'TestVar<10'],
537+
];
538+
539+
$activity = $this->createMock(\ProcessMaker\Nayra\Contracts\Bpmn\ActivityInterface::class);
540+
$activity->method('getProperty')
541+
->willReturnCallback(function ($key, $default) use ($rules) {
542+
if ($key === 'assignmentRules') {
543+
return json_encode($rules);
544+
}
545+
546+
return $default;
547+
});
548+
549+
$bpmnDefinition = $this->createMock(\ProcessMaker\Nayra\Storage\BpmnElement::class);
550+
$bpmnDefinition->method('getBpmnElementInstance')
551+
->willReturn($activity);
552+
553+
$token = $this->getMockBuilder(ProcessRequestToken::class)
554+
->onlyMethods(['getBpmnDefinition'])
555+
->getMock();
556+
557+
$token->process_id = $process->id;
558+
$token->process_request_id = $request->id;
559+
$token->process = $process;
560+
561+
$token->expects($this->atLeastOnce())
562+
->method('getBpmnDefinition')
563+
->willReturn($bpmnDefinition);
564+
565+
$result = $token->getAssigneesFromExpression(['TestVar' => 5]);
566+
567+
foreach ($groupUsers as $groupUser) {
568+
$this->assertContains($groupUser->id, $result);
569+
}
570+
}
474571
}

0 commit comments

Comments
 (0)