catch exception to allow check to complete

[michaelmcandrew]

Running cv api system.check on a site that was newly upgraded to gocardless 1.9 I got the following output:

Array
(
    [is_error] => 1
    [error_message] => Expected one OptionValue but found 0
)

Catching the exception fixes the issue for me and I now see "Missing Financial Account for GoCardless" on my status screen.

My might try an be more specific and replace Exception with CiviCRM_API3_Exception. What do you think? Happy to be guided by you on that.

[artfulrobot]

Thanks, @michaelmcandrew. I think the original code checked for a financial account; your PR seems to only check for the existence of direct_debit_gc?

That option is assumed to exist, it's created here, so if it's not there, then presumably the upgrader hasn't run somehow?

So:

• great idea to catch the api exception
• if you do catch it, it's a problem we haven't previously had a check for, but we could do as a way to let people know their system is broke (although it's probably them that broke it!)
• it still needs to check for financial account.

[michaelmcandrew]

Ah, thanks for the clarification. Sorry - I should have read the name of the function! CRM_Contribute_PseudoConstant::getRelationalFinancialAccount()). Let me do some more investigation and get back to you...

[michaelmcandrew]

Did a little bit more investigation. The upgrade failed at step 0002.

There was a helpful message in the log which I found after looking at https://github.com/artfulrobot/uk.artfulrobot.civicrm.gocardless/blob/master/CRM/GoCardless/Upgrader.php#L236 but neither the output of cv ext:upgrade-db or the UI extension upgrade runner mentioned this message.

I'm not very familiar with the upgrader interface but the docblock for it says

   * @return TRUE on success
   * @throws Exception
   */

I wonder if it might be a better user experience to throw an Exception instead of returning false.

--- a/src/extensions/uk.artfulrobot.civicrm.gocardless/CRM/GoCardless/Upgrader.php
+++ b/src/extensions/uk.artfulrobot.civicrm.gocardless/CRM/GoCardless/Upgrader.php
@@ -226,8 +226,9 @@ class CRM_GoCardless_Upgrader extends CRM_GoCardless_Upgrader_Base {
         'sequential' => 1,
       ]);
      if (!$payment_instrument['count'] == 1) {
-       Civi::log()->error("GoCardless upgrade_0002 expected to find a payment instrument with name direct_debit_gc but found none. Cannot perform upgrade step.");
-       return FALSE;
+       $message = "GoCardless upgrade_0002 expected to find a payment instrument with name direct_debit_gc but found none. Cannot perform upgrade step.";
+       Civi::log()->error($message);
+       throw Exception($message)
      }
      $payment_instrument_id = $payment_instrument['values'][0]['value'];

[artfulrobot]

Does throwing an exception generate a user facing message then in cli/web updater?

Also looks like we should be using $this->ctx->log not Civi::log()?

[mattwire]

@michaelmcandrew Looks like this is waiting for your response?

[michaelmcandrew]

Does throwing an exception generate a user facing message then in cli/web updater?

I don't know - that is what I was wondering :)

[michaelmcandrew]

Having looked into this further, the payment processor config screen has a field to allow you to set the payment method. @artfulrobot - should the gocardless_civicrm_check (and the extension more generally) use that rather than hard code direct_debit_gc?

[artfulrobot]

@michaelmcandrew I'm not sure what the use-case for that is? It's not hard to make it optional, though it needs to be there as a default.

Do you want to fix this PR and extend it to cover that case, or close it? (you could open an issue instead if you want)

[michaelmcandrew]

This client had a direct debit payment provider before go cardless and they wanted to use the same payment method, I think. In any case we have worked around it now. I was posting here in case it was useful / for information. It's not crucial now and I don't think I will find any more time to work on it, so might make most sense just to close this now. Your call.

[artfulrobot]

@michaelmcandrew yeah, I think it's an edge case really. Since this is a PR that doesn't do what it said, and the problem diagnosis has evolved, I'll close it. Feel free to open another or an issue etc.

[michaelmcandrew]

sounds good - thanks for the engagement and help.