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-21133: Price set error with NULL financial types #10947

Merged
merged 2 commits into from
Sep 10, 2017

Conversation

monishdeb
Copy link
Member

@monishdeb monishdeb commented Sep 5, 2017

Overview

Steps to replicate bug:

  1. On a new site, go to the default rain forest event
  2. on amount tab convert to price set
  3. note that the price set is still not visible on manage event amount tab
  4. check civicrm_price_set.financial_type_id & see no valuel.

Before

Coverting a non-quick config price-options into Price Set under Event's fee section result redirects you to Manage price set page to edit the coverted Price set. But when you revisit the fee-section the price-set is no longer there. Underlying cause for this bug is price_set.financial_type is sets to NULL

After

After the fix the covereted price-set picks up the financial type of related event and now price-set field is visible under Event's Fee section


@seamuslee001
Copy link
Contributor

@eileenmcnaughton are you able to test this as the reporter on the issue?

WHERE cd.entity_id = (%1) AND cd.entity_table = 'civicrm_event' ";
INNER JOIN civicrm_event ce ON cd.entity_id = ce.id AND cd.entity_table = 'civicrm_event'
SET cps.is_quick_config = 0, cps.financial_type_id = IF(cps.financial_type_id IS NULL, ce.financial_type_id, cps.financial_type_id)
WHERE cd.entity_id = (%1) ";
Copy link
Contributor

Choose a reason for hiding this comment

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

It didn't work for me just now - I'm retrying with

WHERE cd.entity_id = %1

@eileenmcnaughton
Copy link
Contributor

@monishdeb the theory behind this is right - but the sql does not work as it is because it INNER JOINs the civicrm_discount table. I think maybe that sql change makes sense for the discount price set but it does not work when no early payment option exists

$sql = "UPDATE
civicrm_price_set cps
INNER JOIN civicrm_price_set_entity cpse ON cps.id = cpse.price_set_id
INNER JOIN {$context} ce ON cpse.entity_id = ce.id
Copy link
Member Author

Choose a reason for hiding this comment

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

we cannot use %1 here as the eventual query is translated as INNER JOIN 'civicrm_event', eventually causes DB error.

@monishdeb
Copy link
Member Author

@eileenmcnaughton can you please check now?

if ($context == 'civicrm_event') {
$sql = "UPDATE
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm nervous about removing this query - I think it would do something useful if an early bird discount is configured....

Copy link
Member Author

@monishdeb monishdeb Sep 7, 2017

Choose a reason for hiding this comment

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

And I was wondering if the discount table might be configured with a different price-set then event because as per UI it's not possible. And if not possible then it's safe as the because the update query does the same job as earlier. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@monishdeb I just tested now what happens before merging this change if you configure an early discount price set and then convert the price set.

The result is that 2 price sets now exist - one for the discount and that one has a correct financial type but the start & end dates are not carried over to the price_field table and since there are now 2 price sets only one can be assigned.

I guess it fails to work badly enough that I think I think it is OK to remove the attempt to convert the early bird discount rate. I think the CORRECT fix (but out of scope) would be to insert those rows into the main price set with the dates. However, there is some complexity then as the main price set fields would not be date dependent and would also show. Arguably that is a good thing (when I have done events with early bird pricing I usually make both know so people can see they are getting a discount).

Upshot is this does fix the problem, and it alters another problem that no-one has noticed. I'm going to merge it.

@eileenmcnaughton eileenmcnaughton merged commit 3afbd1f into civicrm:master Sep 10, 2017
@monishdeb monishdeb deleted the CRM-21133 branch September 11, 2017 05:54
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