From e90d45e425d41bcf98221a017e7d6530200d5a7d Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Fri, 29 May 2026 10:54:29 +0200 Subject: [PATCH] fix(export): type numeric answers as numbers in exports Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/Service/SubmissionService.php | 33 +++++++++---- tests/Unit/Service/SubmissionServiceTest.php | 49 ++++++++++++++++++++ 2 files changed, 74 insertions(+), 8 deletions(-) diff --git a/lib/Service/SubmissionService.php b/lib/Service/SubmissionService.php index a7873b12c..ca1e325b7 100644 --- a/lib/Service/SubmissionService.php +++ b/lib/Service/SubmissionService.php @@ -33,6 +33,7 @@ use OCP\IUserManager; use OCP\IUserSession; use OCP\Mail\IMailer; +use PhpOffice\PhpSpreadsheet\Cell\DataType; use PhpOffice\PhpSpreadsheet\IOFactory; use PhpOffice\PhpSpreadsheet\Spreadsheet; use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet; @@ -697,15 +698,31 @@ private function validateShortQuestion(array $question, string $data): bool { } private function setCellValue(Worksheet $activeWorksheet, int $column, int $row, mixed $value, string $fileFormat): void { - // Explicitly set the type of the value to string for values that start with '=' to prevent it being interpreted as formulas - if (is_string($value)) { - $activeWorksheet->getCell([$column, $row]) - ->setValueExplicit($fileFormat === 'csv' - ? $this->escapeCSV($value) - : $value, - ); - } else { + if (!is_string($value)) { $activeWorksheet->setCellValue([$column, $row], $value); + return; + } + + // CSV is escaped (not typed) to neutralise formula-triggering values when opened in a spreadsheet app. + if ($fileFormat === 'csv') { + $activeWorksheet->getCell([$column, $row]) + ->setValueExplicit($this->escapeCSV($value), DataType::TYPE_STRING); + return; + } + + // Store numeric-looking answers (e.g. Radio labels "1"–"5") as numbers so they can be aggregated. + // The round-trip check keeps values whose canonical numeric form would differ (leading-zero phone + // numbers, "1e5", oversized ids, ...) as text, and the leading-character check preserves the + // anti-formula protection for values starting with '=', '+', '-' or '@'. + if (is_numeric($value) + && !in_array($value[0], ['=', '+', '-', '@'], true) + && (string)(+$value) === $value) { + $activeWorksheet->setCellValue([$column, $row], +$value); + return; } + + // Explicitly type as string so values that start with '=' are not interpreted as formulas. + $activeWorksheet->getCell([$column, $row]) + ->setValueExplicit($value, DataType::TYPE_STRING); } } diff --git a/tests/Unit/Service/SubmissionServiceTest.php b/tests/Unit/Service/SubmissionServiceTest.php index 21fed31d2..f71f54448 100644 --- a/tests/Unit/Service/SubmissionServiceTest.php +++ b/tests/Unit/Service/SubmissionServiceTest.php @@ -35,6 +35,8 @@ use OCP\IUserManager; use OCP\IUserSession; use OCP\Mail\IMailer; +use PhpOffice\PhpSpreadsheet\Cell\DataType; +use PhpOffice\PhpSpreadsheet\Spreadsheet; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -618,6 +620,53 @@ public function testGetSubmissionsDataThrowsExceptionOnInvalidFormat() { $this->submissionService->getSubmissionsData($form, 'invalid'); } + public static function dataSetCellValueType(): array { + return [ + // Radio (and similar choice) answers are always strings, even when the option + // labels are pure numbers. For spreadsheet formats they must be stored as numbers + // so they can be aggregated (e.g. averaged) in the spreadsheet application. + 'radio-numeric-label-xlsx' => ['xlsx', '3', DataType::TYPE_NUMERIC, 3], + 'radio-numeric-label-ods' => ['ods', '5', DataType::TYPE_NUMERIC, 5], + 'numeric-zero' => ['xlsx', '0', DataType::TYPE_NUMERIC, 0], + 'numeric-decimal' => ['xlsx', '3.5', DataType::TYPE_NUMERIC, 3.5], + // Values starting with a formula-trigger character keep the string typing so they + // are never interpreted as formulas. + 'formula' => ['xlsx', '=SUM(A1:A2)', DataType::TYPE_STRING, '=SUM(A1:A2)'], + 'negative-number' => ['xlsx', '-5', DataType::TYPE_STRING, '-5'], + 'plus-prefixed' => ['xlsx', '+5', DataType::TYPE_STRING, '+5'], + 'at-prefixed' => ['xlsx', '@foo', DataType::TYPE_STRING, '@foo'], + // Strings whose canonical numeric form would differ must stay text to avoid data + // loss (leading-zero phone numbers, scientific notation, trailing decimal zeros, …). + 'leading-zero' => ['xlsx', '0123456789', DataType::TYPE_STRING, '0123456789'], + 'scientific' => ['xlsx', '1e5', DataType::TYPE_STRING, '1e5'], + 'trailing-decimal-zero' => ['xlsx', '3.50', DataType::TYPE_STRING, '3.50'], + 'text' => ['xlsx', 'Option A', DataType::TYPE_STRING, 'Option A'], + // CSV is always escaped and kept as string, even for pure numbers. + 'csv-number' => ['csv', '3', DataType::TYPE_STRING, '3'], + 'csv-formula' => ['csv', '=danger', DataType::TYPE_STRING, "'=danger"], + ]; + } + + /** + * @dataProvider dataSetCellValueType + */ + public function testSetCellValueDataType(string $fileFormat, string $value, string $expectedType, mixed $expectedValue): void { + $spreadsheet = new Spreadsheet(); + $worksheet = $spreadsheet->getActiveSheet(); + + TestCase::invokePrivate($this->submissionService, 'setCellValue', [$worksheet, 1, 1, $value, $fileFormat]); + + $cell = $worksheet->getCell([1, 1]); + + $this->assertSame($expectedType, $cell->getDataType()); + if ($expectedType === DataType::TYPE_NUMERIC) { + $this->assertEquals($expectedValue, $cell->getValue()); + } else { + // assertSame guards against a numeric-looking string being silently coerced. + $this->assertSame($expectedValue, $cell->getValue()); + } + } + /** * Setting up a very simple default CsvTest */