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

Fix pricefield pseudoconstant. #17364

Merged
merged 1 commit into from
May 26, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented May 20, 2020

Overview

Fixes PriceField entity to build options for the price_set_id pseudoconstant field

Before

$result = civicrm_api3('PriceField', 'getoptions', [
  'field' => "price_set_id",
]);

Returns nothing

After

Above returns

{

    "is_error": 0,
    "version": 3,
    "count": 8,
    "values": {
        "1": "Contribution Amount",
        "7": "Fall Fundraiser Dinner",
        "3": "Help Support CiviCRM!",
        "4": "Member Signup and Renewal",
        "2": "Membership Amount",
        "5": "Pledge for CiviCRM!",
        "6": "Rain-forest Cup Youth Soccer Tournament",
        "8": "Summer Solstice Festival Day Concert"
    }
}

Technical Details

The price field turns out not to be returning pseudoconstant fields for 2 reasons

  1. the label field is defined but the name field is not
  2. any price fields with no domain are filtered out. In my single domain db this is
    all of them.

I think we can safely understand NULL domain_id as 'not restricted by domain'. For some tables
(membershipType) it is a required field but for PriceField it is not.

I would be inclined to say it should be required less often. However, where it is required thenn
the OR IS NULL should never be true

Comments

@seamuslee001 @colemanw

@civibot
Copy link

civibot bot commented May 20, 2020

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

Also note that for memberships the domain id filtering is not actually seen as a good thing by AUG - eileenmcnaughton/org.civicrm.multisite#26

@eileenmcnaughton
Copy link
Contributor Author

@yashodha maybe of interest to you

@pradpnayak
Copy link
Contributor

Test failure looks related, but change make sense. Good to merge once test passes

The price field turns out not to be returning pseudoconstant fields for 2 reasons
1) the label field is defined but the name field is not
2) any price fields with no domain are filtered out. In my single domain db this is
all of them.

I think we can safely understand NULL domain_id as 'not restricted by domain'. For some tables
(membershipType) it is a required field but for PriceField it is not.

I would be inclined to say it should be required less often. However, where it is required thenn
the OR IS NULL should never be true
@seamuslee001
Copy link
Contributor

Merging as per @pradpnayak 's review

@seamuslee001 seamuslee001 merged commit 67d42e3 into civicrm:master May 26, 2020
@seamuslee001 seamuslee001 deleted the price_pseudo branch May 26, 2020 00:08
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.

3 participants