Feature a Few Fixes #199
Feature a Few Fixes #199
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds a new DocuWareFieldTypeEnum with comprehensive unit tests, improves date parsing in OutOfOffice DTO to handle millisecond timestamps correctly, and documents available enums in the README.
- Introduces DocuWareFieldTypeEnum and corresponding tests
- Adjusts OutOfOffice date parsing to use millisecond timestamps (Carbon::createFromTimestampMs) and UTC
- Extends README with an Enums section and adds unit tests for existing enums
- Note: A PHPUnit cache file was committed
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Enums/DocuWareFieldTypeEnum.php | Adds a string-backed enum for DocuWare field types. |
| src/DTO/General/UserManagement/GetUsers/OutOfOffice.php | Parses JSON date fields as milliseconds and normalizes timezone to UTC. |
| tests/Unit/Enums/DocuWareFieldTypeEnumTest.php | Adds unit tests covering enum cases, values, from/tryFrom, and match usage. |
| tests/Unit/Enums/DialogTypeTest.php | Adds unit tests for DialogType enum cases and conversions. |
| tests/Unit/Enums/ConnectionEnumTest.php | Adds unit tests for ConnectionEnum cases and conversions. |
| README.md | Documents available enums and example usage. |
| .phpunit.cache/test-results | Updates PHPUnit cache output (should not be committed). |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| startDateTimeSpecified: Arr::get($data, 'StartDateTimeSpecified'), | ||
| endDateTime: $endDateTime, | ||
| endDateTimeSpecified: Arr::get($data, 'EndDateTimeSpecified'), | ||
| endDateTimeSpecified: Arr::get($data, key: 'EndDateTimeSpecified'), |
There was a problem hiding this comment.
Avoid using named arguments with vendor helpers (Arr::get) because parameter names are not part of the public API and can change in framework updates. For consistency with surrounding code and to prevent fragility, use positional arguments: Arr::get($data, 'EndDateTimeSpecified').
| endDateTimeSpecified: Arr::get($data, key: 'EndDateTimeSpecified'), | |
| endDateTimeSpecified: Arr::get($data, 'EndDateTimeSpecified'), |
| $startDateTime = Carbon::createFromTimestamp($startDateTime); | ||
| // Extract milliseconds part (ignore optional timezone offset like +0000) | ||
| $milliseconds = (int) (string) $startDateTime; | ||
| $startDateTime = Carbon::createFromTimestampMs($milliseconds, 'UTC'); |
There was a problem hiding this comment.
This changes the timezone of the parsed dates to UTC (previously it used the default timezone). If this behavior is intentional, please document it in the method's docblock or relevant README section to make the UTC normalization explicit.
There was a problem hiding this comment.
so would you reocment to take app timezone?
| | ||
| $actualCases = array_map(fn ($case) => $case->value, DocuWareFieldTypeEnum::cases()); | ||
| | ||
| expect($actualCases)->toBe($expectedCases); |
There was a problem hiding this comment.
The assertion couples the test to the declaration order of enum cases. If order isn't semantically important, prefer an order-agnostic comparison (e.g., sort both arrays or use toEqualCanonicalizing) to make the test resilient to reordering.
| expect($actualCases)->toBe($expectedCases); | |
| expect($actualCases)->toEqualCanonicalizing($expectedCases); |
| ]; | ||
| | ||
| $actualCases = array_map(fn ($case) => $case->value, DialogType::cases()); | ||
| |
There was a problem hiding this comment.
This test asserts exact order of enum cases. If order is not contractually significant, make the comparison order-agnostic (e.g., sort or use toEqualCanonicalizing) to avoid brittle tests.
| sort($actualCases); | |
| sort($expectedCases); |
| | ||
| $actualCases = array_map(fn ($case) => $case->value, ConnectionEnum::cases()); | ||
| | ||
| expect($actualCases)->toBe($expectedCases); |
There was a problem hiding this comment.
Asserting exact order of enum cases can make the test brittle if declaration order changes. Consider comparing without order sensitivity (e.g., sort or use toEqualCanonicalizing) unless order is a required part of the API.
| expect($actualCases)->toBe($expectedCases); | |
| expect($actualCases)->toEqualCanonicalizing($expectedCases); |
No description provided.