Skip to content

Improve financial trxn spec to require required fields #22571

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

Merged
merged 1 commit into from
Apr 14, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Improve financial trxn spec to require required fields

Before

It turns out the apiv4 conformance test only passes because it
is bypassing the BAO create method because of the mis-named BAO which @pradpnayak has been working on. The problem is ultimately that the test is not passing in the right parameters in part because the spec is not requiring all the parameters required at the BAO.

After

This updates the spec to be close to the v3 api requirements - it is still calling the wrong method but if we can get this to work then @pradpnayak's rename will work.

Technical Details

Not this will fail :-( because it messes with the test expectations

  • I don't know how to create complex valid test data for it though....

I guess we could create a contribution & pass it as entity_id - test conformance is hard

Comments

This is actually the first time I've ever gotten the v4 testConformance test to run locally :-). I hacked it per the patch below since it is never able to find the service - @colemanw why do we do the complicated, but hard to run, service config here - it doesn't seem to add much....

image

@civibot
Copy link

civibot bot commented Jan 19, 2022

(Standard links)

@colemanw
Copy link
Member

@eileenmcnaughton the person who wrote this test is a huge fan of services, but I'm not wedded to it at all and am happy to drop it in favor of something more practical.

@eileenmcnaughton
Copy link
Contributor Author

api_v3_ContributionTest::testCreateUpdateContributionPaymentInstrument
Undefined array key "currency"

/home/jenkins/bknix-dfl/build/core-22571-7ma39/web/sites/all/modules/civicrm/tests/phpunit/api/v3/ContributionTest.php:4083
/home/jenkins/bknix-dfl/build/core-22571-7ma39/web/sites/all/modules/civicrm/tests/phpunit/api/v3/ContributionTest.php:1411
/home/jenkins/bknix-dfl/build/core-22571-7ma39/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:262
/home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar:671

It turns out the apiv4 conformance test only passes because it
is bypassing the BAO create method - which requires more parameters

Not this will fail :-( because it messes with the test expectations
- I don't know how to create complex valid test data for it though....

I guess we could create a contribution & pass it as entity_id
@eileenmcnaughton
Copy link
Contributor Author

@colemanw @monishdeb I've finally brought this to a passing state ....

@monishdeb
Copy link
Member

  • General standards
    • (r-explain) PASS
    • (r-user) PASS
    • (r-doc) PASS
    • (r-run) PASS Tested on local, tried to pass invalid currency parameter, rightfully set to default currency. Checked FinancialTrxn.create API4 signature
  • Developer standards

@monishdeb monishdeb merged commit f652eb8 into civicrm:master Apr 14, 2022
@eileenmcnaughton eileenmcnaughton deleted the trxn branch April 14, 2022 04:20
@eileenmcnaughton
Copy link
Contributor Author

Thanks @monishdeb - I'll see if that gets us past #23190 test fail!

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.

3 participants