Skip to content

Commit e90d45e

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 1c232d4 commit e90d45e

2 files changed

Lines changed: 74 additions & 8 deletions

File tree

lib/Service/SubmissionService.php

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
use OCP\IUserManager;
3434
use OCP\IUserSession;
3535
use OCP\Mail\IMailer;
36+
use PhpOffice\PhpSpreadsheet\Cell\DataType;
3637
use PhpOffice\PhpSpreadsheet\IOFactory;
3738
use PhpOffice\PhpSpreadsheet\Spreadsheet;
3839
use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet;
@@ -697,15 +698,31 @@ private function validateShortQuestion(array $question, string $data): bool {
697698
}
698699

699700
private function setCellValue(Worksheet $activeWorksheet, int $column, int $row, mixed $value, string $fileFormat): void {
700-
// Explicitly set the type of the value to string for values that start with '=' to prevent it being interpreted as formulas
701-
if (is_string($value)) {
702-
$activeWorksheet->getCell([$column, $row])
703-
->setValueExplicit($fileFormat === 'csv'
704-
? $this->escapeCSV($value)
705-
: $value,
706-
);
707-
} else {
701+
if (!is_string($value)) {
708702
$activeWorksheet->setCellValue([$column, $row], $value);
703+
return;
704+
}
705+
706+
// CSV is escaped (not typed) to neutralise formula-triggering values when opened in a spreadsheet app.
707+
if ($fileFormat === 'csv') {
708+
$activeWorksheet->getCell([$column, $row])
709+
->setValueExplicit($this->escapeCSV($value), DataType::TYPE_STRING);
710+
return;
711+
}
712+
713+
// Store numeric-looking answers (e.g. Radio labels "1"–"5") as numbers so they can be aggregated.
714+
// The round-trip check keeps values whose canonical numeric form would differ (leading-zero phone
715+
// numbers, "1e5", oversized ids, ...) as text, and the leading-character check preserves the
716+
// anti-formula protection for values starting with '=', '+', '-' or '@'.
717+
if (is_numeric($value)
718+
&& !in_array($value[0], ['=', '+', '-', '@'], true)
719+
&& (string)(+$value) === $value) {
720+
$activeWorksheet->setCellValue([$column, $row], +$value);
721+
return;
709722
}
723+
724+
// Explicitly type as string so values that start with '=' are not interpreted as formulas.
725+
$activeWorksheet->getCell([$column, $row])
726+
->setValueExplicit($value, DataType::TYPE_STRING);
710727
}
711728
}

tests/Unit/Service/SubmissionServiceTest.php

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@
3535
use OCP\IUserManager;
3636
use OCP\IUserSession;
3737
use OCP\Mail\IMailer;
38+
use PhpOffice\PhpSpreadsheet\Cell\DataType;
39+
use PhpOffice\PhpSpreadsheet\Spreadsheet;
3840
use PHPUnit\Framework\MockObject\MockObject;
3941
use Psr\Log\LoggerInterface;
4042
use Test\TestCase;
@@ -618,6 +620,53 @@ public function testGetSubmissionsDataThrowsExceptionOnInvalidFormat() {
618620
$this->submissionService->getSubmissionsData($form, 'invalid');
619621
}
620622

623+
public static function dataSetCellValueType(): array {
624+
return [
625+
// Radio (and similar choice) answers are always strings, even when the option
626+
// labels are pure numbers. For spreadsheet formats they must be stored as numbers
627+
// so they can be aggregated (e.g. averaged) in the spreadsheet application.
628+
'radio-numeric-label-xlsx' => ['xlsx', '3', DataType::TYPE_NUMERIC, 3],
629+
'radio-numeric-label-ods' => ['ods', '5', DataType::TYPE_NUMERIC, 5],
630+
'numeric-zero' => ['xlsx', '0', DataType::TYPE_NUMERIC, 0],
631+
'numeric-decimal' => ['xlsx', '3.5', DataType::TYPE_NUMERIC, 3.5],
632+
// Values starting with a formula-trigger character keep the string typing so they
633+
// are never interpreted as formulas.
634+
'formula' => ['xlsx', '=SUM(A1:A2)', DataType::TYPE_STRING, '=SUM(A1:A2)'],
635+
'negative-number' => ['xlsx', '-5', DataType::TYPE_STRING, '-5'],
636+
'plus-prefixed' => ['xlsx', '+5', DataType::TYPE_STRING, '+5'],
637+
'at-prefixed' => ['xlsx', '@foo', DataType::TYPE_STRING, '@foo'],
638+
// Strings whose canonical numeric form would differ must stay text to avoid data
639+
// loss (leading-zero phone numbers, scientific notation, trailing decimal zeros, …).
640+
'leading-zero' => ['xlsx', '0123456789', DataType::TYPE_STRING, '0123456789'],
641+
'scientific' => ['xlsx', '1e5', DataType::TYPE_STRING, '1e5'],
642+
'trailing-decimal-zero' => ['xlsx', '3.50', DataType::TYPE_STRING, '3.50'],
643+
'text' => ['xlsx', 'Option A', DataType::TYPE_STRING, 'Option A'],
644+
// CSV is always escaped and kept as string, even for pure numbers.
645+
'csv-number' => ['csv', '3', DataType::TYPE_STRING, '3'],
646+
'csv-formula' => ['csv', '=danger', DataType::TYPE_STRING, "'=danger"],
647+
];
648+
}
649+
650+
/**
651+
* @dataProvider dataSetCellValueType
652+
*/
653+
public function testSetCellValueDataType(string $fileFormat, string $value, string $expectedType, mixed $expectedValue): void {
654+
$spreadsheet = new Spreadsheet();
655+
$worksheet = $spreadsheet->getActiveSheet();
656+
657+
TestCase::invokePrivate($this->submissionService, 'setCellValue', [$worksheet, 1, 1, $value, $fileFormat]);
658+
659+
$cell = $worksheet->getCell([1, 1]);
660+
661+
$this->assertSame($expectedType, $cell->getDataType());
662+
if ($expectedType === DataType::TYPE_NUMERIC) {
663+
$this->assertEquals($expectedValue, $cell->getValue());
664+
} else {
665+
// assertSame guards against a numeric-looking string being silently coerced.
666+
$this->assertSame($expectedValue, $cell->getValue());
667+
}
668+
}
669+
621670
/**
622671
* Setting up a very simple default CsvTest
623672
*/

0 commit comments

Comments
 (0)