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

Use description from schema if available when using entityForm #12971

Merged

Conversation

mattwire
Copy link
Contributor

Overview

Further development of entityForm.

This adds the (default) ability to automatically show the description from the entity schema instead of hardcoding in the entityFields property on the class.

Before

Converting to entityForm requires moving description from tpl to setEntityFields() in form class.

After

Converting to entityForm requires verifying metadata description in xml schema and updating if necessary. Description does not need to be hardcoded in class.

Technical Details

We get the metadata using API.getfield. Technically this means we are now calling this twice (as it is also called by addField later on).

Comments

@mlutfy Please could you comment from a translation perspective.. ie. do the schema elements get translated?
@eileenmcnaughton Next stage of entityForm...

@civibot
Copy link

civibot bot commented Oct 19, 2018

(Standard links)

@civibot civibot bot added the master label Oct 19, 2018
@@ -1292,7 +1292,7 @@ public function addDateRange($name, $from = '_from', $to = '_to', $label = 'From
*
* Return string
*/
private function getApiAction() {
protected function getApiAction() {
Copy link
Contributor

Choose a reason for hiding this comment

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

changing visibility can break forms that extend the parent - I did a grep & found a few places that implement function getApiAction() with 'protection' but none with private so I guess that is fine. Also it's a bit intuitive that changing up from private is probably ok since private is not available to override

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so basically we're making it accessible from child classes which is needed to get the right API action to retrieve the metadata.

@mattwire mattwire force-pushed the entityfields_metadatadescription branch from 19e56f8 to 59f04ea Compare October 20, 2018 21:26
@eileenmcnaughton
Copy link
Contributor

@mattwire So this is a good thing to do. I note a few things

  1. some enotices
    screenshot 2018-10-21 10 22 19

  2. I think I'd probably tweak the description of the description field rather than override to block -

screenshot 2018-10-21 10 25 48

  • changing 'verbose' to 'generic' seems like it would be fine
  1. re translation - here is the DAO
        'description' => [
          'name' => 'description',
          'type' => CRM_Utils_Type::T_STRING,
          'title' => ts('Relationship Description'),
          'description' => 'Optional verbose description of the relationship type.',
          'maxlength' => 255,
          'size' => CRM_Utils_Type::HUGE,
          'table_name' => 'civicrm_relationship_type',
          'entity' => 'RelationshipType',
          'bao' => 'CRM_Contact_BAO_RelationshipType',
          'localizable' => 1,
          'html' => [
            'type' => 'Text',
          ],
        ],

You can see description is not translated - I think maybe it should be @colemanw @mlutfy - any concerns if we add ts around description field when generating DAOs

@@ -221,4 +222,21 @@ protected function isDeleteContext() {
return ($this->_action & CRM_Core_Action::DELETE);
}

protected function setEntityFieldsMetadata() {
foreach ($this->entityFields as $field => &$props) {
if ($props['not-auto-addable']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This may need to change to

Suggested change
if ($props['not-auto-addable']) {
if (!empty($props['not-auto-addable'])) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Done :-)

@mattwire mattwire force-pushed the entityfields_metadatadescription branch from 59f04ea to d5e4784 Compare October 21, 2018 09:17
@mattwire
Copy link
Contributor Author

  1. I think I'd probably tweak the description of the description field rather than override to block -

I didn't want to touch the actual descriptions until we got this PR into shape and merged. And I think there will be situations where there is a description in the schema that we don't want to show on the form but may popup elsewhere (eg. API explorer). So blocking this particular description was more of a proof of concept to show we can override/hide them if desired.

@eileenmcnaughton
Copy link
Contributor

@mattwire OK - we'd better resolve the ts issue then - @colemanw ....

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Oct 24, 2018
This relates to the attempt here civicrm#12971
to use description directly in the Entity Forms & questions as to whether it is translated.

Currently they are not but there seems like no downside in translating them
@eileenmcnaughton
Copy link
Contributor

If we can resolve #13005 then we can merge this

@eileenmcnaughton
Copy link
Contributor

@mattwire the other PR is merged so merging this so we can 'mature it up'

@eileenmcnaughton eileenmcnaughton merged commit b965352 into civicrm:master Oct 25, 2018
@mattwire mattwire deleted the entityfields_metadatadescription branch October 25, 2018 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants