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/core#890 - Multiple line item shown on view contribution if parti… #16956

Merged
merged 1 commit into from
Jun 27, 2020

Conversation

jitendrapurohit
Copy link
Contributor

…cipant is transferred to another contact

Overview

Reopened #14104 with some changes to financial item entries as per #14104 (comment) (Solution 1 proposal).

Multiple line item shown on view contribution if participant is transferred to another contact

Before

To replicate -

  • Create a priceset for an event with Text / Numeric Quantity field type. Amount = $10.
  • Register contact A to the event using the above price option.
  • Transfer the event participation to another contact B.
  • View contribution on the main contact, it displays double line items for the participation.

image

I think the older participant status is set to "transferred" so it should remove the contribution link from the line item table?

After

Line item is displayed correctly.

image

Comments

Gitlab - https://lab.civicrm.org/dev/core/issues/890

@civibot
Copy link

civibot bot commented Apr 2, 2020

(Standard links)

@eileenmcnaughton
Copy link
Contributor

@jitendrapurohit the test fail relates

@jitendrapurohit
Copy link
Contributor Author

Have updated the PR with the test fix @eileenmcnaughton

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Apr 4, 2020
This is the extraction portion of civicrm#16956

On testing that I hit  an error but getting the extraction merged first should make it easierr
to review future iterations as it  will reduce noise in the code.

Note I made the  function non-static since it is still on the form & passed in
event ID since we have it but it's otherwise pretty much the same as the
extraction part of that PR
@eileenmcnaughton
Copy link
Contributor

@jitendrapurohit I tested this with a different field type and got an error :-(

Screen Shot 2020-04-04 at 2 25 57 PM

Since I worked through the code change I pulled out the extraction into a separate PR to get that merged & make it easier to work through this - #16976

@jitendrapurohit jitendrapurohit force-pushed the core-890 branch 2 times, most recently from 75d6311 to 14ebbd7 Compare April 9, 2020 11:16
@jitendrapurohit
Copy link
Contributor Author

@eileenmcnaughton Have updated the PR and fixed the test. I tried to replicate the above db error but wasn't able to see any problems with different combinations of fields.

image

From the backtrace, it seems event id was not retrieved on your side. This PR also modifies its value so it should already be handled. Can you pls confirm if you still notice the above error?

Thanks.

@@ -247,8 +247,6 @@ public static function checkProfileComplete($fields, &$errors, $self) {
}
if (!$email && !(CRM_Utils_Array::value('first_name', $fields) &&
CRM_Utils_Array::value('last_name', $fields))) {
$defaults = $params = ['id' => $eventId];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unnecessary lines. $eventId is an undeclared variable.

*
* @throws \CRM_Core_Exception
* @throws \CiviCRM_API3_Exception
*/
public function transferParticipantRegistration($toContactID, $fromParticipantID, $eventID) {
$query = 'select role_id, source, fee_level, is_test, is_pay_later, fee_amount, discount_id, fee_currency,campaign_id, discount_amount from civicrm_participant where id = ' . $fromParticipantID;
protected function transferParticipantRegistration($toContactID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So in terms of where we are going with this code - I'm hoping it would be moved to the BAO at some point as a move() function & called from the api. I mention this because this change makes the function more rather than less tied to the form it is on - which is the opposite direction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool. I've updated the PR to keep the fromParticipantID param :)

@@ -522,7 +518,7 @@ public function transferParticipantRegistration($toContactID, $fromParticipantID
$value_to['fee_amount'] = $dao->fee_amount;
}
$value_to['contact_id'] = $toContactID;
$value_to['event_id'] = $eventID;
$value_to['event_id'] = $this->_event_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

given the above query we should probably just get event_id in the query

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. Have updated with the api4 call and created the toParticipant with the retrieved params 👍

@eileenmcnaughton
Copy link
Contributor

@jitendrapurohit OK - the fatal errors occur when there is no associated contribution

@jaapjansma
Copy link
Contributor

test this please

@jaapjansma
Copy link
Contributor

@jitendrapurohit can you have a look at the failing test?

@jitendrapurohit
Copy link
Contributor Author

Hmm, it looks like the details page does not display any specific test which is failing.

From the console output https://test.civicrm.org/job/CiviCRM-Core-PR/33503/console, It seems the failure is resulted during the execution of EventTest::testTemplateFilterByDefault function?

not ok 1 - Error: EventTest::testTemplateFilterByDefault

API_Exception: Unknown api parameter: getHaving in /home/jenkins/bknix-dfl/build/core-16956-mxxj/web/sites/all/modules/civicrm/Civi/Api4/Generic/AbstractAction.php on line 215

I tried executing the same on my local and it displays a success message on completion.

jitendra$ phpunit5 tests/phpunit/api/v4/Action/EventTest.php           
Parsing schema description /Users/jitendra/src/civicrm/xml/schema/Schema.xml
Extracting database information
Extracting table information
PHPUnit 5.7.27 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)

Time: 1.18 seconds, Memory: 28.00MB

OK (1 test, 1 assertion)

@jitendrapurohit
Copy link
Contributor Author

retest this please

@jaapjansma
Copy link
Contributor

@jitendrapurohit are you able to fix the conflicting files?

@jaapjansma jaapjansma added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Jun 8, 2020
@jitendrapurohit
Copy link
Contributor Author

@jaapjansma Sorry for the delay. I've now updated the PR with the conflict fixes. Thanks.

@jitendrapurohit
Copy link
Contributor Author

@jaapjansma can you pls remove the needs work label from this PR? This was updated with the conflict fixes and also passes the jenkin test process.

@mattwire mattwire added ready for review and removed needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state labels Jun 24, 2020
@eileenmcnaughton
Copy link
Contributor

I re-tested this & it seems to work fine. Issues from previous comments have been addressed. Merging

@eileenmcnaughton eileenmcnaughton merged commit aef4c7b into civicrm:master Jun 27, 2020
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.

4 participants