IA-3628: Change requests: filter use default datasource by default, add advanced filters #1860
Conversation
…atasource-default-add-advanced-filters
…atasource-default-add-advanced-filters
…atasource-default-add-advanced-filters
Co-authored-by: Marc Hertzog <[email protected]>
Co-authored-by: Marc Hertzog <[email protected]>
Co-authored-by: Marc Hertzog <[email protected]>
There was a problem hiding this comment.
Good job, that feature is not as easy as it can seem. A few changes to make:
- The treeview should reset when we change the source:
Screen.Recording.2024-12-13.at.12.17.32.mov
- Deep linking of the advanced filters doesn't work
Screen.Recording.2024-12-13.at.12.26.05.mov
-
I think the "source" filter should be moved out of the advanced filters in order to match the layout of org unit filters
-
Please checkout the comment about moving some code inside the filters
| const initialDataSource = useMemo( | ||
| () => | ||
| dataSources?.find( | ||
| source => | ||
| source.value === defaultSourceVersion.source.id.toString(), | ||
| )?.value || '', | ||
| [dataSources, defaultSourceVersion.source.id], | ||
| ); |
There was a problem hiding this comment.
I think this is where the deep linking is broken. You need to check the params for a source version and use it as initial source
| useEffect(() => { | ||
| if (selectedVersionId) { | ||
| refetchGroups(); | ||
| } | ||
| }, [selectedVersionId, refetchGroups]); |
There was a problem hiding this comment.
Is there a reason you don't pass selectedVersionId to useGetGroupsDropdown and put it in the query key instead of this useEffect ?
| handleChange( | ||
| 'source_version_id', | ||
| selectedSource?.original?.default_version.id, | ||
| ); | ||
| } else { | ||
| setSelectedVersionId(newValue.toString()); | ||
| handleChange('source_version_id', newValue); |
There was a problem hiding this comment.
I think you need additional code here to reset the treeview when changing source/version
| <InputComponent | ||
| type="select" | ||
| disabled={isFetchingDataSources} | ||
| keyValue="source" | ||
| onChange={handleDataSourceVersionChange} | ||
| value={isFetchingDataSources ? '' : dataSource} | ||
| label={MESSAGES.source} | ||
| options={dataSources} | ||
| loading={isFetchingDataSources} | ||
| /> |
There was a problem hiding this comment.
Could you move this one outside of the advanced settings, so it's the same as in Org Units?
|
|
||
| params.source_version_id = selectedVersionId; |
There was a problem hiding this comment.
We try to avoid mutating params directly and use a redirection instead (unless it breaks the whole thing)
| const defaultSourceVersion = useDefaultSourceVersion(); | ||
| const [selectedVersionId, setSelectedVersionId] = useState<string>( | ||
| defaultSourceVersion.version.id.toString(), | ||
| ); | ||
| const [dataSource, setDataSource] = useState<string>( | ||
| defaultSourceVersion.source.id.toString(), | ||
| ); | ||
|
|
||
| params.source_version_id = selectedVersionId; |
There was a problem hiding this comment.
I think you could move all this code to the Filters, since you only pass it as props. You could use a redirection from within the filters instead of mutating the params here
What problem is this PR solving? Explain here in one sentence.
Related JIRA tickets : IA-3628
Self proofreading checklist
Doc
Tell us where the doc can be found (docs folder, wiki, in the code...).
Changes
How to test
Print screen / video
Screencast.from.2024-12-10.18-53-56.webm
Notes
Things that the reviewers should know:
Follow the Conventional Commits specification
The merge message of a pull request must follow the Conventional Commits specification.
This convention helps to automatically generate release notes.
Use lowercase for consistency.
Example:
Note that the Jira reference is preceded by a line break.
Both the line break and the Jira reference are entered in the Add an optional extended description… field.