Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

Current implementation of EmailTemplate.send

2011-12-26
2012-09-09
  • The current implementation of the service EmailTemplate.send
    (sendEmailTemplate.groovy) does not use the fields smtpStartTls, smtpSsl,
    storeHost, storePort, storeProtocol & storeDelete of EmailServer entity. Is it
    the expectation that apps would have to develop their own implementations of
    EmailTemplate.send service for instances where these fields assume non-default
    values? For example, the email server that I am using requires ssl to be
    enabled. However, sendEmailTemplate.groovy does not enable that, so should I
    be writing a separate sendEmailTemplateCustom.groovy to handle this situation?

     
  • David E. Jones
    David E. Jones
    2011-12-26

    The store* fields are for polling an email server, not for sending (they are
    used in the pollEmailServer.groovy script).

    You are correct that the current send email script does not support StartTLS
    or SSL. It simply hasn't been developed yet. If this is something you'd like
    to work on and contribute I'd be happy to review it for free. Otherwise, I'll
    get to it sooner or later... it shouldn't be too much work to wire it up, just
    some research on the email sending library (Commons Mail) which hopefully
    supports it, otherwise it'll involve more work to switch libraries.

     
  • I was able to dig into org.moqui.impl.EmailServices.send#EmailTemplate service
    and eventually make it work.

    I made the following changes to make it work. If you think the direction is
    right and is in sync with general philosophy of moqui, I can send the patch
    for code review.

    In sendEmailTemplate.groovy, transfer SSL and TLS configurations from
    emailServer entity to org.apache.commons.mail.HtmlEmail.

    emailTemplateAttachmentList evaluation was causing a "not a valid field name
    or relationship name for entity" exception. This was because fullEntityName
    and _shortened_EntityName were being compared for getting related
    attachments("moqui.basic.email.EmailTemplate".equals("EmailTemplate")). So
    broaden the comparison expression for reverseRelNode in
    EntityDefinition.getRelationshipNode().

    EmailMessage has emailMessageId + emailTypeEnumId as composite primary key.
    Remove emailTypeEnumId field from composite primary key. So, in effect it
    becomes a singlePk. Otherwise isDoublePk and doublePkSecondaryName would be
    used for storing the entity, which would result in wrong field data getting
    calculated. Alternatively, I could have added a emailTypeEnumId value in the
    call to the auto create#EmailMessage. But, I thought the first solution is
    probably better than the second.

    I feel my approach of changing the primary key definition of EmailMessage may
    be brute-force. Probably because, I do not understand the use cases of
    emailTypeEnumId completely. With my limited understanding I think,
    emailTypeEnumId duplicates the functionality of emailTemplate. The one use
    case I think it could be relevant is, when email is built without using any
    emailTemplate. However, If you can share more info on the usage scenarios of
    emailTypeEnumId, I would probably understand it better.

    I would also suggest one little enhancement, based on my own experience in
    typical web applications. In a lot of scenarios, emails are sent adhoc and do
    not have the need to be persisted for future reference. For e.g., a one time
    broadcast of a greeting / welcome message. Most of the current email servers
    anyway persist them, in mailboxes. So, it would become redundant data from
    application point of view. Hence, persistence of email message should probably
    be based on a service parameter rather than "always". If there is an agreement
    on this feature, I can make change to the service implementation for this as
    well, before sending the patch.

    Thanks

     
  • David E. Jones
    David E. Jones
    2011-12-30

    Thanks for testing all of this Vasanth, and for your detailed feedback. This
    is one part of the project (as you can probably tell) that I haven't yet used
    in another project, and so haven't tested much.

    I looked at the EmailMessage double PK issue, and the is-pk on the
    emailTypeEnumId was a mistake and shouldn't be there.

    Could send over patches for your various changes (either in this forum or on a
    tracker issue)? I'll take a look at these and work with to get some fixes in
    so that it works for your intended use.

     
  • David E. Jones
    David E. Jones
    2011-12-30

    As a follow-up on my last comment: in commit 2d783a6 I removed the second
    field from the EmailMessage PK, and in commit 909ee1d I fixed the issues with
    the auto reverse relationship and full entity name, and with forUpdate passed
    as null to findRelated (and findRelatedOne).

     
  • I was able to pull all the changes you made and tested against my test app and
    so, the only change I needed to do was applying SSL, TLS setting in
    sendEmailTemplate.groovy. I also added _saveMessage _ parameter in the service
    like I suggested in message#3. However, while testing it I found out an issue
    which I fixed as per my understanding of groovy (this is my first serious
    groovy foray). The issue was

    Even when I was setting a boolean parameter of a service with value "false",
    the service implementation is receiving the value as true

    Digging in a bit, I inferred that type coercion of "false" to a
    java.lang.Boolean using groovy (asType) was resulting in a Boolean.TRUE value
    because "false" is not an empty string or a null value. This was in
    StupidUtilities.groovy. In ServiceDefinition also, if the parameter is of type
    boolean and has a false value, and @required is set as true, the service
    validation would always fail. (I noticed your //TODO comment. after uploading
    the patch!)

    I have uploaded the changes as a patch at https://sourceforge.net/tracker/?fu
    nc=detail&aid=3467497&group_id=302949&atid=1277178.