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

Default values imported, boolean values can be true/false/yes/no #2616

Merged
merged 2 commits into from
Oct 12, 2016

Conversation

escopecz
Copy link
Sponsor Member

@escopecz escopecz commented Sep 30, 2016

Q A
Bug fix? Y
New feature? Y
Related user documentation PR URL mautic/documentation#115
Related developer documentation PR URL /
Issues addressed (#s or URLs) #1863, #2502
BC breaks? N
Deprecations? N

Description:

This PR does 2 things:

  1. It imports also default values if the contact fields have any
  2. It allows to import yes/no/true/false as Boolean values.

Steps to test this PR:

  1. Apply this PR.
  2. Test again.

Notice:

  1. All 8 contacts were imported with correct Boolean values.
  2. All contact have the default value in the Phone field.

Steps to reproduce the bug:

  1. Make sure you have at least one contact field with default value. For example Phone field.
  2. Create a new contact field called "Boolean Test" and is type of Boolean.
  3. Try to import the following CSV:
"email", "stage", "company", "boolean_test"
"john2@doe.com", "test", "Google", "No"
"john1@doe.com", "test", "Google", 0
"john3@doe.com", "test", "Google", "false"
"john4@doe.com", "test", "Google", "FALSE"
"johana1@doana.com", "stage A", "Apple", 1
"johana2@doana.com", "stage A", "Apple", "Yes"
"johana3@doana.com", "stage A", "Apple", "true"
"johana4@doana.com", "stage A", "Apple", "TRUE"

Notice:

  1. It will import only 2 out of 8 contacts. The reason is that text Boolean values are not valid.
  2. The default Phone value wasn't imported to the contacts who got saved.

@escopecz escopecz added T1 Low difficulty to fix (issue) or test (PR) feature A new feature for inclusion in minor or major releases bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test labels Sep 30, 2016
@escopecz escopecz added this to the 2.2.1 milestone Sep 30, 2016
@@ -1516,6 +1515,31 @@ public function importLead($fields, $data, $owner = null, $list = null, $tags =
);
}

$booleanTrue = ['1', 'true', 'yes'];
Copy link
Contributor

@alanhartless alanhartless Sep 30, 2016

Choose a reason for hiding this comment

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

This could be simplified with $value = (int) filter_var($fieldData[$leadField['alias'], FILTER_VALIDATE_BOOLEAN);

http://php.net/manual/en/filter.filters.validate.php

Returns TRUE for "1", "true", "on" and "yes". Returns FALSE otherwise.

If FILTER_NULL_ON_FAILURE is set, FALSE is returned only for "0", "false", "off", "no", and "", and NULL is returned for all non-boolean values.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Great suggestion. Implemented.

@mqueme
Copy link
Contributor

mqueme commented Oct 11, 2016

0 contacts were created for me before and after merging this PR (#2)(My boolean field) This value is not valid.

@escopecz
Copy link
Sponsor Member Author

That's odd. I retested and all 8 contacts were imported. Could you double-check the PR was applied before test?

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.

Worked for me with the given csv file. Boolean fields were imported correctly.

@alanhartless alanhartless added the pending-test-confirmation PR's that require one test before they can be merged label Oct 11, 2016
@dongilbert
Copy link
Member

Works as described. Thanks for providing thorough test instructions. +1

@dongilbert dongilbert merged commit 8a2bdc9 into mautic:staging Oct 12, 2016
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 feature A new feature for inclusion in minor or major releases pending-test-confirmation PR's that require one test before they can be merged ready-to-test PR's that are ready to test T1 Low difficulty to fix (issue) or test (PR)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants