[REF] change deprecated function to API4 call #18076
Conversation
|
(Standard links)
|
|
test this please |
|
jenkins retest this please |
|
@MegaphoneJon test fail relates CRM_Contribute_BAO_ContributionTest::testProcessOnBehalfOrganization |
|
I agree it's questionable whether |
| $isNotCurrentEmployer = TRUE; | ||
| } | ||
|
|
||
| // formalities for creating / editing organization. | ||
| $behalfOrganization['contact_type'] = 'Organization'; |
There was a problem hiding this comment.
If this line is removed then when the array gets passed to hooks via createProfileContact(), the array won't contain contact_type anymore. Unless it's getting set somewhere else in between but I can't see where.
There was a problem hiding this comment.
Oh, you're right. I saw that "Organization" was passed as the $ctype argument to createProfileContact(), but that doesn't get passed to the hook. I'll restore this line and put it just above createProfileContact() since it's not needed by the other functions that use $behalfOrganization as an argument.
|
test fail is CRM_Contribute_BAO_ContributionTest.testProcessOnBehalfOrganization CRM_Contribute_BAO_ContributionTest::testProcessOnBehalfOrganization |
|
That test is weird - it's creating an invalid relationship but it's testing 'something' - maybe put the try- catch in your function since a little error seems OK here... |
|
I've added has-test as it's pretty clear CRM_Contribute_BAO_ContributionTest::testProcessOnBehalfOrganization has specific relevant cover |
|
test this please |
MegaphoneJon
force-pushed
the
better-on-behalf-of-2
branch
from
93b813d to
2650e56
Compare
|
OK, I figured it out. The code is good, the test has a bug - which escaped notice because of a complementary bug in the legacy code. This test runs However, the values are passed to that method by reference - so when you run it a second time, the values are no longer valid. Unfortunately, the legacy method fails silently! So the bug in the test went unnoticed. My modification to the test passes the original values to the second call, which allows the relationship to be created and the test passes. |
MegaphoneJon
force-pushed
the
better-on-behalf-of-2
branch
from
2650e56 to
e4addeb
Compare
|
On behalf of the organizations that benefit from this PR I'll say "good sleuthing"! |
|
@demeritcowboy is this good to merge now? |
|
Did some basic testing and no (related) issues. Not a blocker but regarding the exception for duplicates - it's probably not that robust to check the string label to determine the exception type. It's not ts'd at the moment (which I think is correct) but somebody might come along and do that, or even just change the string. The api should probably have a numeric error code for it. |
|
The alternative is to use a specific exception type - which we do in a few places eg that's probably more consistent with other places Anyway - merging this - that could be a follow up |
Overview
CRM_Contribute_Form_Contribution_Confirm::onBehalfOrganization()is some pretty messy code. This refactor is a first step toward addressing that.Before
Deprecated function used.
After
APIv4 used.
Comments
There's a lot more that one could clean up here, but I wanted to keep this change simple for review. Also, I question whether we should be throwing errors on a duplicate relationship at all.