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

Second Fixes #5532 problem with MailjetTransport header X-MJ-CUSTOMID #5701

Merged
merged 3 commits into from Mar 26, 2018
Merged

Second Fixes #5532 problem with MailjetTransport header X-MJ-CUSTOMID #5701

merged 3 commits into from Mar 26, 2018

Conversation

XRaccourci
Copy link
Contributor

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

Description:

Fixes issue #5532 and add another fix to PR #5534.
Delete ALL header 'X-MJ-CUSTOMID' before rewriting the correct header in MailjetTransport.
I have made a mistake in my precedent pull request. (#5534). I thought header value is only one value but it can be an array of value... Because that, to fix the bug with this header for Mailjet, I remove all headers 'X-MJ-CUSTOMID'.

Steps to reproduce the bug:

  1. Create a segment
  2. Add three leads at least in this segment
  3. Create a manual email and set the precedent segment
  4. Add log in Swift_Transport_AbstractSmtpTransport in the end of send function. Log this => $message->getHeaders()->get('X-MJ-CUSTOMID')->toString() before return.
  5. Send this email
  6. View the log. X-MJ-CUSTOMID is the same for the two end differents emails. It's normally different because X-MJ-CUSTOMID is defined like this "leadIdHash-email". After fixes, the header is correct.

Steps to test this PR:

  1. Apply PR
  2. Repeat Steps 1-6 above
  3. Notice value is correct

List deprecations along with the new alternative:

List backwards compatibility breaks:

@mautibot mautibot added the code-review-needed PR's that require a code review before merging label Feb 10, 2018
@XRaccourci
Copy link
Contributor Author

Label: bug

@XRaccourci
Copy link
Contributor Author

Label: Ready to test

@kuzmany
Copy link
Member

kuzmany commented Feb 10, 2018

Label: bug

@mautibot mautibot added the bug Issues or PR's relating to bugs label Feb 10, 2018
@kuzmany
Copy link
Member

kuzmany commented Feb 10, 2018

Label: Ready to test

@mautibot mautibot added the ready-to-test PR's that are ready to test label Feb 10, 2018
@Dcoutelle
Copy link
Contributor

tested and it's OK

@dbhurley dbhurley added this to the 2.13.0 milestone Mar 4, 2018
npracht
npracht approved these changes Mar 20, 2018
View reviewed changes
Copy link
Member

@npracht npracht 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 @Dcoutelle, this is working properly.

@mautibot mautibot removed the code-review-needed PR's that require a code review before merging label Mar 20, 2018
@npracht
Copy link
Member

npracht commented Mar 20, 2018

Label: Pending test confirmation

@mautibot mautibot added the pending-test-confirmation PR's that require one test before they can be merged label Mar 20, 2018
@escopecz
Copy link
Sponsor Member

Resolve the conflict please.

@escopecz escopecz added the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Mar 21, 2018
@XRaccourci
Copy link
Contributor Author

@escopecz It's OK. Conflict is resolved.

@escopecz escopecz removed the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Mar 22, 2018
@mautibot mautibot added the code-review-needed PR's that require a code review before merging label Mar 26, 2018
@alanhartless alanhartless merged commit 80f057e into mautic:master Mar 26, 2018
@dbhurley dbhurley removed the ready-to-test PR's that are ready to test label Mar 26, 2018
@XRaccourci XRaccourci deleted the Issue5532 branch October 1, 2021 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alanhartless alanhartless alanhartless approved these changes

@npracht npracht npracht approved these changes

Assignees
No one assigned
Labels
bug Issues or PR's relating to bugs code-review-needed PR's that require a code review before merging pending-test-confirmation PR's that require one test before they can be merged
Projects
None yet
Milestone
2.13.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@XRaccourci @kuzmany @Dcoutelle @npracht @escopecz @alanhartless @dbhurley @mautibot