HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Validating email confirmation and sending different emails to admins

Submitted by: @import:stackexchange-codereview··
0
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
{

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 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.