Support dkim_sign with outbound.send_email() #1512

Merged
merged 1 commit into from Jun 23, 2016

Conversation

Projects
None yet
2 participants
@baudehlo
Collaborator

baudehlo commented Jun 11, 2016

Fixes #336

Changes proposed in this pull request:

  • Implements dkim_sign with mail sent via outbound.send_email()
  • Adds new hook: hook_pre_send_trans_email.

Checklist:

  • docs updated
  • tests updated
@baudehlo

This comment has been minimized.

Show comment
Hide comment
@baudehlo

baudehlo Jun 11, 2016

Collaborator

I'd appreciate comments on the way I fixed this. It's a bit of a hack. Adds a new hook, though that hook could be useful for other features too, and we can probably improve on the hack over time.

@msimerson @smfreegard

Collaborator

baudehlo commented Jun 11, 2016

I'd appreciate comments on the way I fixed this. It's a bit of a hack. Adds a new hook, though that hook could be useful for other features too, and we can probably improve on the hack over time.

@msimerson @smfreegard

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Jun 11, 2016

Member

I briefly looked it over. It's clever to simulate the connection so that dkim_sign needed no alternations to to sign.

Member

msimerson commented Jun 11, 2016

I briefly looked it over. It's clever to simulate the connection so that dkim_sign needed no alternations to to sign.

@baudehlo

This comment has been minimized.

Show comment
Hide comment
@baudehlo

baudehlo Jun 12, 2016

Collaborator

Yeah it seems to work. I don't want people to assume it's a real connection object though - but I assume docs should take care of that enough.

On Jun 11, 2016, at 7:59 PM, Matt Simerson notifications@github.com wrote:

I briefly looked it over. It's clever to simulate the connection so that dkim_sign needed no alternations to to sign.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Collaborator

baudehlo commented Jun 12, 2016

Yeah it seems to work. I don't want people to assume it's a real connection object though - but I assume docs should take care of that enough.

On Jun 11, 2016, at 7:59 PM, Matt Simerson notifications@github.com wrote:

I briefly looked it over. It's clever to simulate the connection so that dkim_sign needed no alternations to to sign.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Jun 23, 2016

Member

Yeah it seems to work.

If I used outbound, then I'd deploy and test this. Since you already have, I would just merge this and hope it also works for the wider community. If not, we've got plenty more version numbers to get it fixed with.

(I still fall on the "release early, release often" side of the fence. I agree that bug-free releases are a great and noble goal, but shipping code is a form of testing.)

Member

msimerson commented Jun 23, 2016

Yeah it seems to work.

If I used outbound, then I'd deploy and test this. Since you already have, I would just merge this and hope it also works for the wider community. If not, we've got plenty more version numbers to get it fixed with.

(I still fall on the "release early, release often" side of the fence. I agree that bug-free releases are a great and noble goal, but shipping code is a form of testing.)

@baudehlo baudehlo merged commit 432174a into master Jun 23, 2016

2 of 3 checks passed

codecov/project 36.14% (-0.03%) compared to 553d022
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@msimerson msimerson deleted the send_email_dkim_sign branch Jun 24, 2016

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