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

Add New API Call To Get All Unique Fields For Given Entity #11990

Merged

Conversation

vinuvarshith
Copy link
Contributor

@vinuvarshith vinuvarshith commented Apr 17, 2018

Overview

This is a new api that will go through the indexes of an entity table and return all unique fields (including combination indexes).
This feature was proposed in a disscusion here
eileenmcnaughton/nz.co.fuzion.csvimport#13

Before

It's not currently possible to get a list of unique fields (or combinations) for a given entity.

After

A new api is added to get a list of all unique fields for a given entity.

Technical Details

Sample API call using 'Contribution' entity:
$result = civicrm_api3('Contribution', 'getunique');

Result:

{
    "is_error": 0,
    "version": 3,
    "count": 2,
    "values": {
        "UI_contrib_trxn_id": [
            "trxn_id"
        ],
        "UI_contrib_invoice_id": [
            "invoice_id"
        ]
    }
}

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@vinuvarshith vinuvarshith changed the title WIP: Add New API Call To Get All Unique Fields For Given Entity [WIP] Add New API Call To Get All Unique Fields For Given Entity Apr 17, 2018
Copy link
Contributor

@MiyaNoctem MiyaNoctem left a comment

Choose a reason for hiding this comment

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

The logic looks good, just make sure you're following these guidelines:
https://docs.civicrm.org/dev/en/latest/framework/api-architecture/

*
* @return mixed
*/
function civicrm_api3_generic_getUnique($apiRequest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably add a _spec function for this API action too. Check:

https://docs.civicrm.org/dev/en/latest/framework/api-architecture/#spec

Copy link
Contributor

Choose a reason for hiding this comment

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

std is lcase - ie getunique

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Actually our coding standards don't have anything that suggests putting two words together without either camelCase or an underscore. We should be following the agreed standards as much as possible, @eileenmcnaughton https://docs.civicrm.org/dev/en/latest/standards/php/

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah - this is the api functions std not something general

@colemanw
Copy link
Member

If this is a greenfield project, consider using Api V4. https://github.com/civicrm/api4

@eileenmcnaughton
Copy link
Contributor

@colemanw this is proposed to build on extensions that work on api v3 already & TBH we already rejected a change for core & this is the less intrusive core change proposed coming off that. I'm OK with this in principle. Note if we DID add the unique custom field type this should also be extended to return those fields

@vinuvarshith you don't need to query the DB - the DAOs have an indices function you can use.

Note this will need a unit test - also the PR description should include a code snippet calling this api & what the response looks like.

@vinuvarshith
Copy link
Contributor Author

@eileenmcnaughton
I have updated code to use indices functions in DAOs instead of an SQL query.
I have added 2 testcases one for testing 'getunique' (for 'Contact' and for 'Contribution' entities)

@eileenmcnaughton
Copy link
Contributor

@vinuvarshith if this is ready for review can you remove the WIP tag. Also check the comment about casing - I know it's a bit ugly to do getunique - but consistent with getfields getsingle etc

@vinuvarshith
Copy link
Contributor Author

@eileenmcnaughton
Sure. I will remove the WIP tag now. I will change the casing to getunique too.
I did 'getUnique" after looking at "getList" (civicrm_api3_generic_getList)

@vinuvarshith vinuvarshith changed the title [WIP] Add New API Call To Get All Unique Fields For Given Entity Add New API Call To Get All Unique Fields For Given Entity Apr 19, 2018
@seancolsen
Copy link
Contributor

I'd like to request the following change to satisfy r-doc:

  • Can you add a corresponding PR against dev-docs which inserts a new section to the API Actions page describing the action?

@vinuvarshith
Copy link
Contributor Author

@seanmadsen
Added PR against dev-docs here
civicrm/civicrm-dev-docs#522

@eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton merged commit e98d91a into civicrm:master Apr 21, 2018
@totten
Copy link
Member

totten commented Apr 26, 2018

Small correction -- it looks like the tests weren't properly run on this PR. (Jenkins wanted confirmation from an admin before it would run the tests.)

I suspect there's no real harm in this case, though I had a small issue where civilint failed on some unrelated changes. It's updated as part of #12029 (2a7d2b9).

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.

8 participants