dev/core#5654 add support for tagging Afforms using Tag entity #31890

ufundo · 2025-01-27T16:11:26Z

Overview

Adds the ability to tag afforms with Tags (from civicrm_tag table).

Alternative to #31889

Before

no tagging for Afforms

After

everything in add support for tagging Afforms using an Option Group #31889
EXCEPT not using an option group, but allowing Tags to be "used_for" Afforms

Technical Details

This implementation uses the civicrm_tag table.

The advantage over #31889 is we have a nice UI for managing tags, and tags can be reused (e.g. for activities and forms)

On the flipside, this might be a slightly deviant usage of tags, because the used_for field generally expects a table name in order to create db table joins between tagged entities and tags, using the EntityTag bridge (with a dynamic FK entity_table and entity_id). In this case Afform is not a db entity, so cannot be referenced in this way - the values are just being loaded into a pseudoconstant, and then serialized in the Afform meta.

However - I feel like this might be legitimate for tags in an Api4 world - ie the key thing for tags is they reference an entity. If this is a DB entity then great, we get stuff for handling the DB queries out of the box. If not there is a bit more flexibility/work to do in how a BasicEntity uses the tags.

civibot · 2025-01-27T16:11:29Z

🤖 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 · 2025-01-27T16:15:51Z

I think this is my preferred approach over #31889 but interested in other views

ufundo · 2025-01-27T16:24:13Z

As a next step, I would have liked to switch the input on the FormBuilder screen to a crm-autocomplete with a QuickAdd form popup ( using /civicrm/tag/edit?action=add&used_for=Afform )

But that wasn't working for some reason. I think there may be a constraint where Quick Add forms must have 'quick-add' in their url path. @colemanw are you aware of that?

In general one of the reasons I prefer this approach is I think the UI could improve over time if we add more Tag support to SearchKit / FormBuilder (i.e. things like correctly coloured swatches in the SearchKit listing and type-to-create-new-tags-on-the-fly ).

ext/afform/core/managed/AfformTags.mgd.php

colemanw · 2025-01-27T16:35:34Z

@ufundo I feel pretty good about this deviant use of the Tag entity. I vote for this option!

ufundo · 2025-01-27T21:55:50Z

@ufundo I feel pretty good about this deviant use of the Tag entity. I vote for this option!

I was slightly wary the schema builder might blow up somewhere because of the not-physical-table-name in used_for but haven't seen that locally, and tests seem to pass, so maybe this is ok?

colemanw · 2025-01-28T01:35:00Z

I was slightly wary the schema builder might blow up somewhere because of the not-physical-table-name in used_for but haven't seen that locally, and tests seem to pass, so maybe this is ok?

@ufundo I have an idea of how to pre-empt anybody attempting to use these in the EntityTag table. Just pushed 4f9ab8d

colemanw · 2025-01-28T01:36:41Z

ext/afform/core/managed/AfformTags.mgd.php

+// - creates an example tag that can be used for afforms. This is more to demonstrate
+//   how it can be used, as Donor Journey might be useful for some orgs but not for others
+//   @todo think of better example tag that is more universally applicable?


I think it's better not.

As a general principle I think we should not install sample data onto people's sites unless they specifically select the "sample data" option in the installer.

General principle: definitely agree.

My worry here though is that the "Used For" on Tags isn't discoverable enough, so people might not find how to add tags that they can use here, so I thought this might be a reasonable exception to the rule. (Note we add example Tags by default on new installs. Also Tags are by their nature very easy to throw away, so it feels not such an imposition to add this one?)

But I don't mind, can remove if you're not convinced.

Yea, I had the same concern when I added tagging to SearchKit, so my solution was to create the select-or-create tag widget inspired by GitHub tags (try tagging a searchKit, it's cool).
We could probably extract that widget from SearchKit and reuse it here.

Ooh, yes please. Then I can use it in civirules as well...

Ooh I hadn't seen that before. Definitely a useful widget! For formbuilder too please 😉

Component will definitely need a bit of refactoring because the component is using EntityTag api... but would be great definitely. Happy to remove this in the meantime

(Slightly separate point - I find different orgs have different policies on whether inline tag creation is a useful feature or makes it too easy for thousands of unhelpful tags to proliferate. So might be good if there was some way to configure that... At the moment for quickform tag widgets it seems to vary a bit on different screens...)

ufundo · 2025-01-28T09:59:07Z

ext/afform/core/managed/AfformTags.mgd.php

@@ -26,7 +26,7 @@
        'label' => E::ts('Afforms'),
        'is_reserved' => TRUE,
        'is_active' => TRUE,
-        'domain_id' => NULL,
+        'filter' => 1,


What is filter normally used for ?

Not much of anything.

Could it have a more semantic value? Like filter = 'tableless' and then the condition != tableless?

Oh the table column is an int.. so no

I've added a comment for the significance

And removed the example tag

ufundo · 2025-01-28T10:00:51Z

Oh weird I didn't see these fails before https://test.civicrm.org/job/CiviCRM-Test/45834/testReport/

colemanw · 2025-01-28T13:21:04Z

@ufundo this ought to fix it: 6ac1f0a

ext/search_kit/ang/crmSearchTasks/traits/searchDisplayTasksTrait.service.js

ufundo · 2025-01-28T17:56:32Z

@ufundo this ought to fix it: d74439b

Looks good! 💪

…g table

civibot bot added the master label Jan 27, 2025

colemanw reviewed Jan 27, 2025

View reviewed changes

ext/afform/core/managed/AfformTags.mgd.php Outdated Show resolved Hide resolved

ufundo force-pushed the afform-tags-using-entity branch from 0920a6b to 434ab6f Compare January 27, 2025 21:52

ufundo marked this pull request as ready for review January 27, 2025 21:53

colemanw reviewed Jan 28, 2025

View reviewed changes

ufundo commented Jan 28, 2025

View reviewed changes

ext/search_kit/ang/crmSearchTasks/traits/searchDisplayTasksTrait.service.js Outdated Show resolved Hide resolved

ufundo commented Jan 28, 2025

View reviewed changes

ext/search_kit/ang/crmSearchTasks/traits/searchDisplayTasksTrait.service.js Outdated Show resolved Hide resolved

colemanw force-pushed the afform-tags-using-entity branch from d74439b to 6ac1f0a Compare January 28, 2025 18:01

ufundo added 6 commits January 30, 2025 14:12

Afform Tags - add serialized field to Afform entity

8c618ef

Afform Tags - add to FormBuilder individual form edit screen

31ca37c

Afform Tags - add to FormBuilder afform listing

81da5ff

Afform Tags - add to search kit reports listing

53dbcc7

Afform Tags - use Tag entity rather than option group

19a20ca

Afform Tags - include link to Manage Tags from Form tag edit

e920def

ufundo force-pushed the afform-tags-using-entity branch from 6ac1f0a to 4cbc848 Compare January 30, 2025 14:12

ufundo changed the title ~~add support for tagging Afforms using Tag entity~~ dev/core#5654 add support for tagging Afforms using Tag entity Jan 30, 2025

colemanw added 2 commits January 30, 2025 15:26

AfformTags - Filter out afform tags from being used for sql

68ec9a4

Test fix - limit taggable entities to those supported by the entityTa…

b07350d

…g table

ufundo force-pushed the afform-tags-using-entity branch from 4cbc848 to b07350d Compare January 30, 2025 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#5654 add support for tagging Afforms using Tag entity #31890

dev/core#5654 add support for tagging Afforms using Tag entity #31890

ufundo commented Jan 27, 2025

civibot bot commented Jan 27, 2025

ufundo commented Jan 27, 2025 •

edited

Loading

ufundo commented Jan 27, 2025 •

edited

Loading

colemanw commented Jan 27, 2025

ufundo commented Jan 27, 2025

colemanw commented Jan 28, 2025

colemanw Jan 28, 2025

ufundo Jan 28, 2025

colemanw Jan 28, 2025

mattwire Jan 28, 2025

ufundo Jan 28, 2025

ufundo Jan 28, 2025

colemanw Jan 28, 2025

ufundo Jan 28, 2025

ufundo Jan 30, 2025

ufundo Jan 30, 2025

ufundo Jan 30, 2025

ufundo commented Jan 28, 2025

colemanw commented Jan 28, 2025 •

edited

Loading

ufundo commented Jan 28, 2025

dev/core#5654 add support for tagging Afforms using Tag entity #31890

Are you sure you want to change the base?

dev/core#5654 add support for tagging Afforms using Tag entity #31890

Conversation

ufundo commented Jan 27, 2025

Overview

Before

After

Technical Details

civibot bot commented Jan 27, 2025

ufundo commented Jan 27, 2025 • edited Loading

ufundo commented Jan 27, 2025 • edited Loading

colemanw commented Jan 27, 2025

ufundo commented Jan 27, 2025

colemanw commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ufundo commented Jan 28, 2025

colemanw commented Jan 28, 2025 • edited Loading

ufundo commented Jan 28, 2025

ufundo commented Jan 27, 2025 •

edited

Loading

ufundo commented Jan 27, 2025 •

edited

Loading

colemanw commented Jan 28, 2025 •

edited

Loading