dev/core#111 Support Custom Data for MembershipType entity

[mattwire]

Overview

Enable custom data support on the MembershipType form.

Before

Could not view/edit custom data on MembershipType form.

After

Can view/edit custom data on MembershipType form.

Technical Details

• Add generic helper for setDeleteMessage() in EntityFormTrait so we can simplify translation (one string for all forms instead of one for each form).
• Change CRM/Member/Form/MembershipType.php to use MembershipType.create API instead of BAO add (so we can save custom data).
• Change domain_id to non-mandatory for MembershipType.create API and default to current domain (see https://lab.civicrm.org/dev/core/issues/113 for discussion).
• Convert MembershipType form to use EntityFormTrait so we automatically add custom data to the UI and use more generic code.

Comments

@eileenmcnaughton This is a follow on from all the previous ones (eg. #12117, #12118). It should address all the previous issues. Also note that I'm now using cg_extends to add MembershipType (you'll either need to add it manually, or install this extension: https://github.com/mattwire/uk.co.mjwconsult.variablerecurpayments
Which will setup cg_extends for you as well as provide some handy MembershipType custom fields for testing :-)

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[mattwire]

test this please

[eileenmcnaughton]

This would incorrectly be set if 'id' is set - I think we should spin this out & resolve it first - but I think we should support 'current_domain' in the same way we support 'user_contact_id', now - ie. ie would look like

$params['domain_id']['api.default'] => 'current_domain_id';

I would need to understand from @totten why the code in utils handles '('or similar') but I guess we could just 'special-handle' the specific string 'current_domain_id'

  if (!empty($fieldValue) || $fieldValue === '0' || $fieldValue === 0) {
    // if value = 'user_contact_id' (or similar), replace value with contact id
    if (!is_numeric($fieldValue) && is_scalar($fieldValue)) {
      $realContactId = _civicrm_api3_resolve_contactID($fieldValue);
      if ('unknown-user' === $realContactId) {
        throw new API_Exception("\"$fieldName\" \"{$fieldValue}\" cannot be resolved to a contact ID", 2002, array('error_field' => $fieldName, "type" => "integer"));
      }

[eileenmcnaughton]

@mlutfy can you comment on whether this interpolation is equally good from your point of view? I originally assumed that the way it was before (with a line for each entity) was 'better' but not so sure now...

[mattwire]

I started changing it separately for each entity but then realised that nearly every entity with a delete action shares exactly the same wording (except the entity name) so I think this will reduce the number of strings that need translating significantly.

[mlutfy]

I'm not strongly opposed, but it is a loss of usability for non-English.

• CiviCRM does tend to have very verbose messages, which do not always communicate the right thing.
• We do tend to have a lot of copy-paste strings, and when updating them, we should try to be consistent.
• For example, the user might not care about the type of entity, but will react differently if this is going to delete 5000 records, vs deleting 5 records.

Grammatically:

• Assumes that the gender of "records" in other languages will always be the same (since 'records' is in the common string, it forces translators to do acrobatics to choose a gender-neutral word)
• Assumes that the noun (relationship, %1, etc) will be declined in the same way no matter of the context (can cause issues for some Slavic languages, I think)
• Assumes that the word "relationship" will have enough context for translators to use the correct word (this is probably minor, since those entities are well-known, but worth keeping in mind for words such as "Home" or "From").

For French, it forces us to translate in this form:

WARNING: Deleting this option will result in the loss of all records of this type (%1).

i.e.:

Avertissement: La suppression de cette option entraînera la perte de toutes les entrées de ce type (Relation).

Where as, it is currently translated as:

Avertissement: La suppression de cette option entraînera la perte de toutes les relations de ce type.

The latter makes it much more clear that we are deleting relationships, not just abstract records.

If the message looks awkward, users will ignore it. The parenthesis and incorrect capitalisation communicates to the user that this is a nerdy message, so they can glance over and look for the nearest "OK" button without thinking about it.

Conclusion: if it's clear from the context, we can remove the entity type (%1) completely. This way the user is more likely to read the message and think about what they are doing, but it can lead to frustration if they don't really know what the impact is. If we really want to communicate something useful, tell the user how many records will be deleted.

[eileenmcnaughton]

So - I'm not sure if it is clear without the world - I'm not inclined to make a change with a negative impact on usability for non-anglophones where there is such a minor code repetition decrease

[mattwire]

@eileenmcnaughton This is now updated and rebased. I removed the "translation" helper for setDeleteMessage() I realise it doesn't really help (per @mlutfy comments). Also removed the domain_id as MembershipType API has already been updated in core to make this optional. Pending tests passing I think this should be ok to merge.

[eileenmcnaughton]

@mattwire I'm not ready to accept that we can't use the entity fields.tpl yet - I want to try to push a bit further on that before accepting it. The main reason is that I want to set a bar for adding custom fields to an entity. I'm happy to merge this without the one line that delivers the main functionality - {include file="CRM/common/customDataBlock.tpl"} and then take a look at how we can go further on this

[eileenmcnaughton]

Per previous comments - adding merge on pass - I'll take a look & see if we can do anything with the tpl

[eileenmcnaughton]

test this please