dev/core#1158 mailing labels: explicitly set primary flag

[lcdservices]

Overview

Fix mailing label primary location flag. Problem occurs if the global "Search Primary Details Only" option is turned off.

Before

Selecting primary location when generating mailing labels would not consistently export the correct address.

After

Selecting primary location works correctly.

Technical Details

The CRM_Contact_BAO_Query constructor (which is called by apiQuery) supports explicitly passing a parameter to search primary location only. If absent, it sets the value based on the global setting. The mailing label task called apiQuery and incorrectly assumed the primary record would be returned. This change explicitly sets that value if no location type is selected (i.e. the primary location is requested).

[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).

[eileenmcnaughton]

@lcdservices it doesn't look like there are any tests on this class yet - it would be good to get something. Generally when there are no tests on a form I'm happy just to get 'something' - ie a test that loads the form using $this->getFormObject() & checks that it's possible to call 'submit' (which usually needs extracting from postProcess)

I have to say everything about turning off "Search Primary Details Only" gives me the heeby jeebies

[MegaphoneJon]

@lcdservices This is a very similar issue to core#484, but PR #11274 was rejected because it failed in FULL_GROUP_BY scenarios. Flagging that this needs to be tested against FGB-enabled servers.'

@eileenmcnaughton Any time a bug on "Search Primary Details Only is off" comes up you flag your discomfort. I'd like to hear more about that. Is it a criticism of the feature itself? The code quality? I suspect it's that it leads to bugs like these, where too much of the code assumes this feature doesn't exist, but I'm curious.

[eileenmcnaughton]

@MegaphoneJon yeah my worry is that it was retrofitted onto code that wasn't written for it & there are just so many places that it feels risky (& in fact we've seen a few bugs on it)

On the bright side - the test server will enforce FGB mode - so as long as we get some tests here we'll have that covered

[jaapjansma]

Test this please

[jaapjansma]

Betty and I have reviewed this PR.

• General standards
  • Explain
    • PASS : The goal/problem/solution have been adequately explained in the PR and on Gitlab
  • User impact
    • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature
  • Documentation
    • PASS: The changes do not require documentation.
  • Run it
    • PASS: Tested the issue in dmaster first. The issue occurred there: the primary address was NOT shown in the mailing labels, as it should have.
      We then tested in the test environment with the PR, and the issue was now solved: the primary address was always shown on the mailing label.
• Developer standards
  • Technical impact (r-tech)
    • PASS: The change preserves compatibility with existing callers/code/downstream.
  • Code quality (r-code)
    • PASS: The functionality, purpose, and style of the code seems clear+sensible.
  • Maintainability (r-maint)
    • PASS: The change is trivial enough that it does not require tests. However when a test was provided it would have been even more wonderful.
  • Test results (r-test)
    • PASS automated tests are passing

@mattwire or @eileenmcnaughton our advice is to merge this, is one of you able to do this?

[mattwire]

I'm going to merge this based on @jaapjansma review. It would be really good to get a test on this but the scope of the change is really small (a single form) and we are not changing anything about how the apiQuery function works. So the risk of breakage (other than on the form) is also really small