Fix readability & caching on CRM_Contact_BAO_Relationship::isInheritedMembershipInvalidated #15061
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(Standard links)
|
eileenmcnaughton
changed the title
| return [$relTypeId, $isDeletable]; | ||
| $membershipType = CRM_Member_BAO_MembershipType::getMembershipType($membershipValues['membership_type_id']); | ||
| $relTypeIds = $membershipType['relationship_type_id']; | ||
| $membshipInheritedFrom = $membershipValues['owner_membership_id'] ?? NULL; |
There was a problem hiding this comment.
I just broke the IF into 2 because it was hard to read & a later fix would be more nuanced on the second part
|
Refactoring relates to #14410 |
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Aug 17, 2019
…relationship This pulls out some of the good work & test written in civicrm#14410, but treating the parts as separate bugs with separate cleanup requirements. I put up cleanup in civicrm#15061 And this addresses the need for the relationship to not be deleted if a valid one still exists - wrangled out into a separate function
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Aug 17, 2019
…relationship This pulls out some of the good work & test written in civicrm#14410, but treating the parts as separate bugs with separate cleanup requirements. I put up cleanup in civicrm#15061 And this addresses the need for the relationship to not be deleted if a valid one still exists - wrangled out into a separate function
eileenmcnaughton
force-pushed
the
agile
branch
from
e6d1976 to
bd9dab5
Compare
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Aug 18, 2019
…relationship This pulls out some of the good work & test written in civicrm#14410, but treating the parts as separate bugs with separate cleanup requirements. I put up cleanup in civicrm#15061 And this addresses the need for the relationship to not be deleted if a valid one still exists - wrangled out into a separate function
|
@seamuslee001 this failed on something to do with the cache - possibly I need to add a clear. Before I dig - do you agree with adding this cache? |
|
I think that makes sense |
eileenmcnaughton
force-pushed
the
agile
branch
2 times, most recently
from
7356f2c to
f8c1556
Compare
|
@seamuslee001 looks like it was the 'clear' thing - passing now |
…dMembershipInvalidated This removes an sql query & some hard wrangling & uses the api - wrapped in a caching helper.
eileenmcnaughton
force-pushed
the
agile
branch
from
f8c1556 to
eb151aa
Compare
|
Actually I just pushed a tweak so they are re-running |
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Aug 21, 2019
…relationship This pulls out some of the good work & test written in civicrm#14410, but treating the parts as separate bugs with separate cleanup requirements. I put up cleanup in civicrm#15061 And this addresses the need for the relationship to not be deleted if a valid one still exists - wrangled out into a separate function
|
@seamuslee001 this is passing now |
|
I think the code looks fine to me and i think we should have enough test coverage in this are. Merging |
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Aug 29, 2019
…relationship This pulls out some of the good work & test written in civicrm#14410, but treating the parts as separate bugs with separate cleanup requirements. I put up cleanup in civicrm#15061 And this addresses the need for the relationship to not be deleted if a valid one still exists - wrangled out into a separate function
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Oct 14, 2019
…relationship This pulls out some of the good work & test written in civicrm#14410, but treating the parts as separate bugs with separate cleanup requirements. I put up cleanup in civicrm#15061 And this addresses the need for the relationship to not be deleted if a valid one still exists - wrangled out into a separate function
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Oct 25, 2019
…relationship This pulls out some of the good work & test written in civicrm#14410, but treating the parts as separate bugs with separate cleanup requirements. I put up cleanup in civicrm#15061 And this addresses the need for the relationship to not be deleted if a valid one still exists - wrangled out into a separate function
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Oct 26, 2019
…relationship This pulls out some of the good work & test written in civicrm#14410, but treating the parts as separate bugs with separate cleanup requirements. I put up cleanup in civicrm#15061 And this addresses the need for the relationship to not be deleted if a valid one still exists - wrangled out into a separate function
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Oct 27, 2019
…relationship This pulls out some of the good work & test written in civicrm#14410, but treating the parts as separate bugs with separate cleanup requirements. I put up cleanup in civicrm#15061 And this addresses the need for the relationship to not be deleted if a valid one still exists - wrangled out into a separate function
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Oct 28, 2019
…relationship This pulls out some of the good work & test written in civicrm#14410, but treating the parts as separate bugs with separate cleanup requirements. I put up cleanup in civicrm#15061 And this addresses the need for the relationship to not be deleted if a valid one still exists - wrangled out into a separate function
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Mostly code readability but also improves caching
Before
Sql query & some hard-to-read & unnecessary wrangling
After
More readable. Introduction of a cached function improves performance
Technical Details
@seamuslee001 looking at this I was going to use the 'fields' cache but of course the name was wrong so I figured adding a generic 'metadata' cache would cover a lot of uses (including probably what 'fields' currently is....
Comments