Add cc bcc csv comma delimited V2#21
Conversation
- Add comma-delimited CSV option - Add cc and bcc fields - Allow inline values for Recipient, cc, bcc
V1.0.3A
- Add comma-delimited CSV option - Add cc and bcc fields
Remove note for feature that was not added.
- Add comma-delimited CSV option - Add cc and bcc fields
- Add cc and bcc fields
- Add comma-delimited CSV option
Address comments
Address comment
Address comment
Address comments
Address Liam's comments
Updated to V1.0.4
Updated method per coordination with Liam
Fixed bug in handling empty cc and bcc
Fix list within list
liamlynch-data
left a comment
There was a problem hiding this comment.
That's much better - thanks for the changes!
Re:
I did NOT incorporate changing to SMTP.send_message(msg)
Ok no worries - perhaps we'll change it later, it doesn't matter too much
Also, I did not incorporate hard coded CC or BCC fields
Not really an issue either
There are a couple more things to address here - if you can make those changes then I'll do a full test on it (I gave it a quick test already, seems fine once I fixed that double line issue).
Address comment
Address comments
Address comment
Address comments
|
@liamlynch-data - Just made final adjustments! |
sendmail_changes.patch
sendmail_changes.patch
sendmail_changes.patch
|
All changes from the sendmail_changes.patch have now been incorporated! |
|
LGTM and tested. The following tests were run with the patch code applied: Basic tests Default: Normal email, no CC/BCC, Email body in table (text), no attachments ✅ Add CC/BCC fields ✅ Error validation Values: Email body Attachments Email body plain text - with attach variable - not relevant, SKIP Regression Channels ✅ CC/BCC ✅ |
liamlynch-data
left a comment
There was a problem hiding this comment.
Approving - see previous comment for details
|
@alexbourret @MayeulRousselet - as discussed it would be great if one of you could take a quick look, given the validation changes and the issues with multiple recipients we fixed |
alexbourret
left a comment
There was a problem hiding this comment.
LGTM, just update the plugin.json with the new tags and category, and maybe clarify the expected format in the cc and bcc columns
Revise to minor update.
Adjusted language for expected data format (recipients, cc, bcc)
There was a problem hiding this comment.
Regression found:
if Subject (column) or Body (column) is a datetime column we have this error:
[18:05:25] [INFO] [dku.utils] - TypeError: unsupported operand type(s) for +: 'datetime.datetime' and 'str'
Works with previous version 1.0.3 in release Version: 14.3.2 here https://released-design.qa.managedinstances.dkucloud-dev.com/projects/TEDDY_COPY/flow/
error caused by the input dataset which contains an empty row so no bug here 😅
|
Found an issue but already there before: it is a Python limitation. so works with {{ attachments['16rows'].html_table }} |
RoyTeddy
left a comment
There was a problem hiding this comment.
LGTM and tested 👍
Conditional formatting works:
- in attachements ✅
- in body ✅
...
This one seems pre-existing, nothing to do with the change, and kind of as designed (or a limitation we can't do much about) - so we can totally ignore this can't we @RoyTeddy ? |
@liamlynch-data yes we can ignore it 😉 |
Yes this is not a regression, v 1.0.3 of the plugin does this, e.g.:
|
MayeulRousselet
left a comment
There was a problem hiding this comment.
Arriving late to the party, but I have a fundamental issue: If there are multiple recipient in the same row, and 1 CC/BCC, the CC/BCC will get spammed as he receive one email per recipient.
2 solutions:
- Cheapest: having a warning next to
RecipientandCC/BCC - Hardest, but better: havnig the possibility to send the same email to multiple recipient (need a DSS change)
I think we need to do the cheap option for now - @holstbone would you be able to add the warning? If you're not sure what needs doing I could send you a patch, but it should be simple Re " the possibility to send the same email to multiple recipient" - we have a card already for this, but it's not scheduled: I'd also like to reiterate that we should really fix the following issue before we release with this (not necessarily in the same PR but before we release a version of the plugin): |
Logged as a blocker in SH @liamlynch-data |
Updated descriptions for cc_column and bcc_column to include a note about duplicate data causing multiple emails.
|
Hi @MayeulRousselet and @liamlynch-data I have updated the descriptions for cc_column and bcc_column to include a note about duplicate data causing multiple emails. Added text: "Note: Duplicate data will cause multiple emails." Hopefully this addresses your concern! |
|
Hi @MayeulRousselet - I wanted to follow up on two items: 1.) Does my adjustment made on April 15th address your concern? See: #21 (comment) 2.) Regarding Shortcut #298155, did you actually test on this Pull Request (with the new code)? We did some better handling of types, but not certain if it specifically addresses the issue in the Shortcut. See: https://app.shortcut.com/dataiku/story/298155/sendmail-plugin-wrong-column-type-for-email-fields-could-be-handled-better |
Re 2) - I raised this after testing this PR, so this is something we still need to fix. But I can do it after this one is merged, no worries. |


@liamlynch-data - I created a clean, new pull request here.
(original PR here )
NOTES:
I did NOT incorporate changing to
SMTP.send_message(msg), for two reasons:sendmailmethod was selected by the Plugin dev team, so updates should probably best be led by them.Also, I did not incorporate hard coded CC or BCC fields, primarily because the UI/UX gets really messy. Since "Recipient" field is always pulled from the dataset, then the best UI/UX is to also always pull the "cc" and "bcc" from the dataset as well.