Supports computed email subject

[rein1410]

Description

EmailEventListener's setSubject function how accepts either a string or a callback function that contains Event, RequestContext and Injector as arguments. Allowing better email subject customization.

The email plugin's TemplateLoader now has an optional function loadSubject that has Injector, RequestContext and LoadTemplateInput along with the original subject configured from the EmailEventListener as arguments.

These changes allow the user to more freely define their email subjects based on their injected providers, the event or the incoming request.

Should be noted that the TemplateLoader's subject will override the EmailEventListener's subject

Breaking changes

No

Checklist

📌 Always:

• I have set a clear title
• My PR is small and contains a single feature
• I have checked my own PR

👍 Most of the time:

• I have added or updated test cases
• I have updated the README if needed

[michaelbromley]

A couple of the tests are failing because the assertions are wrong

[michaelbromley]

Thank you for this great PR!

@martijnvdbrug since you were involved in the original issue, do you have any feedback on this implementation?

[martijnvdbrug]

Thank you for this great PR!

@martijnvdbrug since you were involved in the original issue, do you have any feedback on this implementation?

Will review this Tuesday!

[martijnvdbrug]

Nice work, and nice work on the mocks in the e2e tests 👌

I am wondering if we need the templateLoader.loadSubject() function. It's a bit confusing to me that there are now 2 ways to load a subject: templateLoader.loadSubject and setSubject(). You could even use both methods, in which case it is not clear which subject function is used.

Can we remove the templateLoader.loadSubject, and have the template loader only handle templates? What do you think about this? Maybe I am missing something...

[rein1410]

In my opinion, I think it's fine having the templateLoader.loadSubject() function. Since I find it particularly useful in cases where you want to reduce code redundancy, as devs could create something like an EmailTemplateService with a getSubject function to get their desired subject based on the provided arguments across all emails without needing to call setSubject on each of the EmailSubjectHandler they created. And honestly this is why I created this, so that my team could spend less time rewriting redundant code.

I also noted the templateLoader.loadSubject() will override the EmailEventListener's subject. So if you have a special kind of handler that fetches subject differently and don't want the templateLoader to rewrite this, you can define your own logic to ignore this like having the EmailTemplateService.getSubject() return null on this kind of handler and get the original subject instead because templateLoader.loadSubject() already has the original subject that you configured on the loader.

[michaelbromley]

Thanks for the review @martijnvdbrug

I tend to bias toward adding fewer APIs in general, but I also appreciate the case for the TemplateLoader.loadSubject() method. But I must admit having 2 APIs to do the same thing does make me wary.

One other concern is that I think having loadSubject on a class that is for loading templates is a little less cohesive. I'd tend to agree with this.

Just to explore this further before we commit to one path or the other, let's imagine how code reuse could look without the new method on TemplateLoader:

// subject.service.ts

export class SubjectService {

  loadSubject(event: Event, ctx: RequestContext): string {
    // ... custom logic here
  }
}

// email-event-handlers.ts

function loadEmailSubject(event: Event, ctx: RequestContext, injector: Injector) {
  return injector.get(SubjectService).loadSubject(event, ctx);
}


for (const handler of myEmailHandlers) {
  handler.loadSubject(loadEmailSubject);
}

And compare this to an implementation with TemplateLoader.loadSubject()

// subject.service.ts

export class SubjectService {

  loadSubject(ctx: RequestContext, input: LoadTemplateInput & { subject: string; }): string {
    // ... custom logic here
  }
}

// my-template-loader.ts

export class MyTemplateLoader implements TemplateLoader {

    async loadTemplate(/* ... */ ): Promise<string> {
        // ... omitted
    }

    async loadSubject(injector: Injector, ctx: RequestContext, input: LoadTemplateInput & { subject: string; }): Promise<string> {
        return injector.get(SubjectService).loadSubject(ctx, input);
    }
}

The amount of code involved in both approaches is comparable I'd say. What do you think @rein1410?

[rein1410]

I guess a for loop could be a good alternative to the TemplateLoader.loadSubject() function. Then sure, let me omit it from the commit then. Then after this I hope that we are good to go @michaelbromley?

[rein1410]

Done! @michaelbromley

[michaelbromley]

Thank you @rein1410 👍

[martijnvdbrug]

Ah, this just got merged during me reading this 😆 , sorry for the late response. I agree with the solution though. Thanks @rein1410 for picking this up.