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

dev/core#3077 - CiviCase - Call hooks when creating relationships #22814

Merged
merged 2 commits into from
Feb 26, 2022

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Feb 22, 2022

Overview

Refactors creating relationships when opening a new case to use APIv4 (temporarily using the BAO method), which calls hooks.

Before

No hooks called.

After

Api used, hooks called, code easier to read.

Technical Details

The attempt to use APIv4 failed if the case client is an Organization, due to some extra (faulty?) validation & processing which happens at that layer. IMO that needs tests and fixing, but for now, this calls BAO_Relationship::add() directly.

Comments

I stepped through the unit tests and confirmed that this function has good test coverage.

@civibot
Copy link

civibot bot commented Feb 22, 2022

(Standard links)

@seamuslee001
Copy link
Contributor

Looks fine to me @mattwire @demeritcowboy work for you?

@mattwire
Copy link
Contributor

Works for me :-)

@demeritcowboy
Copy link
Contributor

One of the exact same things happens as in the previous attempt to use api: It crashes for org clients. I haven't tested further because I now expect all the same issues will come up.

@colemanw
Copy link
Member Author

@demeritcowboy do you remember what issue or PR that happened before?

@demeritcowboy
Copy link
Contributor

I had linked to it in my comment on #22810 (comment) - it was #15030

@colemanw colemanw force-pushed the civiCaseRelationshipHooks branch from 0bd466e to a4fd3fd Compare February 24, 2022 03:45
@demeritcowboy
Copy link
Contributor

In terms of whether it works or not this version works (note: the PR title/description is no longer accurate). I'll leave the r-tech discussion about add() to others - I would normally put merge-ready.

@colemanw colemanw changed the title CiviCase - Use APIv4 and call hooks when creating relationships CiviCase - Call hooks when creating relationships Feb 24, 2022
@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Feb 24, 2022
@colemanw
Copy link
Member Author

Thanks for the r-run @demeritcowboy. I've updated the description & added the label.

@demeritcowboy demeritcowboy merged commit 1f3e4c2 into civicrm:master Feb 26, 2022
@demeritcowboy demeritcowboy changed the title CiviCase - Call hooks when creating relationships dev/core#3077 - CiviCase - Call hooks when creating relationships Feb 26, 2022
@colemanw colemanw deleted the civiCaseRelationshipHooks branch November 8, 2022 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants