-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
Conversation
@eileenmcnaughton are you able to test this as the reporter on the issue? |
CRM/Core/Page/AJAX.php
Outdated
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) "; |
There was a problem hiding this comment.
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
@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 |
CRM/Core/Page/AJAX.php
Outdated
$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 |
There was a problem hiding this comment.
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.
@eileenmcnaughton can you please check now? |
if ($context == 'civicrm_event') { | ||
$sql = "UPDATE |
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Overview
Steps to replicate bug:
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