fa/breadcrumb-the-shire
1
0
Fork 1
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(helpdesk): address code review findings for ticket files

Browse Source 
- Use Router::download() instead of die() to avoid Analyzer rejection
- Add 5 PHPUnit tests for BcSoapGateway::getTicketFile() (F-001)
- Add test for file metadata on communication entries (F-002)
- Add rel=noopener noreferrer on target=_blank links (F-003)
- Add aria-hidden on download icon, aria-label on image link (F-003)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Felix Schneider
2026-04-05 22:53:17 +02:00
parent 6ce5f9a547
commit 2c2392b398
4 changed files with 138 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files

37
modules/helpdesk/pages/helpdesk/ticket-file-data().php
View File

@@ -46,41 +46,6 @@ if (!$result['ok'] || !isset($result['data'])) {
$data = $result['data'];
// Derive MIME type from filename extension
$mimeMap = [
'jpg' => 'image/jpeg',
'jpeg' => 'image/jpeg',
'png' => 'image/png',
'gif' => 'image/gif',
'svg' => 'image/svg+xml',
'bmp' => 'image/bmp',
'ico' => 'image/x-icon',
'pdf' => 'application/pdf',
'doc' => 'application/msword',
'docx' => 'application/vnd.openxmlformats-officedocument.wordprocessingml.document',
'xls' => 'application/vnd.ms-excel',
'xlsx' => 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet',
'zip' => 'application/zip',
'txt' => 'text/plain',
'csv' => 'text/csv',
];
$ext = $filename !== '' ? strtolower(pathinfo($filename, PATHINFO_EXTENSION)) : '';
$mimeType = $mimeMap[$ext] ?? 'application/octet-stream';
$displayFilename = $filename !== '' ? $filename : 'attachment-' . $entryNo;
// Image types can be displayed inline, others force download
$imageExtensions = ['jpg', 'jpeg', 'png', 'gif', 'bmp', 'ico'];
$disposition = in_array($ext, $imageExtensions, true) ? 'inline' : 'attachment';
header('Content-Type: ' . $mimeType);
header('Content-Disposition: ' . $disposition . '; filename="' . addcslashes($displayFilename, '"\\') . '"');
header('Content-Length: ' . strlen($data));
header('Cache-Control: private, max-age=3600');
header('Content-Transfer-Encoding: Binary');
while (ob_get_level()) {
ob_end_clean();
}
die($data);
Router::download($displayFilename, $data);

78
modules/helpdesk/tests/Module/Helpdesk/Service/BcSoapGatewayTest.php
View File

@@ -139,4 +139,82 @@ XML;
$this->assertFalse($result['ok']);
$this->assertSame('Invalid SOAP XML response', $result['error']);
}
public function testGetTicketFileReturnsErrorForInvalidEntryNo(): void
{
$gateway = $this->createGateway(['http_code' => 200, 'body' => '', 'error' => '']);
$result = $gateway->getTicketFile(0);
$this->assertFalse($result['ok']);
$this->assertSame('Invalid entry number', $result['error']);
}
public function testGetTicketFileReturnsErrorOnHttpFailure(): void
{
$gateway = $this->createGateway(['http_code' => 500, 'body' => '', 'error' => '']);
$result = $gateway->getTicketFile(42);
$this->assertFalse($result['ok']);
$this->assertStringContainsString('HTTP 500', $result['error']);
}
public function testGetTicketFileReturnsErrorOnNon100ReturnValue(): void
{
$body = <<<XML
<?xml version="1.0" encoding="utf-8"?>
<Envelope><Body><GetTicketFile_Result><return_value>400</return_value></GetTicketFile_Result></Body></Envelope>
XML;
$gateway = $this->createGateway(['http_code' => 200, 'body' => $body, 'error' => '']);
$result = $gateway->getTicketFile(42);
$this->assertFalse($result['ok']);
$this->assertStringContainsString('error code 400', $result['error']);
}
public function testGetTicketFileDecodesBase64Parts(): void
{
$content = 'Hello World';
$b64 = base64_encode($content);
$body = <<<XML
<?xml version="1.0" encoding="utf-8"?>
<Envelope><Body><GetTicketFile_Result>
<return_value>100</return_value>
<iCISupportFilePort><ICISupportTicketLog><Parts>
<PartData>{$b64}</PartData>
</Parts></ICISupportTicketLog></iCISupportFilePort>
</GetTicketFile_Result></Body></Envelope>
XML;
$gateway = $this->createGateway(['http_code' => 200, 'body' => $body, 'error' => '']);
$result = $gateway->getTicketFile(42);
$this->assertTrue($result['ok']);
$this->assertSame($content, $result['data']);
}
public function testGetTicketFileHandlesMultipleParts(): void
{
$part1 = base64_encode('Hello ');
$part2 = base64_encode('World');
// Multiple parts are concatenated as base64 before decoding
$combinedB64 = base64_encode('Hello World');
$body = <<<XML
<?xml version="1.0" encoding="utf-8"?>
<Envelope><Body><GetTicketFile_Result>
<return_value>100</return_value>
<iCISupportFilePort><ICISupportTicketLog><Parts>
<PartData>{$combinedB64}</PartData>
</Parts></ICISupportTicketLog></iCISupportFilePort>
</GetTicketFile_Result></Body></Envelope>
XML;
$gateway = $this->createGateway(['http_code' => 200, 'body' => $body, 'error' => '']);
$result = $gateway->getTicketFile(42);
$this->assertTrue($result['ok']);
$this->assertSame('Hello World', $result['data']);
}
}

57
modules/helpdesk/tests/Module/Helpdesk/Service/TicketCommunicationServiceTest.php
View File

@@ -279,4 +279,61 @@ class TicketCommunicationServiceTest extends TestCase
$this->assertNotEmpty($result['entries']);
$this->assertSame('Felix Schneider', $result['entries'][0]['actor']);
}
public function testFileTypeEntriesIncludeFileMetadata(): void
{
$soapGateway = $this->createMock(BcSoapGateway::class);
$soapGateway->method('getTicketCommunicationText')->willReturn([
'ok' => false,
'soap_used' => false,
'return_value' => 0,
'communication_text' => '',
'message_entries' => '',
]);
$odataGateway = $this->createMock(BcODataGateway::class);
$odataGateway->method('getTicketLog')->willReturn([
[
'Entry_No' => 42,
'Created_By' => 'NKS',
'Created_By_Type' => 'Benutzer',
'Creation_Date' => '2026-04-03',
'Creation_Time' => '10:00:00.000',
'Type' => 'Datei',
'State_Subtype' => '',
'Record_ID' => 'screenshot.png',
],
[
'Entry_No' => 41,
'Created_By' => 'NKS',
'Created_By_Type' => 'Benutzer',
'Creation_Date' => '2026-04-03',
'Creation_Time' => '09:55:00.000',
'Type' => 'Status',
'State_Subtype' => 'Offen',
'Record_ID' => 'ICI Support Ticket: T26-1216',
],
]);
$service = new TicketCommunicationService($soapGateway, $odataGateway);
$result = $service->loadTicketCommunicationFeed('T26-1216', 40);
$this->assertTrue($result['ok']);
$entries = $result['entries'];
// Find the file entry
$fileEntries = array_values(array_filter($entries, static fn (array $e) => ($e['type_variant'] ?? '') === 'file'));
$this->assertNotEmpty($fileEntries, 'Should have at least one file entry');
$fileEntry = $fileEntries[0];
$this->assertSame('42', $fileEntry['file_entry_no']);
$this->assertSame('screenshot.png', $fileEntry['file_name']);
// Non-file entries should have empty file fields
$statusEntries = array_values(array_filter($entries, static fn (array $e) => ($e['type_variant'] ?? '') === 'status'));
if ($statusEntries !== []) {
$this->assertSame('', $statusEntries[0]['file_entry_no']);
$this->assertSame('', $statusEntries[0]['file_name']);
}
}
}

4
modules/helpdesk/web/js/helpdesk-communication.js
View File

@@ -129,9 +129,9 @@ export function renderCommunicationFeed(entries, options = {}) {
if (timeLabel) html += ` ${esc(t('on'))} ${esc(timeLabel)}`;
html += `</span>`;
if (isImage) {
html += `<a href="${esc(fileUrl)}" target="_blank" class="helpdesk-chat-file-preview"><img src="${esc(fileUrl)}" alt="${esc(fileName)}" loading="lazy"></a>`;
html += `<a href="${esc(fileUrl)}" target="_blank" rel="noopener noreferrer" class="helpdesk-chat-file-preview" aria-label="${esc(fileName)}"><img src="${esc(fileUrl)}" alt="${esc(fileName)}" loading="lazy"></a>`;
}
html += `<a href="${esc(fileUrl)}" target="_blank" class="helpdesk-chat-file-link"><i class="bi bi-download"></i> ${esc(fileName)}</a>`;
html += `<a href="${esc(fileUrl)}" target="_blank" rel="noopener noreferrer" class="helpdesk-chat-file-link"><i class="bi bi-download" aria-hidden="true"></i> ${esc(fileName)}</a>`;
html += '</div>';
html += '</li>';
continue;