[REF] Minor code cleanup on the setting of contact greetings. #15949
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)
|
I'm trying to review civicrm#15725 but through no fault of that PR it hits on one of the parts of the Export code that I still find unreadable :-( This is a further readability fix & it starts to point me to how to get past the underlying WTF about this code. Note that I removed the IF around sharedAddress. From Monish's PR I found out that we have a fairly long-standing regression where the sharedAddress code doesn't actually work :-( so I'm comfortable this won't break anything :-). We also have good test cover on this chunk from probably around when the shared address part got broken.... Looking in the UI there is no implication that the greeting for a shared address would display differently - which is the only explanation I can think of for different handling here. Of course until we remove the later IF the shared address would do things differently - erm if it worked. ALl of which is a long way of saying the removal of the IF won't change anything
eileenmcnaughton
force-pushed
the
export_ref
branch
from
1efff0a
to
cf7c5cb
Compare
|
Added Merge on pass, reviewed the code it makes sense to me. Agree with the explanation provided on the removal of the if |
|
unrelated fail - merging |
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Nov 25, 2019
This is a follow on from civicrm#15949 This doesn't make any change - it just improves legibility
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Nov 25, 2019
This is a follow on from civicrm#15949 This doesn't make any change - it just improves legibility
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
Minor code tidy up
Before
A bunch of 3 line code calls
After
Replaced with a bunch of one line calls. Also an IF removed - see below
Technical Details
I'm trying to review #15725 but through no fault of that PR it hits
on one of the parts of the Export code that I still find unreadable :-( This is a further readability fix & it
starts to point me to how to get past the underlying WTF about this code.
Note that I removed the IF around sharedAddress. From Monish's PR I found out that we have a
fairly long-standing regression where the sharedAddress code doesn't actually work :-( so
I'm comfortable this won't break anything :-). We also have good test cover on this chunk from
probably around when the shared address part got broken....
Looking in the UI there is no implication that the greeting for a shared address would display differently -
which is the only explanation I can think of for different handling here. Of course until we
remove the later IF the shared address would do things differently - erm if it worked.
ALl of which is a long way of saying the removal of the IF won't change anything
Comments