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

Add entity financial account bridge entity #19927

Merged
merged 1 commit into from
Apr 7, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Add entity financial account bridge entity

Before

Bridge entity not described

After

Now it is

Technical Details

@colemanw I've been digging around & can see that this is required -but is not in itself enough - ie

image

What else is needed?

Comments

@civibot
Copy link

civibot bot commented Mar 28, 2021

(Standard links)

@civibot civibot bot added the master label Mar 28, 2021
@colemanw
Copy link
Member

@eileenmcnaughton if the table doesn't contain exactly 2 FKs, then you have to add an annotation to say which 2 entities it is supposed to link together. Like this:

* @bridge near_contact_id far_contact_id

@eileenmcnaughton
Copy link
Contributor Author

@colemanw ah - so there is an entity_id -> entity_table

In addition I think the account_relationship option values would work rather like the record_type_id in activity contact table (ie they are almost like different entitiies)

image

* @see https://docs.civicrm.org/dev/en/latest/financial/financialentities/#financial-accounts
*
* @bridge entity_id financial_account_id
*
Copy link
Member

@colemanw colemanw Mar 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton if you want the AccountRelationship field automatically shown when adding join in searchKit (like relationship type id is shown for related contacts or activity contact type is shown for activities) do this:

Suggested change
*
* @ui_join_filters account_relationship

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colemanw I've pushed that up - although note the whole thing doesn't work atm

@colemanw
Copy link
Member

I see the problem @eileenmcnaughton. The field entity_table doesn't have any pseudoconstant, therefore the joiner has no idea what possible tables it can join to.

@eileenmcnaughton
Copy link
Contributor Author

@colemanw right - the only entity that does have one is entity_tag I guess.
The possibilities are

  • financial_type
  • payment_processor
  • option_value.value WHERE option_group_id:name = Payment Instrument

(that last doesn't help!)

@colemanw
Copy link
Member

More than just entityTag, there's quite a few entity_table fields with pseudoconstants. It's pretty much required to get a bridge join to work.
I don't understand that last one - how does that point to a table name?

@eileenmcnaughton
Copy link
Contributor Author

@colemanw here is the latter part of the table

image

The entity_id actually holds the id not the value it seems - which is a nasty non standard use of the option_value table but might be helpful here

@colemanw
Copy link
Member

Yes that is actually helpful. So our pseudoconstant list just needs to contain 3 options and we should be good to go, at least as far as SearchKit is concerned.

@eileenmcnaughton
Copy link
Contributor Author

eileenmcnaughton commented Mar 28, 2021

@colemanw ok - that seemed to work - let's see how jenkins gets on

@colemanw
Copy link
Member

I don't think the test failure is related but retest this please just to be sure.

@eileenmcnaughton
Copy link
Contributor Author

@colemanw sadly it is - it's the contribution recur entity defaults tripping us up (AGAIN) - I added this
#19934

@colemanw
Copy link
Member

colemanw commented Apr 7, 2021

@eileenmcnaughton I've merged the other. Can you rebase this pls?

@eileenmcnaughton
Copy link
Contributor Author

@colemanw done - lets see how it goes!

@eileenmcnaughton
Copy link
Contributor Author

@colemanw yep passing now!

@colemanw colemanw merged commit 5c1bf1d into civicrm:master Apr 7, 2021
@colemanw
Copy link
Member

colemanw commented Apr 7, 2021

Looks great!

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