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/return path #276

Merged
merged 13 commits into from
Apr 24, 2023
Merged

Fix/return path #276

merged 13 commits into from
Apr 24, 2023

Conversation

peidrao
Copy link
Contributor

@peidrao peidrao commented Apr 17, 2023

After seeing the issues cited in the issue #274. It was seen that AWS_SES_RETURN_PATH was not working as it should.

How to solve the problem of receivaing emails

We create a configuration to add an e-mail that receives feedback: AWS_SES_FROM_EMAIL

Why do we do this?

Before, we had it

source = settings.AWS_SES_RETURN_PATH

However

django-ses/README.rst

Lines 482 to 484 in 5dc270e

``AWS_SES_RETURN_PATH``
Instruct Amazon SES to forward bounced emails and complaints to this email.
For more information please refer to http://aws.amazon.com/ses/faqs/#38

The source had a use improper. Source to be used as the "From" address for the email. As the documentation says

How is the "From address email"?

AWS_SES_FROM_EMAIL has been added to solve the problem.

Tests

It was added some tests, but, i am open to hearing other cases to be reproduced

Drastic changes by @rodrigondec

Answer to question AWS_SES_FROM_EMAIL

Yes.

It will cause a breaking change on the following scenery

AWS_SES_RETURN_PATH = "my_default_from_email@email.com"

SESBackend().send_messages(
    [
        {
            "from": "ignored_from_email@email.com",
            "to": "to_email@email.com",
            "subject": "some subject",
            "message": "some message"
        }
    ]
)

Before this change, this message would be sent with the FromEmailAddress=my_default_from_email@email.com.

After this change, if the attribute AWS_SES_FROM_EMAIL is not set with AWS_SES_FROM_EMAIL=my_default_from_email@email.com, the email will be sent with the from email of the EmailMessage, which would be FromEmailAddress=ignored_from_email@email.com.

The correct usage AFTER THE CHANGE would be

AWS_SES_FROM_EMAIL = "my_default_from_email@email.com"

SESBackend().send_messages(
    [
        {
            "from": "ignored_from_email@email.com",
            "to": "to_email@email.com",
            "subject": "some subject",
            "message": "some message"
        }
    ]
)

@@ -38,6 +38,7 @@
AWS_SES_RETURN_PATH_ARN = getattr(settings, 'AWS_SES_RETURN_PATH_ARN', None)

USE_SES_V2 = getattr(settings, 'USE_SES_V2', False)
AWS_SES_FROM_EMAIL = getattr(settings, 'AWS_SES_FROM_EMAIL', None)
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is a breaking change, as it changes what AWS_SES_RETURN_PATH is used for right?

What happens to an existing project if they upgrade to this new version but don't declare AWS_SES_FROM_EMAIL? Will something break?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

It will cause a breaking change on the following scenery

AWS_SES_RETURN_PATH = "my_default_from_email@email.com"

SESBackend().send_messages(
    [
        {
            "from": "ignored_from_email@email.com",
            "to": "to_email@email.com",
            "subject": "some subject",
            "message": "some message"
        }
    ]
)

Before this change, this message would be sent with the FromEmailAddress=my_default_from_email@email.com.

After this change, if the attribute AWS_SES_FROM_EMAIL is not set with AWS_SES_FROM_EMAIL=my_default_from_email@email.com, the email will be sent with the from email of the EmailMessage, which would be FromEmailAddress=ignored_from_email@email.com.

The correct usage AFTER THE CHANGE would be

AWS_SES_FROM_EMAIL = "my_default_from_email@email.com"

SESBackend().send_messages(
    [
        {
            "from": "ignored_from_email@email.com",
            "to": "to_email@email.com",
            "subject": "some subject",
            "message": "some message"
        }
    ]
)

And this scenery could be a test itself! And it could act as a regression test.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't mind, please add this example to the PR description and update the changelog, that will make it easier to merge this PR!

Copy link
Contributor

Choose a reason for hiding this comment

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

Will do!

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
tests/test_backend.py Outdated Show resolved Hide resolved
tests/test_backend.py Outdated Show resolved Hide resolved
README.rst Outdated
``AWS_SES_RETURN_PATH``
Instruct Amazon SES to forward bounced emails and complaints to this email.
``AWS_SES_FROM_EMAIL``
Optional. The email addres to be used as the "From" address for the email. The address that you specify has to be verified.
For more information please refer to http://aws.amazon.com/ses/faqs/#38
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this link needs to be updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you talking about this link?

Suggested change
For more information please refer to http://aws.amazon.com/ses/faqs/#38
For more information please refer to http://aws.amazon.com/ses/faqs/#38

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it doesn't go to any specific question (#38)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I read it and didn't find anything on it.
We could use the boto recommendation. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, seems like a more fitting link! Thank you for suggesting

README.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@pcraciunoiu pcraciunoiu left a comment

Choose a reason for hiding this comment

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

Just small changes and suggestion to update with an example.

I think this can be merged and I will bump only a minor version, though there is a risk we would get some complaints as people upgrade. So I want to have a very clear explanation in the changelog and link to this PR, so people can understand why we're changing how this setting behaves.

Thanks for working on this!

peidrao and others added 4 commits April 18, 2023 09:52
Co-authored-by: Paul Craciunoiu <paul@uplift.ltd>
Co-authored-by: Paul Craciunoiu <paul@uplift.ltd>
Co-authored-by: Rodrigo Castro <rodrigondec@gmail.com>
Co-authored-by: Rodrigo Castro <rodrigondec@gmail.com>
@pcraciunoiu
Copy link
Contributor

Looks like tests are failing now. Let me know when it's ready!

@peidrao
Copy link
Contributor Author

peidrao commented Apr 24, 2023

Looks like tests are failing now. Let me know when it's ready!

I made the changes (PR description and link change).

Anything else? Before being merged?

Copy link
Contributor

@pcraciunoiu pcraciunoiu left a comment

Choose a reason for hiding this comment

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

Alright let's give this a go! I'll update the changelog.

@pcraciunoiu pcraciunoiu merged commit 0b2a7a8 into django-ses:master Apr 24, 2023
14 checks passed
@pcraciunoiu
Copy link
Contributor

This has been released in v3.4.0 - https://pypi.org/project/django-ses/3.4.0/

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

Successfully merging this pull request may close these issues.

None yet

3 participants