CRM-13231 Incremental upgrade 4.3.4 fails if no COGS and EXP default account set

Pradeep, I'm thinking that maybe we should allow people to not have Expense accounts or Cost of Sales accounts. Can you confirm that there is appropriate handling of financial_types that don't have relations to these types of accounts? This would mean, for example, that the code would gracefully handle Premiums in their absence. Check if there are other places where these types of accounts are used.

After that, could you change the upgrade code appropriately? Thanks, Joe

1. Joe recommended to create a fatal error and roll back contribution transaction when a contribution has fee_amount and not Expense account relationship for FT at the time of payment processor callback. Failing a completed transaction seems bit odd to me.

2. I have created a form rule for backoffice contribution to check if selected FT has Expense account is and fee is recorded and also while adding/updating FT and payment processor for contribution page.

An alternative approach for #1 will be to recreate default EXP and COGS i.e Premiums and Banking Fees accounts if not exists during upgrade and creating there account relationships.

Kurund any thoughts?

I kind of agree with Pradeep, handling #1 during upgrade. Joe any reasons for not using this approach..

Dave, what is your recommendation here? The problem is that the accounting info exported will be erroneous unless fixed, so this does seem like a business error.

I agree that we should fix the upgrades to:

• create default COGS and EXPENSE accounts if they aren't found
• create the corresponding COGS and EXPENSE transactions for all 'not yet exported to a batch' contributions, so that any subsequent exports will be balanced.

Another question: do you know of any payment processors that are filling in bank fees on IPN pingback? We should try to get them to properly create the FT for expenses, and then we should start raising errors, or at least logging them, if the financial type doesn't have the COGS and EXP accounts needed. My worry is that we have no user to alert at the time of the pingback, and logs are typically not reviewed regularly. Thoughts?

If we ensure in upgrade that there are default EXP and COGS accounts, then I think it's sufficient to write an error to the log if folks have subsequently removed or disabled them. We could also not allow user to delete / disable a COGS or EXP account if it's the only one of that type (or give them a big warning about "Unbalanced transactions may be created if you disable or delete this account."

Regarding IPN and fees, I don't know the answer to that. Maybe create a separate issue to investigate what the code is currently doing (BaseIPN ?).

Meanwhile, I'm transferring back to Pradeep to get the upgrade fixes in place.

Unfortunately it is a bit more complicated. We'd have to by default create COGS and EXP accounts, and by default add them to every FT. I think that would be the safest approach, but I don't want to put lots of resources into saving people from themselves on edge cases.

fixed upgrade issue, created default COGS and EXP account if arent present, added form rule to check if financial type has expense account configured when recording fees and also payment processor. Added warning message at the time of deleting financial type account relationship. Submitted PR at https://github.com/civicrm/civicrm-core/pull/1663

I've tested an upgrade from 4.3.5 where I deleted the cost of sales account relationships and default cost of sales financial account. I had to do this via phpMyAdmin because the UI wouldn't let me do it (even w/o these patches). Not sure how the original reporter got their DB in the condition reported (i.e. no expense accounts). Things worked as expected. It would be best if we could get the original reporter to run these patches against his DB. However, they don't seem to have broken anything in the upgrade so I merged the PR.

Note that the PR does NOT create a default COGS account if not present (despite the previous comment). It only throws the postUpgrade warning if a financial type w/ a cost of sales account relationship isn't linked to a contribution_product. I think this was a late decision about how to proceed, but just verifying here ???

I've applied your patch to CRM/Upgrade/Incremental/php/FourThree.php to give the warning on missing financial_type for products - updated to include a reasonable message w/ link to a stub wiki page.

Remaining steps:

• draft in the instructions / script attachment at:
  http://wiki.civicrm.org/confluence/display/CRMDOC/Fixing+Issues+Caused+by+Missing+Cost+of+Goods+Account+-+4.3+Upgrades

• review and merge the PR at
  https://github.com/civicrm/civicrm-core/pull/1665

• Verify that the intent was NOT to create a COGS if not present.

Once this is done, you can close the issue.

> Not sure how the original reporter got their DB in the condition reported (i.e. no expense accounts).

I can't remember precisely, but I mostly did some imports via the API (v3). I never manipulated the database directly.

Added script in 4.3.6 to add default COG account if not present and submitted PR at https://github.com/civicrm/civicrm-core/pull/1666.

I have verified PR https://github.com/civicrm/civicrm-core/pull/1665, its working for me.

Steps to create financial transaction for COGS:

1. Add a financial type for product and also for individual premium products associated with contribution pages. NB. The financial type should have both, "Cost of Sales Account is" and "Premium Inventory Account is" relationship types.

2. Import the script file attached to this issue.

All financial transactions are created as per the spec mentioned in http://wiki.civicrm.org/confluence/display/CRM/CiviAccounts+4.3+Data+Flow#CiviAccounts4.3DataFlow-AddPREMIUMtoonlineorofflinecontribution

Pradeep, please try creating an entry like the erroneous one via the API to see if you can reproduce error. Then add error checking into BAO called by API to ensure the bad data cannot be created via API.

I tried replicating the issue via the API, but there is no reference of create or edit functions related to financial type.

Dgg tried upgrading from 4.3.5 to 4.3.6 and deletion of expense account and COGS has been handled in 4.3.4. However, Björn Endres had tried upgrading from 4.3.1 to 4.3.5. He must have changed/deleted the default financial type for COGS and EXP in 4.3.1. Hence the break in 4.3.4.

Form rule bug fix was critical (couldn't save a premium product otherwise.
I ran the upgrade against a 4.3.1 DB (sample data), and also ran the script.sql attached to the new wiki page:
http://wiki.civicrm.org/confluence/display/CRMDOC/Fixing+Issues+Caused+by+Missing+Cost+of+Goods+Account+-+4.3+Upgrades

No errors, no unexpected results.