Feat/add query when mapping #40
Conversation
Reviewer's Guide by SourceryThis pull request introduces functionality to map CSV headers to base variables, test the mappings, and save them. It includes a new component, TestCsvMappings, for testing the mappings and displays any issues found. It also updates the CsvApiSelectorForm to fetch and display CSV headers instead of database tables. New queries and mutations are added for fetching CSV headers, saving mappings, and testing mappings. Sequence diagram for handling CSV header selectionsequenceDiagram
participant CsvApiSelectorForm
participant handleTableSelect
participant setFormData
CsvApiSelectorForm->>handleTableSelect: handleTableSelect(csvHeaderSelected, basevariable)
handleTableSelect->>setFormData: Updates formData with selected CSV header
setFormData-->>CsvApiSelectorForm: Updates form data
Sequence diagram for testing CSV mappingssequenceDiagram
participant TestCsvMappings
participant useTestCsvMappings
participant API
TestCsvMappings->>useTestCsvMappings: useTestCsvMappings(formData, baselookup, conn_type)
useTestCsvMappings->>API: POST /flatfile_mapper/test/{conn_type}/mapped_variables/{baselookup}
API-->>useTestCsvMappings: Returns test results
useTestCsvMappings-->>TestCsvMappings: Returns test results
TestCsvMappings->>TestCsvMappings: Displays test results
Sequence diagram for saving CSV mappingssequenceDiagram
participant CsvApiSelectorForm
participant useSaveCsvMappings
participant API
CsvApiSelectorForm->>useSaveCsvMappings: useSaveCsvMappings(formData, baselookup, conn_type)
useSaveCsvMappings->>API: POST /flatfile_mapper/add/{conn_type}/mapped_variables/{baselookup}
API-->>useSaveCsvMappings: Returns save status
useSaveCsvMappings-->>CsvApiSelectorForm: Returns save status
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @MaryKilewe - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider creating a shared component for the alert messages to avoid repetition.
- The testing and saving logic in
TestCsvMappings.jscould be simplified by extracting common operations into helper functions.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| queryFn: fetchSourceSystemInfo, | ||
| refetchInterval: 1800000, // refresh every 30min | ||
|
|
||
| }); |
There was a problem hiding this comment.
suggestion: Dynamic query keys for useQuery hooks.
Several useQuery hooks (e.g., useGetBaseVariables, useGetMappedBaseVariables, useGetRepositoryLoadedData) use static query keys like ['baseRepo']. To avoid potential caching conflicts when baseRepo changes, consider including the variable in the query key (e.g., ['baseRepo', baseRepo]).
Suggested implementation:
export const useGetBaseVariables = (baseRepo) => useQuery({
queryKey: ['baseRepo', baseRepo],export const useGetMappedBaseVariables = (baseRepo) => useQuery({
queryKey: ['baseRepo', baseRepo],| @@ -0,0 +1,94 @@ | |||
| import {API_URL} from "../../constants"; | |||
There was a problem hiding this comment.
issue (complexity): Consider creating a query hook factory to avoid repeated fetching logic and reduce overall complexity in the file by centralizing shared logic and reducing boilerplate code for each query hook..
The repeated fetching logic and query hook creation adds extra boilerplate and increases the file’s complexity. You can reduce duplication by creating a single query hook factory that wraps the fetch/transform code. For example:
const createQueryHook = <T, Args extends any[]>(
queryKey: (args: Args) => any[],
getUrl: (...args: Args) => string,
transform?: (data: any) => T,
refetchInterval = 1800000
) => {
return (...args: Args) => useQuery({
queryKey: queryKey(args),
queryFn: async () => {
const res = await fetch(getUrl(...args));
const jsonData = await res.json();
return transform ? transform(jsonData) : jsonData;
},
refetchInterval
});
};Now you can replace, for example, useGetBaseVariables with:
export const useGetBaseVariables = createQueryHook(
([baseRepo]) => ['baseRepo', baseRepo],
(baseRepo) => `${API_URL}/dictionary_mapper/base_variables/${baseRepo}`,
(data) => data?.data ?? null
);Repeat similar patterns for the other query hooks. This way, you centralize the shared logic and reduce complexity while keeping functionality intact.
| @@ -0,0 +1,65 @@ | |||
| import { useMutation, useQueryClient } from "@tanstack/react-query"; | |||
There was a problem hiding this comment.
issue (complexity): Consider creating a reusable helper function and a generic mutation hook factory to avoid code duplication in API calls and mutation logic, improving maintainability and reducing complexity
You can reduce duplication by extracting the shared API call logic into a reusable helper and a generic mutation hook factory. For example:
import { useMutation, useQueryClient } from "@tanstack/react-query";
import { API_URL } from "../../constants";
const postRequest = async (endpoint, { baselookup, formData, conn_type }) => {
const res = await fetch(`${API_URL}/${endpoint}/${conn_type}/mapped_variables/${baselookup}`, {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify(formData),
});
const jsonData = await res.json();
if (!res.ok) {
return { status_code: res.status, data: jsonData?.detail ?? [] };
}
return { status_code: res.status || 500, data: jsonData?.data ?? [] };
};
const createCsvMappingHook = (endpoint, queryKey) => {
return () => {
const queryClient = useQueryClient();
return useMutation({
mutationFn: (payload) => postRequest(endpoint, payload),
onSuccess: () => queryClient.invalidateQueries({ queryKey }),
});
};
};
export const useSaveCsvMappings = createCsvMappingHook("flatfile_mapper/add", ['mappings']);
export const useTestCsvMappings = createCsvMappingHook("flatfile_mapper/test", ['test_mappings']);By abstracting the shared logic, you maintain functionality while reducing complexity and code duplication.
| { !querySaved ? ( | ||
| <AnimateButton> | ||
| <Button disableElevation disabled={disableSave} fullWidth size="medium" type="button" variant="contained" | ||
| color="primary" onClick={()=>handleSubmit()}> | ||
| Save | ||
| {spinner && | ||
| <CircularProgress style={{"color":"black","marginLeft":"10px"}} size="1rem"/> | ||
| } | ||
| </Button> | ||
| </AnimateButton> | ||
| ):( | ||
| <AnimateButton> | ||
| <Button disableElevation disabled={disableSave} fullWidth size="medium" type="button" variant="contained" | ||
| color="success" onClick={() => window.location.href = `${FRONTEND_URL}/schema/config?baselookup=${baselookup}`}> | ||
| Finish | ||
| </Button> | ||
| </AnimateButton> | ||
| )} |
There was a problem hiding this comment.
suggestion (code-quality): Invert ternary operator to remove negation (invert-ternary)
| { !querySaved ? ( | |
| <AnimateButton> | |
| <Button disableElevation disabled={disableSave} fullWidth size="medium" type="button" variant="contained" | |
| color="primary" onClick={()=>handleSubmit()}> | |
| Save | |
| {spinner && | |
| <CircularProgress style={{"color":"black","marginLeft":"10px"}} size="1rem"/> | |
| } | |
| </Button> | |
| </AnimateButton> | |
| ):( | |
| <AnimateButton> | |
| <Button disableElevation disabled={disableSave} fullWidth size="medium" type="button" variant="contained" | |
| color="success" onClick={() => window.location.href = `${FRONTEND_URL}/schema/config?baselookup=${baselookup}`}> | |
| Finish | |
| </Button> | |
| </AnimateButton> | |
| )} | |
| { querySaved ? | |
| <AnimateButton> | |
| <Button disableElevation disabled={disableSave} fullWidth size="medium" type="button" variant="contained" | |
| color="success" onClick={() => window.location.href = `${FRONTEND_URL}/schema/config?baselookup=${baselookup}`}> | |
| Finish | |
| </Button> | |
| </AnimateButton> | |
| : | |
| <AnimateButton> | |
| <Button disableElevation disabled={disableSave} fullWidth size="medium" type="button" variant="contained" | |
| color="primary" onClick={()=>handleSubmit()}> | |
| Save | |
| {spinner && | |
| <CircularProgress style={{"color":"black","marginLeft":"10px"}} size="1rem"/> | |
| } | |
| </Button> | |
| </AnimateButton>} | |
Explanation
Negated conditions are more difficult to read than positive ones, so it is bestto avoid them where we can. By inverting the ternary condition and swapping the
expressions we can simplify the code.
| const data = fetch(API_URL+"/dictionary_mapper/base_schemas") | ||
| .then((res) => | ||
| res.json(), | ||
| ); | ||
| return data; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const data = fetch(API_URL+"/dictionary_mapper/base_schemas") | |
| .then((res) => | |
| res.json(), | |
| ); | |
| return data; | |
| return fetch(API_URL+"/dictionary_mapper/base_schemas") | |
| .then((res) => | |
| res.json(), | |
| ); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
| const data = fetch(API_URL+"/dictionary_mapper/base_schema_variables/"+baselookup) | ||
| .then((res) => | ||
| res.json(), | ||
| ); | ||
| return data; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const data = fetch(API_URL+"/dictionary_mapper/base_schema_variables/"+baselookup) | |
| .then((res) => | |
| res.json(), | |
| ); | |
| return data; | |
| return fetch(API_URL+"/dictionary_mapper/base_schema_variables/"+baselookup) | |
| .then((res) => | |
| res.json(), | |
| ); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
Summary by Sourcery
Adds functionality to map CSV headers to base variables, allowing users to define mappings between CSV data and a base repository. It also includes testing and saving functionalities for these mappings.
New Features:
Tests: