anymail / django-anymail

Django email backends and webhooks for Amazon SES, Brevo (Sendinblue), MailerSend, Mailgun, Mailjet, Postmark, Postal, Resend, SendGrid, SparkPost, Unisender Go and more
https://anymail.dev
BSD 3-Clause "New" or "Revised" License
1.65k stars 125 forks source link

Unisender Go: new ESP #352

Closed Arondit closed 5 months ago

Arondit commented 7 months ago

https://github.com/anymail/django-anymail/issues/349

medmunds commented 7 months ago

Thanks for this!

I'll take a closer look over the weekend

Arondit commented 7 months ago

Sorry, this time it must pass.

medmunds commented 7 months ago

Sorry, I don't know why it requires me to re-approve the tests workflow every time.

You can run the tests on your own machine with python runtests.py in the project root. More options: https://anymail.dev/en/stable/contributing/#testing

Incidentally, I'm trying to get an anymail.dev subdomain set up in a Unisender Go account to run integration tests, but they seem to have some problems validating subdomains. (And in general, I'm seeing a lot of 50x and 429 errors just trying to navigate their dashboard—though I suppose that might be some sort of geopolitical filtering issue, since I'm connecting from the U.S.)

Arondit commented 7 months ago

I connected the service through U.S. VPN, but haven't got any errors. I am not sure, if the problems is somehow related with geopolitical issues. Tell me pls, how can i help you to test the service? If it matters, I already use all this code in production with >1000 emails per day. It just lays in project directory, which is not very scalable.

Also, if the problem remains, i can ask service tech support to help somehow.

medmunds commented 7 months ago

I did manage to get an Anymail test domain set up in Unisender Go. (Their dashboard seems to be working fine now; maybe they were just having a bad day last week.)

I'll try to put together some live integration tests.

Arondit commented 7 months ago

Hello again. Thank you for comments, it will take a bit of time to fix them all. They also require adding new tests for extra parameters, which i did not include before. I will fix them all and update PR on Monday.

Arondit commented 7 months ago

I don't really understand now, what went wrong again. Locally everything works fine. Also the error is weird, AttributeError: module 'importlib' has no attribute 'util'. But python's had importlib.util since 3.3. I have no idea, how i could break it.
Sorry for so much mess-up.

medmunds commented 7 months ago

@Arondit the 'importlib' has no attribute 'util' error isn't a problem in your code, it's a problem in the hatch packaging tool that Anymail uses. One of hatch's dependencies changed out from under it a few days hours ago, in a way that surfaced this bug in hatch. (We run weekly builds to catch problems like this; tomorrow's test run would have noticed it, but your PR got there first.)

There's a PR just opened in hatch to fix the problem, but if that doesn't make it into a release soon, I'll try to update Anymail to work around it.

Until then, if the tests run fine on your local machine, that should be good.

medmunds commented 7 months ago

Tests are working again. (Bug in hatch was fixed and a patch released last night.)

Arondit commented 7 months ago

Hello again. So, tests are working, all checks are passed and comments are closed. Is there anything else I should do? Also, did it work with integration tests for this ESP?

medmunds commented 7 months ago

There are still a handful of unresolved change requests from earlier. (They might be hiding behind a not-so-helpful GitHub "load more" link like this

image

in the discussion above.)

The two most important ones are handling for cc and bcc recipients (which I think will currently overwrite the to list), and parsing the send status out of Unisender Go's API response.

Also, while the unit tests are helpful, it might be a good idea to also implement at least one or two basic functional tests using the RequestsBackendMockAPITestCase, to verify that the API key, url, and basic payload structure are as expected—and more importantly, that future changes don't break that. (Here's an example from the Postmark tests. And another that would have caught the cc issue.)

medmunds commented 6 months ago

@Arondit I'd be happy to work on resolving the remaining handful of issues from the original review, unless you are already working on them. Just let me know.

Also, do you know if it's true that Unisender Go does not support "cc" or "bcc"? (Or did I miss something in their docs?)

Arondit commented 6 months ago

@medmunds i'm working on it, just was busy at work those days, sorry for 2 weeks of silence.

About "bcc" and "cc". Yes, you're right. They don't support it through their WEB API. There is a possibility to use SMTP API instead, which supports "cc" and "bcc" in a strict mode. Is it a problem?

medmunds commented 6 months ago

@Arondit no worries, and no hurry, just didn't want to conflict with any work you might be doing.

On cc/bcc, I was asking related to this review comment. We can just handle cc/bcc as unsupported features. (Rather than trying to use their SMTP API. Anymail's current design doesn't really work with SMTP.)

medmunds commented 6 months ago

[Updated to pick up changes from main branch]

Arondit commented 6 months ago

Fix all missed comments (yeah, you're right, GitHub've hidden them from me).

medmunds commented 6 months ago

Thanks, it's looking great. I have a few things I wanted to double check, and then can get this merged.

I added some live integration tests. If you want to try them locally:

export ANYMAIL_TEST_UNISENDER_GO_API_KEY="your-api-key"
export ANYMAIL_TEST_UNISENDER_GO_API_URL="https://go1.unisender.ru/ru/transactional/api/v1"  # or "go2"
export ANYMAIL_TEST_UNISENDER_GO_DOMAIN="your.validated.sending.domain"
# optional:
export ANYMAIL_TEST_UNISENDER_GO_TEMPLATE_ID="id of email template in your account"

python runtests.py tests.test_unisender_go_integration

(The integration tests send actual email to an anymail.dev null mailbox, so each run will use up about five sends from your quota.)

Arondit commented 6 months ago

After small fixes tests worked. Thank you. Also, '/' in the end of ANYMAIL_TEST_UNISENDER_GO_API_URL appeared to be significant.

Снимок экрана 2024-02-22 в 23 55 34
medmunds commented 5 months ago

Fixed a handful of issues and consistency with other ESP backends.

I just want to update the docs a little and then will merge.

medmunds commented 5 months ago

Hmm, it looks like cc and bcc are possible through the web API: https://godocs.unisender.ru/cc-and-bcc. (I think they just updated the API docs—I don't remember seeing that a few weeks ago.)

Basically, we'd need to generate an entry in "recipients" for each of the to, cc, and bcc emails, and then construct "To" and "CC" headers that list all of their respective addresses. Unisender Go (effectively) just treats those headers as text to include verbatim in the generated message, without trying to parse them.

There might be some tricky cases to consider when using Anymail's batch sending mode. (I think for batch send we just skip the common "To" header, but keep the common "CC" header. But need to think about this a little more.)

Also we'd want to test whether non-ASCII display names require special handling.

Arondit commented 5 months ago

Add one unit test and fix integration test (bcc address needs to be sent on approved domain). Also thank you very much for work around added cc/bcc feature. I started to do the same today earlier and noticed, you'd already done it, but better. I guess, now it is ready to merge, isn't it?

medmunds commented 5 months ago

Good catch on the integration test, thanks.

I emailed Unisender Go tech support earlier today about the display-name quoting bugs and a couple other issues. Hoping to hear back from them once they get into the office, but if not I'll go ahead and merge this with the workarounds.

medmunds commented 5 months ago

@Arondit OK, this is merged and will be in the next release!

Thanks again for all your work on this.

medmunds commented 5 months ago

(Released in Anymail 10.3)

Arondit commented 5 months ago

nice, thank you