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-21031 Fix for id instead of financialtype name #10823

Merged

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Aug 6, 2017

@eileenmcnaughton
Copy link
Contributor

Seems sensible & safe as it only affects a browse page @seamuslee001 - does this tie in at all with your work on disabled financial types & do you have any comment?

@seamuslee001
Copy link
Contributor

So this will have the issue of as far as i can tell returning blank value i.e. NULL if the Financial Type that is used by a price option is disabled (I believe that is the default of CRM_Utils_Array::value). This is because https://github.com/civicrm/civicrm-core/blob/master/CRM/Contribute/PseudoConstant.php#L110. Now in this case that might make sense given that the FT isn't really active and therefore shouldn't we be showing a difference.

@seamuslee001
Copy link
Contributor

I would also add that this is an improvement as i can't see how $financialType is getting set at all in the browse() function

@eileenmcnaughton
Copy link
Contributor

Ok @seamuslee001 that seems like this is good to merge then. I think you could make a pretty good argument for blank for a disabled type (& an even better one for showing it highlighted in red with a strike through, but I won't hold out for that)

@eileenmcnaughton eileenmcnaughton merged commit 74862f0 into civicrm:master Aug 6, 2017
@mattwire
Copy link
Contributor Author

mattwire commented Aug 6, 2017

@seamuslee001 @eileenmcnaughton I actually wanted to use CRM_Core_PseudoConstant::getKey but couldn't work out what params I needed for financial_type_id.

@eileenmcnaughton
Copy link
Contributor

CRM_Core_PseudoConstant::getKey('CRM_Contribution_BAO_Contribution', 'financial_type_id', $value)

@eileenmcnaughton
Copy link
Contributor

(ie. financial_type_id is a field on the contribution BAO/DAO

@mattwire mattwire deleted the CRM-21031_financialtypedisplay branch August 7, 2017 08:34
@mattwire
Copy link
Contributor Author

mattwire commented Aug 7, 2017

@eileenmcnaughton So that's what I actually thought it should be, but that doesn't return a value:
$type = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'financial_type_id', 1); // Expected result: "Donation"'; Actual result: "false"

This works: $financialTypes = CRM_Contribute_BAO_Contribution::buildOptions('financial_type_id', 'get');

@eileenmcnaughton
Copy link
Contributor

Hmm - OK so I got that wrong, there are 3 functions
CRM_Core_PseudoConstant::getKey
CRM_Core_PseudoConstant::getName
CRM_Core_PseudoConstant::getLabel

so, depending on whether you wanted the unique name or the label one of the latter 2 should work

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