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

Fix Participant import, add tests #23703

Merged
merged 7 commits into from
Jun 7, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jun 7, 2022

Overview

Fix Participant import, add tests

Currently includes - #23692 which I will rebase out

Before

Fatal error in MapField

After

Error fixed - this also adds full-flow test & makes the same changes as recently made for Membership.

In the process of this & QA I found errors affecting the other imports too

  • campaign options were not being handled
  • there was a bug in SaveMappings meaning it would not add new fields to an existing saved mapping
  • comma separated values were not being trimmed - I think this was pre-existing in Participant import but 'Attendee, Volunteer' should be handled correctly and now is & is tested

General updates to the way import works - per #23689

The underlying changes are

  • switch to storing field names not labels in civicrm_mapping_filed
    use a persistent table to hold the data being imported
  • download errors on the fly from this table rather than pre-generated csvs
  • stop using the form get and set methods to pass information around (use the userJob table)
  • standardise field validation and transformation based on metadata - when a field is retrieved using getTransformedValue the - - returned value will already have dates, money, option values etc transformed to the right format. If they are invalid they will be transformed to invalid_import_value - this makes most of the remaining formatting and validation pointless

Technical Details

Comments

@civibot
Copy link

civibot bot commented Jun 7, 2022

(Standard links)

@civibot civibot bot added the master label Jun 7, 2022
@eileenmcnaughton eileenmcnaughton force-pushed the import_unblock branch 9 times, most recently from 8304f4f to 4a6f7a5 Compare June 7, 2022 01:26
@eileenmcnaughton
Copy link
Contributor Author

@monishdeb are you able look at this one - it's the same as membership

@aydun
Copy link
Contributor

aydun commented Jun 7, 2022

Strange help text - "Participants made by ...":

 Select 'Individual' if you are importing Participants made by individual persons. Select 'Organization' or 'Household' if you are importing Participants made by contacts of that type. 

Got error after hitting Continue on Step 1 (upload data)

( ! ) Fatal error: Declaration of CRM_Event_Import_Parser_Participant::validateValues($values) must be compatible with CRM_Import_Parser::validateValues(array $values): void in /home/jenkins/bknix-dfl/build/core-23703-3lxzt/web/sites/all/modules/civicrm/CRM/Event/Import/Parser/Participant.php on line 124

@eileenmcnaughton
Copy link
Contributor Author

@aydun argh - that function got merged to the parent in another PR - I've just removed it.

That strange text is prob unchanged

This file is shared by memberships, contributions, participants, activities
@@ -126,6 +126,39 @@ public static function convertMappingFieldLabelsToNames(): bool {
}
}

// Participant fields...
// Yes - I know they could be combined - but it's also less confusing this way.
Copy link
Member

Choose a reason for hiding this comment

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

This comment isn't crystal-clear to me. What could be combined where?

@colemanw
Copy link
Member

colemanw commented Jun 7, 2022

Happy to see this merged as the test coverage is good.

@eileenmcnaughton
Copy link
Contributor Author

there is still some breakage in here - fixing now - but the problem is that managing the PRs is what is breaking things - so now I'm re-fixing stuff :-( - but I'll have that updated in a few & I think getting it merged will actually take use forwards as it will settle stuff down - will fix the newly broken stuff & then we can re-review once merged

@eileenmcnaughton
Copy link
Contributor Author

Just adding screenshots to show the above is fixed -

image

image

image

image

And then with the mapping fixed

image

image

@eileenmcnaughton
Copy link
Contributor Author

@aydun can you re-test this once merged? I think that will give us more stability and it is actively broken now without this merged

@eileenmcnaughton eileenmcnaughton merged commit 90998dc into civicrm:master Jun 7, 2022
@eileenmcnaughton eileenmcnaughton deleted the import_unblock branch June 7, 2022 21:54
@aydun
Copy link
Contributor

aydun commented Jun 8, 2022

The dropdowns all seem to be working correctly.

  1. Field Mappings are not being saved

  2. Event title vs event id:

The file mapping dropdown shows 'Event'. Looking at your screenshots shows this is expected to be the event title - and that works ok. However:
a) In your screenshots, Step 2 shows:
Screenshot-10
but on Step 3 it shows the title field mapping to the id:
Screenshot-12
... which is wrong/confusing.

b) I assumed 'Event' would be event id, so I set up the mapping as:
Screenshot-13
which leads to:
Screenshot-14
but fails the validation with
Screenshot-15

The 'missing required fields: event' message is confusing - since the mapping includes Event and the data line has a value for that field. A message like 'no matching event found' would be more helpful.

It seems not to be possible to import by event id.

Maybe the single 'Event' mapping dropdown should be replaced with 'Event Title' and 'Event ID'.

  1. Step 1 asks about the Contact Type. However, if contacts are specified by contact id, the contact type is ignored, so my import file using contact_id=1 (an org) imported ok despite selecting a contact type of 'Individual'.

I assume (ie not looked at the code) that contact type is required to select the appropriate Unsupervised Duplicate Matching rule. The help text says: "Include the data fields used for contact 'matching' based on your configured Unsupervised Duplicate Matching rules. For the default duplicate matching rules, you would include a column in each row with the contributors' Email Address."

However, the only contact fields available are contact id, email and external id - so if the Unsupervised rule includes other fields it is not possible to use those in the mapping and dedupe rule.

For simplicity, I'd get rid of the Contact Type and Unsupervised dedupe rule and just explicitly match against the email if specified. On the other hand, I'd like to merge Import Contacts and Import Participants where contact type would be significant.

@eileenmcnaughton
Copy link
Contributor Author

Thanks heaps @aydun - addressing your feedback over at
#23733

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