feat: option to change email on profile page#63
Open
iliaskhat wants to merge 1 commit intosumocoders:mainfrom
Open
feat: option to change email on profile page#63iliaskhat wants to merge 1 commit intosumocoders:mainfrom
iliaskhat wants to merge 1 commit intosumocoders:mainfrom
Conversation
Reviewer's GuideImplements an email change workflow on the user profile page using a Symfony form and Messenger message, including UI template, handler, message object, entity method, and translations. Sequence diagram for the new email change workflowsequenceDiagram
actor User
participant Browser
participant ProfileController
participant ChangeEmailType
participant MessageBusInterface
participant ChangeEmailHandler
participant UserRepository
participant Database
User->>Browser: Navigate to /user/profile
Browser->>ProfileController: GET /user/profile
ProfileController->>ChangeEmailType: createForm(ChangeEmailType, ChangeEmail)
ChangeEmailType-->>ProfileController: Form instance
ProfileController-->>Browser: Render email.html.twig with form
User->>Browser: Fill email and submit
Browser->>ProfileController: POST /user/profile (form data)
ProfileController->>ChangeEmailType: handleRequest(Request)
ChangeEmailType-->>ProfileController: isSubmitted() && isValid() = true
ProfileController->>MessageBusInterface: dispatch(ChangeEmail message)
MessageBusInterface-->>Browser: Redirect to user_profile
MessageBusInterface-->>ChangeEmailHandler: Deliver ChangeEmail
ChangeEmailHandler->>UserRepository: find(userId)
UserRepository-->>ChangeEmailHandler: User entity or null
alt User found
ChangeEmailHandler->>User: changeEmail(email)
User->>User: set email
ChangeEmailHandler->>UserRepository: save()
UserRepository->>Database: Persist changes
Database-->>UserRepository: Success
else User not found
ChangeEmailHandler-->>MessageBusInterface: Return without action
end
Updated class diagram for email change featureclassDiagram
class ProfileController {
- TranslatorInterface translator
- MessageBusInterface messageBus
+ __construct(translator TranslatorInterface, messageBus MessageBusInterface)
+ __invoke(request Request, user User) Response
}
class ChangeEmailType {
+ buildForm(builder FormBuilderInterface, options array) void
+ configureOptions(resolver OptionsResolver) void
}
class ChangeEmail {
+ int userId
+ string email
+ __construct(userId int, email string)
}
class ChangeEmailHandler {
- UserRepository userRepository
+ __construct(userRepository UserRepository)
+ __invoke(message ChangeEmail) void
}
class User {
- string email
+ changeEmail(email string) void
}
class UserRepository {
+ find(id int) User
+ save() void
}
ProfileController --> ChangeEmailType : creates form
ProfileController --> ChangeEmail : populates
ProfileController --> MessageBusInterface : dispatches
ChangeEmailHandler --> ChangeEmail : handles
ChangeEmailHandler --> UserRepository : uses
UserRepository --> User : manages
User --> ChangeEmail : email value source
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
ProfileController, the breadcrumb argument was changed from the translation keyuser_profileto the literalEmail; consider keeping a translation key here for consistency with the rest of the localized breadcrumbs. - The new
User::changeEmail()method directly assigns$this->emailand may bypass any normalization or checks thatsetEmail()(or existing logic) performs; consider delegating to the existing setter or centralizing email handling to avoid divergence.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ProfileController`, the breadcrumb argument was changed from the translation key `user_profile` to the literal `Email`; consider keeping a translation key here for consistency with the rest of the localized breadcrumbs.
- The new `User::changeEmail()` method directly assigns `$this->email` and may bypass any normalization or checks that `setEmail()` (or existing logic) performs; consider delegating to the existing setter or centralizing email handling to avoid divergence.
## Individual Comments
### Comment 1
<location path="src/Controller/User/ProfileController.php" line_range="48-50" />
<code_context>
+ return $this->redirectToRoute('user_profile');
+ }
+
+ return $this->render('user/profile/email.html.twig', [
'user' => $user,
+ 'form' => $form,
]);
}
</code_context>
<issue_to_address>
**issue (bug_risk):** The form should be passed as a FormView (`$form->createView()`) rather than the Form object itself.
Twig form helpers require a `FormView`. Passing the `FormInterface` (`'form' => $form`) will cause a runtime error. Please change this to `'form' => $form->createView()` to align with Symfony’s expected usage.
</issue_to_address>
### Comment 2
<location path="src/Controller/User/ProfileController.php" line_range="26" />
<code_context>
+ ) {
+ }
+
+ #[Breadcrumb('Email')]
public function __invoke(
+ Request $request,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using a literal 'Email' label for the breadcrumb might bypass translation and be inconsistent with other breadcrumbs.
Other breadcrumbs use translation keys (e.g. `user_profile`), while this uses the literal `'Email'`. Unless `Breadcrumb` resolves this as a key, it will remain untranslated in non-English locales. Please use a dedicated key (e.g. `user_profile_email`) and add it to the translation files for consistency.
Suggested implementation:
```
#[Breadcrumb('user_profile_email')]
```
1. Add a `user_profile_email` entry to your translation files (e.g. `translations/messages.en.yaml`, `messages.fr.yaml`, etc.), such as:
- `messages.en.yaml`: `user_profile_email: 'Email'`
- `messages.fr.yaml`: `user_profile_email: 'E-mail'`
2. If your project uses domain-specific translations for breadcrumbs (e.g. `breadcrumbs.en.yaml`), add the key there instead, matching how other breadcrumb labels are defined.
</issue_to_address>
### Comment 3
<location path="src/MessageHandler/User/ChangeEmailHandler.php" line_range="21-29" />
<code_context>
+ return;
+ }
+
+ $user->changeEmail((string) $message->email);
+
+ $this->userRepository->save();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Casting a nullable email to string risks unintentionally persisting an empty email.
Because `ChangeEmail::$email` is `?string`, `(string) $message->email` will convert `null` to `''`. That hides an invalid state and can lead to empty emails being stored. Prefer making `email` non-nullable in `ChangeEmail`, or explicitly handle `null` here (e.g., return early) instead of relying on the cast.
```suggestion
$user = $this->userRepository->find($message->userId);
if ($user === null) {
return;
}
if ($message->email === null) {
return;
}
$user->changeEmail($message->email);
$this->userRepository->save();
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+48
to
+50
| return $this->render('user/profile/email.html.twig', [ | ||
| 'user' => $user, | ||
| 'form' => $form, |
There was a problem hiding this comment.
issue (bug_risk): The form should be passed as a FormView ($form->createView()) rather than the Form object itself.
Twig form helpers require a FormView. Passing the FormInterface ('form' => $form) will cause a runtime error. Please change this to 'form' => $form->createView() to align with Symfony’s expected usage.
| ) { | ||
| } | ||
|
|
||
| #[Breadcrumb('Email')] |
There was a problem hiding this comment.
suggestion (bug_risk): Using a literal 'Email' label for the breadcrumb might bypass translation and be inconsistent with other breadcrumbs.
Other breadcrumbs use translation keys (e.g. user_profile), while this uses the literal 'Email'. Unless Breadcrumb resolves this as a key, it will remain untranslated in non-English locales. Please use a dedicated key (e.g. user_profile_email) and add it to the translation files for consistency.
Suggested implementation:
#[Breadcrumb('user_profile_email')]
- Add a
user_profile_emailentry to your translation files (e.g.translations/messages.en.yaml,messages.fr.yaml, etc.), such as:messages.en.yaml:user_profile_email: 'Email'messages.fr.yaml:user_profile_email: 'E-mail'
- If your project uses domain-specific translations for breadcrumbs (e.g.
breadcrumbs.en.yaml), add the key there instead, matching how other breadcrumb labels are defined.
Comment on lines
+21
to
+29
| $user = $this->userRepository->find($message->userId); | ||
|
|
||
| if ($user === null) { | ||
| return; | ||
| } | ||
|
|
||
| $user->changeEmail((string) $message->email); | ||
|
|
||
| $this->userRepository->save(); |
There was a problem hiding this comment.
suggestion (bug_risk): Casting a nullable email to string risks unintentionally persisting an empty email.
Because ChangeEmail::$email is ?string, (string) $message->email will convert null to ''. That hides an invalid state and can lead to empty emails being stored. Prefer making email non-nullable in ChangeEmail, or explicitly handle null here (e.g., return early) instead of relying on the cast.
Suggested change
| $user = $this->userRepository->find($message->userId); | |
| if ($user === null) { | |
| return; | |
| } | |
| $user->changeEmail((string) $message->email); | |
| $this->userRepository->save(); | |
| $user = $this->userRepository->find($message->userId); | |
| if ($user === null) { | |
| return; | |
| } | |
| if ($message->email === null) { | |
| return; | |
| } | |
| $user->changeEmail($message->email); | |
| $this->userRepository->save(); |
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.
Summary by Sourcery
Add support for updating a user’s email address from the profile section using a form and message handler.
New Features:
Enhancements:
Documentation: