[#2995]Fix header validation for all-repeatable forms. #3074
Conversation
stellanl
requested review from
valllllll2000 and
muloem
and removed request for
valllllll2000
There was a problem hiding this comment.
The root cause of issues was that the check for a question column was failing in the special case where there were only repeat question groups and no questions for the base sheet. However, maybe we can revise the implementation to address the fact that our assumption that there would always be a question on the base sheet has now changed.
Perhaps we can check to make sure that all sheets are validated and if no questions are found on any of them, we throw this particular "Question not found" error.
GAE/src/org/waterforpeople/mapping/dataexport/RawDataSpreadsheetImporter.java
Outdated
Show resolved
Hide resolved
GAE/src/org/waterforpeople/mapping/dataexport/RawDataSpreadsheetImporter.java
Outdated
Show resolved
Hide resolved
GAE/src/org/waterforpeople/mapping/dataexport/RawDataSpreadsheetImporter.java
Outdated
Show resolved
Hide resolved
GAE/src/org/waterforpeople/mapping/dataexport/RawDataSpreadsheetImporter.java
Outdated
Show resolved
Hide resolved
GAE/src/org/waterforpeople/mapping/dataexport/RawDataSpreadsheetImporter.java
Outdated
Show resolved
Hide resolved
GAE/src/org/waterforpeople/mapping/dataexport/RawDataSpreadsheetImporter.java
Outdated
Show resolved
Hide resolved
…ll places where we assume sheet 0 is base.
| sheet.addMergedRegion(new CellRangeAddress(0, 0, 0, i - 1)); | ||
|
|
||
| for (int j = 0; j < questionColumns; j++) { | ||
| row1.createCell(i + j).setCellValue(j + "|Question" + j); |
There was a problem hiding this comment.
Maybe a good idea to simulate what the real question IDs look like.. e.g. 1230001
| File file1 = createValidTestSpreadsheet("/tmp/valid1.xlsx"); | ||
| Map<Integer,String> errors = dimp.validate(file1); | ||
| for (Integer i: errors.keySet()) { | ||
| System.out.printf("Unexpected error, row %d: %s", i, errors.get(i)); |
There was a problem hiding this comment.
In this case rather than print out the errors, since it’s supposed to be a valid test sheet, you should assert that errors is empty and that should be enough. If it isn’t empty I believe it prints out the error map
| Map<Integer,String> errors = dimp.validate(file); | ||
| assertEquals(1, errors.size()); | ||
| String err1 = errors.get(-11); | ||
| assertNotEquals(err1, null); |
There was a problem hiding this comment.
May be more accurate to assertEquals() and actually copy the error text here?
Before the PR (what is the issue or what needed to be done)
Forms with the first or all groups repeatable were not imported correctly
The solution
Handle that, chiefly by changing the verify() method.
Screenshots (if appropriate)
Checklist