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

[ready-for-core-team-review]CRM-16189, Code to add 'Record Payment' link on Contribution View and Contribution tab #8870

Merged
merged 15 commits into from
Dec 27, 2016

Conversation

pradpnayak
Copy link
Contributor

@pradpnayak pradpnayak commented Aug 11, 2016

@pradpnayak pradpnayak changed the title CRM-16189, Code to add 'Record Payment' link on Contribution View and Contribution tab [ready-for-core-team-review]CRM-16189, Code to add 'Record Payment' link on Contribution View and Contribution tab Aug 11, 2016
@jitendrapurohit
Copy link
Contributor

jitendrapurohit commented Aug 12, 2016

While working on it for QA, just found a DB error on Edit Participant Form :(.

Steps to recreate-

  1. Add Event Registration from a Contact Summary Page -> Save.
  2. Click on Edit Link from the Events tab.

Database Error Code: Not unique table/alias: 'civicrm_participant_payment', 1066

$values['id'], 'participant_id', 'contribution_id'
);
$this->assign('contactId', $values['contact_id']);
$this->assign('hasPayment', $values['id']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A variable named hasPayment should use a boolean like isset($values['id']) rather than a numeric or NULL.

Just because a contribution is returned, I don't think that is equivalent to there being a payment already. Could you elaborate with a comment somewhere the reasons why this would be so?

@pradpnayak
Copy link
Contributor Author

@monishdeb can you verify this PR?

@monishdeb
Copy link
Member

sure

@monishdeb
Copy link
Member

monishdeb commented Sep 21, 2016

@pradpnayak there is unit test failure related to this PR

CRM_Contribute_BAO_ContributionTest::testIsPaymentFlag
Mismatch count for is payment flag.
Failed asserting that 1 matches expected 2.

/home/jenkins/buildkit/build/core-8870-7fl1d/sites/all/modules/civicrm/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php:502

@pradpnayak
Copy link
Contributor Author

is test failure related to my PR?

@monishdeb
Copy link
Member

Nope, CRM_Contribute_Form_ContributionTest::testSubmitCreditCardPayPal not a related test failure and there's already a fix for it #9041

@jitendrapurohit
Copy link
Contributor

jitendrapurohit commented Sep 26, 2016

@pradpnayak I did a general click around testing which worked for almost all the cases for events, contribution, etc. except for some minor use-case which I've listed below:

  1. The "Record Refund" link seems to be missing a required param id. Steps to replicate -
  • Register a participant to an Event selecting an amount of say ($1000) -> Save.
  • Edit Participation -> Perform Change Selection and select a lower amount for registration Eg - $800. (Contribution status gets changed to Pending Refund).
  • Navigate to Contribution Tab. Record Refund from more link hold a value of %%pid%% for id param. Clicking on it will lead to a fatal error. However, Record Refund button displayed on View Contribution Page has a correct link associated with it.
  1. Completing a Payment from Contribution Record Payment link does not update Event registration status. Steps -
  • Register a participant to an Event selecting an amount of say ($1000).
  • Enter a lower amount(Eg. $800) in the Amount textbox under Payment Information section -> Save. (Participant status is saved as Partially Paid).
  • Navigate to Contribution Tab. Record Payment for the balance amount. (Contribution status gets changed to Completed.)
  • Participant status remains unaffected instead of updating itself to Registered. Balance amount is correctly updated to $0. Screenshot.
  1. Record Refund for a new Offline Contribution doesn't update the contribution status.
  • Create a new Offline Contribution for a contact with status Pending Refund -> Save.
  • View Contribution -> Click Record Refund button. -> Save. Though a success message is displayed The payment record has been processed, but contribution status does not update itself.

Thanks.

@yashodha yashodha added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Sep 28, 2016
@yashodha
Copy link
Contributor

@pradpnayak : Let us know when done.

@Edzelopez
Copy link
Contributor

Rebased PR

@Edzelopez
Copy link
Contributor

Jenkins, retest this please

@Edzelopez
Copy link
Contributor

@jitendrapurohit could you please test the scenario again? I've tested after the latest changes and it looks like it has been resolved.

Thanks!

@jitendrapurohit
Copy link
Contributor

@Edzelopez # 1 seems to be fixed as it shows correct pid instead of %%pid%% :-) though I think # 2 and # 3 still need to be addressed ?

@Edzelopez
Copy link
Contributor

Edzelopez commented Dec 16, 2016

@jitendrapurohit

Point. 2 looks fine to me
Screenshot

For point. 3 I don't think a scenario should arise to prompt one to create a Pending Refund contribution directly. Currently, core has only handled status change for Pending Refund contributions. @JoeMurray could you think of any case where one would need to create a Pending Refund contribution? If not, we should likely disallow the user to do so.

@jitendrapurohit
Copy link
Contributor

Point 2 still fails to update participant status for me :(. Note that the contribution Record Payment link is the one which does not update the Participant status. It works fine on Record Payment link from Edit Participation Form.

I think both forms are meant for the same functionality but have a different display and params -

Edit Event Participation Form -> Record Payment
URL - civicrm/payment?action=add&reset=1&component=event&id=56&cid=202

Contribution Form -> Record Payment
URL - civicrm/payment?reset=1&id=100&cid=202&action=add&component=contribution

Should this be included/fixed in the PR?

@Edzelopez
Copy link
Contributor

Edzelopez commented Dec 19, 2016

@jitendrapurohit Thanks for specifying the issue in detail. I can replicate it now. Will submit a fix soon.

EDIT: Fix submitted. @jitendrapurohit could you please let me know if the issue is resolved?

pradpnayak and others added 15 commits December 19, 2016 14:58
----------------------------------------
* CRM-16189: Improve support for Accrual Method bookkeeping
  https://issues.civicrm.org/jira/browse/CRM-16189
----------------------------------------
* CRM-16189: Improve support for Accrual Method bookkeeping
  https://issues.civicrm.org/jira/browse/CRM-16189
----------------------------------------
* CRM-16189: Improve support for Accrual Method bookkeeping
  https://issues.civicrm.org/jira/browse/CRM-16189
----------------------------------------
* CRM-16189: Improve support for Accrual Method bookkeeping
  https://issues.civicrm.org/jira/browse/CRM-16189
…r deferred payments

----------------------------------------
* CRM-16189: Improve support for Accrual Method bookkeeping
  https://issues.civicrm.org/jira/browse/CRM-16189

Conflicts:

	CRM/Core/BAO/FinancialTrxn.php
----------------------------------------
* CRM-16189: Improve support for Accrual Method bookkeeping
  https://issues.civicrm.org/jira/browse/CRM-16189
----------------------------------------
* CRM-16189: Improve support for Accrual Method bookkeeping
  https://issues.civicrm.org/jira/browse/CRM-16189
… pending pay later and partially paid contribution

----------------------------------------
* CRM-16189: Improve support for Accrual Method bookkeeping
  https://issues.civicrm.org/jira/browse/CRM-16189

Conflicts:
	CRM/Contribute/Form/AdditionalPayment.php
----------------------------------------
* CRM-16189:
  https://issues.civicrm.org/jira/browse/CRM-16189

Conflicts:
	CRM/Contribute/Form/AdditionalPayment.php
----------------------------------------
* CRM-16189: Improve support for Accrual Method bookkeeping
  https://issues.civicrm.org/jira/browse/CRM-16189
----------------------------------------
* CRM-16189: Improve support for Accrual Method bookkeeping
  https://issues.civicrm.org/jira/browse/CRM-16189
----------------------------------------
* CRM-16189: Improve support for Accrual Method bookkeeping
  https://issues.civicrm.org/jira/browse/CRM-16189
----------------------------------------
* CRM-16189: Improve support for Accrual Method bookkeeping
  https://issues.civicrm.org/jira/browse/CRM-16189
----------------------------------------
* CRM-16189: Improve support for Accrual Method bookkeeping
  https://issues.civicrm.org/jira/browse/CRM-16189
@Edzelopez
Copy link
Contributor

@jitendrapurohit any news on this?

Copy link
Contributor

@jitendrapurohit jitendrapurohit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All noted issues seems to be fixed. Thanks :-)

@monishdeb monishdeb removed the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Dec 27, 2016
@monishdeb monishdeb merged commit fab6656 into civicrm:master Dec 27, 2016
@pradpnayak
Copy link
Contributor Author

Thanks :)

@pradpnayak pradpnayak deleted the CRM-16189-10 branch February 22, 2017 11:06
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.

8 participants