PORTALS-3272: Add multi-select dropdown menu to observation pages to filter

[kianamcc]

Jira Ticket: https://sagebionetworks.jira.com/browse/PORTALS-3272?atlOrigin=eyJpIjoiMjFjMGU0MDJiZWZmNDU2NmI1NmE3YTBlYzRkOGVhY2UiLCJwIjoiaiJ9

[jay-hodgson]

I don't think this logic belongs here. Am I correct in assuming that in this case it will render many ObservationCard components? Could this logic be moved to the ObservationCard (if it should be filtered out, then may it returns an empty react fragment?)?

[nickgros]

At the very least, this logic should be less specific to ObservationCards, and should utilize the Synapse tables faceted column features to get filter values and apply the filter to the rows server-side.

[nickgros]

I think a good example of a similar component is the QuerySortSelector, which renders a similar control to toggle the sort of data rendered by CardContainerLogic. A similar pattern could be followed to render this filter control.

[nickgros]

Instead of filtering client-side, we should make this new UI control apply a facet filter and pass it to Synapse in the QueryBundleRequest.

To actually get the valid observationType, the Observations MaterializedView should be updated to make observationType a faceted column. Once that's done, Synapse will give us the set of observationType values in the QueryResultBundle.facets object.

[nickgros]

New file is very close to what I was expecting and is so much less code than the original implementation! I had some questions and pointed out opportunities to clean up

[nickgros]

Commented out on purpose?

[nickgros]

Is the only change here to extract this renderCards method and change the if/else to switch? Is this necessary?

[nickgros]

Nice! Super simple addition here.

[nickgros]

Prefer using function and removing React.FC (which I think is deprecated)

Suggested change

	const ColumnFilter: React.FC<FilterProps> = props => {
	function ColumnFilter (props: FilterProps) {

[nickgros]

Could you add some tests for this component? The QuerySortSelector has a corresponding QuerySortSelector.test.tsx file that you can use as a base. Unfortunately, that one is older, so it's not type-checked and co-located with the QuerySortSelector component file.

For the ColumnFilter.test.tsx, could you co-locate the file with your new component file, and ensure it passes typechecking?

[nickgros]

Could you convert this to use MUI's Autocomplete instead of using Bootstrap 4 + react-select? In general we are trying to migrate away from Bootstrap, and while we still use react-select, prefer to use MUI when possible.

[nickgros]

I think you can just do something like

removeSelectedFacet(columnName)
values.forEach(value => addValueToSelectedFacet(columnName, value))

Where removeSelectedFacet and addValueToSelectedFacet can be retrieved from the queryContext object.

[nickgros]

Instead of storing the selection in state, we can make the source of truth the current query and eliminate this state variable.

You can do something like

const selectedFacet = getCorrespondingSelectedFacet({columnName}, currentQuery.query.selectedFacets)
const selectedValues = (selectedFacet as FacetColumnValuesRequest).facetValues

getCorrespondingSelectedFacet is a utility function you can import.

[nickgros]

Why do you need this SQL? Sorry, I don't totally understand the feature

[nickgros]

We can use the ROW_ID meta-column, so here we can collect the row IDs

Suggested change

	const observationIds = selectedRows
	.map(row => row.values[0]) // Get observationId - identifier but not all rows have this
	.filter(observationId => observationId)
	.map(observationId => `'${observationId}'`)
	const observationTexts = selectedRows
	.filter(row => !row.values[0]) // Only take rows without observationId
	.map(row => row.values[4]) // Get observationText - all rows have this
	.filter(text => text)
	.map(text => `'${text}'`)
	let sql = ''
	if (!observationIds.length) {
	sql = `SELECT * FROM syn51735464
	WHERE observationText IN (${observationTexts.join(', ')})`
	} else {
	sql = `SELECT * FROM syn51735464
	WHERE observationId IN (${observationIds.join(', ')})${
	observationTexts.length > 0
	? ` OR observationText IN (${observationTexts.join(', ')})`
	: ''
	}`
	}
	const selectedRowIds = selectedRows.map(row => row.rowId!)

[nickgros]

We can add additional filters using the searchParams prop (which needs a better name/API design, but that would be a massive change).

We add the sqlOperator prop to tell our code to add the searchParams as a filter with the IN clause.

And we can also add the lockedColumn to hide any UI that would indicate the ROW_ID filter (it may not make a difference now, but we can add it to be safe)

Suggested change

	<CardContainerLogic
	filterColumnName="observationType"
	sql={sql}
	type={OBSERVATION_CARD}
	initialLimit={3}
	/>
	<CardContainerLogic
	filterColumnName="observationType"
	sql={observationsSql}
	searchParams={{["ROW_ID"]: selectedRowIds}}
	sqlOperator="IN"
	lockedColumn={{ columnName: "ROW_ID" }}
	type={OBSERVATION_CARD}
	initialLimit={3}
	/>

[nickgros]

This test isn't necessary--we have a few existing tests like these but most of them are vestiges of when our component tests were very basic.

You may also want to remove the column-filter test ID. It seems like it isn't necessary.

Suggested change

	it('renders the ColumnFilter component correctly', () => {
	render(ColumnFilterComponent)
	expect(screen.getByTestId('column-filter')).toBeInTheDocument()
	})

[nickgros]

Suggested change

	const columnFilterDiv = screen.getByTestId('column-filter')
	expect(columnFilterDiv).toBeInTheDocument()

[nickgros]

I like how you approached making this a unit test. That said, we've tended to avoid mocking the entire react-query module because it becomes difficult to create mocks in components that contain multiple instances of useQuery. If we were to extend this component with another query, we'd have to totally change this mock approach, building dynamic logic into useQuery mock.

Instead, I think you could continue to extend the QueryContext mock by making the queryMetadataQueryOptions something like this:

{
  queryKey: ['queryMetadataQueryOptions']
  queryFn: () => data
}

It looks like we've actually done this in another test, FullTextSearch.test.ts

[nickgros]

We don't typically do it, but I like how you mocked useQueryContext instead of using the real one, so no need to change that.

[nickgros]

When you use the real react-query, you'll need to wrap the component under test in a QueryContext. We created a context wrapper for all tests, so when you call render, you can just do

render(<ColumnFilter {...props} />, { wrapper: createWrapper() })

createWrapper will come from TestingLibraryUtils.tsx

[nickgros]

You can get type-safe mocks with the following pattern:

const mockUseQueryContext = jest.mocked(useQueryContext)

Suggested change

	describe('ColumnFilter tests', () => {
	const executeQueryRequest = jest.fn()
	const addValueToSelectedFacet = jest.fn()
	const removeSelectedFacet = jest.fn()
	beforeEach(() => {
	;(useQueryContext as jest.Mock).mockReturnValue({
	describe('ColumnFilter tests', () => {
	const executeQueryRequest = jest.fn()
	const addValueToSelectedFacet = jest.fn()
	const removeSelectedFacet = jest.fn()
	beforeEach(() => {
	mockUseQueryContext.mockReturnValue({

Please remove all of these unnecessary casts where you could use jest.mocked

[nickgros]

You should recreate the React element in each test. You can share props between tests if you want to keep the verbosity down, e.g.

const props = (
    filterColumnName: "program",
    removeSelectedFacet: removeSelectedFacet,
    addValueToSelectedFacet: addValueToSelectedFacet
}

render(<ColumnFilter {...props} />)

[nickgros]

Suggested change

	// const addValueToSelectedFacet = queryContext.addValueToSelectedFacet
	// const addValueToSelectedFacet = queryContext.removeSelectedFacet

[nickgros]

filterColumnName will always be defined.

Also, remove the other unused props for the functions that are coming from context

Suggested change

	export type FilterProps = {
	filterColumnName?: string
	addValueToSelectedFacet?: (
	facet: { columnName: string },
	value: string,
	) => void
	removeSelectedFacet?: (facet: { columnName: string }) => void
	}
	export type FilterProps = {
	filterColumnName: string
	}

[nickgros]

filterColumnName is always defined (after you update the prop type)

Suggested change

	{ columnName: filterColumnName || '' },
	{ columnName: filterColumnName },

[nickgros]

filterColumnName is always defined (after you update the prop type)

Suggested change

	const facetIdentifier = { columnName: filterColumnName || '' }
	const facetIdentifier = { columnName: filterColumnName }

[nickgros]

If you aren't using MUI-specific styling logic, div is the same as Box with less overhead. The testid is unnecessary after you make suggested text changes

Suggested change

	<Box data-testid="column-filter">
	<div>

[nickgros]

Suggested change

	</Box>
	</div>

[nickgros]

1. Since we changed the prop to be a UniqueFacetIdentifier, you can just pass it in here
2. You shouldn't need to call executeQueryRequest after calling removeSelectedFacet/addValueToSelectedFacet

Suggested change

	const facetIdentifier = {
	columnName: topLevelEnumeratedFacetToFilter.columnName,
	}
	removeSelectedFacet(facetIdentifier)
	values.forEach(value => addValueToSelectedFacet(facetIdentifier, value))
	executeQueryRequest(request => request)
	removeSelectedFacet(topLevelEnumeratedFacetToFilter)
	values.forEach(value => addValueToSelectedFacet(topLevelEnumeratedFacetToFilter, value))

[nickgros]

You can just pass this since it is a facet identifier

Suggested change

	{ columnName: topLevelEnumeratedFacetToFilter.columnName },
	topLevelEnumeratedFacetToFilter,

[nickgros]

fantastic work!