Add handling of CSV::MalformedCSVError for product_importer #5566
Conversation
There was a problem hiding this comment.
Hey @rioug welcome to OFN, we really appreciate your contribution!
It's the correct change, I left some comments for you to change or comment.
Thank you!
spec/models/product_importer_spec.rb
Outdated
| let(:importer) { import_data csv_data } | ||
|
|
||
| # NOTE an unquoted \n will create a non valid line which will fail entry validation hence why we are only testing with \r | ||
| it "should raise an unquoted field error if data include unquoted filed with \r character" do |
There was a problem hiding this comment.
NOTE is not needed, you can just right your comment after #
There was a problem hiding this comment.
I dont understand "we are only testing with \r"
The test csv contains \n
Why do you say you are only testing with \r?
There was a problem hiding this comment.
The test data has \n at the end of the lines to create a "valid" csv file. I mentioned we only test with \r as adding a \n in a random place will not trigger the error we are testing, only a \r will. Does that make sense ? Maybe I need to reword my comment.
| @@ -237,6 +237,15 @@ def rows | |||
| error_message: e.message)) | |||
| end | |||
| [] | |||
| rescue CSV::MalformedCSVError => e | |||
| # NOTE the init_product_importer method calls build_entries and | |||
There was a problem hiding this comment.
can you please remove NOTE here as well?
just
# the init_product_importer method calls build_entries and
There was a problem hiding this comment.
A rule for code comments I love is: if you need to comment a line of code, extract method and make it a method comment. In this case:
rescue CSV::MalformedCSVError => e
add_malformed_csv_error(e.message)
[]
end
# This error is raised twice because init_product_importer calls both build_entries and buils_all_entries
def add_malformed_csv_error(error_message)
return if errors.added?(:importer, I18n.t('admin.product_import.model.malformed_csv', error_message: error_message))
errors.add(:importer, I18n.t('admin.product_import.model.malformed_csv',error_message: error_message))
endI think this looks nicer, what do you think?
There was a problem hiding this comment.
this change will also help with the codeclimate warning:
Assignment Branch Condition size for rows is too high. [<2, 21, 6> 21.93/15]
There was a problem hiding this comment.
A rule for code comments I love is: if you need to comment a line of code, extract method and make it a method comment.
Make sense, although I feel like It's just adding a method to comply with the codeclimate rules. Anyway it does make the code a bit more readable.
Rubocop still throw a warning with the suggested changes :
app/models/product_import/product_importer.rb:226:5: C: Metrics/AbcSize: Assignment Branch Condition size for rows is too high. [<2, 15, 5> 15.94/15]
I am not sure if there is a way to make it better, what do you think ?
There was a problem hiding this comment.
although I feel like It's just adding a method to comply with the codeclimate rules.
imo code climate rules are very important and readability is fundamental. It's adding a method to make the code a lot more readable and less complex. The complexity rule is very important. Messy code will create bugs and head aches.
The warning is there but now it's 15.94 , not 21. It means it's less complex.
I think we can leave it like that 👍
|
I fixed the various typos and made the suggested changes. It should be good now. |
| } | ||
| let(:importer) { import_data csv_data } | ||
|
|
||
| # an unquoted \n will create a non valid line which will fail entry validation hence why we are only testing with \r |
There was a problem hiding this comment.
oh, I got it now!
If you add a \n in the middle of a line it will create a non valid line 👍
I was confused because the \n is there in the end of each line! Maybe "an unquoted \n in the middle of a line" could help the next reader :-)
|
Hey @rioug , Reproduced the bug - Error 500 and bugsnag got triggered, when uploading the file you attached (Thanks!). After this PR it's the warning is visible (instead of error 500/bugsnag).
PS: I noticed there are issues on Code climate, which shows the "Assignment Branch Condition size for rows is too high" warning. Is this a minor detail? Ping @luisramos0 @Matt-Yorkley |
|
yeah, that codeclimate issue was discussed in the code review and improved, although not fixed. |
What? Why?
Closes #5132
The change adds handling of CSV::MalformedCSVError so it will display an error message when someone tries to import products with a malformed csv file. Namely, a file with a non quoted field containing the characters \r or \n.
What should we test?
Test the product import function, accessible via admin > products > import
You can use the file linked in this comment :#5132 (comment)
A flash message with the following error "Importer Product Import encountered a malformed CSV: Unquoted fields do not allow \r or \n (line 3)." should appear upon uploading a problematic file.
Note, an unquoted \n will result in a file with some non valid line, which will give at least one error on the next import screen. It will not display a flash message.
Release notes
Add handling of malformed CVS file for the product import function.
Changelog Category: Fixed
How is this related to the Spree upgrade?
I have no idea what's involved with the upgrade, so I can't comment on any possible conflict. I'll just note that the product importer is used in this controller :
ProductImportController < Spree::Admin::BaseController