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

dev/core#1204 Data import for custom membership fields is done as tex… #15122

Closed
wants to merge 3 commits into from
Closed

dev/core#1204 Data import for custom membership fields is done as tex… #15122

wants to merge 3 commits into from

Conversation

sushantpaste
Copy link
Contributor

Overview

Data import for custom membership fields is done as text Instead of value.

  • Create a custom data used for Membership having Data Field with Alphanumeric and Field Type Autocomplete-Select
  • The values in the look up should be numeric.
  • Try to update the membership record using import for the custom field with text not the value.

Before

Check the field in DB, the data is imported but stored as text not value - so this doesn't show up in the UI.

After

Check the field in DB, the data is imported and stored as value - so show up in the UI.

Comments

https://lab.civicrm.org/dev/core/issues/1204

@civibot
Copy link

civibot bot commented Aug 23, 2019

(Standard links)

@civibot civibot bot added the master label Aug 23, 2019
@eileenmcnaughton
Copy link
Contributor

@sushantpaste can we get a test on this CRM_Member_Import_Parser_MembershipTest has some to use as a starting point and there is a CustomDataTrait that will be helpful

@@ -666,6 +666,17 @@ public function membership_format_params($params, &$values, $create = FALSE) {
}
}
}
if ($type == 'Autocomplete-Select') {
Copy link
Contributor

Choose a reason for hiding this comment

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

@sushantpaste @eileenmcnaughton

This looks like repetitive code and could be centralised in one place without having to do fixes in all components separately.
https://github.com/civicrm/civicrm-core/blob/master/CRM/Event/Import/Parser/Participant.php#L527
https://github.com/civicrm/civicrm-core/blob/master/CRM/Contribute/Import/Parser/Contribution.php#L666

so that all components correctly format custom data for all field types.

Copy link
Contributor

Choose a reason for hiding this comment

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

@yashodha I totally agree!

@yashodha
Copy link
Contributor

@sushantpaste @eileenmcnaughton

This looks like repetitive code and could be centralised in one place without having to do fixes in all components separately.
https://github.com/civicrm/civicrm-core/blob/master/CRM/Event/Import/Parser/Participant.php#L527
https://github.com/civicrm/civicrm-core/blob/master/CRM/Contribute/Import/Parser/Contribution.php#L666

so that all components correctly format custom data for all field types.

@yashodha
Copy link
Contributor

yashodha commented Aug 27, 2019

@sushantpaste Can we centralize the code for importing contact custom fields as well since that is also doing same thing?

https://github.com/civicrm/civicrm-core/blob/master/CRM/Contact/Import/Parser.php#L878

@eileenmcnaughton
Copy link
Contributor

@sushantpaste have you written a unit test for civi before? Is there something I can do to get you up & running writing them?

@sushantpaste
Copy link
Contributor Author

@sushantpaste have you written a unit test for civi before? Is there something I can do to get you up & running writing them?

@eileenmcnaughton Haven't added any tests previously can you help me with this. I think also there is no proper document for writing test. If you can help for writing tests for this time, that would be great and help to write tests in future.

@eileenmcnaughton
Copy link
Contributor

I'm very happy to help on this @sushantpaste - the first thing I would try to do is see if you can run them locally - I'd probably focus on running the CRM_Contact_Import_Parser_ContactTest - here https://github.com/civicrm/civicrm-core/pull/15190/files#diff-560b2cafc594a807893f61844ed09d24

I usually run them through my IDE (Php Storm) but you can use the cli - some info on it is here https://docs.civicrm.org/dev/en/latest/testing/phpunit/ - with --filter you should be able to run just that one class

@eileenmcnaughton
Copy link
Contributor

This test is probably a good starting point - although createCustomGroupWithFieldOfType() doesnt' support all types yet & might need extending

public function testCustomDataLabel() {

@eileenmcnaughton
Copy link
Contributor

I did start the work of writing a test here #17560

I'm going to close this for now as it needs rebasing & a test to be reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants