Support field type conversion for Multi Choice fields #7356
Conversation
labkey-adam
left a comment
labkey-adam
left a comment
There was a problem hiding this comment.
Just a few questions and minor suggestions (that you can ignore)
| @Override | ||
| public boolean meetsCriteria(ColumnRenderProperties col, Object value, Object[] filterValues) | ||
| { | ||
| throw new UnsupportedOperationException("Conditional formatting not yet supported for Multi Choices"); |
There was a problem hiding this comment.
Seems like "arrays" would be the more general term (vs. "Multi Choices") since these operators could be used in other scenarios. But I see that "Multi Choices" is what we're using in all the existing CompareTypes, so I guess it's fine.
| ColumnInfo columnInfo = getFieldMap().get(key); | ||
| if (columnInfo != null && columnInfo.getPropertyType() == PropertyType.MULTI_CHOICE && value instanceof String strVal) | ||
| { | ||
| // Multi-choice values array is converted to string: "{value1,value2,...}", so strip off the braces before converting |
There was a problem hiding this comment.
Are these comma separated or semi-colon separated?
There was a problem hiding this comment.
This '{a, b}' format is what's returned by MVFK when casting array to string array_to_string(core.sort(array_agg(field))). @labkey-matthewb did mention possibly have MVFK to return ARRAY[] instead of string for the array type columns in the future.
| // Multi-choice values array is converted to string: "{value1,value2,...}", so strip off the braces before converting | ||
| if (strVal.startsWith("{") && strVal.endsWith("}")) | ||
| return ConvertUtils.convert(strVal.substring(1, strVal.length() - 1), columnInfo.getJavaClass()); | ||
| // TODO: return columnInfo.convert(strVal.substring(1, strVal.length() - 1)); |
There was a problem hiding this comment.
Is this TODO for later? Or can it be removed?
There was a problem hiding this comment.
This is to be updated after merging in the related branch that refactored ConverterUtil.
| * @param changes The set of change events. Each QueryPropertyChange is associated with a single table or query. | ||
| */ | ||
| void queryChanged(User user, Container container, ContainerFilter scope, SchemaKey schema, @NotNull QueryProperty property, @NotNull Collection<QueryPropertyChange<?>> changes); | ||
| void queryChanged(User user, Container container, ContainerFilter scope, SchemaKey schema, @Nullable String queryName, @NotNull QueryProperty property, @NotNull Collection<QueryPropertyChange<?>> changes); |
There was a problem hiding this comment.
Just curious why queryName had to be added. Each QueryPropertyChange has a QueryDefinition with the name.
There was a problem hiding this comment.
Good point. I'll revert this change and have the queryDef pass the info instead.
| return filterStr; | ||
|
|
||
| // Only act when changing away from MULTI_CHOICE | ||
| if (oldType.getPropertyType() != PropertyType.MULTI_CHOICE || newType.getPropertyType() == PropertyType.MULTI_CHOICE) |
There was a problem hiding this comment.
This method is called only when oldType.getPropertyType() == PropertyType.MULTI_CHOICE, so the first check isn't technically needed.
| return filterStr; | ||
|
|
||
| // Only act when changing to MULTI_CHOICE | ||
| if (oldType.getPropertyType() == PropertyType.MULTI_CHOICE || newType.getPropertyType() != PropertyType.MULTI_CHOICE) |
There was a problem hiding this comment.
This method is called only when newType.getPropertyType() == PropertyType.MULTI_CHOICE, so the second check isn't technically needed.
| SQLFragment rename = new SQLFragment("ALTER TABLE "); | ||
| rename.appendIdentifier(change.getSchemaName()).append(".").appendIdentifier(change.getTableName()); | ||
| rename.append(" RENAME COLUMN ").appendIdentifier(tempColumnIdent).append(" TO ").appendIdentifier(columnIdent); | ||
| statements.add(rename); |
There was a problem hiding this comment.
Are admins allowed to add indexes or unique constraints to multichoice fields? I hope not, but obviously they'd get dropped here.
There was a problem hiding this comment.
We don't allow creating index on UI for MC field types. We also show a warning when converting from/to multi choice that configured conditional formatting will be lost. We can modify the message to say that index will be lost as well.
There was a problem hiding this comment.
I'd say don't bother updating the message. It's pretty obscure and few would expect indexes or unique constraints to be maintained on a multi-choice.
| } | ||
| else if (column.getJdbcType() == JdbcType.ARRAY) | ||
| { | ||
| // Converting from text to text[] requires an intermediate column and transformation |
There was a problem hiding this comment.
Admin could have created an index or unique constraint on this text field, right? Is it okay if the index/constraint is dropped?
Rationale
This pr supports converting Multi Choice field types to/from other field types (primarily text, text choice).
Related Pull Requests
Changes