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#40 Fix for non-allocation of payment to fully reversed checkboxes option #15740

Merged
merged 1 commit into from
Nov 6, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Nov 6, 2019

Overview

Fixes an issue whereby
civicrm_entity_financial_trxn records for the civicrm_financial_item table are not created when
refunding against line items with a quantity of zero - ie where there were selected checkboxes & they were reversed.

Before

  1. create an event price set with at least 2 fields - one is checkboxes
  2. Register a participant, selecting some of both (back office is fine)
  3. edit the registration and de-select the checkboxes (leaving the quantity as 0)
  4. add a refund payment against the contribution
    -> no entity_financial_trxn records are created linking the financial_items to the refund payment. The refund payment is thus missing on the bookkeeping report

After

entity_financial_trxn records created

Technical Details

https://lab.civicrm.org/dev/financial/issues/98

The reason this check was there relates to former logic - now we have a calculated allocation
we don't need this check.

I want to do a test but need to do a bit of work to get it set up & I think this should hit 5.20
as it relates to an issue identified by @kcristiano in reviewing patches merged to that version

Comments

This addresses an issue partially identified in https://lab.civicrm.org/dev/financial/issues/98 whereby
civicrm_entity_financial_trxn records for the civicrm_financial_item table are not created when
refunding against line items with a quantity of zero  - ie where there were selected checkboxes & they
were reversed.

The reason this check was there relates to former logic - now we have a calculated allocation
we don't need this check.

I want to do a test but need to do a bit of work to get it set up & I think this should hit 5.20
as it relates to an issue identified by Kevin in reviewing patches merged to that version
@civibot
Copy link

civibot bot commented Nov 6, 2019

(Standard links)

@civibot civibot bot added the master label Nov 6, 2019
@kcristiano
Copy link
Member

@eileenmcnaughton

I ran this through the same r-run as I detailed here: #15705 (comment)

I used my client databse filled with checkboxes and all sorts of oddities

Contributions look good:

image

Bookkeeping report ties out - all DR and CR

image

Database Tables look good

Contributions:

select c.id,c.contact_id,c.total_amount,c.fee_amount,c.net_amount,c.trxn_id,c.receive_date,c.contribution_status_id,cov.name 
from civicrm_contribution c 
join civicrm_option_value cov on cov.value = c.contribution_status_id and cov.option_group_id = 11 and cov.value = c.contribution_status_id where c.id = 16096;
+-------+--------------+----------------+--------------+--------------+-----------+---------------------+--------------------------+-----------+
|    id |   contact_id |   total_amount |   fee_amount |   net_amount |   trxn_id | receive_date        |   contribution_status_id | name      |
|-------+--------------+----------------+--------------+--------------+-----------+---------------------+--------------------------+-----------|
| 16096 |        39328 |            975 |            0 |          975 |    <null> | 2019-11-06 14:44:00 |                        1 | Completed |
+-------+--------------+----------------+--------------+--------------+-----------+---------------------+--------------------------+-----------+
1 row in set
Time: 0.007s

Line Items:

select li.id,li.entity_table,li.entity_id,li.contribution_id,li.label,li.line_total,li.financial_type_id,ft.name 
from civicrm_line_item li join civicrm_financial_type ft on ft.id = li.financial_type_id where li.contribution_id = 16096
+--------+---------------------+-------------+-------------------+------------------------------------+--------------+---------------------+-------------------------------------+
|     id | entity_table        |   entity_id |   contribution_id | label                              |   line_total |   financial_type_id | name                                |
|--------+---------------------+-------------+-------------------+------------------------------------+--------------+---------------------+-------------------------------------|
| 166277 | civicrm_participant |        4724 |             16096 | Regular Member                     |          820 |                  29 | Annual Meeting - Registration       |
| 166278 | civicrm_participant |        4724 |             16096 | Companion 1                        |          100 |                  31 | Annual Meeting - Guest Registration |
| 166279 | civicrm_participant |        4724 |             16096 | Guest 1 Lunch on Wednesday         |            0 |                  51 | Annual Meeting - Meals and Breaks   |
| 166280 | civicrm_participant |        4724 |             16096 | Guest 1 Lunch on Thursday          |            0 |                  51 | Annual Meeting - Meals and Breaks   |
| 166281 | civicrm_participant |        4724 |             16096 | Guest 1 Lunch on Friday            |            0 |                  51 | Annual Meeting - Meals and Breaks   |
| 166282 | civicrm_participant |        4724 |             16096 | Yes I'd like to purchase SRL 90.2B |           35 |                  44 | SRL Back Issue Sales                |
| 166283 | civicrm_participant |        4724 |             16096 | Donate to Student Travel Fund      |           20 |                  16 | Contribution - Student Travel       |
+--------+---------------------+-------------+-------------------+------------------------------------+--------------+---------------------+-------------------------------------+
7 rows in set
Time: 0.012s

Financial Transactions

select cft.id,cft.from_financial_account_id, cft.to_financial_account_id,cft.trxn_date,cft.trxn_id, cft.total_amount,cft.status_id 
from civicrm_financial_trxn cft 
where cft.trxn_date >= '2019-11-06'
+-------+-----------------------------+---------------------------+---------------------+-----------+----------------+-------------+
|    id |   from_financial_account_id |   to_financial_account_id | trxn_date           |   trxn_id |   total_amount |   status_id |
|-------+-----------------------------+---------------------------+---------------------+-----------+----------------+-------------|
| 18126 |                      <null> |                         6 | 2019-11-06 14:44:00 |    <null> |           1200 |           1 |
| 18127 |                      <null> |                         7 | 2019-11-06 14:45:42 |    <null> |           -225 |           1 |
| 18128 |                           7 |                         6 | 2019-11-06 14:46:00 |    <null> |           -225 |           7 |
+-------+-----------------------------+---------------------------+---------------------+-----------+----------------+-------------+
3 rows in set
select eft.* from civicrm_entity_financial_trxn eft
join civicrm_financial_trxn cft on cft.id = eft.financial_trxn_id and cft.trxn_date >= '2019-11-06' 
order by eft.financial_trxn_id
+-------+------------------------+-------------+---------------------+----------+
|    id | entity_table           |   entity_id |   financial_trxn_id |   amount |
|-------+------------------------+-------------+---------------------+----------|
| 77070 | civicrm_contribution   |       16096 |               18126 |     1200 |
| 77076 | civicrm_financial_item |       57935 |               18126 |       35 |
| 77075 | civicrm_financial_item |       57934 |               18126 |       75 |
| 77074 | civicrm_financial_item |       57933 |               18126 |       75 |
| 77073 | civicrm_financial_item |       57932 |               18126 |       75 |
| 77072 | civicrm_financial_item |       57931 |               18126 |      100 |
| 77071 | civicrm_financial_item |       57930 |               18126 |      820 |
| 77077 | civicrm_financial_item |       57936 |               18126 |       20 |
| 77081 | civicrm_financial_item |       57939 |               18127 |      -75 |
| 77080 | civicrm_financial_item |       57938 |               18127 |      -75 |
| 77079 | civicrm_financial_item |       57937 |               18127 |      -75 |
| 77078 | civicrm_contribution   |       16096 |               18127 |     -225 |
| 77083 | civicrm_financial_item |       57937 |               18128 |      -75 |
| 77082 | civicrm_contribution   |       16096 |               18128 |     -225 |
| 77085 | civicrm_financial_item |       57939 |               18128 |      -75 |
| 77084 | civicrm_financial_item |       57938 |               18128 |      -75 |
+-------+------------------------+-------------+---------------------+----------+

I think this is good to merge. We'll do more testing in 5.20 RC to shake out any issues, but this looks great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants