refactor: user commands#155
Conversation
…ovider and character initialization
…ling and context resolution
…veUserContextAction for user context handling
…formationDTO for profile updates
…solveUserContextAction for user resolution
…nd improve type consistency
📝 WalkthroughWalkthroughRemoved the legacy UserCharacterResolver/ResolvedUserCharacter and related tests; introduced DTO-driven provider/user/character orchestration (ResolveUserProviderDTO, ProviderResolver, ResolveUserContextService, LinkUserToProviderAction, CharacterInitializerAction), new information/profile actions/DTOs, updated bot commands, message handling, controllers, config and .env placeholders. Changes
Sequence Diagram(s)sequenceDiagram
participant Discord as Discord / Client
participant Service as ResolveUserContextService
participant Provider as ProviderResolver
participant Linker as LinkUserToProviderAction
participant CharInit as CharacterInitializerAction
participant DB as Database
Note over Discord,Service: incoming operation (message / welcome / profile)
Discord->>Service: handle(ResolveUserProviderDTO)
Service->>Provider: handle(dto)
Provider->>DB: firstOrCreate(provider, tenant_id, provider_id)
DB-->>Provider: Provider
Provider-->>Service: Provider
Service->>Linker: handle(Provider)
alt provider.model exists
Linker->>DB: fetch User
DB-->>Linker: User
else new user
Linker->>DB: create User (UUID)
DB-->>Linker: User
Linker->>DB: update Provider.model_id
end
Linker-->>Service: User
Service->>CharInit: ensure(User, tenantId)
CharInit->>DB: firstOrCreate(user_id, tenant_id)
DB-->>CharInit: Character
CharInit-->>Service: Character
Service-->>Discord: UserContext(User, Character, Provider)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (6)
app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (1)
166-170: Consider using structured logging format for consistency.The error logging wraps the message in an array
[$throwable->getMessage()], but other files in this PR (e.g.,WelcomeMember.php) use a more structured format with named keys like['exception' => $throwable]. Consider aligning for consistency and better log parsing.- $this->logger()->error('Error EditProfileCommand:', [$throwable->getMessage()]); + $this->logger()->error('Error EditProfileCommand:', [ + 'exception' => $throwable->getMessage(), + 'provider_id' => $interaction->user->id ?? null, + ]);app-modules/message/src/Actions/NewMessage.php (1)
32-36: Potentially unnecessaryrefresh()call.
ResolveUserContextActioncallsCharacterInitializerAction::ensure(), which returns a fresh model fromfirstOrCreate. The immediaterefresh()on line 34 may be redundant unless there's concurrent modification concern between the action call and this line.If no concurrent updates are expected, consider removing the refresh:
$userContext = resolve(ResolveUserContextAction::class)->handle($userDto); - $userContext->character->refresh(); - $characterEntity = CharacterEntity::make($userContext->character->toArray());app-modules/user/src/ValueObjects/UserContext.php (1)
11-29: Well-structured value object.The
UserContextvalue object cleanly encapsulates the resolved user, character, and provider. Consider addingreadonlyto the class declaration for immutability, aligning withResolveUserContextActionwhich is already declared asreadonly class.-final class UserContext +final readonly class UserContext {app-modules/user/src/Actions/LinkUserToProviderAction.php (1)
19-28: Consider wrapping user creation and provider update in a transaction.If the
$provider->update()fails after the user is created, you'll have an orphaned user record. Wrapping both operations in a database transaction ensures atomicity.+use Illuminate\Support\Facades\DB; + final class LinkUserToProviderAction { public function handle(Provider $provider): User { if ($provider->model instanceof User) { return $provider->model; } - $user = User::query()->create([ - 'id' => Uuid::uuid4()->toString(), - 'username' => $provider->username, - 'name' => $provider->username, - 'is_donator' => false, - ]); - - $provider->update([ - 'model_id' => $user->id, - ]); - - return $user; + return DB::transaction(function () use ($provider) { + $user = User::query()->create([ + 'id' => Uuid::uuid4()->toString(), + 'username' => $provider->username, + 'name' => $provider->username, + 'is_donator' => false, + ]); + + $provider->update([ + 'model_id' => $user->id, + ]); + + return $user; + }); } }app-modules/provider/src/DTO/ResolveUserProviderDTO.php (1)
9-9: Consider marking the class asfinalfor consistency.Other DTOs in this PR (e.g.,
UpsertInformationDTO) are declared asfinal. For consistency and to prevent unintended inheritance of a data transfer object, consider adding thefinalmodifier.-class ResolveUserProviderDTO +final class ResolveUserProviderDTOapp-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (1)
188-189: Consider logging the full exception for better debugging.While the error message prefix is helpful for searchability, logging only the message loses the stack trace. Consider including the exception itself for debugging purposes.
} catch (Throwable $throwable) { - $this->logger()->error('Error IntroductionCommand', [$throwable->getMessage()]); + $this->logger()->error('Error IntroductionCommand', [ + 'message' => $throwable->getMessage(), + 'exception' => $throwable, + ]);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app-modules/bot-discord/src/Actions/UserCharacterResolver.php(0 hunks)app-modules/bot-discord/src/DTO/ResolvedUserCharacter.php(0 hunks)app-modules/bot-discord/src/Events/WelcomeMember.php(2 hunks)app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php(1 hunks)app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php(4 hunks)app-modules/bot-discord/tests/Feature/Actions/UserCharacterResolverTest.php(0 hunks)app-modules/character/src/Actions/CharacterInitializerAction.php(1 hunks)app-modules/message/src/Actions/NewMessage.php(1 hunks)app-modules/provider/src/Actions/ProviderResolver.php(1 hunks)app-modules/provider/src/DTO/ResolveUserProviderDTO.php(1 hunks)app-modules/user/src/Actions/InformationUserAction.php(1 hunks)app-modules/user/src/Actions/LinkUserToProviderAction.php(1 hunks)app-modules/user/src/Actions/ResolveUserContextAction.php(1 hunks)app-modules/user/src/Actions/UpdateProfile.php(1 hunks)app-modules/user/src/DTO/UpsertInformationDTO.php(1 hunks)app-modules/user/src/ValueObjects/UserContext.php(1 hunks)
💤 Files with no reviewable changes (3)
- app-modules/bot-discord/src/DTO/ResolvedUserCharacter.php
- app-modules/bot-discord/src/Actions/UserCharacterResolver.php
- app-modules/bot-discord/tests/Feature/Actions/UserCharacterResolverTest.php
🧰 Additional context used
🧬 Code graph analysis (11)
app-modules/provider/src/Actions/ProviderResolver.php (2)
app-modules/provider/src/Models/Provider.php (1)
Provider(28-96)app-modules/provider/src/DTO/ResolveUserProviderDTO.php (1)
ResolveUserProviderDTO(9-33)
app-modules/user/src/Actions/LinkUserToProviderAction.php (2)
app-modules/provider/src/Models/Provider.php (2)
Provider(28-96)model(51-54)app-modules/user/src/Entities/InformationEntity.php (1)
update(36-44)
app-modules/user/src/Actions/InformationUserAction.php (2)
app-modules/user/src/DTO/UpsertInformationDTO.php (1)
UpsertInformationDTO(9-33)app-modules/user/src/Models/Information.php (1)
Information(13-43)
app-modules/user/src/ValueObjects/UserContext.php (2)
app-modules/character/src/Models/Character.php (1)
Character(28-128)app-modules/provider/src/Models/Provider.php (1)
Provider(28-96)
app-modules/bot-discord/src/Events/WelcomeMember.php (3)
app-modules/provider/src/DTO/ResolveUserProviderDTO.php (2)
ResolveUserProviderDTO(9-33)make(21-32)app-modules/user/src/Actions/ResolveUserContextAction.php (1)
ResolveUserContextAction(12-28)app-modules/bot-discord/src/Events/MessageReceivedEvent.php (1)
handle(27-56)
app-modules/character/src/Actions/CharacterInitializerAction.php (1)
app-modules/character/src/Models/Character.php (1)
Character(28-128)
app-modules/message/src/Actions/NewMessage.php (3)
app-modules/provider/src/DTO/ResolveUserProviderDTO.php (3)
ResolveUserProviderDTO(9-33)__construct(11-19)make(21-32)app-modules/user/src/Actions/ResolveUserContextAction.php (1)
ResolveUserContextAction(12-28)app-modules/character/src/Entities/CharacterEntity.php (1)
CharacterEntity(9-66)
app-modules/user/src/Actions/UpdateProfile.php (5)
app-modules/provider/src/Actions/ProviderResolver.php (1)
ProviderResolver(10-28)app-modules/provider/src/DTO/ResolveUserProviderDTO.php (1)
ResolveUserProviderDTO(9-33)app-modules/user/src/DTO/UpdateProfileDTO.php (2)
UpdateProfileDTO(9-49)__construct(11-21)app-modules/user/src/DTO/UpsertInformationDTO.php (1)
UpsertInformationDTO(9-33)app-modules/user/src/Actions/InformationUserAction.php (1)
InformationUserAction(10-23)
app-modules/provider/src/DTO/ResolveUserProviderDTO.php (1)
app-modules/provider/src/Models/Provider.php (1)
Provider(28-96)
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (5)
app-modules/provider/src/Models/Provider.php (2)
Provider(28-96)user(59-62)app-modules/provider/src/DTO/ResolveUserProviderDTO.php (1)
ResolveUserProviderDTO(9-33)app-modules/user/src/Actions/InformationUserAction.php (1)
InformationUserAction(10-23)app-modules/user/src/Actions/ResolveUserContextAction.php (1)
ResolveUserContextAction(12-28)app-modules/user/src/DTO/UpsertInformationDTO.php (1)
UpsertInformationDTO(9-33)
app-modules/user/src/Actions/ResolveUserContextAction.php (7)
app-modules/character/src/Models/Character.php (1)
Character(28-128)app-modules/character/src/Actions/CharacterInitializerAction.php (2)
CharacterInitializerAction(10-21)ensure(12-20)app-modules/provider/src/Models/Provider.php (1)
Provider(28-96)app-modules/provider/src/Actions/ProviderResolver.php (1)
ProviderResolver(10-28)app-modules/provider/src/DTO/ResolveUserProviderDTO.php (1)
ResolveUserProviderDTO(9-33)app-modules/user/src/ValueObjects/UserContext.php (1)
UserContext(11-30)app-modules/user/src/Actions/LinkUserToProviderAction.php (1)
LinkUserToProviderAction(11-32)
🔇 Additional comments (8)
app-modules/user/src/Actions/InformationUserAction.php (1)
10-23: Clean action implementation.The implementation correctly uses
updateOrCreatefor upserting user information. The field mapping aligns with theInformationmodel's fillable fields.app-modules/bot-discord/src/Events/WelcomeMember.php (1)
32-60: Welcome message sends even if user resolution fails.The try-catch block (lines 32-48) catches resolution errors and logs them, but the welcome message (lines 54-60) executes regardless of success or failure. Verify this is the intended behavior - should new members still see the welcome message if their user record fails to persist?
If the welcome message should only be sent on successful resolution, consider:
try { $userDto = ResolveUserProviderDTO::make([ 'tenant_id' => $tenantProvider->tenant_id, 'provider' => $tenantProvider->provider, 'provider_id' => $member->user->id, 'model_type' => User::class, 'username' => $member->user->username, 'avatar' => $member->user->avatar, ]); resolve(ResolveUserContextAction::class)->handle($userDto); } catch (Throwable $throwable) { Log::error('Erro ao resolver usuário no evento WelcomeMember', [ 'exception' => $throwable, 'member_id' => $member->user->id ?? null, ]); + return; }app-modules/provider/src/Actions/ProviderResolver.php (1)
14-26: Profile attributes only set on creation, not updated.
firstOrCreatesetsusername,avataronly when creating a new record. If a user changes their Discord username or avatar, subsequent calls won't update these fields in the provider record.If updates are needed, consider using
updateOrCreate:- return Provider::query()->firstOrCreate( + return Provider::query()->updateOrCreate( [ 'provider' => $dto->provider, 'tenant_id' => $dto->tenantId, 'provider_id' => $dto->providerId, 'model_type' => $dto->modelType, ], [ 'username' => $dto->username, 'email' => $dto->email, 'avatar' => $dto->avatar, ] );app-modules/character/src/Actions/CharacterInitializerAction.php (1)
10-21: Clean character initialization.The
ensuremethod appropriately usesfirstOrCreatewith the composite key[user_id, tenant_id], relying on model/database defaults forreputation,experience, anddaily_bonus_claimed_at.app-modules/user/src/DTO/UpsertInformationDTO.php (1)
9-32: LGTM!Clean DTO implementation with proper separation between camelCase properties and snake_case array keys. The nullable optional fields and factory method pattern are well-implemented.
app-modules/user/src/Actions/ResolveUserContextAction.php (1)
12-27: LGTM!Well-structured orchestration action with clear separation of concerns. The readonly class with constructor injection follows best practices, and the composition of ProviderResolver → LinkUserToProviderAction → CharacterInitializerAction creates a clean resolution pipeline.
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (2)
141-150: Good refactoring to the new action-based pattern.The transition from the old resolver to the new
ResolveUserProviderDTOandResolveUserContextActionflow is clean and aligns with the modular architecture.
168-177: Verify the display intent for "Nome/Nickname" field.The "Nome/Nickname" field at line 171 only displays the nickname, even though users enter both name and nickname in the form. Consider whether this should display the name, the nickname, or both (e.g.,
$userInformation->name . ' (' . $userInformation->nickname . ')').
1pride
left a comment
1pride
left a comment
There was a problem hiding this comment.
need a few adjusts as explained on the files.
…rContextService and UpdateProfileService for consistency
…ProfileDTO for improved consistency
…rove error handling
…nd improve role handling
…roved consistency
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app-modules/user/src/Services/UpdateProfileService.php (1)
29-32: Potential null access on$provider->user.The previous review's concern remains valid:
ProviderResolver::handle()may create a provider without settingmodel_id, causing$provider->userto returnnullon Line 32. This will fail when buildingUpsertInformationDTO.Consider calling
LinkUserToProviderAction(as done inResolveUserContextService) to ensure the user is linked before accessing$provider->user:🔎 Proposed fix to ensure user exists
+use He4rt\User\Actions\LinkUserToProviderAction; + final readonly class UpdateProfileService { public function __construct( private ProviderResolver $providerResolver, + private LinkUserToProviderAction $linkUserToProviderAction, private InformationUserAction $informationUserAction, ) {} public function handle(UpdateProfileDTO $profileDTO): void { $providerDto = ResolveUserProviderDTO::make([ 'provider_id' => $profileDTO->providerId, 'provider' => $profileDTO->provider, 'tenant_id' => $profileDTO->tenantId, 'model_type' => User::class, ]); $provider = $this->providerResolver->handle($providerDto); + $user = $this->linkUserToProviderAction->handle($provider); $informationDto = UpsertInformationDTO::make([ - 'user' => $provider->user, + 'user' => $user, 'name' => $profileDTO->name, 'nickname' => $profileDTO->nickname, 'linkedin_url' => $profileDTO->linkedinUrl, 'github_url' => $profileDTO->githubUrl, 'birthdate' => $profileDTO->birthdate, 'about' => $profileDTO->about, ]); $this->informationUserAction->handle($informationDto); } }
🧹 Nitpick comments (2)
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (2)
120-121: Remove redundant null coalescing operators.The
?? nulloperators on these lines are redundant. The$components->get()method already returnsnullwhen the component doesn't exist or has no value, making the fallback unnecessary.🔎 Proposed simplification
- 'linkedin_url' => $components->get('custom_id', 'linkedin_url')->value ?? null, - 'github_url' => $components->get('custom_id', 'github_url')->value ?? null, + 'linkedin_url' => $components->get('custom_id', 'linkedin_url')?->value, + 'github_url' => $components->get('custom_id', 'github_url')?->value,
127-137: LGTM! Clear success feedback to user.The success message properly acknowledges the user's submission and provides helpful context.
Consider extracting the color value
'4b0080'to a class constant for reusability, as it's used twice (lines 133 and 162).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.env.example(1 hunks)app-modules/bot-discord/src/Events/WelcomeMember.php(1 hunks)app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php(3 hunks)app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php(2 hunks)app-modules/message/src/Actions/NewMessage.php(1 hunks)app-modules/user/src/Http/Controllers/UsersController.php(2 hunks)app-modules/user/src/Services/ResolveUserContextService.php(1 hunks)app-modules/user/src/Services/UpdateProfileService.php(1 hunks)config/he4rt.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app-modules/user/src/Http/Controllers/UsersController.php (1)
app-modules/user/src/Services/UpdateProfileService.php (1)
UpdateProfileService(14-44)
app-modules/user/src/Services/ResolveUserContextService.php (7)
app-modules/character/src/Models/Character.php (1)
Character(28-128)app-modules/character/src/Actions/CharacterInitializerAction.php (2)
CharacterInitializerAction(10-21)ensure(12-20)app-modules/provider/src/Models/Provider.php (1)
Provider(28-96)app-modules/provider/src/Actions/ProviderResolver.php (1)
ProviderResolver(10-28)app-modules/provider/src/DTO/ResolveUserProviderDTO.php (1)
ResolveUserProviderDTO(9-33)app-modules/user/src/Actions/LinkUserToProviderAction.php (1)
LinkUserToProviderAction(11-32)app-modules/user/src/ValueObjects/UserContext.php (1)
UserContext(11-30)
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (4)
app-modules/provider/src/DTO/ResolveUserProviderDTO.php (1)
ResolveUserProviderDTO(9-33)app-modules/user/src/Actions/InformationUserAction.php (1)
InformationUserAction(10-23)app-modules/user/src/DTO/UpsertInformationDTO.php (1)
UpsertInformationDTO(9-33)app-modules/user/src/Services/ResolveUserContextService.php (1)
ResolveUserContextService(13-29)
app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (1)
app-modules/user/src/Services/UpdateProfileService.php (1)
UpdateProfileService(14-44)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 82-82: [UnorderedKey] The GUILD_RULE_ID key should go before the WELCOME_CHANNEL_ID key
(UnorderedKey)
🪛 PHPMD (2.15.0)
app-modules/bot-discord/src/Events/WelcomeMember.php
23-23: Avoid unused parameters such as '$discord'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (17)
config/he4rt.php (1)
22-23: LGTM! Configuration entries added correctly.The new
welcome_channelandguild_rule_idconfiguration entries are well-structured with sensible defaults, allowing the system to fall back gracefully if environment variables are not set..env.example (1)
81-82: LGTM! Environment placeholders added.The new environment variable placeholders align with the configuration updates in
config/he4rt.php.app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (2)
11-11: LGTM! Service migration completed correctly.The migration from
UpdateProfiletoUpdateProfileServiceis correctly implemented, maintaining compatibility with the existing DTO construction flow.Also applies to: 142-142
167-167: LGTM! Improved error logging.The error logging now uses a structured format with a clear prefix and context array, which will improve debugging and log aggregation.
app-modules/message/src/Actions/NewMessage.php (2)
24-47: LGTM! User resolution refactored to DTO-driven service.The migration from the legacy resolver to the new
ResolveUserContextServiceis well-executed. The DTO construction includes all required fields, and the service ensures proper provider, user, and character initialization. The character refresh before experience updates ensures data consistency.
49-56: LGTM! Comprehensive error logging added.The error handler now logs all relevant context (provider_id, tenant_id, error message, and stack trace), which will significantly aid in debugging message processing failures.
app-modules/user/src/Services/ResolveUserContextService.php (1)
1-29: LGTM! Well-designed orchestration service.The
ResolveUserContextServiceprovides clean separation of concerns by orchestrating three discrete actions (provider resolution, user linking, and character initialization) in a logical sequence. Each action handles entity creation when necessary, ensuring the returnedUserContextcontains fully initialized entities.app-modules/bot-discord/src/Events/WelcomeMember.php (3)
25-25: LGTM! Channel configuration externalized.Replacing the hardcoded channel ID with
config('he4rt.channels.welcome_channel')improves configurability and addresses the previous concern about environment-specific values.
32-62: LGTM! Robust error handling for user resolution.The try-catch block around user context resolution provides comprehensive error logging with all relevant context (tenant_id, provider, provider_id, exception). The fallback welcome message appropriately informs the user about the issue while directing them to contact moderation.
64-77: LGTM! Clear and actionable welcome message.The success path sends a well-structured welcome message that guides new members to use the
/apresentarcommand, providing clear next steps for onboarding.app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (7)
56-56: LGTM! Improved placeholder examples.The updated placeholders ('Fulano de Tal', 'Fulano123') are more culturally appropriate and provide clearer examples for Portuguese-speaking users.
Also applies to: 63-63
104-113: LGTM! User context resolution implemented correctly.The flow properly constructs the ResolveUserProviderDTO and resolves the user context through the ResolveUserContextService, following the new modular architecture.
125-125: LGTM! Persistence follows the new architecture.The use of InformationUserAction aligns with the refactored action-based design and properly captures the result for subsequent operations.
176-176: LGTM! Enhanced error logging.Adding the context string 'Error IntroductionCommand' improves error traceability and debugging.
38-48: Verify config key exists and consider renaming for clarity.The pre-check logic correctly prevents duplicate introductions. However,
config('he4rt.channels.guild_rule_id')uses a confusing namespace—'channels' typically suggests channel IDs, not role IDs.Please verify that this config key exists and returns the correct role ID. Consider renaming to something like
config('he4rt.roles.presented_role_id')for clarity.#!/bin/bash # Description: Verify the config key exists and check its usage context # Search for the config key definition rg -nP "guild_rule_id" --type=php -C3 # Search for other role-related config keys to understand the naming pattern rg -nP "config\(['\"]he4rt\.(roles|channels)" --type=php -C2
139-163: The welcome channel is properly configured with a default value inconfig/he4rt.php, and the->send()operation is already protected by a try-catch block that logs errors. No changes needed.Likely an incorrect or invalid review comment.
165-173: Ensure the bot has MANAGE_ROLES permission and the guild_rule_id role exists.The role assignment implementation is correct. The code properly collects existing roles, adds the guild_rule_id, and calls
setRoles()with the complete array. The user is checked for the role beforehand (line 39) to avoid redundant assignments. However,setRoles()will fail silently if the bot lacks MANAGE_ROLES permission. Verify:
- The bot has MANAGE_ROLES permission in the guild
- The configured guild_rule_id (
546150872397119491by default) exists as a valid role
…hannel and role IDs
…pi into refactor/user-commands
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.env.example (1)
81-82: Consider adding comments to document the new environment variables.The new
WELCOME_CHANNEL_IDandPRESENTATION_ROLE_IDvariables lack explanatory comments. Adding brief descriptions would help developers understand their purpose and expected values.💡 Suggested documentation
DISCORD_TOKEN= + +# Discord welcome flow configuration +# WELCOME_CHANNEL_ID: Channel ID where welcome messages are sent WELCOME_CHANNEL_ID= +# PRESENTATION_ROLE_ID: Role ID assigned to users after introduction PRESENTATION_ROLE_ID=app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (1)
38-38: Fix typo in property name.The property is named
welcomeChanelIdbut should bewelcomeChannelId(missing 'n' in "Channel").🔎 Proposed fix
- private readonly string $welcomeChanelId; + private readonly string $welcomeChannelId;Also update line 43:
- $this->welcomeChanelId = config('he4rt.channels.welcome_channel_id'); + $this->welcomeChannelId = config('he4rt.channels.welcome_channel_id');And line 174:
- ->send($this->welcomeChanelId); + ->send($this->welcomeChannelId);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.env.example(1 hunks)app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php(2 hunks)config/he4rt.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (4)
app-modules/provider/src/DTO/ResolveUserProviderDTO.php (1)
ResolveUserProviderDTO(9-33)app-modules/user/src/Actions/InformationUserAction.php (1)
InformationUserAction(10-23)app-modules/user/src/DTO/UpsertInformationDTO.php (1)
UpsertInformationDTO(9-33)app-modules/user/src/Services/ResolveUserContextService.php (1)
ResolveUserContextService(13-29)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 82-82: [UnorderedKey] The PRESENTATION_ROLE_ID key should go before the WELCOME_CHANNEL_ID key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Perform Pest Tests / Run
🔇 Additional comments (6)
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (6)
48-58: LGTM!The pre-check correctly prevents duplicate presentations by validating the role before showing the modal, improving user experience by avoiding unnecessary form fills. This addresses the past review feedback about moving validations to the handle method.
114-123: LGTM!The user resolution flow correctly uses the new modular architecture with
ResolveUserProviderDTOandResolveUserContextService, replacing the legacyUserCharacterResolverapproach as intended by the PR objectives.
132-132: LGTM!Setting
birthdatetonullcorrectly addresses the past review comment about the field being referenced but not present in the form. TheUpsertInformationDTOaccepts nullable birthdate, so this is handled correctly.
135-174: LGTM!The information persistence flow correctly uses
InformationUserAction, and the success messages properly utilize the persisted user information fields (name, nickname, about, GitHub, LinkedIn) in the welcome embed. This addresses the past review feedback about using gathered information in the embed sent to the welcome channel.
176-184: Verify that role assignment persists correctly.The role assignment logic correctly uses
setRoles()(addressing past feedback aboutaddRole()not working). However, ensure thatsetRoles()successfully persists the role change to Discord's API, as some DiscordPHP operations are asynchronous and may fail silently.You may want to verify this works as expected in testing, or consider adding explicit error handling or confirmation for the role assignment operation if the Discord library provides callbacks or promises for async operations.
186-193: LGTM!Error handling is properly implemented with enhanced logging context ('Error IntroductionCommand') for easier debugging, while maintaining a user-friendly error message. The try-catch block appropriately handles exceptions from provider resolution, user context resolution, information persistence, and role assignment operations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (1)
132-132: Birthdate field is set to null but not present in the form.Line 132 sets
birthdatetonull, but there's no birthdate TextInput in the modal (lines 60-96). This is dead code. Either add a birthdate field to the form or remove this parameter from the DTO.
🧹 Nitpick comments (1)
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (1)
125-135: Consider validating URL formats for optional fields.The LinkedIn and GitHub URL inputs are accepted without format validation. While the modal enforces length constraints, malformed URLs could be persisted.
Example validation approach
$linkedinUrl = $components->get('custom_id', 'linkedin_url')?->value; $githubUrl = $components->get('custom_id', 'github_url')?->value; if ($linkedinUrl && !filter_var($linkedinUrl, FILTER_VALIDATE_URL)) { throw new \InvalidArgumentException('Invalid LinkedIn URL format'); } if ($githubUrl && !filter_var($githubUrl, FILTER_VALIDATE_URL)) { throw new \InvalidArgumentException('Invalid GitHub URL format'); } $informationDto = UpsertInformationDTO::make([ 'user' => $userContext->user, 'name' => $components->get('custom_id', 'name')->value, 'nickname' => $components->get('custom_id', 'nickname')->value, 'about' => $components->get('custom_id', 'about')->value, 'linkedin_url' => $linkedinUrl, 'github_url' => $githubUrl, 'birthdate' => null, ]);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (6)
app-modules/provider/src/Models/Provider.php (2)
Provider(28-96)user(59-62)app-modules/provider/src/DTO/ResolveUserProviderDTO.php (1)
ResolveUserProviderDTO(9-33)app-modules/tenant/src/Models/Tenant.php (1)
Tenant(23-104)app-modules/user/src/Actions/InformationUserAction.php (1)
InformationUserAction(10-23)app-modules/user/src/DTO/UpsertInformationDTO.php (1)
UpsertInformationDTO(9-33)app-modules/user/src/Services/ResolveUserContextService.php (1)
ResolveUserContextService(13-29)
🔇 Additional comments (8)
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (8)
11-17: Imports align with the new modular architecture.The new imports (ResolveUserProviderDTO, InformationUserAction, UpsertInformationDTO, User, ResolveUserContextService) are correctly added and used throughout the refactored persistData method.
48-58: Early validation prevents duplicate presentations.The role check correctly prevents users from submitting the form multiple times by validating membership before showing the modal. This addresses the previous feedback about moving validations to the handle method.
60-96: Modal form is well-structured.The form fields have appropriate validation constraints (min/max length, required/optional flags) and updated placeholders that provide clear guidance to users.
114-123: User context resolution follows the new modular pattern.The code correctly constructs ResolveUserProviderDTO with Discord interaction data and resolves the user context using the new ResolveUserContextService, aligning with the PR's architectural refactor.
130-131: Null-safe operators correctly handle optional fields.The optional fields
linkedin_urlandgithub_urlnow use the null-safe operator (?->value), which properly prevents null pointer exceptions when users leave these fields empty.
176-184: Role assignment logic is correctly implemented.The code collects existing role IDs, appends the presentation role ID, and applies them using
setRoles(). This approach correctly updates the member's roles without losing existing assignments.
186-193: Enhanced error logging improves debugging.The error handler now logs with the context string "Error IntroductionCommand" alongside the exception message, making it easier to trace issues in production logs.
40-44: No action needed. The config valuespresentation_role_idandwelcome_channel_idare properly defined inconfig/he4rt.phpwith default Discord IDs. They cannot be missing or null at runtime.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (1)
167-167: Improve error logging format and context.The current logging format passes an indexed array
[$throwable->getMessage()]as context, which is not idiomatic. Consider these improvements:
- Use proper context keys for better log readability
- Include the full exception object to capture stack traces
🔎 Recommended logging format
- $this->logger()->error('Error EditProfileCommand:', [$throwable->getMessage()]); + $this->logger()->error('Error EditProfileCommand', [ + 'error' => $throwable->getMessage(), + 'exception' => $throwable, + ]);This provides structured context with the exception object, enabling better debugging with full stack traces in your logs.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (1)
app-modules/user/src/Services/UpdateProfileService.php (1)
UpdateProfileService(14-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Perform Pest Tests / Run
🔇 Additional comments (2)
app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (2)
11-11: LGTM: Service import updated correctly.The import change from
UpdateProfiletoUpdateProfileServicealigns with the refactoring described in the PR objectives and is used consistently at line 142.
142-142: LGTM: Service resolution updated correctly.The service resolution change to
UpdateProfileService::classis consistent with the import and follows Laravel's dependency injection pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (3)
176-184: Role assignment may need promise handling.The
setRoles()method in Discord.php typically returns aPromise. If this promise is not handled, any errors during role assignment will be silently swallowed. Consider handling the promise or usingaddRole()for a single role addition.🔎 Suggested refactor using addRole
- $actualRoles = []; - - foreach ($interaction->member->roles as $role) { - $actualRoles[] = $role->id; - } - - $actualRoles[] = $this->roleId; - - $interaction->member->setRoles($actualRoles); + $interaction->member->addRole($this->roleId)->done( + null, + fn ($error) => $this->logger()->error('Failed to add role', ['error' => $error->getMessage()]) + );
186-188: Enhance error logging with more context.The error log only includes the message. For better debugging, consider including the full exception and contextual data, similar to the pattern used in
WelcomeMember.php.🔎 Suggested improvement
} catch (Throwable $throwable) { - $this->logger()->error('Error IntroductionCommand', [$throwable->getMessage()]); + $this->logger()->error('Error IntroductionCommand', [ + 'user_id' => $interaction->user->id ?? null, + 'guild_id' => $interaction->guild_id ?? null, + 'exception' => $throwable->getMessage(), + 'trace' => $throwable->getTraceAsString(), + ]);
143-143: Use hex string format for consistency with other Discord embeds.The color format
(string) hexdec('4b0080')produces a decimal string and is valid in Laracord, but other Discord embeds in the codebase (EditProfileCommand, ProfileCommand) use hex strings like'800080'instead. Change to->color('800080')to match the pattern.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.env.exampleapp-modules/bot-discord/config/bot-discord.phpapp-modules/bot-discord/src/Events/WelcomeMember.phpapp-modules/bot-discord/src/SlashCommands/IntroductionCommand.php
🚧 Files skipped from review as they are similar to previous changes (1)
- .env.example
🧰 Additional context used
🧬 Code graph analysis (2)
app-modules/bot-discord/src/Events/WelcomeMember.php (5)
app-modules/provider/src/Models/Provider.php (2)
Provider(28-96)user(59-62)app-modules/provider/src/DTO/ResolveUserProviderDTO.php (2)
ResolveUserProviderDTO(9-33)make(21-32)app-modules/tenant/src/Models/Tenant.php (1)
Tenant(23-104)app-modules/user/src/Services/ResolveUserContextService.php (1)
ResolveUserContextService(13-29)app-modules/bot-discord/src/Events/MessageReceivedEvent.php (1)
handle(27-56)
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (4)
app-modules/provider/src/DTO/ResolveUserProviderDTO.php (1)
ResolveUserProviderDTO(9-33)app-modules/user/src/Actions/InformationUserAction.php (1)
InformationUserAction(10-23)app-modules/user/src/DTO/UpsertInformationDTO.php (1)
UpsertInformationDTO(9-33)app-modules/user/src/Services/ResolveUserContextService.php (1)
ResolveUserContextService(13-29)
🪛 PHPMD (2.15.0)
app-modules/bot-discord/src/Events/WelcomeMember.php
23-23: Avoid unused parameters such as '$discord'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Perform Pest Tests / Run
🔇 Additional comments (9)
app-modules/bot-discord/config/bot-discord.php (1)
9-11: LGTM!The new
roles.presentationconfig follows the existing pattern and properly usesenv()with a fallback default. This aligns well with the refactored command flow that now references config values instead of hardcoded IDs.app-modules/bot-discord/src/Events/WelcomeMember.php (4)
32-42: DTO construction and service usage looks correct.The DTO is properly constructed with all required fields from the
ResolveUserProviderDTOdefinition. The service call is wrapped appropriately in try-catch with good contextual logging.
43-62: Error handling is well-structured.The catch block provides good observability with contextual logging and gracefully informs the user about the failure. The defensive null-coalescing on
tenant_idandprovideris unnecessary since$tenantProvideris guaranteed to exist at this point (fromfirstOrFail()), but it's harmless.
64-77: Clean success path implementation.The welcome message is well-formatted and uses the config-driven channel ID. The embed provides clear instructions for new members.
23-23: Unused$discordparameter is likely required by parent signature.The static analysis flags this as unused, but this parameter is typically required by the Laracord
Eventclass's method signature. Suppressing this warning with a docblock annotation (e.g.,@SuppressWarnings(PHPMD.UnusedFormalParameter)) or using@phpstan-ignore-next-linewould silence the false positive if desired.app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (4)
48-58: Role pre-check logic is correct.The early validation prevents users from presenting multiple times by checking for the configured role. This addresses the concern from past reviews about moving validation to the handle method.
60-102: Modal definition is well-structured.The form fields have appropriate validation constraints (min/max lengths, required flags). The birthdate field was intentionally removed with
nullpassed to the DTO, addressing the past review comment.
125-133: DTO construction is correct with birthdate set to null.The birthdate field is explicitly set to
nullsince there's no corresponding input in the modal. This correctly addresses the past review comment about the missing birthdate field. The null-safe operators on optional fields (linkedin_url,github_url) are properly applied.
40-44: Parent constructor call may be necessary depending on Laracord's implementation.While no other commands in the codebase define custom constructors or call
parent::__construct(), this alone doesn't confirm it's unnecessary. The SlashCommand parent class from the Laracord framework may require initialization that isn't obvious from examining the local codebase. Verify the Laracord framework documentation or source code to determine ifparent::__construct()needs to be invoked for proper initialization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.env.example (1)
81-83: Reorder environment variables alphabetically.The dotenv-linter tool flags that
HE4RT_AUTO_REPORT_CHANNEL_IDshould be ordered beforeHE4RT_PRESENTATIONS_CHANNEL_ID. While this doesn't affect functionality, maintaining alphabetical order improves consistency and readability.🔎 Proposed reordering
DISCORD_TOKEN= -HE4RT_PRESENTATIONS_CHANNEL_ID= HE4RT_AUTO_REPORT_CHANNEL_ID= +HE4RT_PRESENTATIONS_CHANNEL_ID= HE4RT_PRESENTATION_ROLE_ID=
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.env.example
🧰 Additional context used
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 82-82: [UnorderedKey] The HE4RT_AUTO_REPORT_CHANNEL_ID key should go before the HE4RT_PRESENTATIONS_CHANNEL_ID key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Perform Pest Tests / Run
2 participants
This pull request refactors and modernizes the user, provider, and character resolution flow across the Discord bot and related modules. The main change is the replacement of the old
UserCharacterResolverapproach with a more modular, action-based system that separates concerns for provider resolution, user linking, and character initialization. Additionally, new DTOs and actions are introduced to streamline context resolution, and error logging is improved for better debugging. Several files and tests related to the old approach are removed, and usages are updated accordingly.Core architectural refactor:
UserCharacterResolverand its DTO, along with associated tests, in favor of a new modular action-based system for resolving user, provider, and character relationships. [1] [2] [3]ProviderResolver,LinkUserToProviderAction,CharacterInitializerAction, andResolveUserContextAction, each handling a specific part of the user-provider-character resolution process. [1] [2] [3] [4]ResolveUserProviderDTO, to standardize data transfer for user-provider resolution.Refactoring usages across modules:
WelcomeMemberevent,IntroductionCommand, andNewMessageaction to use the newResolveUserContextActionand related DTOs, replacing direct calls to the old resolver. [1] [2] [3] [4] [5]User information management improvements:
InformationUserActionto handle upserting user information, and refactoredIntroductionCommandto use it, improving data consistency and separation of concerns. [1] [2]Error handling and logging:
EditProfileCommandandIntroductionCommandfor more informative debugging output. [1] [2]Minor UX improvements:
Summary by CodeRabbit
New Features
Bug Fixes
Configuration
Removed
✏️ Tip: You can customize this high-level summary in your review settings.