django-anymail

django-anymail copied to clipboard

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

• [x] Backend
• [x] Custom headers for metadata and tags
• [x] Tracking webhook
• [x] Integration tests
• [x] Docs
• [x] Readme/project/tox/workflows

Thanks for this!

I'll take a closer look over the weekend

Sorry, this time it must pass.

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.)

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.

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.

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.

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.

@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.

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

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?

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 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.)

@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?)

@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?

@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.)

[Updated to pick up changes from main branch]

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

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.)

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

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

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

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.

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?

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.

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

Thanks again for all your work on this.

(Released in Anymail 10.3)

nice, thank you