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

[NFC] Add ImportProcessor class & unit tests + extend existing cover #15072

Merged
merged 1 commit into from
Aug 21, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Aug 19, 2019

Overview

Adds a bunch of helpers which will allow us to massively cleanup loadSavedMapping and add test cover. This change is NFC of itself as it does not start to USE the new helpers as yet. The goal is to bring in the usage alongside tests of the live code

Before

Currently to get the value for 'type' the code looks like

          list($mappingName, $mappingContactType, $mappingLocation, $mappingPhoneType, 
        $mappingImProvider, $mappingRelation, $mappingOperator, $mappingValue, 
    $mappingWebsiteType) = CRM_Core_BAO_Mapping::getMappingFields($savedMapping, TRUE);

            $locationId = isset($mappingLocation[$i]) ? $mappingLocation[$i] : 0;
            $phoneType = isset($mappingPhoneType[$i]) ? $mappingPhoneType[$i] : NULL;
            // default for IM/phone when mapping with relation is true
            $typeId = NULL;
            if (isset($phoneType)) {
              $typeId = $phoneType;
            }
            elseif (isset($imProvider)) {
              $typeId = $imProvider;
            }

After

we can replace the above with

$type = $processor->getPhoneOrIMTypeID($i);

Similar applies to all the other hard-to-read code. We are really just doing a fairly simple mapping conversion & it doesn't need to be so hard.

Also the CRM_Core_BAO_Mapping::getMappingFields function will be close to obsolete once this has been adopted

Technical Details

This adds the full ImportProcessor class from the WIP pr #15034 - minus the functions that
replace loadSavedMapping) along with tests. It does NOT start to use the new class as yet.

Comments

Note that #15068 will need rebasing if this is merged first & vice versa

The goal is to bring this class into use along with extending tests but this is 'safe' in that no functional code is changed

@civibot
Copy link

civibot bot commented Aug 19, 2019

(Standard links)

@civibot civibot bot added the master label Aug 19, 2019
@eileenmcnaughton eileenmcnaughton changed the title Add ImportProcessor class & unit tests + extend existing cover [NFC] Add ImportProcessor class & unit tests + extend existing cover Aug 19, 2019
@eileenmcnaughton eileenmcnaughton force-pushed the test_ext2 branch 6 times, most recently from b2eab3e to 921de6b Compare August 19, 2019 06:11
This adds the full ImportProcessor class from the WIP pr civicrm#15034 - minus the functions that
replace loadSavedMapping) along with tests. It does NOT start to use the new class as yet.

Note that civicrm#15068 will need rebasing if this is merged first & vice versa

The goal is to bring it into use along with extending tests but this is 'safe' in that no funcitonal code is changed
@eileenmcnaughton
Copy link
Contributor Author

@totten @demeritcowboy @seamuslee001 so what was the decision on this one?

wmfgerrit pushed a commit to wikimedia/wikimedia-fundraising-crm that referenced this pull request Aug 21, 2019
Note this adds 2 fields to the field mapping and one group that I manually added on prod while testing. Dev
envs will need some manual attention to this.

This is based on 2 classes I have put up for core - the MetadataTrait is merged and
the ImportProcessor is partially merged
civicrm/civicrm-core#15072
(I'm adding tests & cleanup to core based on it as the 'justification' for it being in core
- in fact the import code is needlessly hard & could really be cleaned up but review enthusiasm is
a little low so will not push too far into it).

Per the test this can be called by

drush --user=1  cvapi TargetSmart.import csv=../sites/default/files/import1.tsv  batch_size=1000 offset=5 debug=1

Note the user needs to be set to avoid an unfortunate permission gotcha

The file that ships in the test works for prod / staging - the contact ids match test contacts
but for local testing the contact id needs editing.

Bug : T228714

Change-Id: Ic8e36c095ab8132a8fad7ece7d1ce20bc6b558cb
@seamuslee001
Copy link
Contributor

Seems fine to me, Code in the import processor isn't fully live yet and it adds tests merging

@seamuslee001 seamuslee001 merged commit 8ff004f into civicrm:master Aug 21, 2019
@seamuslee001 seamuslee001 deleted the test_ext2 branch August 21, 2019 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants