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.

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

Improve search of authorized and unauthorized studies #4521

Closed
Closed
Show file tree
Hide file tree
Changes from 9 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
18 changes: 10 additions & 8 deletions src/shared/components/query/filteredSearch/Phrase.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -125,13 +125,14 @@ export class ListPhrase implements Phrase {
public match(study: CancerTreeNode): boolean {
let anyFieldMatch = false;
for (const fieldName of this.fields) {
let anyPhraseMatch = false;
if (!_.has(study, fieldName)) {
continue;
}
const fieldValue = (study as any)[fieldName];
if (fieldValue) {
for (const phrase of this._phraseList) {
anyPhraseMatch =
anyPhraseMatch || matchPhraseFull(phrase, fieldValue);
}
let anyPhraseMatch = false;
for (const phrase of this._phraseList) {
anyPhraseMatch =
anyPhraseMatch || matchPhraseFull(phrase, fieldValue);
}
anyFieldMatch = anyFieldMatch || anyPhraseMatch;
}
Expand Down Expand Up @@ -162,7 +163,8 @@ function matchPhrase(phrase: string, fullText: string) {

/**
* Full match using lowercase
* Need to convert boolean to string before applying lowercase
*/
function matchPhraseFull(phrase: string, fullText: string) {
return fullText.toLowerCase() === phrase.toLowerCase();
function matchPhraseFull(phrase: string, toMatch: boolean | string | number) {
return _.toString(toMatch).toLowerCase() === phrase.toLowerCase();
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ import {
} from 'shared/components/query/filteredSearch/field/CheckboxFilterField';
import { CancerTreeSearchFilter } from 'shared/lib/query/textQueryUtils';
import { ListPhrase } from 'shared/components/query/filteredSearch/Phrase';
import {
toFilterFieldOption,
toFilterFieldValue,
} from 'shared/components/query/filteredSearch/field/FilterFieldOption';

describe('CheckboxFilterField', () => {
describe('createQueryUpdate', () => {
Expand All @@ -12,7 +16,7 @@ describe('CheckboxFilterField', () => {
nodeFields: ['studyId'],
form: {
input: FilterCheckbox,
options: ['a', 'b', 'c', 'd', 'e'],
options: ['a', 'b', 'c', 'd', 'e'].map(toFilterFieldOption),
label: 'Test label',
},
} as CancerTreeSearchFilter;
Expand Down Expand Up @@ -48,7 +52,11 @@ describe('CheckboxFilterField', () => {
it('removes all update when only And', () => {
const checked = dummyFilter.form.options;
const toRemove: ListPhrase[] = [];
const result = createQueryUpdate(toRemove, checked, dummyFilter);
const result = createQueryUpdate(
toRemove,
checked.map(toFilterFieldValue),
dummyFilter
);
expect(result.toAdd?.length).toEqual(0);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,15 @@ import {
} from 'shared/lib/query/textQueryUtils';
import { FieldProps } from 'shared/components/query/filteredSearch/field/FilterFormField';
import { ListPhrase } from 'shared/components/query/filteredSearch/Phrase';
import {
FilterFieldOption,
toFilterFieldValue,
} from 'shared/components/query/filteredSearch/field/FilterFieldOption';

export type CheckboxFilterField = {
input: typeof FilterCheckbox;
label: string;
options: string[];
options: FilterFieldOption[];
};

export const FilterCheckbox: FunctionComponent<FieldProps> = props => {
Expand All @@ -43,19 +47,19 @@ export const FilterCheckbox: FunctionComponent<FieldProps> = props => {
});

for (const option of options) {
const isChecked = isOptionChecked(option, relevantClauses);
const isChecked = isOptionChecked(option.value, relevantClauses);
if (isChecked) {
checkedOptions.push(option);
checkedOptions.push(option.value);
}
}

return (
<div className="filter-checkbox">
<h5>{props.filter.form.label}</h5>
<div>
{options.map((option: string) => {
const id = `input-${option}`;
let isChecked = checkedOptions.includes(option);
{options.map((option: FilterFieldOption) => {
const id = `input-${option.displayValue}`;
Copy link

@BasLee BasLee Mar 7, 2023

Choose a reason for hiding this comment

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

Filter can now still be matched to the wrong checkbox, when their displayValues are the same. Maybe better to get rid of the ID by wrapping the checkbox by its label?

                            <label [..] >
                                <input [..] />
                                {' '}
                                {option.displayValue}
                            </label>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now, the id will consist of displayvalue and optionvalue combined to ensure that they are unique. Enwrapped with label.

let isChecked = checkedOptions.includes(option.value);
return (
<div
style={{
Expand All @@ -66,11 +70,11 @@ export const FilterCheckbox: FunctionComponent<FieldProps> = props => {
<input
type="checkbox"
id={id}
value={option}
value={option.displayValue}
checked={isChecked}
onClick={() => {
isChecked = !isChecked;
updatePhrases(option, isChecked);
updatePhrases(option.value, isChecked);
const update = createQueryUpdate(
toRemove,
checkedOptions,
Expand All @@ -89,7 +93,7 @@ export const FilterCheckbox: FunctionComponent<FieldProps> = props => {
padding: '0 0 0 0.2em',
}}
>
{option}
{option.displayValue}
</label>
</div>
);
Expand Down Expand Up @@ -159,7 +163,8 @@ export function createQueryUpdate(
toAdd = [];
} else if (onlyNot || moreAnd) {
const phrase = options
.filter(o => !optionsToAdd.includes(o))
.filter(o => !optionsToAdd.includes(o.value))
.map(toFilterFieldValue)
.join(FILTER_VALUE_SEPARATOR);
toAdd = [new NotSearchClause(createListPhrase(prefix, phrase, fields))];
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export type FilterFieldOption = {
value: string;
displayValue: string;
};

export function toFilterFieldOption(option: string) {
return { value: option, displayValue: option };
}

export function toFilterFieldValue(option: FilterFieldOption) {
return option.value;
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,22 @@ import { SearchClause } from 'shared/components/query/filteredSearch/SearchClaus
import { Phrase } from 'shared/components/query/filteredSearch/Phrase';
import './ListFormField.scss';
import { toQueryString } from 'shared/lib/query/textQueryUtils';
import { FilterFieldOption } from 'shared/components/query/filteredSearch/field/FilterFieldOption';

export type ListFilterField = {
label: string;
input: typeof FilterList;
options: string[];
options: FilterFieldOption[];
};

export const FilterList: FunctionComponent<FieldProps> = props => {
const form = props.filter.form as ListFilterField;
const allPhrases = toUniquePhrases(props.query);
const queryString = toQueryString(props.query);
return (
<div className="filter-list">
<h5>{props.filter.form.label}</h5>
{form.options.map(option => {
const update = props.parser.parseSearchQuery(option);
const update = props.parser.parseSearchQuery(option.value);
return (
<li className="dropdown-item">
<a
Expand All @@ -32,7 +32,7 @@ export const FilterList: FunctionComponent<FieldProps> = props => {
});
}}
>
{option}
{option.displayValue}
</a>
</li>
);
Expand Down
22 changes: 19 additions & 3 deletions src/shared/lib/query/QueryParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ import {
import {
AndSearchClause,
FILTER_SEPARATOR,
SearchClause,
NOT_PREFIX,
NotSearchClause,
SearchClause,
} from 'shared/components/query/filteredSearch/SearchClause';
import { FilterCheckbox } from 'shared/components/query/filteredSearch/field/CheckboxFilterField';
import { getServerConfig, ServerConfigHelpers } from 'config/config';
Expand All @@ -18,6 +18,7 @@ import {
ListPhrase,
Phrase,
} from 'shared/components/query/filteredSearch/Phrase';
import { toFilterFieldOption } from 'shared/components/query/filteredSearch/field/FilterFieldOption';

export class QueryParser {
/**
Expand All @@ -38,7 +39,7 @@ export class QueryParser {
input: FilterList,
options: ServerConfigHelpers.skin_example_study_queries(
getServerConfig()!.skin_example_study_queries || ''
),
).map(toFilterFieldOption),
},
},
/**
Expand All @@ -49,10 +50,25 @@ export class QueryParser {
nodeFields: ['referenceGenome'],
form: {
input: FilterCheckbox,
options: [...referenceGenomes],
options: [...referenceGenomes].map(toFilterFieldOption),
label: 'Reference genome',
},
},
/**
* Show Authorized Studies
*/
{
phrasePrefix: 'authorized',
nodeFields: ['readPermission'],
form: {
input: FilterCheckbox,
options: [
{ value: 'true', displayValue: 'Authorized' },
{ value: 'false', displayValue: 'Unauthorized' },
],
label: 'Controlled access',
},
},
];
}

Expand Down