adapt-security/adapt-authoring-mailer

adapt-authoring-mailer's Issues

Can we refactor send to return the same data as nodemailer .sendMail

May be useful...

Can we also refactor this a bit to return early (and throw errors as it's async), the code's a bit difficult to follow by eye in its current form - something like:

(in fact the first check shouldn't be needed, as the module shouldn't initialise if it's not been enabled)

async send(to, subject, text, html) {
    if (!this.useSmtp || !this.isConfigValid || !this.transporter) {
      throw new Error(this.app.lang.t("info.smtpNotEnabled"));
    }
    if (!this.isConnectionVerified) {
      // connection not tested before, should we do testConnection() here before sending?
    }
    if (!MailerUtils.isValidEmail(to)) {
      throw new Error(this.app.lang.t("error.smptEmailValidationFailed", {email: to}));
    }
    try {
      const info = await this.transporter.sendMail({to, subject, text, html});
      this.log('info', `${this.app.lang.t('info.sentSuccessfully')} ${info.response}`);
      return info;
    } catch(e) {
      this.log('error', 'error.sendFailed');
      throw this.formatError('sendFailed', e);
    }
  }

Typos in lang strings

• smptUrlValidationFailed: string should be Validation of smtp connection parameters failed.
• smptEmailValidationFailed: key should be smtpEmailValidationFailed
• smptUrlValidationFailed: key should be smtpUrlValidationFailed
• smptParamsValidationFailed: key should be smtpParamsValidationFailed

Should we remove config options?

I'm thinking it may be a bit less hassle to remove all SMTP connection config options except connectionUrl (to cut down on the logic that's needed in mailer)

Remove app reference

https://github.com/adapt-security/adapt-authoring-mailer/blob/master/lib/mailermodule.js#L12

As we've moved away from setting references on app. Think it's fine to load the module as normal, i.e.

const mailer = await this.app.waitForModule('mailer');
mailer.send(a, b, c);

Logic seems a bit off when checking connection strings

See mailermodule.js#L57-L72.

If you have a valid connectionUrl, the logic falls through to the else, and then fails if you don't have the port etc. defined (which you shouldn't do if you're using a connection string).

Could potentially look into added a string format for connectionUrl

i.e. moving the validation to jsonschema module

This might turn out to be a bit tricky given the possible params...

isReady override can be moved to init

I'd generally advise against overriding setReady. It's fine to add the logic to the bottom of init, then calling isReady:

 async init() {
    this.isConfigValid = false;
    this.isConnectionVerified = false;
    await this.readConfig();
    if (this.useSmtp) this.createTransporter();
    
    if (this.useSmtp && this.isConfigValid && this.transporter) {
      this.log('debug', this.app.lang.t("info.smtpOk"));
    } else {
      this.log('debug', this.app.lang.t("info.smtpNotEnabled"));
    }
    this.setReady();
  }

As a side-note: if you're overriding another function, it's best to call the super function directly rather than copying the behaviour (obviously if you want to modify the super behaviour, that's fine):

function isReady() {
   // ... my custom code
  super.isReady(); // or other way of calling depending on what you're doing, i.e. super.isReady.apply(scope, args)
}

Add REST API docs

See AdaptFrameworkModule for an example (it's recommended to add a separate file with API defs for non-trivial APIs to reduce clutter (see apidefs.js).

Rename useSmtp config option

I think just calling this enable works better, and fits with some of the options in other modules.

Would also be nice to return really early is enable is false (maybe have the check here)