[Import] [Membership] Cleanup, fix form breakage (unreleased regression)

[eileenmcnaughton]

Overview

[Import] [Membership] Cleanup

with things having been merged in different orders to when I wrote them the focus on the Contact import left some others broken - I'm going through fixing them. In looking at the membership import it is fundamentally a simple import and after fixing one bit here & another there & adding a bunch of tests I wound up pushing through & adopting the methodology and functions worked out for the (more complicated) contact import.

I going through 'rounds' of review on each import - adding tests, UI testing, fixing & then coming back & doing again so that they all get QA - and are working in the rc.

Please scrol to the bottom for my QA testing information

Before

Form errors

After

The import now uses the data table, uses lots of shared & standardised methods, has more tests - oh and it doesn't have that nasty fatal error

Technical Details

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

Comments

Fixes https://lab.civicrm.org/dev/core/-/issues/1819
I tested https://lab.civicrm.org/dev/core/-/issues/1204 against this & it is fixed - although whether it was already fixed I don't know

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

Test fails are because of need to adjust test for recent merge of import queue pr

[monishdeb]

@eileenmcnaughton the test failure seems related :

CRM_Member_Import_Parser_MembershipTest::testImportOverriddenMembershipWithInvalidOverrideEndDate
Trying to access array offset on value of type null

/home/jenkins/bknix-dfl/build/core-23689-3kbst/web/sites/all/modules/civicrm/CRM/Member/BAO/MembershipType.php:399
/home/jenkins/bknix-dfl/build/core-23689-3kbst/web/sites/all/modules/civicrm/CRM/Member/Import/Parser/Membership.php:398
/home/jenkins/bknix-dfl/build/core-23689-3kbst/web/sites/all/modules/civicrm/tests/phpunit/CRM/Member/Import/Parser/MembershipTest.php:262
/home/jenkins/bknix-dfl/build/core-23689-3kbst/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:262
/home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar:671

[eileenmcnaughton]

@monishdeb yeah - it didn't fail locally - but I pushed up a small change in the hope it helps - it is to do with the test not the import code

[eileenmcnaughton]

Ah - it's something to do with membership set up - def test related - but hence it didn't fail in isolation - still digging

[eileenmcnaughton]

OK - the test failed because it now fails during validateValues rather than import - the mapField form calls validateValues and Preview calls import

[eileenmcnaughton]

@monishdeb it has passed!

[monishdeb]

• General standards

  • (r-explain) PASS
  • (r-user) PASS
  • (r-doc) PASS : I think PR description explains in detail
  • (r-run) PASS: I have tested the patch in the test-build generated http://core-23689-3kgyf.test-3.civicrm.org:8001 Confirmed that the fatal errors are fixed.

• Developer standards

  • (r-tech) PASS
  • (r-code) PASS
  • (r-maint) PASS
  • (r-test) PASS

[eileenmcnaughton]

thanks heaps