Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev/financial#6 Fix creating of template contribution when it has custom data #21470

Merged
merged 2 commits into from
Mar 14, 2022

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Sep 14, 2021

Overview

https://lab.civicrm.org/dev/financial/-/issues/6


UPDATE: The original issue seems to be fixed between 5.44-5.45 - CiviCRM no longer crashes with a "data already exists" DB error. BUT it still doesn't do the right thing because any hook that tries to insert custom data on create gets ignored and the original values are used instead.
I've updated the test to demonstrate that a hook can insert custom data on a "Contribution.create" and the inserted data will end up in the database and not the copied custom data from the original.

It is good practise to insert entity+custom data in the same API call and this PR changes that to be the case, also uses API4 for a bit more modern-ness.


If there is an extension installed that automatically calculates custom data for a contribution on "create" this can cause CiviCRM to crash because:

Before

  1. API3 Contribution.create runs and creates the new contribution (without custom data being copied) and triggers hooks.
  2. Hooks fire (in this case ukgiftaid extension calculates and inserts it's custom data for new contribution).
  3. $contribution->copyCustomFields() runs which then triggers a database exception because the data already exists.

After

  1. API4 Contribution.create runs and creates the new contribution with copied custom data and triggers hooks.
  2. Hooks fire (in this case ukgiftaid extension calculates and inserts it's custom data for new contribution).

Technical Details

Explained above.

Comments

@jaapjansma Are you able to review this one? It is a bug with the template contribution create.

@civibot
Copy link

civibot bot commented Sep 14, 2021

(Standard links)

@civibot civibot bot added the master label Sep 14, 2021
@mlutfy mlutfy changed the title Fix creating of template contribution when it has custom data dev/financial#6 Fix creating of template contribution when it has custom data Sep 18, 2021
@mattwire mattwire force-pushed the templatecontributioncreate branch from 73d6979 to b0ccbba Compare January 28, 2022 23:59
@jaapjansma
Copy link
Contributor

test this please

1 similar comment
@BettyDolfing
Copy link

test this please

@jaapjansma
Copy link
Contributor

  • 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 intuitive or 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: We tested this by installing Gift Aid extension, Form Processor in which we setup the repeating of a recurring contribution. We also created a manual payment processor type and we then created a contribution page with the gift aid profile and recurring contribution. We then created a new recurring contribution and repeated it through the form processor and checked for errors and whether the custom data has been repeated.
  • 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 does not sufficiently improve test coverage, but special circumstances make it important to accept the change anyway.
    • Test results (r-test)
      • PASS: The test results are all-clear.

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

@jaapjansma jaapjansma added the merge ready PR will be merged after a few days if there are no objections label Mar 14, 2022
@eileenmcnaughton eileenmcnaughton merged commit 18e8018 into civicrm:master Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-test master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants