Change Email review #5688
Change Email review #5688
Conversation
Related to #5362
|
Reminder: If you just check with MimeKit |
|
@agriffard you could rebase on dev after merging #5710 |
|
ok, dev merged and IEmailAddressValidator used in ChangeEmailViewModel. |
src/OrchardCore.Modules/OrchardCore.Users/ViewModels/ChangeEmailViewModel.cs
Outdated
Show resolved
Hide resolved
| using Microsoft.Extensions.Localization; | ||
| using MimeKit; | ||
| using Microsoft.Extensions.Localization; | ||
| using OrchardCore.Email; | ||
|
|
||
| namespace OrchardCore.Users.ViewModels | ||
| { | ||
| public class ChangeEmailViewModel : IValidatableObject |
There was a problem hiding this comment.
Do we really need IValidatableObject? IEmailAddressValidator can handle the validation logic, so you can check directly in the POST
| @@ -49,7 +55,7 @@ public async Task<IActionResult> Index() | |||
|
|
|||
| var user = await _userService.GetAuthenticatedUserAsync(User); | |||
|
|
|||
| return View(new ChangeEmailViewModel() { Email = ((User)user).Email }); | |||
| return View(new ChangeEmailViewModel(_emailAddressValidator) { Email = ((User)user).Email }); | |||
There was a problem hiding this comment.
The service needs to be resolved in the Vallidate method
There was a problem hiding this comment.
OK: Resolve EmailAddressValidator from Validate method
| @@ -38,7 +38,7 @@ public class ChangeEmailController : Controller | |||
| _userService = userService; | |||
| _userManager = userManager; | |||
| _siteService = siteService; | |||
| _emailAddressValidator = emailAddressValidator ?? throw new ArgumentNullException(nameof(emailAddressValidator)); | |||
| _emailAddressValidator = emailAddressValidator; | |||
There was a problem hiding this comment.
I'm not sure why we don't check from null in constructor which is useful for public APIs, if the intend to prevent DI from throws, it will if the service is not there
There was a problem hiding this comment.
The DI container will throw a DI exception before it ever reaches this null check so a null check here is unreachable code
There was a problem hiding this comment.
How? I think the check need to be in services public APIs, not in controller as you said
React to @sebastienros's comments.
Related to #5362