Fix missing currency for event registration when configured via "quick config"

[mattwire]

Overview

This fixes a pretty obscure issue that can be triggered under the following conditions:

1. Even registration page configured with multiple payment processors and no default. (Eg. Stripe + pay later where Stripe is not default in CiviCRM).
2. 3 "non priceset" payment options configured under fees.

This triggers an AJAX call like this when you select a payment processor:

https://d7.local.mjw.pt/civicrm/payment/form?formName=Register&is_back_office=&pre_profile_id=12&processor_id=9&cid=202&payment_instrument_id=undefined&currency=undefined&snippet=json

which causes on the PHP side currency to be set to the string "undefined":

$form->getVar('currency') === 'undefined';

which means if you subsequently try to use that value as the currency via an API3 call it fails because "currency" is treated as a magic value in API3 and gets validated against the list of currencies in CiviCRM and throws an exception.

Confused yet?

Before

When retrieving the currency in some obscure situations it all goes wrong.

After

It does not all go wrong anymore.

Technical Details

Described above.

Comments

This is obviously obscure enough that no-one noticed that the API3 exception was also returning invalid data!

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

[seamuslee001]

@mattwire the 2nd and 3rd commits here look like they relate not sure about the 1st shouldn't that be in its own PR?

[mattwire]

Hi @seamuslee001 Thankyou I accidently pushed some local commits. Now cleaned up to just the one relevant commit.

[jaapjansma]

test this please

[jaapjansma]

test this please

[jaapjansma]

@mattwire we (@kainuk, @BettyDolfing and I) are doing PR reviews and we tried to review this PR. We could reproduce this on dmaster.demo.civicrm.org however we could not login into the test sites.

The admin section of civicrm suddenly logs us out.

We are not sure whether this caused by something in your pr or whether the test site is broken. Can you have a look at the test site? And check whether the problem is adequately solved or whether this caused by something on the test site. If the latter is the case we should probably involve @bgm.

[mattwire]

Hi @jaapjansma the test site seems to be working ok for me

[jaapjansma]

• General standards
  • Explain (r-explain)
    • PASS : The goal/problem/solution have been adequately explained in the PR.
  • User impact (r-user)
    • PASS: The change would be unnoticeable for a majority of users who work with this feature.
  • Documentation (r-doc)
    • PASS: The changes do not require documentation.
  • Run it (r-run)
    • PASS: I have tested this on dmaster.demo.civicrm.org and on the test site of this PR. In both cases I added another payment processor and set it to default. I set CIviCRM's default currency to EUR and then created an event with fees in the EUR currency. I checked the non default payment processor and the pay later option. I then checked the live registration page and opened firebug (f12 in firefox) and checked which url was send to the browser when selecting a payment method. On dmaster the url had a parameter currency=undefined on the test site of this pr the url had a parameter currency=EUR.
• 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.
  • Test results (r-test)
    • PASS: The test results are all-clear.

@demeritcowboy, @eileenmcnaughton or @colemanw can one of you merge this PR?

[demeritcowboy]

Thanks @jaapjansma