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

Add custom field type Hidden #27235

Merged
merged 1 commit into from Oct 16, 2023
Merged

Add custom field type Hidden #27235

merged 1 commit into from Oct 16, 2023

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Aug 31, 2023

Overview

Adds a "hidden" custom field type which can collect data on forms without being seen (e.g. by storing a value from the url or set by javascript)

Technical Details

  • Adds a custom field html_type (aka input_type) of "hidden"
  • Also adds a "hidden" input_type to FormBuilder
  • This effectively hides the field and the label on any core form, profile, or FormBuilder form.
  • This improves the flexibility of FormBuilder. FormBuilder already allows input_types to be changed on a per-form basis, and this adds to the available options. So hidden fields can be un-hidden (widget changed to Textfield) and conversely, regular fields can now be changed to hidden.
  • Profiles aren't very flexible, and don't allow html_types to be changed. A hidden field will be that way on every profile.

@civibot
Copy link

civibot bot commented Aug 31, 2023

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

➡️ Online demo of this PR 🔗

@larssandergreen
Copy link
Contributor

Comparing to the TrackingFields, I think users will have the reasonable expectation that a hidden custom field won't show up in a emails, but I think with this all the message templates that add all custom fields are going to include these hidden fields.

So if you have a hidden field for tracking where donors clicked, that's going to be sent to donors in their contribution receipt.

@MegaphoneJon
Copy link
Contributor

I like this - but I think we need to spec this out a bit before committing to an approach.

I think the appropriate replacement for tracking fields isn't a "hidden" custom field type, but an "is hidden" checkbox on the profile field configuration.

"Hidden" IMO should be an HTML type, not a data type. This allows us to maintain strict data typing in API4, and overall seems more likely to play well with other code.

Do hidden fields appear in SK? I'd lean yes. Should they be editable? I suppose so. But maybe Eileen's is_hidden suggestion is better because we could maintain pseudoconstants for SK editing? I'm undecided.

@MegaphoneJon
Copy link
Contributor

Also pinging @twomice since CRM-20922 suggests he has an interest

@colemanw
Copy link
Member Author

colemanw commented Sep 1, 2023

@MegaphoneJon I've expanded the PR description to clarify that this does add a new html_type not a data_type, and FormBuilder does allow those to be changed on a per-form basis.

@MegaphoneJon
Copy link
Contributor

OK, I accidentally got some real-world testing on this because I forgot this is part of the Megaphone Civi flavor.

Hidden fields work as expected in profiles. Yay!
On backend QuickForms of type "Page", the label appears, but the value does not. On QuickForms of type "Form", neither the label nor value appears.

In SearchKit, attempting to use these in the WHERE clause just gives an endless spinning circle. Adding it as a column does not display a result. However, you can use Rewrite on a display to show the "raw" value, not the label, and that works.

I think there's also some interaction here between the former HTML type and the current one. In the examples above, the field has an option_group_id set. I wonder if this should be a disallowed configuration.

@colemanw
Copy link
Member Author

@MegaphoneJon thanks for the feedback. I've updated the PR and I think it addresses everything you noted.

Comment on lines -136 to +138
switch ($customFieldBAO->data_type) {
case "String":
case "Int":
case "Float":
case "Money":
// if Multi Select field is selected in custom field
if ($customFieldBAO->html_type == 'Text') {
$action -= CRM_Core_Action::BROWSE;
}
break;

case "ContactReference":
case "Memo":
case "Date":
case "Boolean":
case "StateProvince":
case "Country":
case "File":
case "Link":
$action -= CRM_Core_Action::BROWSE;
break;
// Remove link to edit option group if there isn't one
if (!$customFieldBAO->option_group_id) {
$action -= CRM_Core_Action::BROWSE;
Copy link
Member Author

Choose a reason for hiding this comment

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

This absolutely takes the prize for the most bizarre waste of 20 lines I've ever seen.

Copy link
Member Author

Choose a reason for hiding this comment

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

🎉 🎉 Why I'm awarding this one top prize 🏆 🥇 🏆

  1. Switch statement: why even... dear god why?
  2. The code comment made absolutely no sense.
  3. The rest of the switch statement: LOL
  4. None of it had anything to do with the actual criteria that needed to be checked

@mattwire mattwire merged commit ec33a6d into civicrm:master Oct 16, 2023
3 checks passed
@colemanw colemanw deleted the hidden branch October 16, 2023 21:32
@MegaphoneJon
Copy link
Contributor

Further feedback from user testing:

  • Can't edit the data in the back end (right now the hidden field is not visible when editing a contribution)
  • Can't view the hidden field as a searchkit column (the column is just blank)

But also that overall the last round of changes were all successfully tested.

@colemanw
Copy link
Member Author

colemanw commented Oct 18, 2023

Can't edit the data in the back end (right now the hidden field is not visible when editing a contribution)

I wouldn't expect a hidden field to be editable. I imagine if we changed it to type text then we'd get bug reports that (IMO correctly) complain that hidden fields should be of type hidden not text.

Can't view the hidden field as a searchkit column (the column is just blank)

That does sound like a bug. I'll check...

@colemanw
Copy link
Member Author

@MegaphoneJon sorry I'm unable to reproduce the 2nd bug you mentioned. When I create a new hidden custom field and enter some data (via the API since you can't do it in the UI) I'm able to see it both in the UI and in SearchKit:

image

image

However, if the field was created prior to that last bugfix (the one that took the prize) then you should check civicrm_custom_field to make sure the field does not have an option_group_id and update it if it does to set that column to null and delete the bogus option group.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants