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

Fix amazon_ses inbound email ESP provider #3509

Merged
merged 17 commits into from
Dec 11, 2023
Merged

Fix amazon_ses inbound email ESP provider #3509

merged 17 commits into from
Dec 11, 2023

Conversation

Lutseslav
Copy link
Contributor

@Lutseslav Lutseslav commented Dec 5, 2023

What this PR does

Fixes django-anymail[amazon-ses] issues according to anymail docs

Which issue(s) this PR fixes

#3508

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)

@Lutseslav Lutseslav requested a review from a team December 5, 2023 16:38
@CLAassistant
Copy link

CLAassistant commented Dec 5, 2023

CLA assistant check
All committers have signed the CLA.

@Lutseslav Lutseslav changed the title Issue #3508. Fix amazon_ses inbound email ESP provider Fix amazon_ses inbound email ESP provider Dec 5, 2023
Copy link
Contributor

@joeyorlando joeyorlando left a comment

Choose a reason for hiding this comment

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

thanks for your contribution @Lutseslav! 🙏

do you mind adding a test which asserts that the Amazon SES provider works?

@Lutseslav
Copy link
Contributor Author

Deal. Shall I put assertion in that location? Which file is the most suitable for that or I'll create the new one with deps assertions?

@joeyorlando
Copy link
Contributor

@Lutseslav I would suggest adding it here as an integration test that uses the API endpoint which you reported the issue on in #3508. Does that make sense?

@Lutseslav
Copy link
Contributor Author

@Lutseslav I would suggest adding it here as an integration test that uses the API endpoint which you reported the issue on in #3508. Does that make sense?
@joeyorlando yeap, seems fine. That'll take some..👌

@Lutseslav
Copy link
Contributor Author

@joeyorlando I've made some kind of amazon_ses checking😅 Hopefully, it's plausible.

result = False

settings.INBOUND_EMAIL_ESP = "amazon_ses"
rf = RequestFactory()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest looking into the rest_framework.test.APIClient instead here (we have many examples throughout the tests in the codebase).

With APIClient you will actually be invoking the post handler that raises the exception according to the stack trace in the issue description in #3508

Comment on lines 15 to 21
try:
inbound_view.post(rf.post('/fake-mock-location'))
result = True
except anymail.exceptions.AnymailAPIError:
# We don't test anymail, but it's invocation ability
result = True
assert result
Copy link
Contributor

Choose a reason for hiding this comment

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

on an aside, if you would like to assert that an exception is raised, you can do something like this 🙂

with pytest.raises(anymail.exceptions.AnymailAPIError):
        inbound_view.post(rf.post('/fake-mock-location'))

From the pytest docs:

Assert that a code block/function call raises expected_exception or raise a failure exception otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I think now it looks better😅

@Lutseslav
Copy link
Contributor Author

We might also need to label PR with pr:no public docs and pr:no changelog? Seems like they are irrelevant to the changes.

@Lutseslav
Copy link
Contributor Author

@joeyorlando Take a look please - all the receive flow is now expected to return 200.

Copy link
Contributor

@joeyorlando joeyorlando left a comment

Choose a reason for hiding this comment

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

@Lutseslav thanks for your patience with the feedback on the test. LGTM 🙏

@joeyorlando joeyorlando added the pr:no public docs Added to a PR that does not require public documentation updates label Dec 11, 2023
@joeyorlando joeyorlando merged commit 0d959a5 into grafana:dev Dec 11, 2023
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants