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

CRM-20297 Display contribution having line items with no price set #10012

Merged
merged 4 commits into from
May 3, 2017

Conversation

agh1
Copy link
Contributor

@agh1 agh1 commented Mar 17, 2017

And while I was at it, I fixed a bunch of hyphens that were being misused as en dashes.

Basically, the API will let you create line items that don't relate to a price field, but the page was assuming that all of them had one. An unwrapped civicrm_api3() call was throwing an exception when viewing the contribution.


agh1 added 4 commits March 17, 2017 15:44
----------------------------------------
* CRM-20297: Fatal error viewing a contribution having a line item with no price field
  https://issues.civicrm.org/jira/browse/CRM-20297
----------------------------------------
* CRM-20297: Fatal error viewing a contribution having a line item with no price field
  https://issues.civicrm.org/jira/browse/CRM-20297
@seamuslee001
Copy link
Contributor

@KarinG @JoeMurray Hey Folks, Can you take a look at this and see if Andrew's heading down the right path for a fix or if we should be trying to fix the API.

@agh1
Copy link
Contributor Author

agh1 commented Mar 17, 2017

For context, this all started because our client has an external site where some contributions are being made. It makes sense that you wouldn't want to force people to maintain price fields/options in CiviCRM to correspond with everything the other site might generate. On principle, the financial type should be sufficient.

@eileenmcnaughton
Copy link
Contributor

I think this is OK - this is one of the few places where I think we ARE using is_quick_config correctly - ie. it is at the display level and quick_config should be a display setting.

I feel happy with the code approach here @seamuslee001 did you check if it still displays correctly?

@KarinG
Copy link
Contributor

KarinG commented Mar 18, 2017

Agree - can't think of any reason right now to require that all lineItems must have a priceSet associated with them. After your fix on View Contribution will 'Item' -> be blank? I think that's fine.

@eileenmcnaughton
Copy link
Contributor

Yeah, It's not that I think it is necessarily valid to not have a line item, but that I think the fix is more correct that what is in place, regardless of that issue

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.

Since we were too facing this error while paying via the webform, I've checked the changes and confirmed that it fixes the issue on view page. Also checked the display of dashes on few pages - works well. Thanks!

@adixon
Copy link
Contributor

adixon commented Apr 7, 2017

Just a note that this will partially fix the issue I've reported here:
https://www.drupal.org/node/2840851
which I assume is similar to jitendrapurohit's error.

But, my conclusion from that exercise was that the api needs better tools for generating contributions based on price fields.

When I note that it's a partial fix - the bit it won't fix is that the membership line item's price field value being generated was actually wrong, not that it was missing (it doesn't always get it wrong, but it does sometimes).

But at least it won't cause a fatal error, which is nicer.

@eileenmcnaughton eileenmcnaughton merged commit e2420e8 into civicrm:master May 3, 2017
@eileenmcnaughton
Copy link
Contributor

Merging based on @jitendrapurohit's review & @adixon review

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.

7 participants