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

Fix encode in webhook campaign action. #5070

Merged
merged 2 commits into from
Dec 21, 2017
Merged

Fix encode in webhook campaign action. #5070

merged 2 commits into from
Dec 21, 2017

Conversation

VitorSavedra
Copy link
Contributor

Q A
Bug fix? Yes
New feature? No
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs)
BC breaks?
Deprecations?

Description:

"Data" in webhook action post encoded strings. Example in "messageText":
image

After fix:
image

Steps to reproduce the bug:

  1. Create action "Send Webhook" in campaign;
  2. Run this campaign;
  3. Check results Data (webhook action) sended in endpoint.

Steps to test this PR:

  1. Apply PR and repeat all steps.

mikerowe81
mikerowe81 approved these changes Oct 9, 2017
View reviewed changes
Copy link
Contributor

@mikerowe81 mikerowe81 left a comment

Choose a reason for hiding this comment

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

This fixes the issue of POST data being double encoded.

@mikerowe81
Copy link
Contributor

I would suggest that the urlencode also be removed from the "headers" data on line 78.

@alanhartless alanhartless added this to the 2.12.1 milestone Dec 13, 2017
@alanhartless alanhartless added the bug Issues or PR's relating to bugs label Dec 13, 2017
alanhartless
alanhartless approved these changes Dec 19, 2017
View reviewed changes
Copy link
Contributor

@alanhartless alanhartless left a comment

Choose a reason for hiding this comment

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

Works and fixed the same for headers

@alanhartless alanhartless added the pending-test-confirmation PR's that require one test before they can be merged label Dec 19, 2017
@javjim javjim self-assigned this Dec 19, 2017
@javjim
Copy link

javjim commented Dec 21, 2017

works properly

@javjim javjim added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged labels Dec 21, 2017
@javjim javjim removed their assignment Dec 21, 2017
@escopecz escopecz merged commit bff6321 into mautic:staging Dec 21, 2017
@dbhurley dbhurley removed the ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged label Dec 21, 2017
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

@mikerowe81 mikerowe81 mikerowe81 approved these changes

Assignees
No one assigned
Labels
bug Issues or PR's relating to bugs
Projects
None yet
Milestone
2.12.1
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@VitorSavedra @mikerowe81 @javjim @alanhartless @escopecz @dbhurley