CiviAccountSync Cancel Contributions is now too enthusiastic

[kenwest]

Since the PR for CIVIXERO-15 the CiviAccountSync Cancel Contributions job is too enthusiastic

1. It cancels contributions that are already cancelled
2. It fails trying to cancel deleted contributions

Cancelling already-cancelled contributions
The job should not do this. It's harmless while the data set is small but will kill the job on larger sets

Deleted contributions
The job uses the CiviCRM API for contribution/create which assumes that if you pass in the contribution Id then it will exist. But if the contribution was deleted in CiviCRM the job fails.

Proposal
Update the query as follows ...

    */
   public static function cancelContributionFromAccountsStatus($params) {
     //get pending registrations
     $sql = "SELECT  cas.contribution_id
       FROM civicrm_account_invoice cas
-       LEFT JOIN civicrm_contribution  civi ON cas.contribution_id = civi.id
+       JOIN civicrm_contribution  civi ON cas.contribution_id = civi.id
       WHERE accounts_status_id =3
+      AND civi.contribution_status_id <> 3
     ";
     $dao = CRM_Core_DAO::executeQuery($sql);
 

[kenwest]

You might consider this overkill but here's a bonus suggestion

Use a job parameter to select the Contributions on which to act
Add a job parameter "current_contribution_status" ...

current_contribution_status=!3
would select Contributions that are not Cancelled ie civi.contribution_status_id not in (3)

current_contribution_status=2
would reproduce the "classic" behaviour and select Contributions that are Pending ie civi.contribution_status_id in (2)

current_contribution_status=2,4,5
would translate to civi.contribution_status_id in (2,4,5)

[eileenmcnaughton]

@kenwest is that before or after this one - 5bae5ac

[kenwest]

@eileenmcnaughton it is after (note I have the WHERE keyword in my proposed change.)

[eileenmcnaughton]

@kenwest I think excluding cancelled statuses makes sense & I think an optional job param of contribution_status_id makes sense too- it should use the api formats which can be handled with

  $whereClause = CRM_Core_DAO::createSQLFilter('contribution_status_id', $params[''contribution_status_id'], CRM_Utils_Type::T_INT);

(e.g then IN, NOT IN etc are supported & if the spec function declares the contribution_status_id field with the 'pseudoconstant' field then it will support words like 'Cancelled'