sklarsa / django-sendgrid-v5

An implementation of Django's EmailBackend compatible with sendgrid-python v5+
MIT License
318 stars 54 forks source link

increase flexibility of _build_sg_mail #90

Closed alichass closed 3 years ago

alichass commented 3 years ago
sklarsa commented 3 years ago

I guess I need to kick off the tests myself? I don't really use GH actions so I'm not too familiar with this workflow.

alichass commented 3 years ago

Thanks for your contribution, and looks like an interesting change! Can you update the test suite to reflect some of this additional logic? I want to ensure that we maintain backwards compatibility while adding the new features, and that there are no conflicts.

Sorry I'm not the most familiar with the django test suite, so it might take me a while to add tests for the additional functionality, but the changes I've made seem to at least pass the tests in nosetests -c nose.cfg -s --with-coverage

sklarsa commented 3 years ago

Thanks for your contribution, and looks like an interesting change! Can you update the test suite to reflect some of this additional logic? I want to ensure that we maintain backwards compatibility while adding the new features, and that there are no conflicts.

Sorry I'm not the most familiar with the django test suite, so it might take me a while to add tests for the additional functionality, but the changes I've made seem to at least pass the tests in nosetests -c nose.cfg -s --with-coverage

No worries! I can add a few quick tests.

Can you run the following to lint/format the code? flake8 sendgrid_backend/ && isort ./

alichass commented 3 years ago

LGTM. I'll handle the tests post-merge. Can you just run the code formatters/linters on your modified files? I added the commands to the CONTRIBUTING file, or you can check out the CI file starting here

Hi so I've corrected the issues thrown up by flake8, everything else seems to be clear. Would you like me to also run black, and if so, should I use default black line-length or 120 as outlined in .flake8?

sklarsa commented 3 years ago

Looks like max line length is 88 (per black config in pyproject.toml. Running black should pick those settings up

  black --check ./
  shell: /usr/bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.9.6/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.9.6/x64/lib
would reformat /home/runner/work/django-sendgrid-v5/django-sendgrid-v5/sendgrid_backend/mail.py
Oh no! 💥 💔 💥
1 file would be reformatted, 11 files would be left unchanged.
Error: Process completed with exit code 1.

I'm going to try to get these tests to run without manual intervention for first-time contributors for a quicker feedback cycle. This is for security reasons, but it's annoying right now 😆

sklarsa commented 3 years ago

Ok... actions should (hopefully) run automatically after your next push

alichass commented 3 years ago

Hi @sklarsa sorry I forgot to mention something important The .personalizations attribute obviously contains the "to" address for each personalization.

This may make it seem like setting the "to" attr on the EmailMessage is unnecessary. It is unnecessary in most cases, eg when using mail.get_connection().send_messages() ; however, the implementation of EmailMessage.send() does a check for self.recipients(), which means an arbitrary "to" must be set - even if it will not be used, if the given function is to be used

sklarsa commented 3 years ago

Can you add usage examples to the readme? That would help clear up some of the field duplication

On Aug 4, 2021, at 2:23 PM, alichass @.***> wrote:

 Hi @sklarsa sorry I forgot to mention something important The .personalizations attribute obviously contains the "to" address for each personalization.

This may make it seem like setting the "to" attr on the EmailMessage is unnecessary. It is unnecessary in most cases, eg when using mail.get_connection().send_messages() ; however, the implementation of EmailMessage.send() does a check for self.recipients(), which means an arbitrary "to" must be set - even if it will not be used, if the given function is to be used

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

alichass commented 3 years ago

@sklarsa sorry not sure where/how you'd wanna structure this in the readme, but this covers the vast majority of info (if you'd like me to add a separate readme section/add it to a specific part lemme know):

Personalizations:

This would pretty much be the "kitchen sink" of usage - theres a lot you can get out of this pattern

email = EmailMessage(from_email="hello@github.com", to=["goodbye@github.com"])
email.personalizations=[
    {"to": ["_to_email_addr_1"], "bcc":[...]  "dynamic_template_data": {...}, ...},
    {"to": ["_to_email_addr_2", "_to_email_addr_3", ...], "bcc":[...]  "dynamic_template_data": {...}, ...},
...
]
email.send()

or (to avoid setting arbitrary to)

email = EmailMessage(from_email="hello@github.com")
email.personalizations=[
    {"to": ["_to_email_addr_1"], "bcc":[...]  "dynamic_template_data": {...}, ...},
    {"to": ["_to_email_addr_2", "_to_email_addr_3", ...], "bcc":[...]  "dynamic_template_data": {...}, ...},
...
]
mail.get_connection().send_messages([email])

Make Private:

This changes nothing apart from "how" the message is delivered - the "to" emails each are given their own personalization copied from the EmailMessage ( this avoids using mass BCC as outlined by Sendgrid). This attr does nothing in if used in conjunction with the .personalizations attribute.

email = EmailMessage(from_email="hello@github.com", to=["goodbye@github.com", "othergreeting@github.com",])
email.dynamic_template_data = {...}
email.make_private = True
email.send()