feat(ZMSKVR-97): Add ICS download functionality for appointment confirmation #1566
Conversation
WalkthroughGenerates and surfaces ICS calendar content: backend maps processes to include optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Controller
participant Mapper as MapperService
participant MHelper as MailTemplateHelper
participant HTTP as App::$http
participant Logger as LoggerService
Client->>Controller: Request appointment/process
Controller->>Mapper: map(Process) → ThinnedProcess
Mapper->>Mapper: generateIcsContent(process)
alt providerId available
Mapper->>MHelper: load templates for provider
MHelper->>HTTP: GET /merged-mailtemplates/{providerId}/ (xtoken=App::$SECURE_TOKEN)
HTTP-->>MHelper: templates payload
MHelper-->>Mapper: templates
Mapper->>Mapper: build ICS (BEGIN:VCALENDAR...)
Mapper-->>Controller: ThinnedProcess{..., icsContent}
else missing provider/templates or error
Mapper->>Logger: log error with context
Mapper-->>Controller: ThinnedProcess{..., icsContent: null}
end
Controller-->>Client: JSON response includes `icsContent`
sequenceDiagram
autonumber
participant Bootstrap
participant App as Application
participant Env as Environment
Bootstrap->>App: initialize()
App->>App: initializeSecureToken()
App->>Env: getenv('ZMS_CONFIG_SECURE_TOKEN')
alt token present & non-empty
App-->>Bootstrap: set App::$SECURE_TOKEN
else missing/empty
App-->>Bootstrap: throw RuntimeException("ZMS_CONFIG_SECURE_TOKEN missing")
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentConfirmControllerTest.php (1)
117-125: Consider extracting ICS validation to a helper method.The ICS content validation pattern (assert key exists, assert contains VCALENDAR, unset for comparison) is repeated across multiple test files (AppointmentCancelControllerTest, AppointmentReserveControllerTest, AppointmentByIdControllerTest, AppointmentPreconfirmControllerTest, AppointmentConfirmControllerTest, AppointmentUpdateControllerTest). This duplication could be reduced by extracting to a shared helper method in the base ControllerTestCase class.
Example helper method:
// In ControllerTestCase.php protected function assertAndStripIcsContent(array &$responseBody, array &$expectedResponse): void { $this->assertArrayHasKey('icsContent', $responseBody); $this->assertStringContainsString('BEGIN:VCALENDAR', $responseBody['icsContent']); unset($responseBody['icsContent']); unset($expectedResponse['icsContent']); }Then in each test:
$this->assertAndStripIcsContent($responseBody, $expectedResponse); $this->assertEquals($expectedResponse, $responseBody);zmscitizenapi/src/Zmscitizenapi/Application.php (1)
63-66: Improve documentation for SECURE_TOKEN.The docblock "Security Token for API Access" is vague. Consider documenting:
- What specific API this token provides access to (based on the PR context, this appears to be for mail template API access used in ICS generation)
- What security guarantees this token provides
- How it should be configured (relationship to other ZMS module tokens)
- Any format requirements beyond non-empty string
Example improved documentation:
/** * Security token for accessing ZMS mail template API endpoints. * Used by MailTemplateHelper to fetch ICS calendar templates. * Must match the SECURE_TOKEN configured in zmsapi module. * * @var string */ public static string $SECURE_TOKEN;Also applies to: 83-90
zmscitizenview/src/types/AppointmentImpl.ts (1)
53-55: Tighten constructor param typing for subRequestCountsCtor uses any[] but class property is SubRequestCount[]. Align for type safety.
Consider updating the constructor signature:
constructor( // ... subRequestCounts: SubRequestCount[], // ... icsContent?: string ) { /* ... */ }zmscitizenapi/tests/Zmscitizenapi/Services/Core/MapperServiceTest.php (1)
170-175: Stabilize appointment stubAdd a return type to hasTime() and consider returning false to skip ICS generation during this unit test (reduces noise/logging).
$appointment = new class { public $date = '1724907600'; public function hasTime(): bool { return false; } // or true if explicitly needed };zmscitizenview/tests/unit/Appointment/AppointmentView.spec.ts (1)
16-19: Place vi.mock before importing the mocked moduleTo avoid subtle ESM mocking issues, move the vi.mock('@/utils/auth', ...) above all imports that reference it (even though Vitest hoists, ordering it first is clearer).
zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (1)
598-611: Avoid unnecessary ICS generation and respect no-attachment domainsGenerating ICS on every mapping can be heavy (HTTP + Twig). Optionally skip when not required.
Add after creating Config:
- $config = new Config(); + $config = new Config(); + // Skip if configuration says ICS is not required (e.g., no-attachment email domains) + if (!Messaging::isIcsRequired($config, $process, 'appointment')) { + return null; + }zmscitizenview/src/components/Appointment/AppointmentView.vue (1)
903-917: Consider using regular quotes for the static filename string.The implementation correctly handles ICS download with proper blob creation and cleanup. Minor style point: Line 911 uses template literal backticks for a static string without interpolation.
Apply this diff for consistency:
- link.download = `Termin.ics`; + link.download = "Termin.ics";zmscitizenapi/src/Zmscitizenapi/Utils/MailTemplateHelper.php (1)
15-15: Consider usingnullas the sentinel value for clarity.The current implementation uses
falseto indicate "not loaded yet", which works but creates confusion:
- Line 47's
?? []operator only handlesnull, notfalse. However, this works becauseloadTemplates()always sets$templates = []first (line 55).- The pattern mixes boolean logic with array operations, reducing readability.
Consider this clearer approach:
- protected $templates = false; + protected ?array $templates = null; public function getTemplate(string $templateName): ?string { - if (!$this->templates) { + if ($this->templates === null) { $this->loadTemplates(); } return $this->templates[$templateName] ?? null; } public function getTemplates(): array { - if (!$this->templates) { + if ($this->templates === null) { $this->loadTemplates(); } return $this->templates ?? []; }This makes the "not loaded yet" state explicit and type-safe. As per coding guidelines, prefer meaningful types over boolean flags for state tracking.
Also applies to: 29-48
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
zmscitizenapi/config.example.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Application.php(2 hunks)zmscitizenapi/src/Zmscitizenapi/Models/ThinnedProcess.php(4 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php(5 hunks)zmscitizenapi/src/Zmscitizenapi/Utils/MailTemplateHelper.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentCancelControllerTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentConfirmControllerTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentPreconfirmControllerTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentReserveControllerTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Services/Core/MapperServiceTest.php(1 hunks)zmscitizenview/resources/loader-test/index.html(1 hunks)zmscitizenview/src/api/models/AppointmentDTO.ts(1 hunks)zmscitizenview/src/components/Appointment/AppointmentView.vue(6 hunks)zmscitizenview/src/components/Appointment/CustomerInfo.vue(1 hunks)zmscitizenview/src/types/AppointmentImpl.ts(3 hunks)zmscitizenview/src/types/ProvideInjectTypes.ts(2 hunks)zmscitizenview/src/utils/de-DE.json(1 hunks)zmscitizenview/src/utils/en-US.json(1 hunks)zmscitizenview/src/zms-appointment.ce.vue(2 hunks)zmscitizenview/tests/unit/Appointment/AppointmentView.spec.ts(2 hunks)zmsentities/schema/citizenapi/thinnedProcess.json(1 hunks)zmsentities/src/Zmsentities/Helper/Messaging.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmscitizenview/src/components/Appointment/CustomerInfo.vuezmscitizenapi/tests/Zmscitizenapi/Services/Core/MapperServiceTest.phpzmscitizenview/src/utils/en-US.jsonzmscitizenview/src/types/AppointmentImpl.tszmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentReserveControllerTest.phpzmsentities/src/Zmsentities/Helper/Messaging.phpzmscitizenview/src/zms-appointment.ce.vuezmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.phpzmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentPreconfirmControllerTest.phpzmscitizenapi/src/Zmscitizenapi/Models/ThinnedProcess.phpzmscitizenapi/src/Zmscitizenapi/Application.phpzmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentConfirmControllerTest.phpzmscitizenview/src/api/models/AppointmentDTO.tszmscitizenapi/src/Zmscitizenapi/Utils/MailTemplateHelper.phpzmscitizenview/src/utils/de-DE.jsonzmscitizenview/src/components/Appointment/AppointmentView.vuezmscitizenapi/config.example.phpzmsentities/schema/citizenapi/thinnedProcess.jsonzmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.phpzmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentCancelControllerTest.phpzmscitizenview/resources/loader-test/index.htmlzmscitizenview/src/types/ProvideInjectTypes.tszmscitizenview/tests/unit/Appointment/AppointmentView.spec.tszmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmscitizenapi/tests/Zmscitizenapi/Services/Core/MapperServiceTest.phpzmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentReserveControllerTest.phpzmsentities/src/Zmsentities/Helper/Messaging.phpzmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.phpzmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentPreconfirmControllerTest.phpzmscitizenapi/src/Zmscitizenapi/Models/ThinnedProcess.phpzmscitizenapi/src/Zmscitizenapi/Application.phpzmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentConfirmControllerTest.phpzmscitizenapi/src/Zmscitizenapi/Utils/MailTemplateHelper.phpzmscitizenapi/config.example.phpzmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.phpzmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentCancelControllerTest.phpzmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php
**/*.{js,jsx,ts,tsx}
⚙️ CodeRabbit configuration file
**/*.{js,jsx,ts,tsx}: Flag any usage of console.log() as it should be replaced with proper logging or removed:
- For development: console.debug()
- For production: Remove console.log() statements or use structured logging
- For errors: Use error console.error()
Example replacement:
// Instead of: console.log('User data:', userData); // Use: console.debug('Processing user data', { userData }); // or for development only: Remove the console.log entirelyFlag specific logging violations:
- console.log(), console.debug(), console.warn() usage (except console.error in catch blocks)
- Any debug output that should use proper logging frameworks
Example replacements:
// Instead of: console.log('User data:', userData); console.debug('Processing...'); // Use: // Remove console.log entirely or use proper logging // Only console.error in catch blocks is acceptable try { processData(userData); } catch (error) { console.error('Processing failed:', error); }Flag JavaScript security and UX issues:
- alert(), confirm(), prompt() usage (poor UX)
- eval() usage (security risk)
- innerHTML with user input (XSS risk)
- Unfiltered user input in DOM manipulation
Example replacements:
// Instead of: alert('Error occurred'); eval(userInput); // Use: // Use proper error handling and UI components this.showErrorNotification('Error occurred'); // Avoid eval() entirelyFlag TODO/FIXME comments that indicate technical debt:
- TODO comments without clear action items
- FIXME comments indicating broken functionality
- HACK comments indicating temporary workarounds
- XXX comments indicating problematic code
These should be converted to proper issues or addressed:
// Instead of: // TODO: fix this later // FIXME: this is broken // Use: // Create proper issue and reference it // @see Issue #123: Refactor validation logicFlag code complexity issues:
- Functions longer than 50 lines
- Deep nesting (...
Files:
zmscitizenview/src/types/AppointmentImpl.tszmscitizenview/src/api/models/AppointmentDTO.tszmscitizenview/src/types/ProvideInjectTypes.tszmscitizenview/tests/unit/Appointment/AppointmentView.spec.ts
🧬 Code graph analysis (5)
zmscitizenapi/src/Zmscitizenapi/Models/ThinnedProcess.php (1)
zmscitizenapi/src/Zmscitizenapi/Models/ThinnedScope.php (1)
ThinnedScope(12-209)
zmscitizenapi/src/Zmscitizenapi/Utils/MailTemplateHelper.php (1)
zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php (1)
LoggerService(14-220)
zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (6)
zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php (2)
LoggerService(14-220)logError(96-127)zmscitizenapi/src/Zmscitizenapi/Utils/ClientIpHelper.php (1)
ClientIpHelper(7-34)zmscitizenapi/src/Zmscitizenapi/Utils/MailTemplateHelper.php (2)
MailTemplateHelper(13-102)getTemplates(42-48)zmsentities/src/Zmsentities/Helper/Messaging.php (3)
Messaging(25-403)generateIcsContent(302-356)getMailIcs(287-300)zmsentities/src/Zmsentities/Config.php (1)
Config(5-78)zmscitizenapi/tests/Zmscitizenapi/Services/Core/MapperServiceTest.php (1)
hasTime(172-172)
zmscitizenview/src/types/ProvideInjectTypes.ts (1)
zmscitizenview/src/api/models/AppointmentDTO.ts (1)
AppointmentDTO(9-100)
zmscitizenview/tests/unit/Appointment/AppointmentView.spec.ts (1)
zmscitizenview/src/utils/auth.ts (1)
isAuthenticated(6-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: call-php-unit-tests / zmsdb-test
🔇 Additional comments (17)
zmscitizenview/src/api/models/AppointmentDTO.ts (1)
94-99: LGTM!The addition of the optional
icsContentproperty is well-structured and properly documented. The type definition is clear and follows the existing interface pattern.zmsentities/schema/citizenapi/thinnedProcess.json (1)
155-162: LGTM!The
icsContentproperty addition follows the established schema pattern, with proper type definition and clear description. The nullable string type is appropriate for optional calendar content.zmscitizenview/src/utils/de-DE.json (1)
179-179: LGTM!The German translation entry is properly placed and follows the existing localization pattern. The translation "Termin ansehen" is clear and consistent with German UI conventions.
zmscitizenview/resources/loader-test/index.html (1)
17-19: LGTM!The addition of the
appointment-detail-urlattribute configures the component appropriately for the test environment and aligns with the new appointment detail view functionality.zmscitizenview/src/utils/en-US.json (1)
167-167: LGTM!The English translation entry is properly placed and maintains consistency with the German locale. The translation is clear and appropriate for the UI action.
zmscitizenview/src/components/Appointment/CustomerInfo.vue (1)
109-109: LGTM!Removing the unused
SelectedAppointmentProviderimport improves code cleanliness and reduces unnecessary dependencies. This is good housekeeping practice.zmsentities/src/Zmsentities/Helper/Messaging.php (1)
321-323: LGTM!The enhanced exception handling improves error diagnostics by storing the status value in
exception->data. This change is consistent with the error handling pattern used elsewhere in the file (e.g., lines 156-158) and provides better context for debugging template-related issues.zmscitizenview/src/types/AppointmentImpl.ts (1)
36-36: ICS field addition looks goodProperty and assignment are consistent with AppointmentDTO.
Also applies to: 71-71
zmscitizenview/src/types/ProvideInjectTypes.ts (1)
3-3: Good move to DTOUsing AppointmentDTO in the provider is cleaner and aligns with API contracts.
Also applies to: 23-24
zmscitizenview/tests/unit/Appointment/AppointmentView.spec.ts (1)
1281-1393: ICS tests are coherentCovers button visibility, method existence, and API call args. Looks good.
zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (1)
386-406: Ensuretimestampmapping matches frontend DTO
The backend caststimestamptostringviastrval(...), butAppointmentDTOuses a numeric type. Verify the DTO’stimestamptype and either cast to an integer inMapperServiceor update the frontend to accept a string.zmscitizenview/src/components/Appointment/AppointmentView.vue (2)
233-246: LGTM! Clear separation of authenticated vs. unauthenticated flows.The button rendering logic appropriately separates concerns: unauthenticated users receive a download option when ICS content is available, while authenticated users are directed to the detail page where they likely have additional options.
919-930: LGTM! Proper URL construction and navigation.The function correctly builds the detail page URL with appropriate fallback and properly passes the appointment ID as a query parameter using the defined constant.
zmscitizenapi/src/Zmscitizenapi/Models/ThinnedProcess.php (2)
51-74: LGTM! Consistent with existing property patterns.The
icsContentproperty is properly declared, initialized, and serialized following the same patterns as other properties in the class. Type hints and null safety are correctly applied.Also applies to: 103-104
118-126: LGTM! Accessor methods follow established patterns.The getter and setter methods are consistent with the existing
captchaTokenaccessors and provide appropriate type safety.zmscitizenapi/src/Zmscitizenapi/Utils/MailTemplateHelper.php (2)
53-84: LGTM! Robust template loading with proper error handling.The method correctly:
- Initializes the templates array early to ensure consistency
- Guards against missing provider ID
- Uses secure authentication token for API calls
- Validates template structure before populating
- Logs errors with detailed context without interrupting execution
91-101: LGTM! Defensive and flexible provider ID extraction.The method correctly handles both object and array provider formats with appropriate null safety checks and type casting.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.php (1)
42-48: Test additions follow consistent pattern.The mail template mock setup and ICS content validation are correctly implemented, consistent with other controller tests. Refer to the earlier comment on AppointmentCancelControllerTest regarding refactoring opportunities for this duplicated pattern.
Also applies to: 125-129
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentPreconfirmControllerTest.php (1)
43-48: Duplication of mail template setup across multiple test methods.This file exhibits the same duplication pattern discussed in AppointmentCancelControllerTest and AppointmentUpdateControllerTest. The mail template mock appears in 7+ test methods. Please refer to the earlier comments recommending extraction into a helper method or
setUp()configuration.Also applies to: 125-129, 217-222, 275-280, 383-388, 432-437, 481-486, 530-535
🧹 Nitpick comments (1)
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentCancelControllerTest.php (1)
47-52: Consider extracting duplicate test setup into a helper method.This mail template mock setup pattern is duplicated across AppointmentCancelControllerTest, AppointmentByIdControllerTest, AppointmentUpdateControllerTest, and AppointmentPreconfirmControllerTest. The ICS content validation is also repeated.
Consider creating a base test helper method in ControllerTestCase:
protected function addMailTemplateMock(int $providerId = 102522): array { return [ 'function' => 'readGetResult', 'url' => "/merged-mailtemplates/{$providerId}/", 'xtoken' => 'hash', 'response' => $this->readFixture("GET_merged_mailtemplates.json") ]; } protected function assertIcsContent(array $responseBody): void { $this->assertArrayHasKey('icsContent', $responseBody); $this->assertStringContainsString('BEGIN:VCALENDAR', $responseBody['icsContent']); }Then use:
$this->setApiCalls([ // ... other mocks ... $this->addMailTemplateMock() ]); $this->assertIcsContent($responseBody); unset($responseBody['icsContent']); unset($expectedResponse['icsContent']);Also applies to: 130-134
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.php(2 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentCancelControllerTest.php(3 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentConfirmControllerTest.php(8 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentPreconfirmControllerTest.php(8 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php(21 hunks)zmscitizenapi/tests/Zmscitizenapi/Services/Appointment/AppointmentUpdateServiceTest.php(2 hunks)zmscitizenapi/tests/Zmscitizenapi/bootstrap.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_merged_mailtemplates.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_merged_mailtemplates.json
🚧 Files skipped from review as they are similar to previous changes (1)
- zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentConfirmControllerTest.php
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentPreconfirmControllerTest.phpzmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.phpzmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.phpzmscitizenapi/tests/Zmscitizenapi/bootstrap.phpzmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentCancelControllerTest.phpzmscitizenapi/tests/Zmscitizenapi/Services/Appointment/AppointmentUpdateServiceTest.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentPreconfirmControllerTest.phpzmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.phpzmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.phpzmscitizenapi/tests/Zmscitizenapi/bootstrap.phpzmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentCancelControllerTest.phpzmscitizenapi/tests/Zmscitizenapi/Services/Appointment/AppointmentUpdateServiceTest.php
🧬 Code graph analysis (1)
zmscitizenapi/tests/Zmscitizenapi/Services/Appointment/AppointmentUpdateServiceTest.php (2)
zmsclient/src/Zmsclient/Http.php (1)
Http(16-260)zmsclient/src/Zmsclient/Result.php (1)
Result(17-248)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
- GitHub Check: call-php-code-quality / module-code-quality (zmsapi, 8.3)
- GitHub Check: call-php-code-quality / module-code-quality (zmscalldisplay, 8.3)
- GitHub Check: call-php-unit-tests / zmsdb-test
🔇 Additional comments (3)
zmscitizenapi/tests/Zmscitizenapi/bootstrap.php (1)
13-14: LGTM! Test environment setup is appropriate.The secure token environment variable is correctly set before loading the bootstrap, enabling tests to access merged mail templates via authenticated API calls.
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentCancelControllerTest.php (1)
159-165: Request verification: Error handling for mail template fetch failures.While the mail template mock is added, there are no test cases covering scenarios where the merged mail templates fetch fails. Consider adding tests to verify graceful degradation.
Do the controllers handle mail template fetch failures gracefully? If the
/merged-mailtemplates/endpoint returns an error, does the appointment operation still succeed without the ICS content, or does it fail the entire request?Consider adding test cases like:
public function testRenderingWithMailTemplateFetchFailure() { $exception = new \BO\Zmsclient\Exception(); $exception->template = 'BO\\Zmsapi\\Exception\\MailTemplate\\MailTemplateNotFound'; $this->setApiCalls([ // ... process and source mocks ... [ 'function' => 'readGetResult', 'url' => '/merged-mailtemplates/102522/', 'xtoken' => 'hash', 'exception' => $exception ] ]); // Verify appointment still works but no icsContent }zmscitizenapi/tests/Zmscitizenapi/Services/Appointment/AppointmentUpdateServiceTest.php (1)
230-273: Service test correctly handles new mail template dependency.The test properly:
- Adds the mail templates JSON structure and mock response
- Increments the expected call count from 2 to 3
- Routes the
/merged-mailtemplates/URL in the callbackThe empty
dataarray inmailTemplatesJson(line 238) is acceptable for this test since it focuses on client data validation rather than template content processing.
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php
Show resolved
Hide resolved
1 participant
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Context
This PR is based on the commits from PR #1545, which has already been merged into
next. However, an issue was subsequently noticed on zms-test, which is why #1545 was initially reverted fromnext(see PR #1565). This PR serves to implement the outstanding issues and then merge them back intonexttogether with all the features that have already been implemented by @ThomasAFink.Summary by CodeRabbit
New Features
Configuration
Localization
Tests