-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Check for Duplicate / Empty columns before attempting to submit import #12799
Conversation
Detect duplicate columns & throw validation exception
lang for duplicte column detection
composer cs
Include Import
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.
Thanks! Can you please investigate refactoring this into a custom validation rule that we pass to ->rule()
of the FileUpload
? That should declutter it a little bit and control sending the validation message to the correct field etc.
For building a custom validation rule, what would you prefer as best practice? Separate namespace within the support / actions package for Validation rules and place in there? or something else? I'm currently investigating as a validation closure function but that's hitting some roadblocks with file validation, so building it as a separate class makes sense; I just need to know where in the repository would be most ideal. EDIT: Disregard this, I was just making a bad call to the validator for validating files. I've built this as an anonymous validation function now. I'll add it to the PR for another check. On closer inspection, might be slightly difficult to move to its own rule class, as it depends on some functions that are specific to the CanImportRecords trait. |
Ideally it would be a closure rule as its used in only a single place, is there a feature of class based rules that isnt available in closure ones? Otherwise, Filament\Actions\Imports\Rules I think |
Was able to make the validation into a closure rule, not fully sure how to attach my commits to the requested changes in all honesty 😬 , but it should now check for duplicate columns and return the names of them as a validation error. |
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.
Thanks. Can you please move the translations to import.modal.form.file.rules
? The current place is for translations related to the failure CSV that is generated after the import finishes?
Nested ifs have been removed, refactored translation string to only use 1 key instead of requiring additional if statement splitting between 2 keys. |
Thanks! |
@aSeriousDeveloper thanks for making this PR. I have tested this on my application. The validation rule works. |
Description
When importing data through the pre-built import action, duplicate column headers will cause a 500 error to be thrown. This is due to the League/Csv package throwing an Uncaught SyntaxError when these are detected.
This fix catches the syntax error and does some logic to determine whether:
A different message is displayed for each, for better user experience. The reason for uniquely detecting empty headers is that seemingly, some spreadsheet software may include completely empty columns even after saving. I'm not fully certain why these would be preserved as this doesn't seem to occur in the latest version of Excel, but it's an issue that some users have had so far when attempting to import data.
Visual changes
Error when submitting spreadsheet with duplicate columns:
New message when submitting with duplicate column headers
New Message when submitting with duplicate empty column headers
Functional changes
composer cs
command.