Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unisender Go: new ESP #352

Merged
merged 34 commits into from
Mar 5, 2024
Merged

Unisender Go: new ESP #352

merged 34 commits into from
Mar 5, 2024

Conversation

Arondit
Copy link
Contributor

@Arondit Arondit commented Jan 10, 2024

#349

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

@medmunds
Copy link
Contributor

Thanks for this!

I'll take a closer look over the weekend

@Arondit
Copy link
Contributor Author

Arondit commented Jan 12, 2024

Sorry, this time it must pass.

@medmunds
Copy link
Contributor

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
Copy link
Contributor Author

Arondit commented Jan 16, 2024

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.

Copy link
Contributor

@medmunds medmunds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for this PR!

I've added several comments/requests. Most of them are about making things more consistent with other Anymail backends.

CHANGELOG.rst Outdated Show resolved Hide resolved
anymail/backends/unisender_go.py Outdated Show resolved Hide resolved
anymail/backends/unisender_go.py Outdated Show resolved Hide resolved
anymail/backends/unisender_go.py Show resolved Hide resolved
anymail/backends/unisender_go.py Outdated Show resolved Hide resolved
anymail/webhooks/unisender_go.py Outdated Show resolved Hide resolved
anymail/webhooks/unisender_go.py Outdated Show resolved Hide resolved
tests/test_unisender_go_payload.py Outdated Show resolved Hide resolved
docs/esps/index.rst Outdated Show resolved Hide resolved
anymail/webhooks/unisender_go.py Show resolved Hide resolved
@medmunds
Copy link
Contributor

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
Copy link
Contributor Author

Arondit commented Jan 19, 2024

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
Copy link
Contributor Author

Arondit commented Jan 24, 2024

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
Copy link
Contributor

medmunds commented Jan 24, 2024

@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
Copy link
Contributor

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

@Arondit
Copy link
Contributor Author

Arondit commented Jan 29, 2024

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor Author

Arondit commented Feb 19, 2024

@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
Copy link
Contributor

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

@Arondit
Copy link
Contributor Author

Arondit commented Feb 20, 2024

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

@medmunds
Copy link
Contributor

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
Copy link
Contributor Author

Arondit commented Feb 22, 2024

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

- Make "/" at end of api_url optional
  (so URLs in Unisender Go API docs work)
- Map Unisender Go "failed_emails" status
  to normalized AnymailRecipientStatus
  status codes
- Correctly handle Unisender Go's response
  for duplicate emails
- Remove unused code
- Don't send unnecessary API payload fields
- Fix amp-html payload construction
- Fix attachment "name" field for inline images
  (must use cid)
- Support per-recipient merge_metadata
- Format send_at per Unisender Go docs
- Handle boolean values for track_clicks, track_opens
- Don't duplicate Unisender Go's restrictions
  on attachment filenames (their API error
  message is adequate)
- Most errors detected by the receiving
  email server should use RejectReason.BOUNCED.
  (INVALID is for address _format_ errors.
  BLOCKED is for Unisender Go policies.)
- No need to modify ESP payload data
  by adding esp_name.
- Most missing AnymailTrackingEvent params
  should be set to None. Missing metadata
  must be set to an empty dict.
In recipient metadata, change name of
generated tracking id from "message_id"
to "anymail_id" to reduce chance of conflicts
with caller metadata. (And for consistency
with same feature in SendGrid backend.)

In webhooks, remove "anymail_id" from
metadata before passing to signal handler.
When not generating unique per-recipient
message ids, use Unisender Go's "job_id"
as Anymail's `message_id` for all recipients.
The same job_id is provided in the send
API response and tracking webhooks.
@medmunds
Copy link
Contributor

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
Copy link
Contributor

medmunds commented Mar 1, 2024

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.

medmunds and others added 7 commits March 2, 2024 11:05
Match method order used by superclass, other ESP backends.
- Parsing bug when to/cc header
  contains quoted display name with comma
  ('"ExampleCo, Ltd." <[email protected]>')
- `reply_to_name` not properly quoted
  (like `to_name` and `from_name` are)
@Arondit
Copy link
Contributor Author

Arondit commented Mar 4, 2024

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
Copy link
Contributor

medmunds commented Mar 5, 2024

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 added 3 commits March 4, 2024 18:39
- Fix capitalization of "Unisender Go"
- Alphabetize ESPs in description
Unisender Go tech support says not only
commas, but also < > @ are prohibited
in To and Cc header display names, even
if properly quoted.
@medmunds medmunds merged commit a71a0d9 into anymail:main Mar 5, 2024
58 checks passed
@medmunds
Copy link
Contributor

medmunds commented Mar 5, 2024

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

Thanks again for all your work on this.

@medmunds
Copy link
Contributor

(Released in Anymail 10.3)

@Arondit
Copy link
Contributor Author

Arondit commented Mar 15, 2024

nice, thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants