iTop
iTop copied to clipboard
Combodo
N°7216 import improves error handling missing or null data
Also, https://github.com/Combodo/iTop/blob/4e53deec9d96093df87b233249a9c071ae458d40/.github/pull_request_template.md?plain=1#L3-L5
Tests are missing. We've prototyped some tests in BulkChangeTest and it proved that the currrent implementation does handle only specific cases.
The following concerns have to be discussed and decided by Product Owers:
- What if a row has more cells than the first one? Currently, this is ignored. Should this be considered as an error too?
- The current implementation aims at raising errors when cells are missing. An alternative could be to consider those missing cells as the equivalent of
. That option can be implemented in the CSV parser and that would involve less cases to test.
Functional review: We rather have a clear error telling that the data are missing columns at line X.
Yesterday, I stumbled across this very same problem. I didn't know a PR existed for it already because of the lack of a proper description. (Looking at you Anne-Catherine :wink:)
What will be the new situation with this PR if a row doesn't have the same number of columns?
PR still have to be reworked, but the goal is that in case of a missing column in a line of your CSV, a clear error messgae will be displayed saying that a column is missing on line X. That way you wan fix the CSV data.
What do you think?
I don't know, I think it displays an error, but I'll let @accognet answer.
So why dit the functional team wrote this?
Functional review: We rather have a clear error telling that the data are missing columns at line X.
So, there must be some knowledge of what the current error would tell?
The current behaviour of this PR is: If the number of columns in a row is not as expected, the import fails with a message like "Issue: Not the expected number of fields (current : 4 fields, expected :5)"
In functional review, we discussed the correct behavior in case of a missing column. Romain was convinced that we had changed the initial behavior of this PR during the technical review. And it wasn't. This explains Molkobain's confusing comment.