N°7216 import improves error handling missing or null data

[accognet]

internal

[Hipska]

Also,

iTop/.github/pull_request_template.md

Lines 3 to 5 in 4e53dee

	IMPORTANT: Please follow the guidelines within this PR template before submitting it, it will greatly help us process your PR. 🙏
	Any PRs not following the guidelines or with missing information will not be considered.

[rquetiez]

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:

1. What if a row has more cells than the first one? Currently, this is ignored. Should this be considered as an error too?
2. 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.

[Molkobain]

Functional review: We rather have a clear error telling that the data are missing columns at line X.

[Hipska]

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 😉)

What will be the new situation with this PR if a row doesn't have the same number of columns?

[Molkobain]

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?

[Hipska]

I agree, but what is the current behaviour with this PR?

[Molkobain]

I don't know, I think it displays an error, but I'll let @accognet answer.

[Hipska]

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?

[accognet]

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.

[Hipska]

Thanks, can you also put a screenshot of that error message please?

[accognet]

[Hipska]

Thanks! I was already suspecting it would be clear on which line the error occurs, this screenshot proves it.

[Hipska]

Does it also work if a row has too many fields?

[accognet]

Yes, it's the same behavior

[rquetiez]

Tests are being reworked:

• regresssion on html escaping
• incomprehensible test option bSetId

Then we'll have to check the coverage.

[rquetiez]

Review of the tests still ongoing:

• The existing test mechanism is confusing and will be improved to facilitate the reading of test cases (automatic replacement of the column 'id' with a real server id only if the column 'id' is given in the test case, and at the given -dehardcoded- position)
• 11, 12 and 13 tests cases to be highly simplified to focus on what is being tested
• Error response not meeting typographic rules: a column ":" must not be preceeded by a space and must be followed by a space

A new review of the tests (and implementation) will be performed then.

[rquetiez]

Review still ongoing:

• Tests are ok
• Error label reviewed ("column" prefered to "field"), dictionaries are being updated
• The change in ajax.csvimport.php has not been explained: what is the improved behavior?

[rquetiez]

The fix in ajax.csvimport.php seems ok though it cannot be associated to a usage.