app-profile#274
Open
BrunoSFreschi wants to merge 2 commits into
Open
Conversation
…nd eager creation on tenant join
📝 WalkthroughWalkthroughThis pull request introduces a new Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app-modules/profile/src/ProfileServiceProvider.php`:
- Around line 25-26: The SQL matching is too strict: update the INSERT detection
in ProfileServiceProvider (the preg_match that checks $event->sql) to accept
optional leading whitespace, optional schema qualification, and either backtick
or double-quote quoting around identifiers (tenant_users), e.g. allow
`schema`.`tenant_users`, "schema"."tenant_users", or tenant_users; similarly,
harden insertColumns() to strip/normalize backticks and double-quotes and to
handle schema-qualified table names before extracting column lists — normalize
the SQL (lowercase for matching, remove/replace surrounding quotes on
identifiers) then apply relaxed regexes so schema-qualified and backtick-quoted
INSERTs are correctly detected and parsed.
In `@app-modules/profile/tests/Feature/ProfileCreationTest.php`:
- Around line 68-77: The test is relying on ordered tenant_id values which is
flaky with UUIDs; remove the explicit orderBy('tenant_id') on the
Profile::query() call and change the assertion that checks tenant IDs (the
expect(...)->toHaveCount(2) and ->and($profiles->pluck('tenant_id')->all())
portion) to assert membership regardless of order (e.g., compare as unordered
sets or use an assertion that canonicalizes/sorts both arrays) so the test
verifies the two tenant IDs are present without depending on their order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c1bc62ca-283d-42d9-bc68-ea5b7468afff
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
app-modules/profile/composer.jsonapp-modules/profile/database/factories/ProfileFactory.phpapp-modules/profile/database/migrations/2026_05_21_000000_create_user_profiles_table.phpapp-modules/profile/phpstan.ignore.neonapp-modules/profile/phpstan.neonapp-modules/profile/src/Enums/SeniorityLevel.phpapp-modules/profile/src/Enums/SocialPlatform.phpapp-modules/profile/src/Enums/StartAvailability.phpapp-modules/profile/src/Listeners/CreateProfileForTenantMember.phpapp-modules/profile/src/Models/Profile.phpapp-modules/profile/src/ProfileServiceProvider.phpapp-modules/profile/tests/Feature/ProfileCreationTest.phpapp-modules/profile/tests/Unit/ProfileEnumTest.phpapp-modules/profile/tests/Unit/ProfileSocialLinksTest.phpcomposer.json
Comment on lines
+25
to
+26
| if (! preg_match('/^insert into "?tenant_users"?/i', $event->sql)) { | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and print the relevant sections with line numbers
FILE="app-modules/profile/src/ProfileServiceProvider.php"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
nl -ba "$FILE" | sed -n '1,120p'Repository: he4rt/heartdevs.com
Length of output: 107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for tenant_users / preg_match patterns in the file
FILE="app-modules/profile/src/ProfileServiceProvider.php"
rg -n "tenant_users|preg_match|QueryExecuted|sql" "$FILE"Repository: he4rt/heartdevs.com
Length of output: 472
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="app-modules/profile/src/ProfileServiceProvider.php"
# show around the listener and helper
sed -n '15,90p' "$FILE" | cat -nRepository: he4rt/heartdevs.com
Length of output: 2166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="app-modules/profile/src/ProfileServiceProvider.php"
# grab full helper function
sed -n '50,120p' "$FILE" | cat -nRepository: he4rt/heartdevs.com
Length of output: 789
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="app-modules/profile/src/ProfileServiceProvider.php"
rg -n "insertColumns\\(|tenant_users|preg_match\\(" "$FILE"Repository: he4rt/heartdevs.com
Length of output: 362
Make SQL matching/parsing resilient to driver-specific INSERT formats.
The current regexes only match INSERT statements that start with insert into "?tenant_users"? and only support optional double-quoting; they miss backtick-quoted and/or schema-qualified inserts, causing the listener to return silently (so profile creation is skipped). This also impacts column extraction in insertColumns().
Proposed hardening
- if (! preg_match('/^insert into "?tenant_users"?/i', $event->sql)) {
+ if (! preg_match('/^\s*insert\s+into\s+(?:(?:"[^"]+"|`[^`]+`)\.)?(?:"tenant_users"|`tenant_users`|tenant_users)\b/i', $event->sql)) {
return;
}
@@
- preg_match('/insert into "?tenant_users"? \((?<columns>[^)]+)\)/i', $sql, $matches);
+ preg_match('/insert\s+into\s+(?:(?:"[^"]+"|`[^`]+`)\.)?(?:"tenant_users"|`tenant_users`|tenant_users)\s*\((?<columns>[^)]+)\)/i', $sql, $matches);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app-modules/profile/src/ProfileServiceProvider.php` around lines 25 - 26, The
SQL matching is too strict: update the INSERT detection in
ProfileServiceProvider (the preg_match that checks $event->sql) to accept
optional leading whitespace, optional schema qualification, and either backtick
or double-quote quoting around identifiers (tenant_users), e.g. allow
`schema`.`tenant_users`, "schema"."tenant_users", or tenant_users; similarly,
harden insertColumns() to strip/normalize backticks and double-quotes and to
handle schema-qualified table names before extracting column lists — normalize
the SQL (lowercase for matching, remove/replace surrounding quotes on
identifiers) then apply relaxed regexes so schema-qualified and backtick-quoted
INSERTs are correctly detected and parsed.
Comment on lines
+68
to
+77
| $profiles = Profile::query() | ||
| ->whereBelongsTo($user) | ||
| ->orderBy('tenant_id') | ||
| ->get(); | ||
|
|
||
| expect($profiles)->toHaveCount(2) | ||
| ->and($profiles->pluck('tenant_id')->all())->toBe([ | ||
| $firstTenant->id, | ||
| $secondTenant->id, | ||
| ]) |
There was a problem hiding this comment.
Avoid order-dependent assertion on tenant UUIDs.
Line 70 and Lines 74-77 make the test depend on tenant_id ordering. With UUID-based IDs, this can be nondeterministic and cause flaky failures. Assert membership without relying on order.
Suggested fix
- $profiles = Profile::query()
- ->whereBelongsTo($user)
- ->orderBy('tenant_id')
- ->get();
+ $profiles = Profile::query()
+ ->whereBelongsTo($user)
+ ->get();
+
+ $tenantIds = $profiles->pluck('tenant_id')->all();
+ $expectedTenantIds = [$firstTenant->id, $secondTenant->id];
+ sort($tenantIds);
+ sort($expectedTenantIds);
expect($profiles)->toHaveCount(2)
- ->and($profiles->pluck('tenant_id')->all())->toBe([
- $firstTenant->id,
- $secondTenant->id,
- ])
+ ->and($tenantIds)->toBe($expectedTenantIds)
->and($profiles->pluck('id')->unique())->toHaveCount(2);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $profiles = Profile::query() | |
| ->whereBelongsTo($user) | |
| ->orderBy('tenant_id') | |
| ->get(); | |
| expect($profiles)->toHaveCount(2) | |
| ->and($profiles->pluck('tenant_id')->all())->toBe([ | |
| $firstTenant->id, | |
| $secondTenant->id, | |
| ]) | |
| $profiles = Profile::query() | |
| ->whereBelongsTo($user) | |
| ->get(); | |
| $tenantIds = $profiles->pluck('tenant_id')->all(); | |
| $expectedTenantIds = [$firstTenant->id, $secondTenant->id]; | |
| sort($tenantIds); | |
| sort($expectedTenantIds); | |
| expect($profiles)->toHaveCount(2) | |
| ->and($tenantIds)->toBe($expectedTenantIds) | |
| ->and($profiles->pluck('id')->unique())->toHaveCount(2); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app-modules/profile/tests/Feature/ProfileCreationTest.php` around lines 68 -
77, The test is relying on ordered tenant_id values which is flaky with UUIDs;
remove the explicit orderBy('tenant_id') on the Profile::query() call and change
the assertion that checks tenant IDs (the expect(...)->toHaveCount(2) and
->and($profiles->pluck('tenant_id')->all()) portion) to assert membership
regardless of order (e.g., compare as unordered sets or use an assertion that
canonicalizes/sorts both arrays) so the test verifies the two tenant IDs are
present without depending on their order.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Principais pontos:
Description
Introduces the new
he4rt/profilemodule as a reusable Laravel package providing user profile management. The module includes a Profile Eloquent model with tenant-scoped data, three enums for seniority levels, social platforms, and availability periods, automatic profile creation when users join tenants, and comprehensive tests covering creation, idempotency, and factory behavior.References
#207(duplicate user recovery from import-profiles),#133(profile command),#102(user profile address)Dependencies & Requirements
he4rt/profile: >=1Contributor Summary
Changes Summary
app-modules/profile/composer.jsonapp-modules/profile/src/Models/Profile.phpapp-modules/profile/src/ProfileServiceProvider.phpapp-modules/profile/src/Enums/SeniorityLevel.phpapp-modules/profile/src/Enums/SocialPlatform.phpapp-modules/profile/src/Enums/StartAvailability.phpapp-modules/profile/src/Listeners/CreateProfileForTenantMember.phpapp-modules/profile/database/migrations/2026_05_21_000000_create_user_profiles_table.phpapp-modules/profile/database/factories/ProfileFactory.phpcomplete()state definitionsapp-modules/profile/tests/Feature/ProfileCreationTest.phpapp-modules/profile/tests/Unit/ProfileEnumTest.phpapp-modules/profile/tests/Unit/ProfileSocialLinksTest.phpapp-modules/profile/phpstan.neonapp-modules/profile/phpstan.ignore.neoncomposer.json