Skip to content

Commit 7524e63

Browse files
nolanpronolanpro
authored
Merge pull request #8690 from ProcessMaker/bugfix/FOUR-28840
FOUR-28840: Available self service tasks not displayed for non-admin users due to inefficient query for filtering
2 parents f7c67e6 + 0a556ce commit 7524e63

8 files changed

Lines changed: 322 additions & 11 deletions

File tree

‎ProcessMaker/Http/Controllers/Api/TaskController.php‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,18 @@ public function index(Request $request, $getTotal = false, User $user = null)
134134

135135
$query = $this->indexBaseQuery($request);
136136

137+
// Get fields from request (sent by frontend)
138+
// If not provided, don't apply select() to maintain backward compatibility (returns all columns)
139+
$fields = $request->input('fields', '');
140+
if ($fields) {
141+
$selectedFields = explode(',', $fields);
142+
// Ensure 'id' is always included for internal logic (e.g., inOverdueQuery at line ~186)
143+
if (!in_array('id', $selectedFields)) {
144+
$selectedFields[] = 'id';
145+
}
146+
$query = $query->select($selectedFields);
147+
}
148+
137149
$this->applyFilters($query, $request);
138150

139151
$this->excludeNonVisibleTasks($query, $request);

‎ProcessMaker/Models/ProcessRequestToken.php‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -783,8 +783,7 @@ public function valueAliasStatus($value, $expression, $callback = null, User $us
783783

784784
$query->whereIn('process_request_tokens.id', $selfServiceTaskIds);
785785
} elseif ($user) {
786-
$taskIds = $user->availableSelfServiceTaskIds();
787-
$query->whereIn('process_request_tokens.id', $taskIds);
786+
$query->whereIn('process_request_tokens.id', $user->availableSelfServiceTasksQuery());
788787
} else {
789788
$query->where('process_request_tokens.is_self_service', 1);
790789
}

‎ProcessMaker/Models/User.php‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -466,11 +466,11 @@ public function removeFromGroups()
466466
$this->groups()->detach();
467467
}
468468

469-
public function availableSelfServiceTaskIds()
469+
public function availableSelfServiceTasksQuery()
470470
{
471471
$groupIds = $this->groups()->pluck('groups.id');
472472

473-
$taskQuery = ProcessRequestToken::select(['id'])
473+
$taskQuery = ProcessRequestToken::select(['process_request_tokens.id'])
474474
->where([
475475
'is_self_service' => true,
476476
'status' => 'ACTIVE',
@@ -490,7 +490,12 @@ public function availableSelfServiceTaskIds()
490490
$query->orWhereJsonContains('self_service_groups->users', (string) $this->id);
491491
});
492492

493-
return $taskQuery->pluck('id');
493+
return $taskQuery;
494+
}
495+
496+
public function availableSelfServiceTaskIds()
497+
{
498+
return $this->availableSelfServiceTasksQuery()->pluck('id');
494499
}
495500

496501
/**

‎ProcessMaker/Traits/TaskControllerIndexMethods.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -383,7 +383,7 @@ private function applyForCurrentUser($query, $user)
383383

384384
$query->where(function ($query) use ($user) {
385385
$query->where('user_id', $user->id)
386-
->orWhereIn('id', $user->availableSelfServiceTaskIds());
386+
->orWhereIn('id', $user->availableSelfServiceTasksQuery());
387387
});
388388
}
389389

‎resources/js/requests/components/RequestDetail.vue‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,13 +184,33 @@ export default {
184184
this.status
185185
}&per_page=${
186186
this.perPage
187-
}${this.getSortParam()}`,
187+
}${this.getSortParam()}${this.getColumnsParam()}`,
188188
)
189189
.then((response) => {
190190
this.data = this.transform(response.data);
191191
this.loading = false;
192192
});
193193
},
194+
/**
195+
* Get the fields parameter for the API request
196+
* @returns {string} The fields parameter for the API request
197+
*/
198+
getColumnsParam() {
199+
const fields = [
200+
'id',
201+
'element_id', // Required by assignableUsers relationship (TokenAssignableUsers::match uses element_id)
202+
'element_name',
203+
'user_id',
204+
'process_id',
205+
'process_request_id',
206+
'status',
207+
'due_at',
208+
'is_self_service',
209+
'is_actionbyemail',
210+
'self_service_groups', // Required by Task resource's addAssignableUsers() method when recalculating assignable users
211+
];
212+
return `&fields=${fields.join(',')}`;
213+
},
194214
getSortParam() {
195215
if (this.sortOrder instanceof Array && this.sortOrder.length > 0) {
196216
return (

‎tests/Feature/SelfServiceOptimizationTest.php‎

Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
<?php
2+
3+
namespace Tests\Feature;
4+
5+
use Illuminate\Foundation\Testing\RefreshDatabase;
6+
use Illuminate\Support\Facades\DB;
7+
use ProcessMaker\Models\Group;
8+
use ProcessMaker\Models\ProcessRequestToken;
9+
use ProcessMaker\Models\User;
10+
use Tests\TestCase;
11+
12+
class SelfServiceOptimizationTest extends TestCase
13+
{
14+
use RefreshDatabase;
15+
16+
/**
17+
* BULLETPROOF TEST: Verifies that the Subquery approach is 100% equivalent
18+
* to the Array approach across all possible edge cases and legacy formats.
19+
*/
20+
public function test_subquery_optimization_is_bulletproof()
21+
{
22+
// 1. Setup Environment
23+
$user = User::factory()->create();
24+
$groupA = Group::factory()->create(['name' => 'Group A']);
25+
$groupB = Group::factory()->create(['name' => 'Group B']);
26+
$user->groups()->attach([$groupA->id, $groupB->id]);
27+
28+
$otherUser = User::factory()->create();
29+
$otherGroup = Group::factory()->create(['name' => 'Other Group']);
30+
31+
// 2. CREATE SCENARIOS
32+
33+
// Scenario 1: New Format - Int ID in groups array
34+
$t1 = $this->createSelfServiceTask(['groups' => [$groupA->id]]);
35+
36+
// Scenario 2: New Format - String ID in groups array (Legacy/JSON inconsistency)
37+
$t2 = $this->createSelfServiceTask(['groups' => [(string) $groupB->id]]);
38+
39+
// Scenario 3: Old Format - Direct ID in array (Very old processes)
40+
$t3 = $this->createSelfServiceTask([$groupA->id]);
41+
42+
// Scenario 4: Direct User Assignment (Int)
43+
$t4 = $this->createSelfServiceTask(['users' => [$user->id]]);
44+
45+
// Scenario 5: Direct User Assignment (String)
46+
$t5 = $this->createSelfServiceTask(['users' => [(string) $user->id]]);
47+
48+
// --- NEGATIVE SCENARIOS (Should NEVER be returned) ---
49+
50+
// Scenario 6: Task for another group
51+
$t6 = $this->createSelfServiceTask(['groups' => [$otherGroup->id]]);
52+
53+
// Scenario 7: Task for another user
54+
$t7 = $this->createSelfServiceTask(['users' => [$otherUser->id]]);
55+
56+
// Scenario 8: Task is not ACTIVE
57+
$t8 = $this->createSelfServiceTask(['users' => [$user->id]], 'COMPLETED');
58+
59+
// Scenario 9: Task is already assigned to someone
60+
$t9 = $this->createSelfServiceTask(['users' => [$user->id]], 'ACTIVE', $otherUser->id);
61+
62+
// 3. THE COMPARISON ENGINE
63+
64+
// Method A: Array Pluck (Memory intensive, prone to crash)
65+
$oldWayIds = $user->availableSelfServiceTaskIds()->sort()->values()->toArray();
66+
67+
// Method B: Subquery (Optimized, safe)
68+
$newWayQuery = $user->availableSelfServiceTasksQuery();
69+
$resultsNewWay = ProcessRequestToken::whereIn('id', $newWayQuery)
70+
->orderBy('id')
71+
->pluck('id')
72+
->toArray();
73+
74+
// 4. ASSERTIONS
75+
76+
// A. Integrity check: Both lists must be identical
77+
$this->assertEquals($oldWayIds, $resultsNewWay, 'FATAL: Subquery results differ from Array results!');
78+
79+
// B. Coverage check: Ensure all positive scenarios are present
80+
$expectedIds = [$t1->id, $t2->id, $t3->id, $t4->id, $t5->id];
81+
sort($expectedIds);
82+
$this->assertEquals($expectedIds, $resultsNewWay, 'Subquery missed one of the valid scenarios.');
83+
84+
// C. Exclusion check: Ensure none of the negative scenarios leaked in
85+
$forbiddenIds = [$t6->id, $t7->id, $t8->id, $t9->id];
86+
foreach ($forbiddenIds as $id) {
87+
$this->assertNotContains($id, $resultsNewWay, "Security breach: Task $id should not be visible.");
88+
}
89+
90+
// D. Performance Logic check: Subquery must be an instance of Eloquent Builder
91+
$this->assertInstanceOf(\Illuminate\Database\Eloquent\Builder::class, $newWayQuery);
92+
}
93+
94+
/**
95+
* STRESS TEST: Demonstrates the performance and stability gap.
96+
* This test creates 10,000 tasks to show how the old way struggles vs the new way.
97+
*/
98+
public function test_large_data_performance_and_stability()
99+
{
100+
$user = User::factory()->create();
101+
$group = Group::factory()->create();
102+
$user->groups()->attach($group);
103+
104+
// Crear dependencias reales para evitar errores de Foreign Key
105+
$process = \ProcessMaker\Models\Process::factory()->create();
106+
$request = \ProcessMaker\Models\ProcessRequest::factory()->create([
107+
'process_id' => $process->id,
108+
]);
109+
110+
echo "\n--- STRESS TEST (10,000 Self-Service Tasks) ---\n";
111+
112+
// 1. Seed 10,000 tasks efficiently using bulk insert
113+
$count = 10000;
114+
$now = now()->toDateTimeString();
115+
$chunkSize = 1000;
116+
117+
for ($i = 0; $i < $count / $chunkSize; $i++) {
118+
$tasks = [];
119+
for ($j = 0; $j < $chunkSize; $j++) {
120+
$tasks[] = [
121+
'process_id' => $process->id,
122+
'process_request_id' => $request->id,
123+
'element_id' => 'node_1',
124+
'element_type' => 'task',
125+
'status' => 'ACTIVE',
126+
'is_self_service' => 1,
127+
'self_service_groups' => json_encode(['groups' => [$group->id]]),
128+
'created_at' => $now,
129+
'updated_at' => $now,
130+
];
131+
}
132+
DB::table('process_request_tokens')->insert($tasks);
133+
}
134+
135+
// 2. Measure OLD WAY (Array of IDs)
136+
$startMemOld = memory_get_usage();
137+
$startTimeOld = microtime(true);
138+
139+
$ids = $user->availableSelfServiceTaskIds();
140+
$resultOld = ProcessRequestToken::whereIn('id', $ids)->count();
141+
142+
$timeOld = microtime(true) - $startTimeOld;
143+
$memOld = (memory_get_usage() - $startMemOld) / 1024 / 1024;
144+
145+
// 3. Measure NEW WAY (Subquery)
146+
$startMemNew = memory_get_usage();
147+
$startTimeNew = microtime(true);
148+
149+
$query = $user->availableSelfServiceTasksQuery();
150+
$resultNew = ProcessRequestToken::whereIn('id', $query)->count();
151+
152+
$timeNew = microtime(true) - $startTimeNew;
153+
$memNew = (memory_get_usage() - $startMemNew) / 1024 / 1024;
154+
155+
// OUTPUT RESULTS
156+
echo 'OLD WAY (Array): Time: ' . number_format($timeOld, 4) . 's | Mem: ' . number_format($memOld, 2) . "MB | Found: $resultOld\n";
157+
echo 'NEW WAY (Subquery): Time: ' . number_format($timeNew, 4) . 's | Mem: ' . number_format($memNew, 2) . "MB | Found: $resultNew\n";
158+
159+
// ASSERTIONS
160+
$this->assertEquals($resultOld, $resultNew, 'Results must be identical!');
161+
162+
// En base de datos reales (no en memoria), la subconsulta suele ser más rápida
163+
// Pero lo más importante es que no tiene límites de placeholders
164+
$this->assertTrue($resultNew > 0);
165+
166+
echo "----------------------------------------------\n";
167+
if ($timeNew > 0) {
168+
echo 'Optimization: ' . number_format(($timeOld / $timeNew), 1) . "x faster\n";
169+
}
170+
}
171+
172+
private function createSelfServiceTask($groups, $status = 'ACTIVE', $userId = null)
173+
{
174+
return ProcessRequestToken::factory()->create([
175+
'is_self_service' => true,
176+
'status' => $status,
177+
'user_id' => $userId,
178+
'self_service_groups' => $groups,
179+
]);
180+
}
181+
}

‎tests/unit/ProcessMaker/FilterTest.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -227,10 +227,10 @@ public function testTaskStatusSelfservice()
227227
],
228228
], ProcessRequestToken::class);
229229

230-
$this->assertEquals(
231-
"select * from `process_request_tokens` where ((`process_request_tokens`.`id` in ({$selfServiceTask->id})))",
232-
$sql
233-
);
230+
$this->assertStringContainsString('select * from `process_request_tokens` where ((`process_request_tokens`.`id` in (select `process_request_tokens`.`id` from `process_request_tokens` where', $sql);
231+
$this->assertStringContainsString('`is_self_service` = 1', $sql);
232+
$this->assertStringContainsString("`status` = 'ACTIVE'", $sql);
233+
$this->assertStringContainsString('json_contains(`self_service_groups`', $sql);
234234
}
235235

236236
public function testTaskStatusActive()

‎tests/unit/ProcessMaker/Models/SelfServiceComparisonTest.php‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
<?php
2+
3+
namespace Tests\Unit\ProcessMaker\Models;
4+
5+
use Illuminate\Foundation\Testing\RefreshDatabase;
6+
use ProcessMaker\Models\Group;
7+
use ProcessMaker\Models\ProcessRequestToken;
8+
use ProcessMaker\Models\User;
9+
use Tests\TestCase;
10+
11+
class SelfServiceComparisonTest extends TestCase
12+
{
13+
use RefreshDatabase;
14+
15+
/**
16+
* Verifies that the results obtained using the ID list (Array)
17+
* and the subquery (Query Builder) are exactly the same.
18+
*
19+
* @return void
20+
*/
21+
public function test_self_service_results_are_identical_between_array_and_subquery()
22+
{
23+
// 1. Scenario configuration
24+
$user = User::factory()->create();
25+
$group = Group::factory()->create();
26+
$user->groups()->attach($group);
27+
28+
// Task 1: Available by GROUP (Should appear)
29+
$task1 = ProcessRequestToken::factory()->create([
30+
'is_self_service' => true,
31+
'status' => 'ACTIVE',
32+
'user_id' => null,
33+
'self_service_groups' => ['groups' => [$group->id]],
34+
]);
35+
36+
// Task 2: Available by direct USER (Should appear)
37+
$task2 = ProcessRequestToken::factory()->create([
38+
'is_self_service' => true,
39+
'status' => 'ACTIVE',
40+
'user_id' => null,
41+
'self_service_groups' => ['users' => [$user->id]],
42+
]);
43+
44+
// Task 3: NOT available (Another group)
45+
ProcessRequestToken::factory()->create([
46+
'is_self_service' => true,
47+
'status' => 'ACTIVE',
48+
'user_id' => null,
49+
'self_service_groups' => ['groups' => [9999]],
50+
]);
51+
52+
// Task 4: NOT available (Already completed)
53+
ProcessRequestToken::factory()->create([
54+
'is_self_service' => true,
55+
'status' => 'COMPLETED',
56+
'user_id' => null,
57+
'self_service_groups' => ['groups' => [$group->id]],
58+
]);
59+
60+
// 2. Execution of both methods
61+
62+
// Method A: Using the IDs array (Preserved behavior)
63+
$idsArray = $user->availableSelfServiceTaskIds();
64+
$resultsFromArray = ProcessRequestToken::whereIn('id', $idsArray)
65+
->pluck('id')
66+
->sort()
67+
->values()
68+
->toArray();
69+
70+
// Method B: Using the subquery (New optimization)
71+
$subqueryBuilder = $user->availableSelfServiceTasksQuery();
72+
$resultsFromSubquery = ProcessRequestToken::whereIn('id', $subqueryBuilder)
73+
->pluck('id')
74+
->sort()
75+
->values()
76+
->toArray();
77+
78+
// 3. Verification
79+
80+
// Check that tasks were found
81+
$this->assertCount(2, $resultsFromArray, 'Exactly 2 tasks should have been found with the old method.');
82+
83+
// Check that both methods return exactly the same results
84+
$this->assertEquals(
85+
$resultsFromArray,
86+
$resultsFromSubquery,
87+
'Task IDs found by both methods MUST be identical.'
88+
);
89+
90+
// Verify specific IDs are correct
91+
$this->assertContains($task1->id, $resultsFromSubquery);
92+
$this->assertContains($task2->id, $resultsFromSubquery);
93+
}
94+
}

0 commit comments

Comments
 (0)