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.

Sign up for GitHub

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

Jump to bottom

Refactored mail transport callbacks (fixes Sparkpost issue with immediate feedback) #5384

Merged
merged 8 commits into from
Dec 27, 2017
Merged

Refactored mail transport callbacks (fixes Sparkpost issue with immediate feedback) #5384

merged 8 commits into from
Dec 27, 2017

Conversation

alanhartless
Copy link
Contributor

@alanhartless alanhartless commented Nov 28, 2017 •
edited
Loading

Q A
Bug fix? y
New feature?
Automated tests included?
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs)
BC breaks?
Deprecations? Y

Description:

This PR refactors the transport callback process to get away from passing around an array. This also fixes Sparkpost to mark a DNC for an immediately rejected recipient since that will not be sent via a webhook.

Steps to test this PR:

  1. Amazon, Mailjet, Mandrill, ElasticeMail and Sparkpost has been refactored to use this. Test their callbacks.
  2. Run the tests.

List deprecations along with the new alternative:

  1. InterfaceCallbackTransport is deprecated in favor of CallbackTransportInterface

@alanhartless alanhartless added the WIP PR's that are not ready for review and are currently in progress label Nov 28, 2017
@alanhartless alanhartless changed the title WIP - refactored mail transport callbacks (fixes Sparkpost issue with immediate feedback) Refactored mail transport callbacks (fixes Sparkpost issue with immediate feedback) Nov 29, 2017
@alanhartless alanhartless added ready-to-test PR's that are ready to test and removed WIP PR's that are not ready for review and are currently in progress labels Nov 29, 2017
@alanhartless alanhartless added this to the 2.13.0 milestone Nov 29, 2017
@alanhartless alanhartless force-pushed the transport-callback branch 2 times, most recently from 9e23e78 to 9221437 Compare December 4, 2017 16:50
@alanhartless alanhartless modified the milestones: 2.13.0, 2.12.1 Dec 12, 2017
@javjim javjim self-assigned this Dec 19, 2017
@Maxell92 Maxell92 mentioned this pull request Dec 21, 2017
Merged
@escopecz escopecz added the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Dec 21, 2017
@alanhartless alanhartless removed the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Dec 21, 2017
@alanhartless
Copy link
Contributor Author

Rebased

@escopecz escopecz assigned escopecz and unassigned javjim Dec 22, 2017
@escopecz
Copy link
Sponsor Member

I started testing Sparkpost webhooks on my public VPS like this:

  1. Configured Sparkpost sending.
  2. Configured Sparkpost webhooks.
  3. Created segment with several contacts. One of them had fake email address.
  4. Sent email to that segment.
    I can see that Sparkpost sent several webhooks in the access log. But the contact with the fake email address is not marked as DNC. In the Sparkpost event log I can see the email sent to the fake address is in "Delayed" state. How long should I wait? Or is this wrong way to test transport callbacks?

@escopecz escopecz added the pending-feedback PR's and issues that are awaiting feedback from the author label Dec 22, 2017
escopecz
escopecz approved these changes Dec 22, 2017
View reviewed changes
Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

Tested with Sparkpost and email address shirleyjgomez@teleworm.us and it bounced successfully 👍

@escopecz escopecz added the pending-test-confirmation PR's that require one test before they can be merged label Dec 22, 2017
@escopecz escopecz removed the pending-feedback PR's and issues that are awaiting feedback from the author label Dec 22, 2017
@alanhartless
Copy link
Contributor Author

Don't merge this yet - seems I lost some tests which makes me wonder what else was lost. Maybe during rebasing. Searching for history through various places to restore.

@alanhartless
Copy link
Contributor Author

Ok I've added back all the tests. Had to rewrite what I had but also added tests for the other transports.  

@javjim
Copy link

javjim commented Dec 27, 2017

Ran unit tests and working properly

@alanhartless alanhartless merged commit 85b17b3 into mautic:staging Dec 27, 2017
@dbhurley dbhurley removed the ready-to-test PR's that are ready to test label Dec 27, 2017
@alanhartless alanhartless deleted the transport-callback branch January 5, 2018 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@escopecz escopecz escopecz approved these changes

Assignees

@escopecz escopecz

Labels
pending-test-confirmation PR's that require one test before they can be merged
Projects
None yet
Milestone
2.12.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alanhartless @escopecz @javjim @dbhurley