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

[REF] Extract chunk of code relating to whether to disabled an inherited relationship #14955

Merged
merged 1 commit into from
Aug 9, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Aug 2, 2019

Overview

Fairly straight forward extraction along with a slight extension of what is enabled of the test @agilewarealok wrote for #14410

Before

Code less readable

After

Code more readable

Technical Details

#14410 stalled & is stale - I've now added the tests but the part that relates to the fix is commented out. This does not bring in any of the fix - it just extracts the code that is fixed for readability & extends the test to cover this codes original functionality. When I was looking through that PR I felt that without the code being broken out a bit I felt really uncomfortable reviewing it so cleaning up the code with an extraction seemed like a good first step towards getting it to a point where the patch could be written in a way that would not stall on getting reviewed. The next step IMHO is to add the early return / if handling into the extracted function & uncomment the next lines of code that cover it.

I did cleanup the test quite a bit in passing

Comments

will rebase once #14954 is merged

@civibot
Copy link

civibot bot commented Aug 2, 2019

(Standard links)

@agileware-pengyi
Copy link
Contributor

Reviewed and tested.
I will rebase #14410 when this gets merged.

@eileenmcnaughton
Copy link
Contributor Author

OK - merging base on @agileware-pengyi review

@eileenmcnaughton eileenmcnaughton merged commit 2fb8548 into civicrm:master Aug 9, 2019
@eileenmcnaughton
Copy link
Contributor Author

@agileware-pengyi I've merged this now - looking at #14410 I feel like part of the change (and part of the tests) are limited to changing what happens in isInheritedMembershipInvalidated() & it would be good to isolate those into a PR.

Ideally we would switch the query in the PR to an api call for better readability.

It's really hard to catch problems in PRs that make multiple changes - hence my focus on trying to keep each review task as small as possible

@eileenmcnaughton eileenmcnaughton deleted the rel_alok_ref branch October 3, 2020 02:03
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.

2 participants