Abstracting an Email Notification Service & testing the the logic of the used abstract factory

Question

In my application I have an Interface IEmailNotification that represents an Email and the concrete implementations have nothing to do with MailMessage class so to be able to send it through SmtpClient I used an adapter to let the communication between SmtpClient and IEmailNotification possible and the job of this adapter is simply delegate the Send call to SmtpClient but after converting from EmailNotification to MailMessage using an abstract Factory which in fact only copying the values from one object to the new created MailMessage object.

The first question is: my factory method takes a parameter of type IEmailNotification, does that violates the main Job of a Factory which is only creating objects?

public interface INotification
    {
        string To { get; }
        string Body { get; }
    }

public interface IEmailNotification : INotification
    {
        string From { get; }
        string Subject { get; }
        bool IsBodyHtml { get; }
        string CC { get;  }
        string BCC { get;  }
        string ReplyToList { get;  }
        List<string> AttachmentsPaths { get; }
    }

public interface IEmailNotificationService : IService
    {
        void Send(IEmailNotification notification);
    }

public class EmailNotificationServiceAdapter : IEmailNotificationService
    {
        private ISmtpClient _client;
        private MailMessageFactory _mailMessageFactory;

        public EmailNotificationServiceAdapter(ISmtpClient client, MailMessageFactory mailMessageFactory)
        {
            _client = client;
            _mailMessageFactory = mailMessageFactory;
        }

        public void Dispose()
        {
            _client.Dispose();
        }

        public void Send(IEmailNotification notification)
        {
            **using (var mailMessage = _mailMessageFactory.CreateMailMessage(notification))**
            {
                _client.Send(mailMessage);
            }
        }
    }

public class EmailMailMessageFactory : MailMessageFactory
{
    public EmailMailMessageFactory(string backupBccEmail)
        : base(backupBccEmail)
    {
    }

    public override MailMessage CreateMailMessage(IEmailNotification emailNotification)
    {
        using (var mailMessage = new MailMessage())
        {
            mailMessage.From = new MailAddress(emailNotification.From);
            mailMessage.To.Add(emailNotification.To);
            mailMessage.Subject = emailNotification.Subject;
            mailMessage.Body = emailNotification.Body;

            if (emailNotification.To.ToLower().Contains("shopfehler"))
                mailMessage.Bcc.Add(new MailAddress(_backupBccEmail));

            if (!string.IsNullOrEmpty(emailNotification.CC))
                mailMessage.CC.Add(emailNotification.CC);

            if (!string.IsNullOrEmpty(emailNotification.BCC))
                mailMessage.Bcc.Add(emailNotification.BCC);

            if (!string.IsNullOrEmpty(emailNotification.ReplyToList))
                mailMessage.ReplyToList.Add(emailNotification.ReplyToList);


            if (emailNotification.AttachmentsPaths.Count > 0)
            {
                foreach (var path in emailNotification.AttachmentsPaths)
                {
                    mailMessage.Attachments.Add(new Attachment(path));
                }
            }

            mailMessage.IsBodyHtml = emailNotification.IsBodyHtml;

            return mailMessage;
        }
    }
}

backupBccEmail is required for manipulating the EmailNotification based on some logical conditions

The second question is: When I tried to test the logic of this method It failed because it uses an external resource at the line of adding new attachments where it cannot find the related paths on the Hard Drive, what do you people think about it?

Unit Test Note: I know that only one assert per test is recommended but for now let's keep it simple & straightforward

[TestFixture]
public class EmailMailMessageFactoryTests
{
    private EmailNotification _emailNotification;

[SetUp]
public void SetUp()
{
    _emailNotification = new EmailNotification("[email protected]", "[email protected];[email protected]", "subject", "body", true)
    {
        AttachmentsPaths = new List<string> { "1", "2" },
        CC = "[email protected]",
        BCC = "[email protected]"
    };
}

[Test]
public void CreateMailMessage_WhenCalled_CreatesMailMessage()
{
    var emailMailMessageFactory = new EmailMailMessageFactory("[email protected]");

    var result = emailMailMessageFactory.CreateMailMessage(_emailNotification);

    Assert.That(result.From.Address, Is.EqualTo(_emailNotification.From));
    Assert.That(result.To[0].Address, Is.EqualTo(_emailNotification.To));
    Assert.That(result.Subject, Is.EqualTo(_emailNotification.Subject));
    Assert.That(result.Body, Is.EqualTo(_emailNotification.Body));
    Assert.That(result.IsBodyHtml, Is.EqualTo(_emailNotification.IsBodyHtml));

    Assert.That(result.Attachments.Count, Is.EqualTo(_emailNotification.AttachmentsPaths.Count));
    //this assert fails "FileNotFoundException"

    Assert.That(result.CC[0].Address, Is.EqualTo(_emailNotification.CC));
    Assert.That(result.Bcc[0].Address, Is.EqualTo(_emailNotification.BCC));
}

}

Maybe I'm misusing what's called Factory Pattern in my code!

thanks in advance

Answer 1

You are disposing the message before it even has a chance to be used by a consumer.

Remove the using block in the factory method

public override MailMessage CreateMailMessage(IEmailNotification emailNotification) {
    var mailMessage = new MailMessage();
    mailMessage.From = new MailAddress(emailNotification.From);
    mailMessage.To.Add(emailNotification.To);
    mailMessage.Subject = emailNotification.Subject;
    mailMessage.Body = emailNotification.Body;

    if (emailNotification.To.ToLower().Contains("shopfehler"))
        mailMessage.Bcc.Add(new MailAddress(_backupBccEmail));

    if (!string.IsNullOrEmpty(emailNotification.CC))
        mailMessage.CC.Add(emailNotification.CC);

    if (!string.IsNullOrEmpty(emailNotification.BCC))
        mailMessage.Bcc.Add(emailNotification.BCC);

    if (!string.IsNullOrEmpty(emailNotification.ReplyToList))
        mailMessage.ReplyToList.Add(emailNotification.ReplyToList);


    if (emailNotification.AttachmentsPaths.Count > 0) {
        foreach (var path in emailNotification.AttachmentsPaths) {
            mailMessage.Attachments.Add(new Attachment(path));
        }
    }

    mailMessage.IsBodyHtml = emailNotification.IsBodyHtml;

    return mailMessage;        
}

Leave the responsibility of disposal to the consumer of the factory.

my factory method takes a parameter of type IEmailNotification, does that violates the main Job of a Factory which is only creating objects?

A factory method can take explicit dependencies which it can use to perform its required functionality.

When I tried to test the logic of this method It failed because it uses an external resource at the line of adding new attachments where it cannot find the related paths on the Hard Drive, what do you people think about it?

Implementation concerns should be encapsulated behind abstractions that avoid tight coupling to external dependencies.

In this case, when you were testing, the Attachment will try to read the file at the provided path. Since those paths may not exist when testing, you should consider refactoring the current design.

Provide an abstraction that would allow the attachment stream to be read in isolation without any adverse behavior.

public interface IFileInfo {
    string Name { get; }
    string PhysicalPath { get; }
    Stream CreateReadStream();
}

Here is a simple implementation that can be used at run-time

public class AttachmentInfo : IFileInfo {
    private readonly FileInfo innerFile;

    public AttachmentInfo(string path) {
        innerFile = new FileInfo(path);
    }

    public string Name => innerFile.Name;

    public string PhysicalPath => innerFile.FullName;

    public Stream CreateReadStream() => innerFile.OpenRead();
}

The email notification can be refactored to use the abstraction for attachments

public interface IEmailNotification : INotification {
    string From { get; }
    string Subject { get; }
    bool IsBodyHtml { get; }
    string CC { get; }
    string BCC { get; }
    string ReplyToList { get; }
    List<IFileInfo> Attachments { get; }
}

Resulting in the factory method to become

public class EmailMailMessageFactory : MailMessageFactory {

    public EmailMailMessageFactory(string backupBccEmail)
        : base(backupBccEmail) {
    }

    public override MailMessage CreateMailMessage(IEmailNotification emailNotification) {
        var mailMessage = new MailMessage {
            From = new MailAddress(emailNotification.From),
            Subject = emailNotification.Subject,
            Body = emailNotification.Body,
            IsBodyHtml = emailNotification.IsBodyHtml
        };
        mailMessage.To.Add(emailNotification.To);

        if (emailNotification.To.ToLower().Contains("shopfehler"))
            mailMessage.Bcc.Add(new MailAddress(_backupBccEmail));

        if (!string.IsNullOrEmpty(emailNotification.CC))
            mailMessage.CC.Add(emailNotification.CC);

        if (!string.IsNullOrEmpty(emailNotification.BCC))
            mailMessage.Bcc.Add(emailNotification.BCC);

        if (!string.IsNullOrEmpty(emailNotification.ReplyToList))
            mailMessage.ReplyToList.Add(emailNotification.ReplyToList);

        if (emailNotification.Attachments.Count > 0) {
            foreach (var file in emailNotification.Attachments) {
                Stream stream = file.CreateReadStream();
                string filename = file.Name;
                var attachment = new Attachment(stream, filename);
                mailMessage.Attachments.Add(attachment);
            }
        }
        return mailMessage;
    }
}

When testing in isolation, a fake stream can be given to the attachment to allow the subject under test to be exercised.

[TestClass]
public class EmailMailMessageFactoryTests {
    [TestMethod]
    public void CreateMailMessage_WhenCalled_CreatesMailMessage() {
        //Arrange
        var stream = new MemoryStream();
        var attachments = new List<IFileInfo> {
            Mock.Of<IFileInfo>(_ => _.Name == "1" && _.CreateReadStream() == stream)
        };
        var _emailNotification = Mock.Of<IEmailNotification>(_ =>
            _.From == "[email protected]" &&
            _.To == "[email protected]" &&
            _.Subject == "subject" &&
            _.Body == "body" &&
            _.IsBodyHtml == true &&
            _.CC == "[email protected]" &&
            _.BCC == "[email protected]" &&
            _.Attachments == attachments
        );
        var emailMailMessageFactory = new EmailMailMessageFactory("[email protected]");

        //Act
        MailMessage result = emailMailMessageFactory.CreateMailMessage(_emailNotification);

        //Assert
        result.From.Address.Should().BeEquivalentTo(_emailNotification.From);
        result.To[0].Address.Should().BeEquivalentTo(_emailNotification.To.Split(',')[0]);
        result.Subject.Should().BeEquivalentTo(_emailNotification.Subject);
        result.Body.Should().BeEquivalentTo(_emailNotification.Body);
        result.IsBodyHtml.Should().Be(_emailNotification.IsBodyHtml);

        result.Attachments.Count.Should().Be(_emailNotification.Attachments.Count);

        result.CC[0].Address.Should().BeEquivalentTo(_emailNotification.CC);
        result.Bcc[0].Address.Should().BeEquivalentTo(_emailNotification.BCC);
    }
}