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

Towards CRM-21140, support at api level for custom data on any entity. #11567

Merged
merged 1 commit into from
Jan 29, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jan 22, 2018

Overview

This extends the ability to support additional entities for custom data by
adding them to the OptionGroup 'cg_extends'. At this stage there
are 2 additional tested (supported) entities - UFGroup, PriceSet, PaymentToken.

Note this 'feature' is expected to be available for the March release (5.0.0.0 I think) so that extension writers can target 5.x for custom data on any entity. This is just progress towards that goal

Will add more next iteration

Before

Not possible to extend entities for custom data outside hard-coded core ones.

After

UFGroup, PriceSet, PaymentToken now possible & tested/supported.

Technical Details

Comments

I will follow up with the next ones I can test later


This extends the ability to support additional entities for custom data by
adding them to the OptionGroup 'cg_extends'. At this stage there
are 2 additional tested (supported) entities - UFGroup, PriceSet, PaymentToken.

Will add more next iteration
@eileenmcnaughton
Copy link
Contributor Author

@MegaphoneJon - any chance you can review this? It's actually a pretty trivial change due to past clean up we have done

@MegaphoneJon
Copy link
Contributor

Yes, I had my eye on it! I've been slammed but I have a need for this, so I'll make some time in the next couple of days.

Copy link
Contributor

@MegaphoneJon MegaphoneJon left a comment

Choose a reason for hiding this comment

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

This looks good! I was even able to test this through the UI with a little bit of trickery, using an entity not covered by the unit tests. I feel good about this.

@MegaphoneJon
Copy link
Contributor

(CiviCRM Review Template DEL-1.0)

  • JIRA (r-jira)
    • PASS : The PR has a JIRA reference.
  • Test results (r-test)
    • PASS: The test results are all-clear.
  • Code quality (r-code)
    • PASS: The functionality, purpose, and style of the code seems clear+sensible.
  • Documentation (r-doc)
    • ISSUE: The developer documentation should be updated.
    • COMMENTS: This should probably be a separate PR against the docs, and shouldn't prevent a merge. It might also be desirable to wait until there's a UI interface for this if one is planned (it would be very simple to create).
  • Maintainability (r-maint)
    • PASS: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests.
  • Run it (r-run)
    • PASS: In addition to running the automated tests, I implemented custom fields on "Financial Type" through the UI. I went to Administer » System Settings » Option Groups, edited the "cg_extends_objects" option group, and entered civicrm_financial_type as a label and FinancialType as a value. Once saved, I changed the label to Financial Type. This changed the label, but the name stayed as civicrm_financial type. Once this was done, I created a new custom group/field and was able to successfully add values to it (for both new and existing records) using FinancialType.create. I could query them successfully using FinancialType.get.
  • User impact (r-user)
    • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
    • COMMENTS: This is a new feature, so shouldn't be noticeable except when creating new custom field groups, where the change is intuitive.
  • Technical impact (r-tech)
    • PASS: The change preserves compatibility with existing callers/code/downstream.
    • COMMENTS: The changed function returns a superset of the values returned by the original function, so this won't cause problems for existing code.

@eileenmcnaughton
Copy link
Contributor Author

@colemanw are you ok to merge this based on @MegaphoneJon review.

Re additional entities - I do plan to chip away at adding them to the unit tests & confirm which others do / don't work. I specifically know the mailing api needs some clean up first (because it does weird stuff with the deprecated $ids parameter.

I will follow up with the docs pr once this is merged

@colemanw
Copy link
Member

Ok, so my understanding is that with this PR we now have 2 approaches to custom data:

  1. Handle it at the BAO level. Most entities do this.
  2. Handle it at the api level.

Is the goal to eventually consolidate on the 2nd approach?

How does this affect the custom data UI? Wouldn't we want additions to that option group to affect this list?
customgroupselector

@eileenmcnaughton
Copy link
Contributor Author

@colemanw - there is no intention at this stage to expose the new options through the UI - that can be up to extension writers to do that as they add functionality.

Regarding the BAO vs api - I think it would be ideal to consolidate on the api but this approach succeeds in at least not doubling up - ie. where it is already through the BAO we are not doing it twice since we have an array of those entities. It's hard to remove from the BAO without ensuring there are no places relying on it

@colemanw
Copy link
Member

How would an extension go about altering that UI? My assumption as a developer would be that adding an entity to that option group would also add it to the list in the UI, but if there is another relatively straightforward way of doing it, ok.

@eileenmcnaughton
Copy link
Contributor Author

Actually I guess that UI might pick it up - I'm not sure how it builds it - at this stage I'm focussed on the scenario where developers want to create additional custom fields against entities in extensions - rather than allowing people to add them themselves.

I guess if an entity is enabled for custom data & that entity happens to be enabled in the UI then it's probably OK - but I wouldn't have tried to add that purposely at this stage. I think stage one is that developers can add custom fields against any entity, including ones that they create themselves.

@MegaphoneJon
Copy link
Contributor

To respond to the question above: Any entities added to the cg_extend_objects OptionGroup WILL show up in the UI in the select box that Coleman screenshot above. In fact "Cases" and "Survey" in that list are generated from the option group.

In fact, during my testing I created custom fields for FinancialType records entirely from the UI. I needed the API to populate/read the values of course - but I was able to extend custom fields to a new entity without the API.

@colemanw
Copy link
Member

Ok, very cool.

@colemanw colemanw merged commit b22376f into civicrm:master Jan 29, 2018
@eileenmcnaughton eileenmcnaughton deleted the api_custom branch January 29, 2018 19:37
@mlutfy mlutfy added this to the 4.7.31 milestone Feb 9, 2018
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.

5 participants