Identity: Productionize IEmailSender interface

[javiercn]

We need to refactor the interface that we had on the template (now on the UI package) into a shape that we like, and find the right place for it. (probably the M.A.I package).

My proposal

public interface IEmailSender 
{
    Task SendAsync(Email email);
}

public class Email
{
    public IList<string> To { get; }
    public string Subject { get; set; }
    public string Body { get; set; }
}

The idea of using a class is to allow for adding new members if necessary in the future without causing breaking changes.

/cc: @HaoK

[HaoK]

What package do you mean by M.A.I, one of the core Identity packages?

[javiercn]

Yep

[HaoK]

Doesn't really belong there, the rest of identity has nothing to do with sending emails (only the default UI does). What's wrong with keeping it in the UI package, its really just meant to be an adapter interface for a real email service...

But now that this is all in the UI package, we are free to change how this works if we have better ideas/names now since the code doesn't show up in the app...

[javiercn]

the rest of identity has nothing to do with sending emails
Is the name what you don't like?

The concept of email registration/etc is already part of identity, so I think it's ok to have the interface at the same layer. If not, where would we put this?

[HaoK]

The only reason you would ever want to use this interface, is if you were configuring the UI, so it really belongs with the UI package. Its not a general purpose email sender interface, its purely an adapter which the UI code uses to send email.

[HaoK]

If there was a general purpose email sending interface, we could use that... but I don't think we should build that as part of identity...

[blowdart]

Needs CC and BCC list. Also PlainText/HTML property, and maybe a headers property.

But yea, needs to be somewhere other than Identity, which would then take a dependency.

[grahamehorner]

@blowdart @HaoK I personally think the email interfaces should be in a System.Net namespace and should have support for digital signatures etc. the transport implementation should also be separated from the data structures 🤔 so a implementation for SMTP would be available as standard, but an option for using SendGrid could be DependencyInjected

https://github.com/dotnet/corefx/blob/master/src/System.Net.Mail/src/System/Net/Mail/MailMessage.cs

[javiercn]

Following up on this. I'm ok with this living on the UI package, so the work here is to do the following:
1) Update the interface to look like the snippet below. (This interface as called out above is not a general purpose email sending interface but an adapter for the UI). The only point of having a class for email is in case we need to extend it in the future, hence the DTO.

public interface IEmailSender 
{
    Task SendAsync(Email email);
}

public class Email
{
    public IList<string> To { get; set; }
    public string Subject { get; set; }
    public string Body { get; set; }
}

2) Change the usage of IEmailSender in the UI to make it optional (Injecting IEnumerable instead of IEmailSender).
3) Get rid of the dummy EmailSender implementation.

@HaoK Does this sound good to you?

[HaoK]

If we change the interface we should probably change the name just to avoid conflicts with existing IEmailSender classes, maybe just add an UI IUIEmailSender and UIEmail?

Is it really better to make it optional instead of a default no-op? What happens when there's no email sender, do we stop rendering ForgotPassword links, or just have it throw? We'd have to probably throw if requireEmailConfirmation = true and there is no email sender. I believe the idea behind making email sending part of the flows in templates for 2.0 was that we didn't think it was realistic for a site not to have an email sender...

[blowdart]

I'm moving this out to 2.2

[HaoK]

I think @javiercn was planning on doing something about this in rc1

[javiercn]

@blowdart Id rather address this for 2.1 just to make sure we have something that doesn't corner us in the future.

[blowdart]

Talk to @mkArtakMSFT - I feel this has been left too late now

[HaoK]

One cheap thing you could do for future proofing in 2.1 is just to make it not an interface... so you can revisit things later

public class EmailSender {
   Send(whatever)
}

Lets you just always register a no-op one, and they can plug in a real one if they want, and you aren't rushed into defining the IEmailSender interface

[HaoK]

I also suggest something really specific like DefaultUIEmailSender

[javiercn]

@HaoK I would just change it to what we discussed above. Switch from parameters to object is cheap, swiching to a class is also cheap, but then you still have to live with the old methods.