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

Move CRM_Core_BAO_FinancialTrxn to CRM_Financial_BAO_FinancialTrxn #22400

Closed
wants to merge 3 commits into from

Conversation

pradpnayak
Copy link
Contributor

Overview

Move CRM_Core_BAO_FinancialTrxn to CRM_Financial_BAO_FinancialTrxn

@civibot
Copy link

civibot bot commented Jan 7, 2022

(Standard links)

@eileenmcnaughton
Copy link
Contributor

@pradpnayak this looks generally good to me - but were you thinking about adding the dummy CRM_Core_BAO_FinancialTrxn which extends the new class name in this PR or a separate one? (if separate we need to be careful we don't merge one pre-rc-cutting & one post

@pradpnayak
Copy link
Contributor Author

Hi @eileenmcnaughton

Sorry for late response, got busy with other work.

Yes I am going to add the dummy CRM_Core_BAO_FinancialTrxn, I was searching for an example, I remember there was something around Task files where Matt or you had created a dummy file with deprecated method, Can't find one, do you have any example that i could use?

Thanks
Pradeep

@eileenmcnaughton
Copy link
Contributor

@pradpnayak
Copy link
Contributor Author

Hi Eileen,

I have pushed the changes, can you please review?

Thanks
Pradeep

@eileenmcnaughton
Copy link
Contributor

@pradpnayak did you see

api\v4\Entity\ConformanceTest::testConformance with data set "FinancialTrxn" ('FinancialTrxn')
PEAR_Exception: DB Error: unknown error

@pradpnayak
Copy link
Contributor Author

looks like test have passed

@eileenmcnaughton
Copy link
Contributor

@pradpnayak it is passing but the nature of the fix bothers me - do you understand why it would be passing in no entity_id when it previously didn;t?

@eileenmcnaughton
Copy link
Contributor

@pradpnayak I figured out why it is failing & the fix needs to be in the unit test not the BAO class - your change exposed a bug in the v4 api whereby it is currently not calling the create method (bad) and hence it got away without passing in all the required params

#22571 is a partial fix but still needs a fix on the test side

@eileenmcnaughton
Copy link
Contributor

@pradpnayak this is stale so I'm closing - but have put up the less ambitious #23190 to keep this progressing

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.

2 participants