Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions packages/components/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions packages/components/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@labkey/components",
"version": "7.28.1",
"version": "7.28.2-fb-mvtcBatch3.1",
"description": "Components, models, actions, and utility functions for LabKey applications and pages",
"sideEffects": false,
"files": [
Expand Down Expand Up @@ -53,7 +53,7 @@
"homepage": "https://github.com/LabKey/labkey-ui-components#readme",
"dependencies": {
"@hello-pangea/dnd": "18.0.1",
"@labkey/api": "1.51.0",
"@labkey/api": "1.51.1-mvtcBatch3.1",
"@testing-library/dom": "~10.4.1",
"@testing-library/jest-dom": "~6.9.1",
"@testing-library/react": "~16.3.2",
Expand Down
7 changes: 7 additions & 0 deletions packages/components/releaseNotes/components.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
# @labkey/components
Components, models, actions, and utility functions for LabKey applications and pages

### version 7.X
*Released*: X 2026
- GitHub Issue 928: Spaces not shown between text choices in identifying fields in editable grid
- GitHub Issue 951: Multi-line values converted to text choices lose multi-line editability
- GitHub Issue 970: Warn about more than 10 items for multi choice fields in editable grid.
- GitHub Issue 987: Multi value filter dialog lets you edit and save without any selected values

### version 7.28.1
*Released*: 3 April 2026
- GitHub Issue 613: Use "Status" instead of "SampleState" in error messaging.
Expand Down
2 changes: 2 additions & 0 deletions packages/components/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,7 @@ import {
getDataDeleteConfirmationData,
getEntityTypeOptions,
getExcludedDataTypeNames,
getFieldDisplayValue,
getOrderedSelectedMappedKeysFromQueryModel,
getParentTypeDataForLineage,
getSampleIdentifyingFieldGridData,
Expand Down Expand Up @@ -1353,6 +1354,7 @@ export {
getEntityTypeOptions,
getEventDataValueDisplay,
getExcludedDataTypeNames,
getFieldDisplayValue,
getExpandQueryInfo,
getFieldFiltersValidationResult,
getFilterForSampleOperation,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,12 @@ describe('PropDescType', () => {
expect(acceptablePropertyType(DATETIME_TYPE, TIME_RANGE_URI)).toBeFalsy();
expect(acceptablePropertyType(DATE_TYPE, TIME_RANGE_URI)).toBeFalsy();
expect(acceptablePropertyType(TIME_TYPE, DATE_RANGE_URI)).toBeFalsy();

// GitHub Issue 951: multiline text cannot convert to text choice
expect(acceptablePropertyType(TEXT_CHOICE_TYPE, MULTILINE_RANGE_URI)).toBeFalsy();
// but multiline text can still convert to other string types
expect(acceptablePropertyType(TEXT_TYPE, MULTILINE_RANGE_URI)).toBeTruthy();
expect(acceptablePropertyType(MULTILINE_TYPE, MULTILINE_RANGE_URI)).toBeTruthy();
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import {
LOOKUP_VALIDATOR_VALUES,
MAX_TEXT_LENGTH,
MULTI_CHOICE_RANGE_URI,
MULTILINE_RANGE_URI,
NUMBER_CONVERT_URIS,
PHILEVEL_NOT_PHI,
SAMPLE_TYPE_CONCEPT_URI,
Expand Down Expand Up @@ -1778,7 +1779,8 @@ export function acceptablePropertyType(newType: PropDescType, originalRangeURI:

// Original field is a uniqueId, text, or multi-line text, can convert to a string type
if (newType.isString() && PropDescType.isString(originalRangeURI)) {
return true;
// GitHub Issue 951: Multi-line values converted to text choices lose multi-line editability
return !(MULTILINE_RANGE_URI === originalRangeURI && newType.isTextChoice());
}

// Original field is a datetime, can convert to a date or time
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,9 @@ export class EditableGrid extends PureComponent<EditableGridProps, EditableGridS
} else if (mod === MODIFICATION_TYPES.REPLACE) {
changes.cellValues = cellValues.set(cellKey, List(newValues));
changesMade = true;
const { message } = getValidatedEditableGridValue(newValues[0].display, column);
const col = columnMap.get(parseCellKey(cellKey).fieldKey);
const isMultiChoiceCol = col?.isMultiChoice;
const { message } = getValidatedEditableGridValue(isMultiChoiceCol ? newValues : newValues[0].display, column ?? col);
changes.cellMessages = cellMessages.set(cellKey, message);
} else if (mod === MODIFICATION_TYPES.REMOVE) {
let values: List<ValueDescriptor> = editorModel.getIn(keyPath);
Expand Down
14 changes: 12 additions & 2 deletions packages/components/src/internal/components/editable/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,12 @@ async function convertRowToEditorModelData(
}
} else if (col.isMultiChoice && Array.isArray(data)) {
const values = data.filter(item => !!item).map(item => ({ raw: item, display: item }));
if (values.length > 10) {
// GitHub Issue 970
message = {
message: 'Too many values. Maximum allowed is 10.',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should add the github issue number as a comment here as well (as done below)

}
}
valueDescriptors.push(...values);
} else {
let display = data;
Expand Down Expand Up @@ -535,8 +541,7 @@ async function prepareInsertRowDataFromBulkForm(
const col = insertColumns[colIdx];
const { message, valueDescriptors } = await convertRowToEditorModelData(data, col, containerPath);
values = values.push(valueDescriptors);

if (message) messages = messages.push(message);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

curious why this if statement was removed? wouldn't we want to check if the message existed before adding it to the list of messages? or is this so that we keep the list items in order between the values and the messages variables?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, this is removed so we keep values and messages aligned with actual cell. The old behavior results in message being shifted to different cells.

messages = messages.push(message);
}

return {
Expand Down Expand Up @@ -1592,6 +1597,11 @@ async function insertPastedData(
unmatched.push(vt);
});

if (values.length > 10) {
// GitHub Issue 970
msg = { message: 'Too many values. Maximum allowed is 10.' };
}

if (unmatched.length) {
const valueStr = unmatched
.slice(0, 4)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { List, Map } from 'immutable';
import { QueryInfo } from '../../../public/QueryInfo';
import { QueryColumn, QueryLookup } from '../../../public/QueryColumn';

import { DATE_RANGE_URI, NON_NEGATIVE_NUMBER_CONCEPT_URI } from '../domainproperties/constants';
import { DATE_RANGE_URI, MULTI_CHOICE_RANGE_URI, NON_NEGATIVE_NUMBER_CONCEPT_URI } from '../domainproperties/constants';

import sampleSetQueryInfoJSON from '../../../test/data/sampleSetAllFieldTypes-getQueryDetails.json';

Expand Down Expand Up @@ -390,6 +390,77 @@ describe('getValidatedEditableGridValue', () => {
});
});

test('multi-choice column with valid values', () => {
const multiChoiceCol = new QueryColumn({
jsonType: 'string',
rangeURI: MULTI_CHOICE_RANGE_URI,
validValues: ['alpha', 'beta', 'gamma'],
});

const validArray = [{ display: 'alpha' }, { display: 'beta' }];
expect(getValidatedEditableGridValue(validArray, multiChoiceCol)).toStrictEqual({
message: undefined,
value: validArray,
});
});

test('multi-choice column with invalid choice', () => {
const multiChoiceCol = new QueryColumn({
jsonType: 'string',
rangeURI: MULTI_CHOICE_RANGE_URI,
validValues: ['alpha', 'beta', 'gamma'],
});

const invalidArray = [{ display: 'alpha' }, { display: 'invalid' }];
expect(getValidatedEditableGridValue(invalidArray, multiChoiceCol)).toStrictEqual({
message: { message: "'invalid' is not a valid choice" },
value: invalidArray,
});
});

test('multi-choice column with duplicate choice', () => {
const multiChoiceCol = new QueryColumn({
jsonType: 'string',
rangeURI: MULTI_CHOICE_RANGE_URI,
validValues: ['alpha', 'beta', 'gamma'],
});

const invalidArray = [{ display: 'alpha' }, { display: 'alpha' }];
expect(getValidatedEditableGridValue(invalidArray, multiChoiceCol)).toStrictEqual({
message: { message: "Duplicate values not allowed: alpha." },
value: invalidArray,
});
});

test('multi-choice column exceeding 10 items', () => {
const multiChoiceCol = new QueryColumn({
jsonType: 'string',
rangeURI: MULTI_CHOICE_RANGE_URI,
validValues: ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k'],
});

const tooMany = Array.from({ length: 11 }, (_, i) => ({ display: String.fromCharCode(97 + i) }));
expect(getValidatedEditableGridValue(tooMany, multiChoiceCol)).toStrictEqual({
message: { message: 'Too many values. Maximum allowed is 10.' },
value: tooMany,
});
});

test('multi-choice column with exactly 10 items is valid', () => {
const values = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j'];
const multiChoiceCol = new QueryColumn({
jsonType: 'string',
rangeURI: MULTI_CHOICE_RANGE_URI,
validValues: values,
});

const tenItems = values.map(v => ({ display: v }));
expect(getValidatedEditableGridValue(tenItems, multiChoiceCol)).toStrictEqual({
message: undefined,
value: tenItems,
});
});

test('required column', () => {
const requiredCol = new QueryColumn({ jsonType: 'string', required: true, caption: 'ReqCol' });

Expand Down
25 changes: 25 additions & 0 deletions packages/components/src/internal/components/editable/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ interface ValidatedValue {
export const getValidatedEditableGridValue = (origValue: any, col: QueryColumn): ValidatedValue => {
// col ?? {} so it's safe to destructure
const { caption, isDateOnlyColumn, jsonType, required, scale, validValues } = col ?? {};
const isMultiChoice = col?.isMultiChoice;
const isDateTimeType = jsonType === 'date';
const isDateType = isDateTimeType && isDateOnlyColumn;
let message;
Expand All @@ -50,6 +51,30 @@ export const getValidatedEditableGridValue = (origValue: any, col: QueryColumn):
message = `Invalid ${noun}, use format ${dateFormat}`;
}
value = dateStrVal ?? origValue;
} else if (isMultiChoice && Array.isArray(origValue)) {
if (origValue.length > 10) {
// GitHub Issue 970
message = 'Too many values. Maximum allowed is 10.';
}
else if (validValues) {
const seen = new Set();
origValue.forEach(val => {
if (message)
return;

const trimmed = val.display?.toString().trim();
if (seen.has(trimmed)) {
message = `Duplicate values not allowed: ${trimmed}.`
}
else {
seen.add(trimmed);
if (validValues.indexOf(trimmed) === -1) {
message = `'${trimmed}' is not a valid choice`;
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

minor but if a value is both a duplicate and invalid, the invalid message will override the duplicate. I assume this is fine and intentional but might be worth changing to an if invalid / else if duplicate to make the logic clear.

})
}

} else if (value != null && value !== '' && !col?.isPublicLookup()) {
if (validValues) {
const trimmed = origValue?.toString().trim();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Map } from 'immutable';

import { extractEntityTypeOptionFromRow, getChosenParentData, sampleGenCellKey } from './actions';
import { extractEntityTypeOptionFromRow, getChosenParentData, getFieldDisplayValue, sampleGenCellKey } from './actions';
import { EntityDataType, EntityIdCreationModel } from './models';
import { DataClassDataType, SampleTypeDataType } from './constants';

Expand Down Expand Up @@ -86,3 +86,33 @@ describe('sampleGenCellKey', () => {
expect(sampleGenCellKey(null, 'Ancestors/Sources/Study', 1)).toBe('ancestors/sources/study&&1');
});
});

describe('getFieldDisplayValue', () => {
test('returns formattedValue when available', () => {
expect(getFieldDisplayValue({ formattedValue: 'formatted', displayValue: 'display', value: 'raw' })).toBe('formatted');
});

test('falls back to displayValue when formattedValue is undefined', () => {
expect(getFieldDisplayValue({ displayValue: 'display', value: 'raw' })).toBe('display');
});

test('falls back to value when formattedValue and displayValue are undefined', () => {
expect(getFieldDisplayValue({ value: 'raw' })).toBe('raw');
});

test('joins array values with comma and space', () => {
expect(getFieldDisplayValue({ value: ['a', 'b', 'c'] })).toBe('a, b, c');
});

test('joins array formattedValue with comma and space', () => {
expect(getFieldDisplayValue({ formattedValue: ['x', 'y'] })).toBe('x, y');
});

test('returns single string value as-is', () => {
expect(getFieldDisplayValue({ value: 'single' })).toBe('single');
});

test('handles single-element array', () => {
expect(getFieldDisplayValue({ value: ['only'] })).toBe('only');
});
});
11 changes: 10 additions & 1 deletion packages/components/src/internal/components/entities/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1363,6 +1363,15 @@ export function getSingleSampleTypeQueryInfo(sampleIds: number[] | string[]): Pr
});
});
}

// GitHub Issue 928: Spaces not shown between text choices in identifying fields in editable grid
export function getFieldDisplayValue(fieldData: any): string {
const val = fieldData.formattedValue ?? fieldData.displayValue ?? fieldData.value;
if (Array.isArray(val))
return val.join(', ');
return val;
}

export function getSampleIdentifyingFieldGridData(
sampleIds: number[] | string[],
sampleQueryInfo?: QueryInfo,
Expand Down Expand Up @@ -1396,7 +1405,7 @@ export function getSampleIdentifyingFieldGridData(
// Issue 52038: the Row has data keyed by name so make sure we do the same here (see QueryColumn index() comments)
const colData = caseInsensitive(row, c.name);
if (colData?.value != null) {
d[c.index] = colData?.formattedValue ?? colData?.displayValue ?? colData?.value;
d[c.index] = getFieldDisplayValue(colData);
}
});
samplesData[rowId] = d;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,10 @@ const OptionRenderer: FC<OptionRendererProps> = props => {
if (item !== undefined) {
let text = resolveDetailFieldLabel(item.get(column.name));
if (!Utils.isString(text)) {
text = text != null ? text.toString() : '';
if (text == null)
text = '';
else if (Array.isArray(text))
text = text.join(', ');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need the else here to retain the part about text = text.toString()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it's safe to drop that. text is either string | string[] or null(ish), the logic should have covered all cases.

}

return (
Expand Down
16 changes: 15 additions & 1 deletion packages/components/src/public/QueryModel/GridFilterModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,21 @@ export const GridFilterModal: FC<Props> = memo(props => {

if (newFilters) {
newFilters
?.filter(newFilter => newFilter !== null)
?.filter(newFilter => {
if (newFilter === null)
return false;

const filterType = newFilter.getFilterType();
if (filterType.getURLSuffix().toLowerCase().startsWith('array') && filterType.isMultiValued()) {
const filterValue = newFilter.getValue();
// GitHub Issue 987: Multi value filter dialog lets you edit and save without any selected values
if (!filterValue || (Array.isArray(filterValue) && filterValue.length === 0)) {
return false;
}
}
return true;

})
.forEach(newFilter => {
updatedFilters.push({
fieldKey: activeFieldKey,
Expand Down
Loading
Loading