Skip to content

Commit cd326bf

Browse files
committed
fix(export): type numeric answers as numbers in exports
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
1 parent 01420d3 commit cd326bf

2 files changed

Lines changed: 76 additions & 10 deletions

File tree

lib/Service/SubmissionService.php

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
use OCP\IUserSession;
3636
use OCP\Mail\IMailer;
3737

38+
use PhpOffice\PhpSpreadsheet\Cell\DataType;
3839
use PhpOffice\PhpSpreadsheet\IOFactory;
3940
use PhpOffice\PhpSpreadsheet\Spreadsheet;
4041
use PhpOffice\PhpSpreadsheet\Writer\Csv;
@@ -348,17 +349,23 @@ private function exportData(array $header, array $data, string $fileFormat, ?Fil
348349
$activeWorksheet->getStyle([$column, $row])
349350
->getAlignment()
350351
->setWrapText(true);
352+
} elseif (!is_string($value)) {
353+
$activeWorksheet->setCellValue([$column, $row], $value);
354+
} elseif ($fileFormat === 'csv') {
355+
// CSV is escaped (not typed) to neutralise formula-triggering values when opened in a spreadsheet app.
356+
$activeWorksheet->getCell([$column, $row])
357+
->setValueExplicit($this->escapeCSV($value), DataType::TYPE_STRING);
358+
} elseif (is_numeric($value)
359+
&& !in_array($value[0], ['=', '+', '-', '@'], true)
360+
&& (string)(+$value) === $value) {
361+
// Store numeric-looking answers (e.g. Radio labels "1"-"5") as numbers so they can be
362+
// aggregated. The round-trip check keeps values whose canonical numeric form would differ
363+
// (leading-zero phone numbers, "1e5", oversized ids, ...) as text.
364+
$activeWorksheet->setCellValue([$column, $row], +$value);
351365
} else {
352-
// Explicitly set the type of the value to string for values that start with '=' to prevent it being interpreted as formulas
353-
if (is_string($value)) {
354-
$activeWorksheet->getCell([$column, $row])
355-
->setValueExplicit($fileFormat === 'csv'
356-
? $this->escapeCSV($value)
357-
: $value,
358-
);
359-
} else {
360-
$activeWorksheet->setCellValue([$column, $row], $value);
361-
}
366+
// Explicitly type as string so values that start with '=' are not interpreted as formulas.
367+
$activeWorksheet->getCell([$column, $row])
368+
->setValueExplicit($value, DataType::TYPE_STRING);
362369
}
363370
}
364371
}

tests/Unit/Service/SubmissionServiceTest.php

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@
3232
use OCP\IUserManager;
3333
use OCP\IUserSession;
3434
use OCP\Mail\IMailer;
35+
use PhpOffice\PhpSpreadsheet\Cell\DataType;
36+
use PhpOffice\PhpSpreadsheet\IOFactory;
3537
use PHPUnit\Framework\MockObject\MockObject;
3638
use Psr\Log\LoggerInterface;
3739

@@ -565,6 +567,63 @@ public function testGetSubmissionsDataThrowsExceptionOnInvalidFormat() {
565567
$this->submissionService->getSubmissionsData($form, 'invalid');
566568
}
567569

570+
public function dataExportNumericTyping() {
571+
return [
572+
// Radio (and similar choice) answers are always strings, even when the option labels are
573+
// pure numbers. For spreadsheet formats they must be written as numbers so they can be
574+
// aggregated (e.g. averaged) in the spreadsheet application.
575+
'radio-numeric-label-xlsx' => ['3', 'xlsx', DataType::TYPE_NUMERIC, 3],
576+
'radio-numeric-label-ods' => ['5', 'ods', DataType::TYPE_NUMERIC, 5],
577+
'numeric-zero' => ['0', 'xlsx', DataType::TYPE_NUMERIC, 0],
578+
'numeric-decimal' => ['3.5', 'xlsx', DataType::TYPE_NUMERIC, 3.5],
579+
// Values starting with a formula-trigger character keep the string typing so they are
580+
// never interpreted as formulas.
581+
'formula' => ['=SUM(A1:A2)', 'xlsx', DataType::TYPE_STRING, '=SUM(A1:A2)'],
582+
'negative-number' => ['-5', 'xlsx', DataType::TYPE_STRING, '-5'],
583+
// Strings whose canonical numeric form would differ must stay text to avoid data loss
584+
// (leading-zero phone numbers, scientific notation, ...).
585+
'leading-zero' => ['0123456789', 'xlsx', DataType::TYPE_STRING, '0123456789'],
586+
'scientific' => ['1e5', 'xlsx', DataType::TYPE_STRING, '1e5'],
587+
'text' => ['Option A', 'xlsx', DataType::TYPE_STRING, 'Option A'],
588+
];
589+
}
590+
591+
/**
592+
* @dataProvider dataExportNumericTyping
593+
*
594+
* @param string $value the raw (string) answer as stored in the database
595+
* @param string $fileFormat spreadsheet format to export to
596+
* @param string $expectedType the expected PhpSpreadsheet cell data type
597+
* @param mixed $expectedValue the expected cell value after a write/read round-trip
598+
*/
599+
public function testExportNumericTyping(string $value, string $fileFormat, string $expectedType, mixed $expectedValue) {
600+
// Hand out real temporary files so the spreadsheet can actually be written and reloaded.
601+
$this->tempManager->expects($this->any())
602+
->method('getTemporaryFile')
603+
->willReturnCallback(fn () => tempnam(sys_get_temp_dir(), 'forms-export-'));
604+
605+
$content = self::invokePrivate(
606+
$this->submissionService,
607+
'exportData',
608+
[['Question 1'], [[$value]], $fileFormat, null],
609+
);
610+
611+
// Reload the produced file and inspect the actual data type of the answer cell.
612+
$reloadPath = tempnam(sys_get_temp_dir(), 'forms-reload-');
613+
file_put_contents($reloadPath, $content);
614+
$cell = IOFactory::load($reloadPath)->getSheet(0)->getCell([1, 2]);
615+
616+
$this->assertSame($expectedType, $cell->getDataType());
617+
if ($expectedType === DataType::TYPE_NUMERIC) {
618+
$this->assertEquals($expectedValue, $cell->getValue());
619+
} else {
620+
// assertSame guards against a numeric-looking string being silently coerced.
621+
$this->assertSame($expectedValue, $cell->getValue());
622+
}
623+
624+
unlink($reloadPath);
625+
}
626+
568627
/**
569628
* Setting up a very simple default CsvTest
570629
*/

0 commit comments

Comments
 (0)