Best way to send multiple email types in ASP.NET MVC

Question

Hi there to the good friends of SO!

This is more of a design question so I'll get into a detailed example.

Let me explain the way we're sending emails. In various parts of the application, we create entries in our Notification table for different kinds of email we might have to send. For eg: The NotificationQueue table looks like this:

NotificationQueueID OrderID         EmailType           Notes       SentDatetime
1                   461196          OrderUpdate         SomeNote1   2020-09-01 14:45:13.153
2                   461194          OrderCancellation   SomeNote2   2020-09-01 14:45:13.153

It's accessed using the property in the DbContext as:

public DbSet<NotificationQueue> NotificationQueues { get; set; }

The different types of email is modeled in an enum:

public enum TypeOfEmail
{
    OrderCancellation,
    OrderUpdate
}

We have a EmailModel class that has a TicketsInNotificationQueue property that has a list of any of the email types we have. For eg: At any given time, it can have list of either UpdatedTickets or CancelledTickets. The email type says what type of tickets are in the TicketsInNotificationQueue property.

public class EmailModel
{
    public EmailModel(TypeOfEmail emailType, TicketsInNotificationQueue ticketsInNotificationQueue)
    {
        EmailType = emailType;
        TicketsInNotificationQueue = ticketsInNotificationQueue;
    }

    public TypeOfEmail EmailType { get; set; }
    public TicketsInNotificationQueue TicketsInNotificationQueue { get; set; }
}

public class TicketsInNotificationQueue
{
    public List<OrderCancellation> CancelledTickets { get; set; }
    public List<OrderUpdate> UpdatedTickets { get; set; }
}

public class OrderCancellation : CommonOrderInformation
{
    public string SomeOrderId { get; set; }
}

public class OrderUpdate: CommonOrderInformation
{
    public string SomeUpdateRelatedProperty { get; set; }
}

public class CommonOrderInformation
{
    public int NotificationQueueId { get; set; }
    public string ReferenceNumber { get; set; }
}

There's a method that retrieves tickets from Notification table:

public async Task<TicketsInNotificationQueue> GetTicketsfromNotificationQueueAsync(TypeOfEmail emailType)
{
    var ticketsInNotificationQueue = new TicketsInNotificationQueue();

    using (var dbCon = GetSomeDbContext())
    {
        var notifications = dbCon.NotificationQueues.Where(x => x.EmailType == emailType.ToString()).ToList();

        foreach (var ntf in notifications)
        {
            if (ntf.EmailType == TypeOfEmail.OrderCancellation.ToString())
            {
                if (ticketsInNotificationQueue.CancelledTickets == null)
                {
                    ticketsInNotificationQueue.CancelledTickets = new List<OrderCancellation>();
                }
                
                ticketsInNotificationQueue.CancelledTickets.Add(new OrderCancellation()
                {
                    NotificationQueueId = ntf.NotificationQueueID,
                    ReferenceNumber = ntf.OrderID,
                    SomeOrderId = "Something from a table."
                });
            }
            else if (ntf.EmailType == TypeOfEmail.OrderUpdate.ToString())
            {
                if (ticketsInNotificationQueue.UpdatedTickets == null)
                {
                    ticketsInNotificationQueue.UpdatedTickets = new List<OrderUpdate>();
                }

                var notes = dbCon.NotificationQueues.FirstOrDefault(x => x.NotificationQueueID == ntf.NotificationQueueID)?.Notes;

                ticketsInNotificationQueue.UpdatedTickets.Add(new OrderUpdate()
                {
                    NotificationQueueId = ntf.NotificationQueueID,
                    ReferenceNumber = ntf.OrderID,
                    SomeUpdateRelatedProperty = "Something from a table."
                });
            }
        }
    }
    return ticketsInNotificationQueue;
}

Now I just take this list, and filter out the notificationIds for the type of tickets that I just received, and work on them down the line. (I need those notificationIds to set the SentDatetime after the notification has been sent).

    var ticketsReceived = false;
    notificationIds = new List<int>();

    if (ticketsInNotificationQueue.CancelledTickets != null && ticketsInNotificationQueue.CancelledTickets.Any())
    {
        ticketsReceived = true;
        notificationIds = ticketsInNotificationQueue.CancelledTickets.Select(x => x.NotificationQueueId).ToList();
    }
    else if (ticketsInNotificationQueue.UpdatedTickets != null && ticketsInNotificationQueue.UpdatedTickets.Any())
    {
        ticketsReceived = true;
        notificationIds = ticketsInNotificationQueue.UpdatedTickets.Select(x => x.NotificationQueueId).ToList();
    }

    if (ticketsReceived)
    {
        // Proceed with the process of sending the email, and setting the `SentDateTime`
    }

The problem I see here is that as the type of emails grows bigger, let's say 10-20, the method to retrieve tickets and filter them out later needs to grow so big that it's going to spin out of control in terms of readability and code manageability which I'm not liking at all. The part where I need to check what emailType is requested in the fetch and what emailType has been received(to get the corresponding notificationIds for SentDateTime update). So is there some other way to design this workflow (I'm even open to using reflection and such) to make it more manageable and concise?

Any help would be greatly appreciated!

Answer 1

There is significant improvements that you can make to the existing system and the existing code. In the interest of having a more complete answer I'm going to recommend a not-too-expensive system overhaul and then proceed to your exact answer.

A different and industry standard approach

You already have the data structure correct, this is a perfect job for distributed persistent queues, where you don't need to worry about querying the database as much; instead you just enqueue the messages and have a processor that deals with them. Since you're using C# and .net, I strongly encourage you to check out Azure Service Bus. This is effectively a large queue where you can send messages (in your case send email requests) and you can enqueue your messages to different channels in the service bus depending on their type.

You could also look into creating a queue processor / which Azure Functions have a trigger out of the box. Once your email is sent, then you can write to your DB, we've sent this email.

So, the good design looks like

• Have distributed persistent queues, channels / enqueue the email requests to them directly.
• If you want to process them at a cadence, run your processor using cron - which most industry solutions support.
• If you want to process them as they are ending up in the queue, use a trigger.

You can enrich your processor based on your scenario, it looks like it has something to do with orders, so you may need to handle cases like not sending an already queued email after an order in cancelled, etc..

Improving what you have

Due to some circumstances, the solution above might not be available to you - so let's get to it.

See how to refactor switch statements (since you have one with if / else ifs)

• https://sourcemaking.com/refactoring/smells/switch-statements
• Ways to eliminate switch in code

You could get this through polymorphism, just create a base mail type and override the behaviors in subclasses. This way you can associate the correct queue with the correct email type.

Example:

var results = await getSomeEmails(OrderMail);
// returns a separate processor inherited from the base one, implemented in different ways.
var processor = ProcessorFactory.Create(OrderMail);
await processor.Send(results);

Some more improvements

 foreach (var ntf in notifications)
        {
            if (ntf.EmailType == TypeOfEmail.OrderCancellation.ToString())

You are checking the email type over and over again unnecessarily in this loop, you should look into moving those statements above the for and check through the passed-in parameter, since you already know the type you're querying for.

Answer 2

Thank you for the answer @Mavi Domates.

But this is what I ended up doing: I modified the EmailModel's TicketsInNotificationQueue property so that instead of having different types of classes for different types of email, we just have one type of common class. This will avoid having us to put those checks for checking what kind of email was requested in the fetch logic and also to retrieve notification Ids down the line (to update SentDateTime after email is sent) as indicated in the original question.

public class EmailModel
{
    public EmailModel(TypeOfEmail emailType, IEnumerable<CommonEmailModel> ticketsInNotificationQueue)
    {
        EmailType = emailType;
        TicketsInNotificationQueue = ticketsInNotificationQueue;
    }

    public TypeOfEmail EmailType { get; set; }
    public IEnumerable<CommonEmailModel> TicketsInNotificationQueue { get; set; }
}

public enum TypeOfEmail
{
    OrderCancellation,
    OrderUpdate
}

I added a new class called: CommonEmailModel and removed all those different email type classes (classes for OrderCancellation, OrderUpdate etc.).

public class CommonEmailModel
{
    // Common to all email types. A lot of email types only need these first 4 properties
    public string EmailType { get; set; }
    public int NotificationQueueId { get; set; }
    public string OrderId { get; set; }
    public string Notes { get; set; }

    // Cancellation related
    public string SomeOrderId { get; set; }

    // Update related
    public string SomeUpdateRelatedProperty { get; set; }

    public static async Task<IEnumerable<CommonEmailModel>> GetEmailBodyRecordsAsync(TypeOfEmail emailType)
    {
        var emailModels = new List<CommonEmailModel>();
        var emailEntries = await EmailNotificationQueue.GetEmailEntriesAsync(emailType);
        var relevantOrdIds = emailEntries.Select(x => x.OrderID).Distinct().ToList();

        using (var dbCon = GetSomeDbContext())
        {
            orders = dbCon.Orders.Where(x => relevantOrdIds.Contains(x.OrdNumber)).ToList();
        }

        foreach (var record in emailEntries)
        {
            var emailModel = new CommonEmailModel
            {
                EmailType = emailType,
                NotificationQueueId = record.NotificationQueueID,
                OrderId = record.OrderID,
                Notes = record.Notes,
                SomeOrderId = orders?.FirstOrDefault(o => o.OrdNumber == record.OrderID)?.SomeOrderIdINeed,
                SomeUpdateRelatedProperty = orders?.FirstOrDefault(o => o.OrdNumber == record.OrderID)?.UpdateRelatedPropertyINeed
            };

            emailModels.Add(emailModel);
        }

        return emailModels;
    }
}

I just get the records the following way:

var emailRecords = await CommonEmailModel.GetEmailBodyRecordsAsync(emailType);

And simply pass this to EmailModel constructor as the ticketsInNotificationQueue parameter. No need to do all that extra check of figuring out if records of certain emailType was requested. The views for OrderCancellation and OrderUpdate will use the common properties and their respective relevant properties that are present in the CommonEmailModel class.

if (emailRecords.Any())
{
    var emailModel = new EmailModel(emailType, emailRecords);
}

Now all I have to do is pass the notification Ids to a method that marks the SentDateTime column with the current timestamp by simply calling:

if (emailWasSent)
{
    await UpdateNotificationSentTimeAsync(emailRecords.Select(t => t.NotificationQueueId));
}

In the future if we keep on adding new emailType (most probably they'll carry the information in those 4 first common properties in CommonEmailModel), we can simply add new properties to the CommonEmailModel to accommodate that and just create a new view. This way I can avoid code repetition and complexity in the fetch and also at the end while updating the SentDateTime.