Support field type conversion for Multi Choice fields#1927
Support field type conversion for Multi Choice fields#1927
Conversation
| if (!isMultiField && !isValidTextChoiceValue(value)) return; | ||
|
|
||
| const values: string[] = []; | ||
| if (isMultiField && Array.isArray(value)) { | ||
| values.push(...value); | ||
| hasMultiValue = hasMultiValue || value.length > 1; | ||
| } else { | ||
| values.push(value); | ||
| } | ||
|
|
||
| const rowLocked = row.IsLocked.value === 1; | ||
| const rowCount = row.RowCount.value; | ||
|
|
||
| values.forEach(val => { | ||
| if (!useCount[val]) { | ||
| useCount[val] = { count: 0, locked: false }; | ||
| } | ||
| useCount[val].count += rowCount; | ||
| useCount[val].locked = useCount[val].locked || rowLocked; | ||
| }); |
There was a problem hiding this comment.
This code block looks to be almost an exact duplicate of the code above for the if statement. Can this be factored out to be shared by both?
| ); | ||
| expect(document.body).toHaveTextContent( | ||
| 'This change will convert the values in the field from dateTime to date. This will cause the Time portion of the value to be removed.' | ||
| 'Confirm Data Type ChangeThis change will convert the values in the field from Text Choice (single select) to Text Choice (multiple select). Filters in saved views might not function as expected and any conditional formatting configured for this field will be removed.' |
There was a problem hiding this comment.
minor but when I look at this in the UI the "Single Select" and "Multiple Select" text is capitalized. Does that matter for this test case?
There was a problem hiding this comment.
Good catch. I don't think it matters in this case.
| if ( | ||
| shouldShowConfirmDataTypeChange( | ||
| field.original.rangeURI ?? field.original.conceptURI, | ||
| typeConvertingTo.rangeURI ?? typeConvertingTo.conceptURI |
There was a problem hiding this comment.
I was thinking the same thing as copilot here. Should the conceptURI take precedence over the rangeURI? won't all fields have a rangeURI, which means it will never fall back to the conceptURI?
| showFilePropertyType: boolean, | ||
| showStudyPropertyTypes: boolean | ||
| ): boolean { | ||
| if (type.hideFromDomainRow) return false; |
There was a problem hiding this comment.
why add this new hideFromDomainRow property when we already have type specific checks below? can't we just say "if type is multi choice text, return false"?
| import { DomainField } from './models'; | ||
| import { MULTI_CHOICE_RANGE_URI } from './constants'; | ||
|
|
||
| describe('TextChoiceOptions', () => { |
There was a problem hiding this comment.
Hurray for another spec file converted! Just curious, did you convert this manually or did you use AI in some capacity? just want to see what others are doing / using.
There was a problem hiding this comment.
Yep this was converted using AI. I first tried Junie but it wasn't successful. But using copilot (Gemini 3) it was a very smooth convert.
| type="checkbox" | ||
| /> | ||
| <span | ||
| title={ |
There was a problem hiding this comment.
will this span title be discoverable by the users? or should this be a ? tooltip type of display?
Oh, is this just for the disabled checkbox case?
| return Filter.create(fieldKey, [], oldFilter.getFilterType()); | ||
| if (allValues && newCheckedValues.length >= allValues.length) { | ||
| return Filter.create(fieldKey, allValues.filter(v => v !== ALL_VALUE_DISPLAY), oldFilter.getFilterType()); | ||
| } |
There was a problem hiding this comment.
I don't know much about this change, but should there be a related jest test for this?
| tableAlias?: string | ||
| ): string { | ||
| const filterType = filter.getFilterType().getURLSuffix(); | ||
| const columnNameSelect = getLegalIdentifier(filter.getColumnName(), tableAlias); |
There was a problem hiding this comment.
there are several calls like this in this file. Seems like each filter type re does this columnNameSelect, which seems like it could be error prone. Thoughts on having just the call done in the begining of the getFilterLabKeySql function and then passing this through to the other usages? (maybe a TODO for another PR?)
…e advanced setting, fix styling
2 participants
Rationale
Instead of two separate data types, this PR modifies TextChoice so multi choice is no longer an explicit option in designer, but rather, can be enabled to checking a "multi" check box after selecting text choice option.
Related Pull Requests
Changes