Thanks for fixing this! Apologies for the ridiculously long turnaround time 😅, I wanted to sit on the implementation design a bit and see if this was the way we wanted to go about modifying EmailService or if there was a more optimal way.

I think returning booleans in the EmailService methods instead of doing something like throwing errors makes sense, since the EmailService historically hasn't let the caller know about success/failure, and can now optionally do so with returning true/false (optional meaning the caller doesn't need to check the return value of the method, it can just make the call like it does now). But for things like tests, we can now assert the success/failure of the methods and increase the granularity of what we're testing (i.e. testing EJS template correctness).

Un-approving the PR because there is a breaking change here that I only found out of by manually testing the API and trying to hit a route.

The issue stems from EmailService in the constructor, where the mailer parameter is being set. It seems like for any service that we inject into a controller (e.g. UserAuthService, UserAccountService, EmailService, pretty much any of the services), routing-controllers does some modification to the parameters of the constructor and sets its own parameters for those constructors. What I mean is this - below is the error trace of the error I was receiving on hitting any API route that depends on EmailService. The first part of the logs is the output of console logging the mailer object passed into the constructor:

ContainerInstance {
  services: [
    { type: [class UserAccountService], value: [UserAccountService] },
    { type: [class StorageService] },
    { type: [class UserAuthService], value: [UserAuthService] },
    { type: [class EventService] },
    { type: [class PermissionsService] },
    { type: [class AttendanceService] },
    { type: [class MerchStoreService] },
    { type: [class FeedbackService] },
    { type: [class ResumeService] },
    { type: [Function: ConnectionManager], value: [ConnectionManager] },
    { type: [class RequestLogger], value: RequestLogger {} },
    { type: [class MetricsRecorder], value: MetricsRecorder {} },
    { type: [class ErrorHandler], value: ErrorHandler {} },
    { type: [class NotFoundHandler], value: NotFoundHandler {} },
    { type: [class AuthController] },
    { type: [Function] }
  ],
  id: undefined
}
warn: 2022-11-09T05:40:27.473Z [request 95e766bc104f54a6bca0c88ff3167564]: TypeError [500]: this.mailer.setApiKey is not a function 
TypeError: this.mailer.setApiKey is not a function
    at new EmailService (/home/shrav/projects/membership-portal/services/EmailService.ts:40:17)
    at ContainerInstance.getServiceValue (/home/shrav/projects/membership-portal/src/ContainerInstance.ts:336:21)
    at ContainerInstance.get (/home/shrav/projects/membership-portal/src/ContainerInstance.ts:119:21)
    at /home/shrav/projects/membership-portal/src/ContainerInstance.ts:358:29
    at Array.map (<anonymous>)
    at ContainerInstance.initializeParams (/home/shrav/projects/membership-portal/src/ContainerInstance.ts:352:27)
    at ContainerInstance.getServiceValue (/home/shrav/projects/membership-portal/src/ContainerInstance.ts:305:47)
    at ContainerInstance.get (/home/shrav/projects/membership-portal/src/ContainerInstance.ts:119:21)
    at Function.Container.get (/home/shrav/projects/membership-portal/src/Container.ts:107:36)
    at Object.getFromContainer (/home/shrav/projects/membership-portal/node_modules/src/container.ts:66:38)
 {"timestamp":"2022-11-08 21:40:27"}
Tue, Nov 8, 2022 9:40 PM PT [IP -] [Trace 95e766bc104f54a6bca0c88ff3167564] POST /api/v2/auth/login 500 38.089ms

As you can see, the mailer object is not of type MailService, but rather, of type ContainerInstance. This is likely because routing-controllers is doing some form of dependency injection with any services that the API layer dependency-injects into its own class.

To fix this issue would require a decently large refactor, since we'd need to somehow still allow mocking of mailer but without actually passing it into the constructor, or we'd need to do something differently than passing in the services via. the constructor of the API layer.

As a result, I'm going to close this PR for now, but leave the issue open still in case anyone wants to pick this up in the future. I think it's at a low-enough severity where we don't necessarily need this immediately, but I do think it will be worth revisiting this PR once we start implementing more email methods and testing which might cause the bugs this very PR was attempting to catch.