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] Reverse if statement #20211

Merged
merged 1 commit into from
May 16, 2021
Merged

[REF] Reverse if statement #20211

merged 1 commit into from
May 16, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented May 3, 2021

Overview

[REF] Reverse if statement

Before

Code sets and then conditionally unsets

After

Code conditionally sets

Technical Details

This splits out a minor code cleanup from #18196 (which is mostly stalled on needs test - #21210 can help with a starting point on that)

I should note that I actually came up with a different result from

https://github.com/civicrm/civicrm-core/pull/18196/files#diff-7117b779a415ec834465eaeffeaddfd50a003a8dab1b11192133adc264960727R60-R63

I compared the two AFTER I created this PR and found the difference. I think mine is right but need some more eyes

  • note the || vs the && because it is now

if (hasPermission OR isselfService)

compared to the original

if (!hasPermission && !isSelfService)

EDIT - change of heart - I've altered again - but really this should be partly split to https://github.com/civicrm/civicrm-core/pull/20210/files#diff-7117b779a415ec834465eaeffeaddfd50a003a8dab1b11192133adc264960727R110 so I now have doubts about merging this when that is not merged

Comments

@civibot
Copy link

civibot bot commented May 3, 2021

(Standard links)

@civibot civibot bot added the master label May 3, 2021
@eileenmcnaughton eileenmcnaughton force-pushed the reorg branch 2 times, most recently from d6d8ad2 to cb187dc Compare May 3, 2021 06:22
This splits out a minor code cleanup from civicrm#18196 (which can probably
build on the unit test work in 20210)
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