patterncsharpMinor
Validating email confirmation and sending different emails to admins
Viewed 0 times
confirmationemailsendingvalidatingdifferentemailsandadmins
Problem
I wrote a method that will confirm email with token send to user emails. It will also send different email depending on the condition for the registered user. If anyone would write the same method, how would you write this code snippet?
```
[AllowAnonymous, Route("confirm-email", Name = AccountControllerRoute.GetConfirmEmail)]
public async Task ConfirmEmail(int accountId, string token)
{
if (accountId == 0 || token == null)
return HttpNotFound();
var account = await AccountManager.FindByIdAsync(accountId);
if (account == null)
return HttpNotFound();
if (account.EmailConfirmed)
return View("ConfirmEmail"); //return page that will say the token has expired
var result = await AccountManager.ConfirmEmailAsync(accountId, token);
var emailBody = string.Empty;
if (result.Succeeded)
{
if(account.Institution == null)
{
// send message notifying admin that a new institution needs review and setup
emailBody = $"{account.UserName}|{account.profile.contact}|{account.profile.institutionName}|{account.profile.institutionAddress}";
await AccountManager.SendEmailAsync(account.Id, EmailNames.NewInstitutionRequest, "");
return View("ConfirmEmail");
}
// Pass Name, Username
emailBody = $"{account.Name}|{account.UserName}";
if (account.Institution.IsClient)
{
if (account.Institution.Licence.AllowedAccounts == 0)
{
await AccountManager.SendEmailAsync(account.Id, EmailNames.NewUserRequest, emailBody);
return View("ConfirmEmail"); // send a different message notifying admin about exceeding allow accounts
}
// send message notifying admin that a new user request for access
await AccountManager.SendEmailAsync(account.Id, EmailNames.NewUserRequest, emailBody);
return View("ConfirmEmail");
}
else
{
```
[AllowAnonymous, Route("confirm-email", Name = AccountControllerRoute.GetConfirmEmail)]
public async Task ConfirmEmail(int accountId, string token)
{
if (accountId == 0 || token == null)
return HttpNotFound();
var account = await AccountManager.FindByIdAsync(accountId);
if (account == null)
return HttpNotFound();
if (account.EmailConfirmed)
return View("ConfirmEmail"); //return page that will say the token has expired
var result = await AccountManager.ConfirmEmailAsync(accountId, token);
var emailBody = string.Empty;
if (result.Succeeded)
{
if(account.Institution == null)
{
// send message notifying admin that a new institution needs review and setup
emailBody = $"{account.UserName}|{account.profile.contact}|{account.profile.institutionName}|{account.profile.institutionAddress}";
await AccountManager.SendEmailAsync(account.Id, EmailNames.NewInstitutionRequest, "");
return View("ConfirmEmail");
}
// Pass Name, Username
emailBody = $"{account.Name}|{account.UserName}";
if (account.Institution.IsClient)
{
if (account.Institution.Licence.AllowedAccounts == 0)
{
await AccountManager.SendEmailAsync(account.Id, EmailNames.NewUserRequest, emailBody);
return View("ConfirmEmail"); // send a different message notifying admin about exceeding allow accounts
}
// send message notifying admin that a new user request for access
await AccountManager.SendEmailAsync(account.Id, EmailNames.NewUserRequest, emailBody);
return View("ConfirmEmail");
}
else
{
Solution
I'm a bit confused by this:
if (account.EmailConfirmed)
return View("ConfirmEmail"); //return page that will say the token has expired
Your comment doesn't correspond with the actual code, especially when that same view is returned elsewhere.
The
Also,
I don't know what you're doing with
I also don't see the point of compiling it inside
What happens inside
What is
if (account.EmailConfirmed)
return View("ConfirmEmail"); //return page that will say the token has expired
Your comment doesn't correspond with the actual code, especially when that same view is returned elsewhere.
The
if (result.Succeeded) should be inverted to avoid 30 lines of indentation. Why not do if (!result.Succeeded) and return View("Error"); immediately afterwards?Also,
result is a fairly meaningless name.I don't know what you're doing with
emailBody, but judging from its format and the comments it looks like you're using it to pass arguments in a delimiter-separated string. That's odd, to say the least.I also don't see the point of compiling it inside
if(account.Institution == null) and then not using its value.What happens inside
if (account.Institution.Licence.AllowedAccounts == 0) looks to be exactly the same as what happens if that isn't the case.What is
EmailNames? An enum? I don't think that class should have a plural name, and it doesn't look like it is an "email name".Context
StackExchange Code Review Q#139348, answer score: 2
Revisions (0)
No revisions yet.