dev/core#5603 Support multi-record CustomGroups for Events using Afform tab display #31606

ufundo · 2024-12-16T16:21:43Z

Overview

Currently only Contacts can have CustomGroups with is_multiple = TRUE. This adds support for creating is_multiple custom groups for Events. These are then displayed as an additional tab in the Manage Events tabset.

Before

only Contacts can have multi-record custom groups

After

Events can have multi-record custom groups
(with AdminUI extension enabled + CustomGroup style set to "Tab with table") these are displayed using an additional in the Manage Events section

Technical Details

This involves trying to generalise some of the plumbing for AfformTabs - essentially changing hard-coded contact_id to entity_id. In the process I've removed AfformTab.tpl and AfformBlock.tpl (the latter change isn't strictly necessary, but feels good step to pave the way for further afform-based functionality for non-Contact entities.

Comments

need to check AfformBlock changes in contact summary editor hook
at the moment effectively requires AdminUI + style = "Tab with table" for these custom groups (otherwise they won't be exposed anywhere in the UI)

civibot · 2024-12-16T16:21:46Z

🤖 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 🔗

ext/afform/core/Civi/Api4/Action/CustomGroup/GetAfforms.php

colemanw · 2024-12-16T18:34:54Z

@ufundo is the switch from contact_id to entity_id going to affect existing user-created afforms?

CRM/Contact/Page/View/Summary.php

ufundo · 2024-12-16T19:42:20Z

@ufundo is the switch from contact_id to entity_id going to affect existing user-created afforms?

It shouldn't do. I passed entity_id and contact_id in the assign here https://github.com/civicrm/civicrm-core/pull/31606/files#r1887316810 , so either should work (and the other will be ignored if not used).

I r-ran as follows:

start on master
making local edits the the Membership tab afform
save to create a local copy with a hard reference to options.contact_id
check out this branch
my locally edited version of membership tab afform still works

colemanw · 2024-12-16T19:44:15Z

It shouldn't do

Ok, and what about editing the afform in the GUI. Will the gui have lost track of that reference now that the name changed?

ext/afform/core/afform.php

ufundo · 2024-12-16T20:20:27Z

Ok, and what about editing the afform in the GUI. Will the gui have lost track of that reference now that the name changed?

Good question. I did look at that briefly and I don't really understand how it works.

GUI seems to recognise options.entity_id and options.contact_id in the markup as references to "Current Contact" :

Though I don't quite know how it does that?

(Note: the GUI never likes that status_id.is_current_member filter)

ufundo · 2024-12-16T20:28:27Z

Noticed an issue with the View links for the individual Custom records -- these are still using a QuickForm that is hard-coded for contact

ufundo · 2024-12-16T20:30:24Z

Issue 2: the event dashboard cleverly creates a quick link to each tab, but these use a subpath rather than a query parameter, and the one for our custom tab doesn't work

ufundo · 2024-12-16T20:57:57Z

c878df0 should fix Issue 2 - it seems fine for the other tabs as well, I'm not really sure why they have dedicated routes?

7092ee0 provides view forms, but maybe a bit heavy duty? I did wonder if there's a way to do something more similar to the afform block on the contact summary?

colemanw · 2024-12-16T21:45:29Z

@ufundo provides view forms, but maybe a bit heavy duty?

Seems fine to me :)

ufundo · 2024-12-16T22:02:02Z

I actually thought maybe better to separate out the change to blocks, so have removed in the latest iteration.

Rolled back the test changes for blocks as well.

Fixed the style, and the other failures looked like flakes, so hoping this will pass.

ufundo · 2024-12-16T22:10:26Z

The other thing I don't like with the view forms: it's only the DisplayOnly meta at the field level that is stopping you from editing data. It feels like it should be a control at the form level. It would be nice to have an afform mode that is like "this form can prefill data, but not submit". Then you could re-use the Update form (and the afblock), but with that "read-only" toggled.

I'd thought of something like that before on the front-end could be neat. User could load the form as read-only, and all the fields are disabled; then (if allowed for that form) they toggle to edit mode, without having to refetch all the data. I guess it would be a bit closer in UX to quickform blocks on the contact summary.

That's a chunk more work though, so maybe the approach here is good enough for now?

colemanw · 2024-12-17T01:43:34Z

@ufundo still looking at a failure in testAfformContactSummaryBlock

colemanw · 2024-12-17T03:09:13Z

@ufundo I agree that the ability to toggle 'readonly' at the form level would be a great feature! Not a blocker to this PR but a very good idea.

ufundo · 2024-12-17T08:35:18Z

retest this please

ufundo · 2024-12-17T10:37:31Z

The remaining test fail seems to be common across all open PRs at the moment => think unrelated

colemanw · 2024-12-17T11:58:47Z

ext/afform/core/templates/afform/tabsets/EventManageTab.tpl

@@ -0,0 +1,2 @@
+{assign 'options' ['entity_id' => $eventId]}


I don't think this is Smarty2-compatible syntax. Also the file needs a trailing newline.

oh this file has crept back in during rebase I think. it shouldnt be there/used

colemanw · 2024-12-17T14:44:52Z

templates/CRM/Contact/Page/View/Summary.tpl

@@ -116,9 +116,14 @@
      </ul>

      {foreach from=$allTabs item=tabValue}
-        {if !empty($tabValue.template)}
+        {if $tabValue.template}


Might this cause an undefined index warning? I think you're right we're not supposed to use empty() in templates but maybe we should use isset() to prevent the e-notice.

I changed to make consistent with https://github.com/civicrm/civicrm-core/pull/31606/files#r1888888349

but maybe should have changed the other way or changed both to isset?

ufundo · 2024-12-17T16:55:50Z

templates/CRM/common/TabHeader.tpl

@@ -29,7 +29,12 @@
      {foreach from=$tabHeader key=tabName item=tabValue}
        {if $tabValue.template}


@colemanw I just updated for consistency with here. I added empty to the bit below to avoid an ENOTICE, so maybe it is good to use for both? Or is isset preferable?

IIRC empty() is not compatible with smarty escaping so it's better to use isset(). I think? Might wanna double check what other templates are doing these days.

https://docs.civicrm.org/dev/en/latest/security/outputs/#gotcha-2-smarty-notices

isset = BAD
empty = not great

preferred option is make sure the variable is assigned before the template layer ~~but given the number of places assign('tabHeader', $someVar) happens that looks non trivial~~ and we have a function for that! addExpectedTabHeaderKeys here we come :)

…t, remove old report related guidance

… for settings tab links

colemanw · 2024-12-19T14:06:33Z

This all looks merge-ready to me! Good work @ufundo

ufundo · 2024-12-19T16:30:20Z

This should fix the Smarty unassigned var error: #31636

civibot bot added the master label Dec 16, 2024

ufundo changed the title ~~Support multi-record CustomGroups for Events using Afform tab display~~ dev/core#5603 Support multi-record CustomGroups for Events using Afform tab display Dec 16, 2024

colemanw reviewed Dec 16, 2024

View reviewed changes

ext/afform/core/Civi/Api4/Action/CustomGroup/GetAfforms.php Outdated Show resolved Hide resolved

colemanw reviewed Dec 16, 2024

View reviewed changes

CRM/Contact/Page/View/Summary.php Outdated Show resolved Hide resolved

colemanw reviewed Dec 16, 2024

View reviewed changes

ext/afform/core/afform.php Outdated Show resolved Hide resolved

ufundo force-pushed the custom-group-tabsets branch 2 times, most recently from b818207 to 1bac07c Compare December 16, 2024 20:09

ufundo force-pushed the custom-group-tabsets branch from 7092ee0 to 5ec170a Compare December 16, 2024 22:00

ufundo force-pushed the custom-group-tabsets branch from 5ec170a to 7d51ae3 Compare December 17, 2024 07:56

ufundo marked this pull request as ready for review December 17, 2024 07:56

colemanw reviewed Dec 17, 2024

View reviewed changes

ufundo force-pushed the custom-group-tabsets branch from 7d51ae3 to 9364b0d Compare December 17, 2024 14:13

colemanw reviewed Dec 17, 2024

View reviewed changes

ufundo commented Dec 17, 2024

View reviewed changes

rename is_multiple to allow_is_multiple in cg_extends

cd83df5

ufundo added 8 commits December 19, 2024 13:55

allow_is_multiple for Event CustomGroups

ce1c914

update Custom Group create is_multiple help text to not assume contac…

2a76331

…t, remove old report related guidance

generalise contact_id filter to entity_id for tabset afforms

2e4ce31

enable tab afforms for non-contact is_multiple CustomGroups

a8867be

get Entity Label using CoreUtil::getInfoItem

c934814

Event Dashboard - use selectedChild param rather than dedicated route…

9aabbef

… for settings tab links

custom afform generation - correct afblock comment

ecd8552

add custom view-only afform

b985d82

ufundo force-pushed the custom-group-tabsets branch from 9364b0d to b985d82 Compare December 19, 2024 13:58

colemanw added the merge ready PR will be merged after a few days if there are no objections label Dec 19, 2024

ufundo mentioned this pull request Dec 19, 2024

[REF] use common helper to set expected keys for tabs in Summary.tpl and TabHeader.tpl #31636

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#5603 Support multi-record CustomGroups for Events using Afform tab display #31606

dev/core#5603 Support multi-record CustomGroups for Events using Afform tab display #31606

ufundo commented Dec 16, 2024 •

edited

Loading

civibot bot commented Dec 16, 2024

colemanw commented Dec 16, 2024

ufundo commented Dec 16, 2024

colemanw commented Dec 16, 2024

ufundo commented Dec 16, 2024

ufundo commented Dec 16, 2024 •

edited

Loading

ufundo commented Dec 16, 2024

ufundo commented Dec 16, 2024

colemanw commented Dec 16, 2024

ufundo commented Dec 16, 2024

ufundo commented Dec 16, 2024 •

edited

Loading

colemanw commented Dec 17, 2024

colemanw commented Dec 17, 2024

ufundo commented Dec 17, 2024

ufundo commented Dec 17, 2024 •

edited

Loading

colemanw Dec 17, 2024

ufundo Dec 17, 2024

colemanw Dec 17, 2024

ufundo Dec 17, 2024

ufundo Dec 17, 2024

ufundo Dec 17, 2024

colemanw Dec 17, 2024

ufundo Dec 19, 2024 •

edited

Loading

colemanw commented Dec 19, 2024

ufundo commented Dec 19, 2024

		@@ -29,7 +29,12 @@
		{foreach from=$tabHeader key=tabName item=tabValue}
		{if $tabValue.template}

dev/core#5603 Support multi-record CustomGroups for Events using Afform tab display #31606

Are you sure you want to change the base?

dev/core#5603 Support multi-record CustomGroups for Events using Afform tab display #31606

Conversation

ufundo commented Dec 16, 2024 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Dec 16, 2024

colemanw commented Dec 16, 2024

ufundo commented Dec 16, 2024

colemanw commented Dec 16, 2024

ufundo commented Dec 16, 2024

ufundo commented Dec 16, 2024 • edited Loading

ufundo commented Dec 16, 2024

ufundo commented Dec 16, 2024

colemanw commented Dec 16, 2024

ufundo commented Dec 16, 2024

ufundo commented Dec 16, 2024 • edited Loading

colemanw commented Dec 17, 2024

colemanw commented Dec 17, 2024

ufundo commented Dec 17, 2024

ufundo commented Dec 17, 2024 • edited Loading

colemanw Dec 17, 2024

Choose a reason for hiding this comment

ufundo Dec 17, 2024

Choose a reason for hiding this comment

colemanw Dec 17, 2024

Choose a reason for hiding this comment

ufundo Dec 17, 2024

Choose a reason for hiding this comment

ufundo Dec 17, 2024

Choose a reason for hiding this comment

ufundo Dec 17, 2024

Choose a reason for hiding this comment

colemanw Dec 17, 2024

Choose a reason for hiding this comment

ufundo Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

colemanw commented Dec 19, 2024

ufundo commented Dec 19, 2024

ufundo commented Dec 16, 2024 •

edited

Loading

ufundo commented Dec 16, 2024 •

edited

Loading

ufundo commented Dec 16, 2024 •

edited

Loading

ufundo commented Dec 17, 2024 •

edited

Loading

ufundo Dec 19, 2024 •

edited

Loading