dev/core#4248 - Fix missing price-set usage table #26090
Conversation
|
No issue was found matching the number given in the pull request title. Please check the issue number. |
|
(Standard links)
|
|
Have reviewed. Code looks good, makes sense and is more readable. Confirmed issue, confirmed fix. One issue, however, is that the users may see an empty info box when a price set is not used for anything:
In my case, $usedBy was not empty because the price set was used by a participant that was registered for the event while the price set was in use. I'm not clear if this was an issue before the regression that this fixes as well, but either way it should be handled. So I think we'd need to check specifically for the three array keys we are interested in, rather than just Out of scope for this, but I noticed this table isn't shown on the settings page for the price set and it probably should be. |
| @@ -13,16 +13,14 @@ | |||
| {include file="CRM/Price/Form/DeleteField.tpl"} | |||
| {elseif $action eq 1024 } | |||
| {include file="CRM/Price/Form/Preview.tpl"} | |||
| {elseif ($usedBy and $action eq 8) or $usedBy.civicrm_event or $usedBy.civicrm_contribution_page} | |||
| {elseif $usedBy} | |||
| <div id="price_set_used_by" class="messages status no-popup"> | |||
| {icon icon="fa-info-circle"}{/icon} | |||
| {if $action eq 8} | |||
| {ts 1=$usedPriceSetTitle}Unable to delete the '%1' Price Field - it is currently in use by one or more active events or contribution pages or contributions or event templates.{/ts} | |||
There was a problem hiding this comment.
| {ts 1=$usedPriceSetTitle}Unable to delete the '%1' Price Field - it is currently in use by one or more active events or contribution pages or contributions or event templates.{/ts} | |
| {ts 1=$usedPriceSetTitle}Unable to delete the '%1' Price Field - it is currently in use by one or more active events or contribution pages or contributions or event templates.{/ts} |
There was a problem hiding this comment.
There is a similar extra space here as well (before or contribution pages), while you're in there.
|
Thanks I'll take a look. For the extra spaces yes good catch it's technically wrong. I'm just not sure it's worth breaking translations for it? ¯\_(ツ)_/¯ |
|
Didn't think of translations. Probably not worth it, unless there's some
way to auto-update all those translations.
…On Fri, Apr 21, 2023 at 3:52 PM demeritcowboy ***@***.***> wrote:
Thanks I'll take a look.
For the extra spaces yes good catch it's technically wrong. I'm just not
sure it's worth breaking translations for it? ¯_(ツ)_/¯
—
Reply to this email directly, view it on GitHub
<#26090 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGCV35BWDM2KYCT6P2B5KALXCL6SXANCNFSM6AAAAAAXFS6I3M>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
demeritcowboy
force-pushed
the
price-use
branch
from
b2f6d53 to
dbdb494
Compare
|
I opted instead for a generic message with similar wording to the others in the case where it's used in a contribution, but not in a page/event config. You've noted there's also an e-warning on the Price Option page - I can take a look at that but could also do separately after. |
| {if $showGenericMessage} | ||
| {if $action neq 8} | ||
| {* We don't have to do anything for delete action because the calling tpl already displays something. *} | ||
| {ts}This price set is used by at least one contribution, but is not used by any active events or contribution pages or event templates.{/ts} |
There was a problem hiding this comment.
Technically could be a contribution or participant
There was a problem hiding this comment.
Thanks @larssandergreen. I tried to re-use the wording that was used in all the other messages. And in the future it might be even more things, which is also one reason I was trying to get rid of the hardcode of all the possible array keys in multiple places (e.g. the addition of event template missed some places, so it would likely happen again).
I'm open to changing it but then maybe to something more generic. What it's actually checking is usage in line items and in civicrm_price_set_entity.
There was a problem hiding this comment.
Maybe just "is used at least one record" or "is used by at least one existing record"? Or contribution, not a big deal.
|
Looks good to me other than a very small comment added. |
demeritcowboy
added
the
merge ready
Overview
https://lab.civicrm.org/dev/core/-/issues/4248
Before
After
Working again.
Technical Details
This reverts parts of both #25016 and #23046 and instead addresses the undefined vars in the tpls. The first PR was attempting to address them in php, but caused a problem which led to the 2nd PR which then caused this problem.
There is a certain amount of code that expects $usedBy to NOT have the keys and be completely empty when nothing is using the field/set. I felt it was easier and less error-prone to keep it that way rather than try to make sure all the keys are always assigned, and then always have to check every key to see if it's also empty. For example in Field.php just a bit lower down on line 246 where it sets the contexts, and in the tpls there are still other places checking
!$usedBy.Comments
Additional note that it was buggy regarding usage in event_templates even before the first PR.