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

RelationshipCache - Add case_id column #21845

Merged
merged 2 commits into from
Oct 19, 2021

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Oct 16, 2021

Overview

This makes it possible to show case relationships in SearchKit.

Before

Cannot filter relationships by case.

After

Can show relationships for specific cases.

Technical Details

This adds a mirror column to the relationship_cache table, since SearchKit can't do the complex joins that would be needed without the field present. SearchKit doesn't support the relationship table, it always uses RelationshipCache per #17781, therefore any data we need from relationships needs to be in the cache table.

While I was at it I also added the Component tag to the column in both tables, so it is only present when CiviCase is enabled.

This will hide that field when CiviCase is disabled
@civibot
Copy link

civibot bot commented Oct 16, 2021

(Standard links)

@civibot civibot bot added the master label Oct 16, 2021
@colemanw colemanw force-pushed the relationship_cache_case_id branch from 4eb1d77 to 97b3849 Compare October 16, 2021 20:31
@demeritcowboy
Copy link
Contributor

added the Component tag to the column in both tables, so it is only present when CiviCase is enabled

I'm not sure what the tag does fully but I assume the above means "only present IN SEARCH KIT UI when CiviCase is enabled".

Otherwise I ran the upgrade and clicked around a bit and it seems ok. I did notice a minor separate thing but I'll make a lab ticket.

For the upgrade I started thinking about the fact that WHERE case_id IS NOT NULL will do a full table scan but I think I've convinced myself it's not worth trying to out-think mysql and the clause is better for sites that don't use civicase.

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label Oct 17, 2021
@colemanw
Copy link
Member Author

added the Component tag to the column in both tables, so it is only present when CiviCase is enabled

I'm not sure what the tag does fully

@demeritcowboy it means the column is only visible to APIv4 when the component is enabled. If tests are happy with it then I think it's a safe addition.

public function upgrade_5_44_alpha1($rev) {
// The runSql task populates the new column, so this addColumn task runs first
$this->addTask('Add case_id column to civicrm_relationship_cache', 'addColumn',
'civicrm_relationship_cache', 'case_id', "int unsigned DEFAULT NULL COMMENT 'FK to civicrm_case'"
Copy link
Contributor

Choose a reason for hiding this comment

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

@colemanw on upgrade your just adding the column not the FK but on install it would have an additional FK right based on the xml?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh, how do we even do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

if (!self::checkFKExists('civicrm_contribution_product', 'FK_civicrm_contribution_product_product_id')) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch @seamuslee001.

@colemanw colemanw force-pushed the relationship_cache_case_id branch from 97b3849 to bcd7e8d Compare October 19, 2021 00:59
@colemanw
Copy link
Member Author

Thanks @seamuslee001 and @demeritcowboy, I think I got the FK added correctly now.

@demeritcowboy
Copy link
Contributor

Thanks works ok for me.

KEY FK_civicrm_relationship_cache_case_id (case_id),
CONSTRAINT FK_civicrm_relationship_cache_case_id FOREIGN KEY (case_id) REFERENCES civicrm_case (id) ON DELETE CASCADE,

I also checked that this doesn't prevent creating a regular relationship where case_id is null since I couldn't remember how that worked with foreign keys.

And interestingly the UPDATE would still do a table scan even if you switch the addition of the foreign key to come before the UPDATE. The foreign key adds an implicit index, so I was thinking it might improve it but it's not clear it does. What I mean is EXPLAIN without the index says "Using where". With it it says "Using where; using index".

@colemanw
Copy link
Member Author

Merge-on-pass?

@demeritcowboy demeritcowboy added merge on pass and removed merge ready PR will be merged after a few days if there are no objections labels Oct 19, 2021
@seamuslee001 seamuslee001 merged commit 81a6a5c into civicrm:master Oct 19, 2021
@seamuslee001 seamuslee001 deleted the relationship_cache_case_id branch October 19, 2021 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants