dev/financial#109 Fix country/province assignation in the contribution invoice

[mlutfy]

Overview

To reproduce:

• Enable Taxes and Invoicing
• Edit the "Contribution Invoice Receipt" message template to include {$country} (other related contact tokens are called {$street_address}, {$email}, etc.
• Go to a contact, create a contribution, and from "view contribution", click "print PDF invoice" to view an invoice.

The country will be empty.

https://lab.civicrm.org/dev/financial/issues/109

Technical Details

• CRM_Contribute_Form_Task_Invoice: I did a bit of code cleanup because the invoice code did not make a ton of sense, and presumably CRM_Core_BAO_Address::getValues() has changed a fair amount since this invoice code was first written. Notably the loop for finding the billing address seemed over-complex.
• CRM_Contribute_Form_Task_Invoice also had an array for billingAddress, which hinted that there could be something more going on, but given that the variable is initialized inside the loop, I could not figure out its purpose.
• `CRM_Core_BAO_Address::getValues() was doing state/province lookup, but it was not storing it in the right variable, so it was not getting returned to the caller.

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

[mlutfy]

We can still have only a "primary" address. CRM_Core_BAO_Address::getValues() only inserts an order by clause when we specify is_billing = 1:

[seamuslee001]

@mlutfy which one is meant to be the abbreviation?

[mlutfy]

@seamuslee001 oops, good catch, auto-complete typo (fixed). I guess that would be a good use-case for a test?

[eileenmcnaughton]

@mlutfy I feel like this is stuck on not having a test

[BettyDolfing]

test this please

[jaapjansma]

Betty and I are reviewing PR's and we came across this one.

• General standards
  • Explain (r-explain)
    • PASS : The goal/problem/solution have been adequately explained in the PR and in the issue.
  • User impact (r-user)
    • PASS: The change would be intuitive.
  • Documentation (r-doc)
    • PASS: The changes do not require documentation.
  • Run it (r-run)
    • PASS: We tested it with a country other than the default country and it showed the country
    • COMMENT: When the country is the same as the default country and the setting Hide Country in Mailing Labels when same as domain country is enabled then the country is still shown. Not sure whether that is relevant for this PR.
• Developer standards
  • Technical impact (r-tech)
    • PASS: The change potentially affects compatibility, but the risks have been sufficiently managed.
  • 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. It even cleans up code about fetching the billing address well done!
  • Test results (r-test)
    • PASS: The test results are all-clear.

@eileenmcnaughton or @mattwire is one of you able to merge this PR?

[mlutfy]

I still need to add a test, alas. It's high on my list, and quickfix, hoping to do it soon.

[jaapjansma]

Our opinion is that this doesn't necessarily needs a test. But a test makes it a surplus

[eileenmcnaughton]

Test cover on everything to do with invoice code is awful - so I'm loathe to let anything be changed on the invoice code without tests.

Generally in an area like this with no tests I consider a 'no error' test to be enough - ie. a test that calls printPDF with no fatal errors - whether or not it tests the change being made. In this case it is progress on testing in that area

[magnolia61]

I can confirm that this patch enables the {$country} token in the invoice template

[mlutfy]

I added a test to check the province/abbreviation in getValues(), and I will add a test for the invoice itself in a separate PR, that will pass once #16569 is merged.

[eileenmcnaughton]

Nice, thanks @mlutfy

[magnolia61]

Thanks, I am using the {$country) token in the updated Contribution Invoice template (#16680)