Improve search of authorized and unauthorized studies #4521
Conversation
fix 9915 improve search of authorized studies add new search filter based on readPermission flag called Controlled access authorized. Modified the search algorithm to accept booleans instead of just strings esp. false value
fix 9915 improve search of authorized studies add new search filter based on readPermission flag called Controlled access authorized. Modified the search algorithm to accept booleans instead of just strings esp. false value
…nathan/cbioportal-frontend into fix-9915-improved-finability
Fix 9915 improved finability
id has to be unique for each filter when there are multiple filters.
There was a problem hiding this comment.
Hi Jag, thank you for making this! The only comment that I have is that now the selection widget are two checkboxes that allow you to unselect both options which would result in an empty study list. Personally I think it would be better to have a single toggle that turns the UNAUTH studies on or off.
| @@ -58,7 +58,7 @@ export const FilterCheckbox: FunctionComponent<FieldProps> = props => { | |||
| <h5>{props.filter.form.label}</h5> | |||
| <div> | |||
| {options.map((option: FilterFieldOption) => { | |||
| const id = `input-${option}`; | |||
| const id = `input-${option.displayValue}`; | |||
There was a problem hiding this comment.
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>There was a problem hiding this comment.
now, the id will consist of displayvalue and optionvalue combined to ensure that they are unique. Enwrapped with label.
Update checkbox label text when filter is applied vs not applied
fix all Authorized or Unauthorized studies scenario. Hide the option if the studies that are all authorized or all unauthorized.
jagnathan
changed the title
There was a problem hiding this comment.
@jagnathan Thanks for the pr, overall it looks good. I put some minor comments there. Feel free to address it when you got a chance.
I think it would be great if we can have some tests for this feature. Do you know if it's easy to add some related tests?
| @@ -1450,6 +1450,15 @@ export class QueryStore { | |||
| return new Set(referenceGenomes); | |||
| } | |||
|
|
|||
| @computed get readPermissions(): Set<string> { | |||
| const studies = Array.from(this.treeData.map_node_meta.keys()).filter( s => typeof((s as CancerStudy).readPermission) !== 'undefined' ); | |||
| console.log(studies); | |||
| ...shownAndAuthorizedStudies.map( | ||
| (study: CancerStudy) => study.studyId | ||
| ) |
There was a problem hiding this comment.
This also shows twice, maybe we can create a constant variable for it
const shownAndAuthorizedStudyIds = shownAndAuthorizedStudies.map(
(study: CancerStudy) => study.studyId
);
Only CancerStudy objects from treeData are filtered based on whether the readPermission field has a value. Refactoring: New const created as shownStudiesLengthstring to identify if there is a filter applied or not.
There was a problem hiding this comment.
@jagnathan Thanks for addressing all the comments, LGTM. Please try to resolve the conflicts and rebase based on the latest master before merge.
fix 9915 improve search of authorized studies add new search filter based on readPermission flag called Controlled access authorized. Modified the search algorithm to accept booleans instead of just strings esp. false value fix 9915 improve search of authorized studies fix 9915 improve search of authorized studies add new search filter based on readPermission flag called Controlled access authorized. Modified the search algorithm to accept booleans instead of just strings esp. false value Use _.toString in matchPhraseFull, and specify type Unnest fieldName check in Phrase.match Introduce FilterFieldOption type that includes a displayValue Update QueryParser.ts Update CheckboxFilterField.tsx id has to be unique for each filter when there are multiple filters. Modify the Select all checkbox to select only authorized studies Update checkbox label text Update checkbox label text when filter is applied vs not applied fix all Authorized or Unauthorized studies scenario fix all Authorized or Unauthorized studies scenario. Hide the option if the studies that are all authorized or all unauthorized.
Only CancerStudy objects from treeData are filtered based on whether the readPermission field has a value. Refactoring: New const created as shownStudiesLengthstring to identify if there is a filter applied or not.
conflict resolution fix due to merge
…nathan/cbioportal-frontend into fix-9915-improved-finability
|
superseded by new PR #4603 |
|
superseded by new PR #4603 |
Fix cBioPortal/icebox#468. Improved search of authorized internal studies. Users can now filter studies based on their read permission (authorized or not authorized) in the search box. This is an optional feature that has to be enabled by settings in portal.properties.