LEAF 4047 email addresses in to cc, LEAF 4126 multioption list formatting #2282
Conversation
aerinkayne
requested review from
mgaoVA,
pete-nerantzinis,
Pelentan,
shaneodd and
jampaul3
| // For each line in template, add that email address, if valid | ||
| foreach($emailListContentList as $emailAddress) { | ||
| $emailContentList = explode(PHP_EOL, trim($this->setContent($emailTemplate))); | ||
| $extractEmailReg = "/([a-z0-9\+_\-]+)(\.[a-z0-9\+_\-]+)*@([a-z0-9\-]+\.)+[a-z]{2,6}/i"; |
There was a problem hiding this comment.
Can we make this a class variable and assign the contents here and use it where needed instead of assigning it these three times? Also, is this one missing the $ before the /i? It is there for the other two.
There was a problem hiding this comment.
I think this is what github did to me/Shane yesterday (showing an older commit state).
This was an original approach that I changed after I realized there were issues with it.
The new version just grabs the trimmed line minus html and passes it to addRecipient and /or addCc.
They both use the updated regex, but it's only in those two places.
Could still update it, more a heads up that the preview is behind for some reason.
There was a problem hiding this comment.
I think he is talking about making the regex a class variable. This is also a variable set on line 221 and 286.
There was a problem hiding this comment.
I can still do that, just thought it was odd that the section of the comment is from a few commits ago. This variable does not exist there anymore and had been gone by the time I PRd it.
There was a problem hiding this comment.
I also second moving the regex to a singular location since I have a feeling that will be updated in the future. Also a a note, we may want to look closer at setting up a regex for email closer to the standards set. This is an example and would be a reason for putting it as a class variable. https://stackoverflow.com/questions/13992403/regex-validation-of-email-addresses-according-to-rfc5321-rfc5322 otherwise everything else appears good.
Pelentan
added
the
With QA
These both impact the Email Template Editor. Since edited code overlaps they are branched together.
4126
updates a multi-option format to display selected options in a list rather than comma separated string.
4047
Added new property for the Email class to set Smarty variables specifically for the To and Cc templates.
This allows admins to use {{field.id}} to assign email recipients for some form question formats.
A separate value allows more control over which input values can be assigned and their formatting.
Updated a malformed email regex and expanded it to include the local fake testing domain ._.
Removed a number of dormant functions after confirmation.
Moved the query to get action events earlier in its function and added a test condition for calling getFields.
This allows field values to be used in the sendback (Return to Requestor) templates and, in the case of multiple events on an action, limits one call to getFields.
front end:
-disabled indicators will not show when using the field selection helper.
-selector has an initial empty value to avoid needing to toggle it for the first form, and adjusted reset behavior.
-fixed a reference to a data property that could cause a display issue for the first indicator value.
-updated selector labels and minor styling updates.
Impact/ Testing
Email Template Editor UI field selector should behave as expected.
Behavior/formatting of email template areas (body, subject, to and cc) should continue to behave as expected.
To Cc templates can dynamically add email addresses based on request fields (currently limited to orgchart emp, radio, dropdown, multiselect, checkbox(es)). Both multiselect and checkboxes selections should show as lists in the email body.