dev/core#1473 - Missing address on /user when location type label dif…

[jitendrapurohit]

…fers from its name

Overview

Missing address on /user when location type label differs from its name.

Before

To replicate -

• Update label of some location type. Eg Home -> HomeEdited
• Visit /user and notice the warnings displayed on the page.

• No address is printed for the contact -

After

Notice Error fixed which displays the address values correctly -

Comments

Gitlab - https://lab.civicrm.org/dev/core/issues/1473

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[demeritcowboy]

I tested this and it works in terms of making it appear, but a couple of r-run things are confusing me so give me a little bit to do a fuller review and sort out which are related and not.

[demeritcowboy]

Ok I've narrowed it down to one real issue and one "concern". The real issue is that this fixes the bug but introduces a new bug, but the old bug is worse than the new bug so overall it's an improvement. Does that make sense? Ok, details:

• General standards
  • [r-explain] PASS
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] Issue
    • Tested adding IM and website to the profile. What's happening now with both of those is that if you do the following it ends up outputting the name as part of the output. It worked properly before the patch.
      • Change the Yahoo IM provider option to Yahoo-x.
      • Add IM Primary to the stock "name and address" profile.
      • On a contact that has a yahoo IM filled in visit the /user page.
      • Not in the field label but in the actual value, it will show screenname (Yahoo), instead of screenname (Yahoo-x).
      • It's similar for website - it will be something like https://www.example.com (Work), instead of whatever "Work" was changed to.
    • (Not an issue just was confusing) The field label on the profile still has the original "Home" wording, but that's because the profile itself has a field where you set the field label. In any case it's not what the PR is about, just was confusing since it seemed related at first.
• Developer standards
  • [r-tech] PASS
  • [r-code] PASS with comments
    • I'm not sure about the decision to use buildOptions. All the buildOptions calls just end up calling PseudoConstant::get anyway without doing anything else, so could just change the existing PseudoConstant::get call to add the $context parameter, e.g. CRM_Core_PseudoConstant::get('CRM_Core_DAO_Address', 'location_type_id', [], 'validate').
    • If for whatever reason it's decided to use buildOptions, then while it ends up not mattering here the address one uses BAO while the other two use DAO. It feels like using BAO would be better, since then if someone adds an override function in the BAO it's in the right place and will get called.
  • [r-maint] Undecided
  • [r-test] PASS

[jitendrapurohit]

Thanks for the detailed testing @demeritcowboy The above issue should be fixed now. Can you pls test and confirm?

[demeritcowboy]

Thanks @jitendrapurohit I retested and it looks ok. That getValues function seems difficult to work with so the inconsistency in $context is difficult to avoid.

[yashodha]

@demeritcowboy Is this ready to merge after the changes?

[demeritcowboy]

Go for it.

[yashodha]

@demeritcowboy thanks! I am merging this now.