-
Notifications
You must be signed in to change notification settings - Fork 2
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
fixed test csv/api mappings #43
Conversation
Reviewer's Guide by SourceryThis pull request fixes issues with CSV/API mappings in the selector form. It updates the No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @MaryKilewe - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using more descriptive variable names to improve readability.
- It looks like there's some commented-out code that could be removed.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 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.
const mappingExists = formData.find(item => item.base_variable_mapped_to.toLowerCase() == baseVariable.toLowerCase()); | ||
if (mappingExists){ | ||
mappingExists.tablename = filteredData[0].tableSelected; | ||
mappingExists.tablename = "-"; | ||
mappingExists.columnname = column; | ||
mappingExists.join_by = join_by; | ||
}else { | ||
formData.push({ | ||
"base_repository": baselookup, | ||
"base_variable_mapped_to": baseVariable, | ||
"is_required": baseVariable, | ||
"columnname": column, | ||
"datatype": "string" | ||
}) | ||
setFormData(formData) | ||
mappingExists.join_by = "-"; | ||
} | ||
// else { | ||
// formData.push({ | ||
// "base_repository": baselookup, | ||
// "base_variable_mapped_to": baseVariable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Potential variable naming issue in handleTableSelect.
In the handleTableSelect function, the parameter is named 'basevariable' (all lowercase), but the code later refers to 'baseVariable'. Similarly, 'column' is referenced without a defined value. This appears to be a mistake—please confirm that the correct variables (likely 'basevariable' and 'csvHeaderSelected' instead) are used to update the mapping.
Summary by Sourcery
Fixes issues with CSV/API mappings in the selector form. It ensures that mappings are correctly updated when a base variable is already mapped and prevents duplicate entries.
Bug Fixes: