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?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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).
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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!)
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?
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
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.
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
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.