-
Notifications
You must be signed in to change notification settings - Fork 243
FOUR-28840: Available self service tasks not displayed for non-admin users due to inefficient query for filtering #8690
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
Open
rodriquelca
wants to merge
6
commits into
develop
Choose a base branch
from
bugfix/FOUR-28840
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+322
−11
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
8a82e0b
FOUR-28840: Available self service tasks not displayed for non-admin …
rodriquelca db81d7f
Add the ability fot task to retrive dinamic fields
rodriquelca 041f951
fix cursor agent notes
rodriquelca 6c8f6b2
fix cursor agent notes 2
rodriquelca 3a9ce16
restore app login
rodriquelca 0a556ce
add id always
rodriquelca File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,181 @@ | ||
| <?php | ||
|
|
||
| namespace Tests\Feature; | ||
|
|
||
| use Illuminate\Foundation\Testing\RefreshDatabase; | ||
| use Illuminate\Support\Facades\DB; | ||
| use ProcessMaker\Models\Group; | ||
| use ProcessMaker\Models\ProcessRequestToken; | ||
| use ProcessMaker\Models\User; | ||
| use Tests\TestCase; | ||
|
|
||
| class SelfServiceOptimizationTest extends TestCase | ||
| { | ||
| use RefreshDatabase; | ||
|
|
||
| /** | ||
| * BULLETPROOF TEST: Verifies that the Subquery approach is 100% equivalent | ||
| * to the Array approach across all possible edge cases and legacy formats. | ||
| */ | ||
| public function test_subquery_optimization_is_bulletproof() | ||
| { | ||
| // 1. Setup Environment | ||
| $user = User::factory()->create(); | ||
| $groupA = Group::factory()->create(['name' => 'Group A']); | ||
| $groupB = Group::factory()->create(['name' => 'Group B']); | ||
| $user->groups()->attach([$groupA->id, $groupB->id]); | ||
|
|
||
| $otherUser = User::factory()->create(); | ||
| $otherGroup = Group::factory()->create(['name' => 'Other Group']); | ||
|
|
||
| // 2. CREATE SCENARIOS | ||
|
|
||
| // Scenario 1: New Format - Int ID in groups array | ||
| $t1 = $this->createSelfServiceTask(['groups' => [$groupA->id]]); | ||
|
|
||
| // Scenario 2: New Format - String ID in groups array (Legacy/JSON inconsistency) | ||
| $t2 = $this->createSelfServiceTask(['groups' => [(string) $groupB->id]]); | ||
|
|
||
| // Scenario 3: Old Format - Direct ID in array (Very old processes) | ||
| $t3 = $this->createSelfServiceTask([$groupA->id]); | ||
|
|
||
| // Scenario 4: Direct User Assignment (Int) | ||
| $t4 = $this->createSelfServiceTask(['users' => [$user->id]]); | ||
|
|
||
| // Scenario 5: Direct User Assignment (String) | ||
| $t5 = $this->createSelfServiceTask(['users' => [(string) $user->id]]); | ||
|
|
||
| // --- NEGATIVE SCENARIOS (Should NEVER be returned) --- | ||
|
|
||
| // Scenario 6: Task for another group | ||
| $t6 = $this->createSelfServiceTask(['groups' => [$otherGroup->id]]); | ||
|
|
||
| // Scenario 7: Task for another user | ||
| $t7 = $this->createSelfServiceTask(['users' => [$otherUser->id]]); | ||
|
|
||
| // Scenario 8: Task is not ACTIVE | ||
| $t8 = $this->createSelfServiceTask(['users' => [$user->id]], 'COMPLETED'); | ||
|
|
||
| // Scenario 9: Task is already assigned to someone | ||
| $t9 = $this->createSelfServiceTask(['users' => [$user->id]], 'ACTIVE', $otherUser->id); | ||
|
|
||
| // 3. THE COMPARISON ENGINE | ||
|
|
||
| // Method A: Array Pluck (Memory intensive, prone to crash) | ||
| $oldWayIds = $user->availableSelfServiceTaskIds()->sort()->values()->toArray(); | ||
|
|
||
| // Method B: Subquery (Optimized, safe) | ||
| $newWayQuery = $user->availableSelfServiceTasksQuery(); | ||
| $resultsNewWay = ProcessRequestToken::whereIn('id', $newWayQuery) | ||
| ->orderBy('id') | ||
| ->pluck('id') | ||
| ->toArray(); | ||
|
|
||
| // 4. ASSERTIONS | ||
|
|
||
| // A. Integrity check: Both lists must be identical | ||
| $this->assertEquals($oldWayIds, $resultsNewWay, 'FATAL: Subquery results differ from Array results!'); | ||
|
|
||
| // B. Coverage check: Ensure all positive scenarios are present | ||
| $expectedIds = [$t1->id, $t2->id, $t3->id, $t4->id, $t5->id]; | ||
| sort($expectedIds); | ||
| $this->assertEquals($expectedIds, $resultsNewWay, 'Subquery missed one of the valid scenarios.'); | ||
|
|
||
| // C. Exclusion check: Ensure none of the negative scenarios leaked in | ||
| $forbiddenIds = [$t6->id, $t7->id, $t8->id, $t9->id]; | ||
| foreach ($forbiddenIds as $id) { | ||
| $this->assertNotContains($id, $resultsNewWay, "Security breach: Task $id should not be visible."); | ||
| } | ||
|
|
||
| // D. Performance Logic check: Subquery must be an instance of Eloquent Builder | ||
| $this->assertInstanceOf(\Illuminate\Database\Eloquent\Builder::class, $newWayQuery); | ||
| } | ||
|
|
||
| /** | ||
| * STRESS TEST: Demonstrates the performance and stability gap. | ||
| * This test creates 10,000 tasks to show how the old way struggles vs the new way. | ||
| */ | ||
| public function test_large_data_performance_and_stability() | ||
| { | ||
| $user = User::factory()->create(); | ||
| $group = Group::factory()->create(); | ||
| $user->groups()->attach($group); | ||
|
|
||
| // Crear dependencias reales para evitar errores de Foreign Key | ||
| $process = \ProcessMaker\Models\Process::factory()->create(); | ||
| $request = \ProcessMaker\Models\ProcessRequest::factory()->create([ | ||
| 'process_id' => $process->id, | ||
| ]); | ||
|
|
||
| echo "\n--- STRESS TEST (10,000 Self-Service Tasks) ---\n"; | ||
|
|
||
| // 1. Seed 10,000 tasks efficiently using bulk insert | ||
| $count = 10000; | ||
| $now = now()->toDateTimeString(); | ||
| $chunkSize = 1000; | ||
|
|
||
| for ($i = 0; $i < $count / $chunkSize; $i++) { | ||
| $tasks = []; | ||
| for ($j = 0; $j < $chunkSize; $j++) { | ||
| $tasks[] = [ | ||
| 'process_id' => $process->id, | ||
| 'process_request_id' => $request->id, | ||
| 'element_id' => 'node_1', | ||
| 'element_type' => 'task', | ||
| 'status' => 'ACTIVE', | ||
| 'is_self_service' => 1, | ||
| 'self_service_groups' => json_encode(['groups' => [$group->id]]), | ||
| 'created_at' => $now, | ||
| 'updated_at' => $now, | ||
| ]; | ||
| } | ||
| DB::table('process_request_tokens')->insert($tasks); | ||
| } | ||
|
|
||
| // 2. Measure OLD WAY (Array of IDs) | ||
| $startMemOld = memory_get_usage(); | ||
| $startTimeOld = microtime(true); | ||
|
|
||
| $ids = $user->availableSelfServiceTaskIds(); | ||
| $resultOld = ProcessRequestToken::whereIn('id', $ids)->count(); | ||
|
|
||
| $timeOld = microtime(true) - $startTimeOld; | ||
| $memOld = (memory_get_usage() - $startMemOld) / 1024 / 1024; | ||
|
|
||
| // 3. Measure NEW WAY (Subquery) | ||
| $startMemNew = memory_get_usage(); | ||
| $startTimeNew = microtime(true); | ||
|
|
||
| $query = $user->availableSelfServiceTasksQuery(); | ||
| $resultNew = ProcessRequestToken::whereIn('id', $query)->count(); | ||
|
|
||
| $timeNew = microtime(true) - $startTimeNew; | ||
| $memNew = (memory_get_usage() - $startMemNew) / 1024 / 1024; | ||
|
|
||
| // OUTPUT RESULTS | ||
| echo 'OLD WAY (Array): Time: ' . number_format($timeOld, 4) . 's | Mem: ' . number_format($memOld, 2) . "MB | Found: $resultOld\n"; | ||
| echo 'NEW WAY (Subquery): Time: ' . number_format($timeNew, 4) . 's | Mem: ' . number_format($memNew, 2) . "MB | Found: $resultNew\n"; | ||
|
|
||
| // ASSERTIONS | ||
| $this->assertEquals($resultOld, $resultNew, 'Results must be identical!'); | ||
|
|
||
| // En base de datos reales (no en memoria), la subconsulta suele ser más rápida | ||
| // Pero lo más importante es que no tiene límites de placeholders | ||
| $this->assertTrue($resultNew > 0); | ||
|
|
||
| echo "----------------------------------------------\n"; | ||
| if ($timeNew > 0) { | ||
| echo 'Optimization: ' . number_format(($timeOld / $timeNew), 1) . "x faster\n"; | ||
| } | ||
| } | ||
|
|
||
| private function createSelfServiceTask($groups, $status = 'ACTIVE', $userId = null) | ||
| { | ||
| return ProcessRequestToken::factory()->create([ | ||
| 'is_self_service' => true, | ||
| 'status' => $status, | ||
| 'user_id' => $userId, | ||
| 'self_service_groups' => $groups, | ||
| ]); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
94 changes: 94 additions & 0 deletions
94
tests/unit/ProcessMaker/Models/SelfServiceComparisonTest.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| <?php | ||
|
|
||
| namespace Tests\Unit\ProcessMaker\Models; | ||
|
|
||
| use Illuminate\Foundation\Testing\RefreshDatabase; | ||
| use ProcessMaker\Models\Group; | ||
| use ProcessMaker\Models\ProcessRequestToken; | ||
| use ProcessMaker\Models\User; | ||
| use Tests\TestCase; | ||
|
|
||
| class SelfServiceComparisonTest extends TestCase | ||
| { | ||
| use RefreshDatabase; | ||
|
|
||
| /** | ||
| * Verifies that the results obtained using the ID list (Array) | ||
| * and the subquery (Query Builder) are exactly the same. | ||
| * | ||
| * @return void | ||
| */ | ||
| public function test_self_service_results_are_identical_between_array_and_subquery() | ||
| { | ||
| // 1. Scenario configuration | ||
| $user = User::factory()->create(); | ||
| $group = Group::factory()->create(); | ||
| $user->groups()->attach($group); | ||
|
|
||
| // Task 1: Available by GROUP (Should appear) | ||
| $task1 = ProcessRequestToken::factory()->create([ | ||
| 'is_self_service' => true, | ||
| 'status' => 'ACTIVE', | ||
| 'user_id' => null, | ||
| 'self_service_groups' => ['groups' => [$group->id]], | ||
| ]); | ||
|
|
||
| // Task 2: Available by direct USER (Should appear) | ||
| $task2 = ProcessRequestToken::factory()->create([ | ||
| 'is_self_service' => true, | ||
| 'status' => 'ACTIVE', | ||
| 'user_id' => null, | ||
| 'self_service_groups' => ['users' => [$user->id]], | ||
| ]); | ||
|
|
||
| // Task 3: NOT available (Another group) | ||
| ProcessRequestToken::factory()->create([ | ||
| 'is_self_service' => true, | ||
| 'status' => 'ACTIVE', | ||
| 'user_id' => null, | ||
| 'self_service_groups' => ['groups' => [9999]], | ||
| ]); | ||
|
|
||
| // Task 4: NOT available (Already completed) | ||
| ProcessRequestToken::factory()->create([ | ||
| 'is_self_service' => true, | ||
| 'status' => 'COMPLETED', | ||
| 'user_id' => null, | ||
| 'self_service_groups' => ['groups' => [$group->id]], | ||
| ]); | ||
|
|
||
| // 2. Execution of both methods | ||
|
|
||
| // Method A: Using the IDs array (Preserved behavior) | ||
| $idsArray = $user->availableSelfServiceTaskIds(); | ||
| $resultsFromArray = ProcessRequestToken::whereIn('id', $idsArray) | ||
| ->pluck('id') | ||
| ->sort() | ||
| ->values() | ||
| ->toArray(); | ||
|
|
||
| // Method B: Using the subquery (New optimization) | ||
| $subqueryBuilder = $user->availableSelfServiceTasksQuery(); | ||
| $resultsFromSubquery = ProcessRequestToken::whereIn('id', $subqueryBuilder) | ||
| ->pluck('id') | ||
| ->sort() | ||
| ->values() | ||
| ->toArray(); | ||
|
|
||
| // 3. Verification | ||
|
|
||
| // Check that tasks were found | ||
| $this->assertCount(2, $resultsFromArray, 'Exactly 2 tasks should have been found with the old method.'); | ||
|
|
||
| // Check that both methods return exactly the same results | ||
| $this->assertEquals( | ||
| $resultsFromArray, | ||
| $resultsFromSubquery, | ||
| 'Task IDs found by both methods MUST be identical.' | ||
| ); | ||
|
|
||
| // Verify specific IDs are correct | ||
| $this->assertContains($task1->id, $resultsFromSubquery); | ||
| $this->assertContains($task2->id, $resultsFromSubquery); | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.