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

Return Token value after url-encoding. #6078

Merged
merged 3 commits into from
Sep 4, 2018
Merged

Conversation

silsha
Copy link
Contributor

@silsha silsha commented May 13, 2018

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

Description:

From the addressed issue:

When using a token like {contactfield=company|true}, the default value |true has two different uses:

  1. When the value true is set as default value, the contactfield is url-encoded and
  2. When there is no value, Mautic will output the default value (true).

This results in a situation, where you can't enable url-encoding and have an empty default value at the same time.

Steps to reproduce the bug:

  1. Create two new contacts – one with a company (Foo & Bar Inc.) and one without a company.
  2. Send both contacts a mail including the token {contactfield=company|true}
    One contact will receive the string Foo+%26+Bar+Inc., the other one will receive true.

Steps to test this PR:

  1. Create two new contacts – one with a company (Foo & Bar Inc.) and one without a company.
  2. Send both contacts a mail including the token {contactfield=company|true}
    One contact will receive the string Foo+%26+Bar+Inc., the other one will receive an empty string.

List backwards compatibility breaks:

  1. It's no longer possible to use true as default value.

@mautibot mautibot added the code-review-needed PR's that require a code review before merging label May 13, 2018
@silsha silsha changed the title Output Token value after url-encoding. Return Token value after url-encoding. May 13, 2018
@npracht
Copy link
Member

npracht commented May 13, 2018

Label: Bug

@mautibot mautibot added the bug Issues or PR's relating to bugs label May 13, 2018
@escopecz escopecz added the ready-to-test PR's that are ready to test label May 21, 2018
@galvani galvani added this to the 2.15.0 milestone May 22, 2018
@galvani galvani self-requested a review May 22, 2018 10:32
Copy link
Contributor

@galvani galvani left a comment

Choose a reason for hiding this comment

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

I have verified it is working. @silsha please update to current staging. Thank you.

@galvani galvani added pending-feedback PR's and issues that are awaiting feedback from the author and removed ready-to-test PR's that are ready to test labels May 22, 2018
@galvani galvani self-assigned this May 22, 2018
@silsha
Copy link
Contributor Author

silsha commented May 23, 2018

@galvani I've merged staging into my branch. 👍

Copy link
Contributor

@galvani galvani left a comment

Choose a reason for hiding this comment

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

Works as suggested.

@escopecz escopecz modified the milestones: 2.15.0, 2.14.0 Jul 10, 2018
@escopecz escopecz added pending-test-confirmation PR's that require one test before they can be merged and removed pending-feedback PR's and issues that are awaiting feedback from the author labels Jul 10, 2018
@dongilbert dongilbert modified the milestones: 2.14.0, 2.14.1 Jul 17, 2018
@silsha
Copy link
Contributor Author

silsha commented Jul 27, 2018

@galvani You don’t seem to have write access (anymore?), so your review doesn’t count towards mergeability.

@npracht npracht added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed 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 labels Aug 20, 2018
@heathdutton heathdutton merged commit b02ca0d into mautic:staging Sep 4, 2018
@escopecz escopecz removed ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged labels Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants