search

ijsto / strapi-provider-email-mailjet

Strapi Email service provider for Mailjet
MIT License
13 stars 11 forks source link

Issue #2 fix #3

Closed dannymatkovsky closed 4 years ago

dannymatkovsky commented 4 years ago

All valid points. But while implementing I found the normalizeTo function looks inconsistent, e.g. recipients params may be an array or a string but semantically similar settings.defaultTo and recipientName (toName) - only a string.

Also I've found out that the plugin's API is different from the native one. Native API has only to and settings.defaultTo params which may be just an email address string, an {Email: ..., Name: ...} object or an array of strings or objects. This actually looks very flexible and simple. Maybe we should stick with this approach here? What do you think?

I could rewrite this PR to make it consistent with native API, but toName and defaultToName will get depracated

ScottAgirs commented 4 years ago

@burlakko sadly, deprecating defaultToName and/or toName is not an option, please consider adding extra handler to ensure capability. LMK if nothing comes to mind, I'll tackle this over the next few days.

ScottAgirs commented 4 years ago

@burlakko thanks again for your input, this should be addressed with Release 3.3.0

dannymatkovsky commented 4 years ago

Oh I actually planned to deal with it on monday, but you already got it done. Thanx!

dannymatkovsky commented 4 years ago

Though I see a potential problem here.

Line 37: To = [{ Email: to, Name: toName || settings.defaultToName }]; will stick the defaultToName even if you're using the different to.

Consider following example

...
  settings: {
      defaultTo: "john.doe@ijs.to",
      defaultToName: "Johnny Bravodoe",
    }
...
strapi.plugins["email"].services.email.send({to: 'hanna@whatever.email', ...})

It will result into [{ Email: 'hanna@whatever.email', Name: 'Johnny Bravodoe' }] recipient, which is obviously not what expected. I believe if to parameter is set the defaultToName should not be used at all

UPD. The same also true for cc and bcc fields UPD 2. Added a quick fix