Afform - Use entity-specific filter key, revert entity_id generic key

[colemanw]

Overview

Reverts 2 bad decisions before they get locked into a release.

Technical Details

Bad decision 1:

Back in #31606 @ufundo tried to move us toward a generic entity_id passed into tabs and blocks instead of contact_id, which seemed sensible, so then in #31836 I made the switch in FormBuilder. Fortunately that hasn't made it into the stable release yet, because I've changed my mind. This switches it back.
The reason to stick with contact_id, event_id, etc. instead of entity_id is that these placements are all context-specific; they don't need to be generalized that much, and it's better that they aren't. This change opens the possibility of multi-entity contexts, like the case summary screen which would have both case_id and contact_id passed into the form.

Bad decision 2:

In #31838 I utilized the unused field "grouping" in the "afform_placement" OptionValues to indicate whether a placement requires a url. I've changed my mind about that too, because I'd rather use "grouping" to indicate contexts (see above) and it's a bigger field better suited to that. So I've switched this flag over to another unused column "filter" which still works fine.

Comments

This doesn't affect any stable releases & these changes have only been in beta for 5 days, so let's revert them now! If anyone actually downloaded the beta in the past 5 days and created an afform for use on the contact summary tab/block/menu, then they'll just have to manually update their form to fix the filter.

[civibot]

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[ufundo]

I am not sure about 1.

with contact_id, event_id, etc. instead of entity_id is that these placements are all context-specific; they don't need to be generalized that much, and it's better that they aren't. This change opens the possibility of multi-entity contexts, like the case summary screen which would have both case_id and contact_id passed into the form.

So those placements are context specific. But the point of this change is to make the afform generation generalisable so we can extend it to ContributionPage and Campaign and Group and... [MyCustomEntity]

At the moment when there are 3 of them it's easy to hard code each one, but it gets harder.

I see the tension with what to do in multi-entity contexts. My first thought was: that is the exceptional case, so handle it differently there? Havent looked at the code closely but is that not possible without reverting the other cases?

(My first first thought was: isn't case_id the actual entity_id here - as contact_id is redundant and can be derived from the case. But the issue is you can enable multi-client cases?)

[ufundo]

Further thought: for multi-entity contexts in general, the [entity_type]_id scheme doesnt always help. What if I have a screen with two contacts (e.g. "relationship details")? Then I'll need a more context-specific way to name my entity slots.

To me that adds support to: use the scheme that works for all single-entity contexts as the general rule, and then handle multi-entity contexts on a case by case basis.

Or if you want a scheme that will work anywhere then [entity_type]_1_id (hello Webform :)). But I think thats maybe a lot of faff in 90% of cases that have a single entity.

[ufundo]

A reason to prefer the hand-tailored for multi-entity contexts is you can provide a bit more clarity.

For a single-entity form it's usually pretty clear. ContactMembershipsTab => entity_id is going to want contact id

But in the CaseSummaryForm it could be good to have case_id and client_id rather than just contact_id because there are many different kinds of related contacts in Case context.

And in RelationshipDetailsForm you prob want near_contact_id and far_contact_id rather than contact_1_id and contact_2_id

[colemanw]

@ufundo I get that this undoes some of our hard work and that's annoying, but hear me out. Every placement is different, with different contextual variables available. FormBuilder needs a way to know what they are. If every placement had only one entity id in scope, then I'd agree all we need to know is what type of entity it is (e.g. a custom field block for Events just needs one word of metadata: "Event" and then FormBuilder knows that entity_id maps to an Event entity. However, for multi-entity contexts we need 2 pieces of metadata: each entity name, and the key it maps to. This gives us maximum flexibility going forward. As to why specifically undo the entity_id keys, well they haven't actually made it into a release yet, so this is the perfect time to do it and not be stuck with backward-compat support for duplicate keys.

If we merge this PR I think I can build something quite cool on top of it which opens up FormBuilder for more placement options. As you say, there are quite a few more possibilities and yes certainly some of them can just name their key entity_id, that's fine, under this proposal they can call it whatever they want.
The way it will work is that any core/extension that wants to expose a context defines it in this option group, adds all the entities and their keys to the "grouping" metadata, and then just passes those exact keys to the embedded afform (note, for placements with no entity in context like the home dashboard, "grouping" stays NULL).
Here's what I think the metadata will look like for the existing placements (the three that go on the contact summary screen):

--- a/ext/afform/core/managed/AfformPlacement.mgd.php
+++ b/ext/afform/core/managed/AfformPlacement.mgd.php
@@ -61,6 +61,7 @@ return [
         'value' => 'contact_summary_tab',
         'label' => E::ts('Contact Summary Tab'),
         'icon' => 'fa-address-card-o',
+        'grouping' => json_encode(['contact_id' => 'Contact']),
         'description' => E::ts('Add tab to contact summary page.'),
       ],
       'match' => ['option_group_id', 'name'],

And here's what our new placements for Event Summary and Case Summary can look like (mgd entity arrays abbreviated, pseudocode):

  [
    'name' => 'AfformPlacement:event_manage_tab',
    'params' => [
      'values' => [
        'value' => 'event_manage_tab',
        'label' => E::ts('Manage Event Tab'),
        'grouping' => json_encode(['event_id' => 'Event']),
        'description' => E::ts('Add as tab on the manage event .'),
      ],
    ],
  ],
  [
    'name' => 'AfformPlacement:case_summary_block',
    'params' => [
      'values' => [
        'value' => 'case_summary_block',
        'label' => E::ts('Case Summary Block'),
        'grouping' => json_encode(['contact_id' => 'Contact', 'case_id' => 'Case']),
        'description' => E::ts('Add to case summary screen.'),
      ],
    ],
  ],

[colemanw]

@ufundo the other reason to use non-generic keys is so they are compatible with (and don't conflict with) each other. Consider that a single afform can have multiple placements (it's a mulltiselect). With the above proposal, FormBuilder will have the metadata it needs to tell which placements are compatible with each other, e.g. if will see that the Contact Summary Tab has {contact_id: Contact}, and the "Case Summary Block" has {contact_id: Contact, case_id: Case} so it will know that a single afform could have both placements and use the contact_id key for both.

OTOH if both the Contact Summary Tab and the Manage Event Tab declare {entity_id: Contact} and {entity_id: Event}, respectively, then you've got the potential to use a key that's sensible in one context and completely bonkers in another.

[ufundo]

[ufundo]

I hear you on the multiple entities case and I think the overall placement thing looks great.

tell which placements are compatible with each other, e.g. if will see that the Contact Summary Tab has {contact_id: Contact}, and the "Case Summary Block" has {contact_id: Contact, case_id: Case} so it will know that a single afform could have both placements and use the contact_id key for both.

So making those compatible does sound like a good goal... But in that case I would lean towards deriving the key directly from the Entity Type. I.e. the placement meta could just be ["Contact","Case"] for CaseSummary and ["Contact"] for ContactSummary. Including the key there seems a bit arbitrary to me, and like it might lead to unnecessary incompatibilities. The thing that makes a form compatible with those two placements is they share a "Contact", the contact_id bit doesn't matter.

In general for forms to be reusable, they need to be able to know what the key is going to be. The case in point at the moment is with these custom field forms: https://github.com/civicrm/civicrm-core/pull/32040/files#r1952210793 - it would be nice if rather than an ever-increasing case statement, we could do $keyAfformNeeds = afform_key_for_entity_type($entityType) (and Afform could use the same function for deriving keys from the flatter placement meta above). At the moment its basically one case, but I think if we want to build out a more universal UI with afform there will be many more cases like this.

The scheme seems trivial at the moment but if we do want to extend to forms with multiple contacts then it's not clear how it goes. I suppose for backward compatibility (and the rarity of repeated entities) it could be like contact_id, contact_2_id? OR you could say repeated entities is the exceptional case, and then you need to hand code the keys?

(Slight sidepoint - it feels to me like the placement thing is going to grow out of civicrm_option_value in the same way from_email_address had. But one for another day.)

[colemanw]

@ufundo I think we're on the same wavelength, I already started using such a function here. And the other spot you mentioned could be switched to use it as well. I was actually planning to roll civicrm_admin_ui_civicrm_tabset back into afform.php and have afform provide both the contact & the event tab placement & then use this function to derive the key.

So then, I agree the explicit naming of the key gets redundant, and really only serves a purpose in the case of multi-entity contexts that have more than one of the same type of entity. Does such a scenario even exist? I can't think of one (relationships... maybe? But there isn't really a relationship screen which would merit an afform placement, and anyway it could be derived from the relationship id)... so no, I think we're good with the simpler grouping that looks like "Contact", or "Contact,Case", which is good since the optionValue "grouping" column doesn't have a pretty way to serialize/unserialize json.

[ufundo]

Nice yeh I think that sounds good!

As you say the placements with repeated entities are going to be rare so I think they can get special treatment. Though if the grouping is an array of values Contact, Event,... etc I guess that could have repeated values at a push.

[ufundo]

Suggested change

	if ($item['extends'] === 'Contact') {
	$entityIdFilter = 'contact_id';
	$afform['placement'] = ['contact_summary_tab'];
	}
	elseif (CoreUtil::isContact($item['extends'])) {
	$entityIdFilter = 'contact_id';
	$afform['placement'] = ['contact_summary_tab'];
	$afform['summary_contact_type'] = [$item['extends']];
	}
	else {
	$entityIdFilter = \CRM_Utils_String::convertStringToSnakeCase($item['extends']) . '_id';
	$entityIdFilter = \CRM_Utils_String::convertStringToSnakeCase($item['extends']) . '_id';
	if ($item['extends'] === 'Contact') {
	$afform['placement'] = ['contact_summary_tab'];
	}
	elseif (CoreUtil::isContact($item['extends'])) {
	// override e.g. "individual_id", we want "contact_id"
	$entityIdFilter = 'contact_id';
	$afform['placement'] = ['contact_summary_tab'];
	$afform['summary_contact_type'] = [$item['extends']];
	}

[colemanw]

Ok, makes sense.