Fix saving custom data & add test

colemanw commented 5 years ago

Before

Custom field saving was broken for some entities.

After

Works for all entities; locked in by conformance test.

Notes

Depends on https://github.com/civicrm/civicrm-core/pull/14535

colemanw commented 5 years ago

/test

colemanw commented 5 years ago

/test

colemanw commented 5 years ago

@monishdeb any idea about these 2 test failures?

monishdeb commented 5 years ago

hmm checking

monishdeb commented 5 years ago

/test

colemanw commented 5 years ago

@monishdeb

Test Result (2 failures / +2) Civi\Test\Api4\Action\CreateWithOptionGroupTest.testGetWithCustomData Civi\Test\Api4\Action\CreateWithOptionGroupTest.testWithCustomDataForMultipleContacts

colemanw commented 5 years ago

@monishdeb I've done a rebase to keep the branch up-to-date. Any luck with the test failures?

colemanw commented 5 years ago

/test

eileenmcnaughton commented 5 years ago

@colemanw in apiv3/ core the callApiSuccess wrapper (mostly) makes sure the sql is out put in fails like this one
https://test.civicrm.org/job/Extension-SHA/578/testReport/junit/(root)/Civi_Test_Api4_Action_CreateWithOptionGroupTest/testGetWithCustomData/

which is really helpful

@monishdeb how are you placed on this - I am under the impression this is blocked on your availablity and in turn is the blocker for api v4 in core for 5.16 (also ping @seamuslee001 )

colemanw commented 5 years ago

@eileenmcnaughton I'm going to try to move this along, but there are 2 biggies in api4 which IMO should get addressed before inclusion in core:

joins are partially broken
getting/saving fields with pseudoconstants (e.g. being able to pass "Meeting" as acvtivity_type_id)

eileenmcnaughton commented 5 years ago

@colemanw if we moved into core in time for 5.16 rc could we resolve those 2 during the rc period (without too much change outside the apiv4 part of the codebase)?

colemanw commented 5 years ago

Hey @monishdeb could you say more about your last commit? The message is "minor fix" but it's a big change and I'm not sure what it's doing.

monishdeb commented 5 years ago

@colemanw yes the commit title was a bit misnomer. Actually, the last commit was about when we get custom table name civicrm_value_* for joins here I added a CoreUtil fn to fetch jonable $links on basis of a custom table name. But then I was stuck on how we could possible add a Bridgable join between custom field/group and custom table :(

colemanw commented 5 years ago

Ok thanks @monishdeb. I'll look into it.

monishdeb commented 5 years ago

Thanks :)

eileenmcnaughton commented 5 years ago

/test

totten commented 5 years ago

/test

eileenmcnaughton commented 5 years ago

@monishdeb @colemanw in the interests of trying to move this along a bit I pulled out the stale commit into it's own PR so we can get it merged & sorted https://github.com/civicrm/org.civicrm.api4/pull/167

eileenmcnaughton commented 5 years ago

@colemanw as I understand it this is still the blocker to getting apiv4 moved into core & it's in your court?

civicrm / org.civicrm.api4