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.

Sign up for GitHub

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

Jump to bottom

Custom field form reform #18419

Merged
merged 3 commits into from
Sep 11, 2020
Merged

Custom field form reform #18419

merged 3 commits into from
Sep 11, 2020

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Sep 9, 2020

Overview

This overhauls the admin form for creating/updating custom fields, with improved validation, easier-to-select defaults, and more flexibility about changing the widget type.

Before

  • Changing the widget type was a separate form

After

  • Changing the widget type integrated into the main form, with improved validation and feedback about option mismatches.

Comments

This PR will need some manual testing. Some workflows to test:

  1. Create a multi-select or checkbox custom field. Populate it with some data for a few contacts, and for at least one contact select multiple options. Go back and change the field type to single-select. You should get a warning about the number of records that will lose data because they contain multiple values.
  2. Create a text input field. Populate a few contacts with data. Go back and try to change it to a field type with an option list. You'll get an error unless the options you entered match the existing data exactly.
  3. Try creating a Country, State, Yes/No, or Date field - setting the default value is now more user-friendly.

@civibot civibot bot added the master label Sep 9, 2020
@civibot
Copy link

civibot bot commented Sep 9, 2020

(Standard links)

@colemanw colemanw mentioned this pull request Sep 9, 2020
Closed
@colemanw colemanw force-pushed the improveCustomFieldForm branch from c707b92 to 1527463 Compare September 9, 2020 16:23
@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor

@highfalutin you've created a monster...

@highfalutin
Copy link
Contributor

@eileenmcnaughton sorry? 😂

@colemanw
Copy link
Member Author

colemanw commented Sep 9, 2020

test this please

@colemanw
Copy link
Member Author

colemanw commented Sep 9, 2020

I know @eileenmcnaughton this is a big diff. (would be smaller but it includes #18356). It's not the sort of thing to review line-by-line as it's overhauling a quickform with a lot of messy code. It just needs to be run.

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Sep 9, 2020 •
edited
Loading

I'm hoping @highfalutin will do UI testing as penance for opening this Pandora's box...

@colemanw colemanw force-pushed the improveCustomFieldForm branch from 1527463 to b3134db Compare September 10, 2020 14:30
@eileenmcnaughton
Copy link
Contributor

Just noting that I agree ui testing is what is required to get this merged. I don't have concerns about the code or test cover & the impact can be judged by testing the form used to manage custom fields & in particular changing between field types

@kcristiano
Copy link
Member

@colemanw Just to be sure before I wade into testing - we do expect data loss on item 1 when converting, am I correct?

I am uneasy about that to be frank. Do we have a gitlab discussion on this?

@colemanw
Copy link
Member Author

@kcristiano This PR is mostly just a UI change. You may want to familiarize yourself with the previous UI, in which it was also possible to change a Multi-Select to a Single-Select (by clicking on "Change input field type" while editing a custom field). Yes, doing so would cause data-loss, and still does. However I think you'll find the new UI in this PR does a better job of warning you about just how much data will be lost before allowing you to proceed.

@kcristiano
Copy link
Member

Thanks @colemanw That makes sense, I'll give this a test.

@kcristiano
Copy link
Member

@colemanw 1 and 3 work exactly as expected.

Create a text input field. Populate a few contacts with data. Go back and try to change it to a field type with an option list. You'll get an error unless the options you entered match the existing data exactly.

I created a text input field and I cannot change the field type at all. Am I doing something wrong, perhpas a screen shot of what is expected here would help.

@colemanw
Copy link
Member Author

1. Create text input field
image

2. Populate some data (which will ultimately match your option values)
image

3. Change the field to a select, matching the option values to the data you entered:
image

If your options don't match existing data you should get an error.

@colemanw
Copy link
Member Author

@kcristiano I don't think it was possible to change text field type prior to this PR, so if you're trying to do a "before" comparison then it won't work. This PR makes it possible and also adds validation to make it safe.

@kcristiano
Copy link
Member

kcristiano commented Sep 11, 2020 •
edited
Loading

--deleted-- problem with local test environment.

@kcristiano
Copy link
Member

@colemanw Never mind. The screens now show up properly - not sure what happened before, but a restart of my local dev has it appearing now:

image

r-run shows this to work as expected.

Would love more UI review, but it looks good to me.

demeritcowboy
demeritcowboy reviewed Sep 11, 2020
View reviewed changes
CRM/Core/DAO/AllCoreTables.php
@@ -280,7 +280,7 @@ public static function getClasses() {
* Get the classname for the table.
*
* @param string $tableName
* @return string
* @return CRM_Core_DAO|NULL
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

The return is a string?

Copy link
Member Author

Choose a reason for hiding this comment

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

For convenience of IDE auto-completion, we kinda hack the @return annotationwhen it's returning a string which is also a class name. In php strings can be used interchangeably with classes, go figure.

@eileenmcnaughton
Copy link
Contributor

@colemanw please self-merge this once you have addressed @demeritcowboy's comments - @kcristiano seems mostly adequate (bar missing the date_type - but we got that spotted now :-))

@colemanw
CustomField - Reformat data when modifying field serialize property.
721c9da 
Adds or removes CRM_Core_DAO::VALUE_SEPARATOR from custom values when switching a field from single to muti-valued.
@colemanw
Custom Field Form - Cleanup country/state leftover cruft
9aea3ce
@colemanw
Custom field admin form reform
9b85cd3 
This overhauls the custom field administration form:
- Gets rid of the difficult-to-use hierarchcal select
- Removes changeFieldType as a separate form
- Allows changing field type on the main form, with improved validation
- Fixes up some metadata
- Improves choosing default values
@colemanw colemanw force-pushed the improveCustomFieldForm branch from b3134db to 9b85cd3 Compare September 11, 2020 18:29
@eileenmcnaughton eileenmcnaughton merged commit a9f031b into civicrm:master Sep 11, 2020
@eileenmcnaughton eileenmcnaughton deleted the improveCustomFieldForm branch September 11, 2020 20:29
@agileware-justin
Copy link
Contributor

https://lab.civicrm.org/dev/core/-/issues/2214

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@demeritcowboy demeritcowboy demeritcowboy left review comments

Assignees
No one assigned
Labels
has-test master merge on pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@colemanw @eileenmcnaughton @highfalutin @kcristiano @agileware-justin @demeritcowboy