Switch to relationship BAO when modifying relationships in change case status so we don't bypass hooks

[mattwire]

Overview

Replace direct SQL UPDATE with Relationship.create API CRM_Contact_BAO_Relationship::add() when changing case status so we don't bypass hooks

We can't use the API (yet) because it contains too much business logic, so switching from direct SQL to the BAO.

Before

Relationship end_date is updated using a direct SQL query, bypassing hooks

After

Relationship end_date is updated using the Relationship.create API CRM_Contact_BAO_Relationship::add() so no hooks are bypassed

Technical Details

Direct SQL query -> BAO call

Comments

I'm not convinced we should necessarily be "reopening" relationships just because we change the case status. Also, I wonder whether we should be changing the active/inactive flag if we change the end_date. But I'm not proposing any change in this PR which retains the same functionality as before.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

@demeritcowboy any thoughts

[demeritcowboy]

Yep I can review - fix seems like a good idea. Regarding the should/shouldn't there's been some back and forth around that (example PRs below). Currently, changing one thing breaks something else since it gets interpreted differently between dashboard, manage case, search, contact relationships tab, and also differences of opinion. There was an idea listed at the first link here:

https://lab.civicrm.org/dev/core/issues/542
#13134
#13510
#13831

[eileenmcnaughton]

@demeritcowboy yes - there is the small change in this PR & the larger discussion it raises...

[demeritcowboy]

If the client is an org, this seems to cause the change case status activity to hang when closing the case. Doesn't seem to happen with current. I'll try to track it down but it will probably be tomorrow (my timezone, so that would be two days from now for you - he he). Hey do you both have horseracing? Why don't you tell me the winners and I'll place a bet in my timezone before the race and split the winnings with you - how has nobody ever thought of this before? (grin)

[eileenmcnaughton]

Ah great plan - what could possibly go wrong

[demeritcowboy]

Actually I can see what it is. The api includes a check that the relationship "is valid" and it ends up throwing an uncaught exception. It seems pre-existing, and I'm thinking would have a problem for any code that uses the api to end an org relationship? But is currently a bit of a blocker.

In general the api for Relationship seems to have a fair amount of business logic, including where it does something with memberships(?) and current employer, so I know the PR seems like a small change but maybe worth a little more review than at first glance.

[mattwire]

@demeritcowboy I've pushed an amended version that does not use the API but calls CRM_Contact_BAO_Relationship::add() directly. That gets us the hooks triggering without most of the additional business logic that the API / create function introduces.

By the way, my specific requirement is actually to prevent the relationship being updated at all.

[demeritcowboy]

Looks like the add() function needs to be fed more params, but otherwise seems like it should work.

Or thinking out loud, if this ends up not working then if the custom code is implementing hooks anyway, would it work to have the custom code call getCaseRoles and save the results somewhere during hook_preprocess, and then restore during postprocess? Not ideal, but just a thought.

[mattwire]

@demeritcowboy Thanks - it seems to me like the "better" fix here is to fixup CRM_Contact_BAO_Relationship::add() to autoload params per the @todo in there. I've done that here in an additional commit - let's see if tests pass and we can break that into it's own PR to merge first.

[demeritcowboy]

@mattwire is this no longer WIP or is there more to do still?

[mattwire]

@demeritcowboy I've opened #15103 to handle the change required to the Relationship BAO so we need to get that reviewed/merged and then rebase/merge this one.

[demeritcowboy]

Thanks @mattwire - will take a look.

[mattwire]

@demeritcowboy If you have time, the relevant PRs have now been merged and this is ready for final review.

[demeritcowboy]

Yep will take a look.

[demeritcowboy]

• General standards
  • [r-explain] PASS
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] PASS
    • Overall looks good. Some notes:
    • I didn't test that the hooks are working, which was the original motivation here, but I assume you've either already used it or will soon.
    • An unrelated thing which I'll make a separate ticket for: Creating a case with a backdated start date doesn't use the backdated date for the start of the case manager relationship (uses "today").
    • I can see there's a very unusual edge case with current employee but I'm not going to worry about it because you'd have to be using civicase in a way that's pretty weird. Something like a 3rd party company that does staff evaluations for other org's already existing employees (since they aren't getting their TPS reports done...), but even then it wouldn't make too much difference.
    • But then it made me notice yet another unrelated issue: If you try to edit a org-individual role using the pencil icon, you can't select the individual, but if you use the add new role button it works fine. Will make another ticket.
• Developer standards
  • [r-tech] PASS
  • [r-code] PASS
  • [r-maint] Undecided
    • Undecided, although earlier one of the tests pointed something out, so there's something there.
  • [r-test] PASS

[eileenmcnaughton]

OK - merging based on the above review