fix(ZMSKVR-129 ZMSKVR-140): fix invalid appointment timestamps array and too many appointments with email at location #895
Conversation
…-email-at-location
…-email-at-location
WalkthroughThis update increases the allowable number of public methods in PHPMD’s ruleset and introduces a new nullable boolean property, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant A as AppointmentUpdateService
participant V as ValidationService
C->>A: Submit appointment update data
A->>V: Call validateClientData(data)
V-->>A: Return validation errors (if any)
alt Errors exist
A-->>C: Return error response
else No errors
A->>A: Process the reserved process and update updates
A-->>C: Return updated process data
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*.php`: Flag any usage of error_log() as it should be re...
⏰ Context from checks skipped due to timeout of 90000ms (16)
🔇 Additional comments (7)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (14)
zmscitizenapi/tests/Zmscitizenapi/Services/Core/ZmsApiFacadeServiceTest.php (1)
129-179: Add test coverage for different emailRequired scenarios.While the test covers the case where
emailRequiredis '1', consider adding test cases for:
- When
emailRequiredis '0'- When
emailRequiredis not set in preferences- How this preference affects appointment creation (possibly in a separate test)
This would ensure robust validation of the email requirement feature that addresses the "too many appointments with email" issue.
Here's a suggested test method to add:
public function testGetScopesWithEmailNotRequired(): void { $provider = new Provider(); $provider->id = 1; $provider->name = 'Test Provider'; $provider->source = 'unittest'; $scope = new Scope(); $scope->id = 1; $scope->provider = $provider; $scope->preferences = [ 'client' => [ 'emailRequired' => '0', 'telephoneActivated' => '1', // ... other preferences ... ] ]; $source = new Source(); $source->providers = new ProviderList([$provider]); $source->scopes = new ScopeList([$scope]); // Mock cache miss $this->cacheMock->method('get')->willReturn(null); // Mock HTTP response $result = $this->createMock(Result::class); $result->method('getEntity')->willReturn($source); $this->httpMock->expects($this->any()) ->method('readGetResult') ->willReturn($result); $result = ZmsApiFacadeService::getScopes(); $thinnedScope = $result->getScopes()[0]; $this->assertFalse($thinnedScope->emailRequired); }zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficeListByServiceControllerTest.php (1)
24-188: Consider adding edge case tests for email configuration.While the current test coverage is good for basic email requirement configurations, consider adding test cases for:
- Changes to email requirements (if supported)
- Invalid email requirement configurations
- Impact on appointment creation/validation
zmscitizenapi/tests/Zmscitizenapi/Services/Core/ValidationServiceTest.php (1)
227-302: Add test cases for edge cases and field constraints.While the test coverage is good, consider adding the following test cases:
- Edge cases for email validation (e.g., very long emails, special characters)
- Maximum length constraints for all fields
- Special characters in family name and custom text
- Behavior when emailRequired is false
Here's an example of additional test cases:
public function testValidateAppointmentUpdateFields(): void { // ... existing test cases ... + + // Test maximum length constraints + $result = ValidationService::validateAppointmentUpdateFields( + str_repeat('a', 256), // Assuming 255 is max length + 'john@example.com', + '+1234567890', + 'Custom text', + $scope + ); + $this->assertContains( + ErrorMessages::get('invalidFamilyName'), + $result['errors'] + ); + + // Test email not required + $emailOptionalScope = new ThinnedScope(); + $emailOptionalScope->emailRequired = false; + $result = ValidationService::validateAppointmentUpdateFields( + 'John Doe', + null, + null, + null, + $emailOptionalScope + ); + $this->assertEmpty($result['errors']);zmscitizenapi/src/Zmscitizenapi/Services/Appointment/AppointmentUpdateService.php (1)
24-24: Potential redundant call togetReservedProcess.
You callgetReservedProcess($clientData->processId, $clientData->authKey)here after having already called it insidevalidateClientData. This could lead to unnecessary duplicate network calls or overhead. Consider eliminating the second call or performing all process retrieval just once (e.g., withinvalidateClientData) and reusing the result.zmscitizenapi/tests/Zmscitizenapi/Services/Appointment/AppointmentUpdateServiceTest.php (1)
23-29: Reflection-based capture of static facade can be fragile.
Using reflection to store and restore static properties is acceptable as a last resort. Consider using a clear public API or built-in mocks if possible. This approach may break if internal details ofZmsApiFacadeServicechange.zmscitizenapi/tests/Zmscitizenapi/Services/Scope/ScopeByIdServiceTest.php (1)
98-98: Consider adding explicit test cases for emailRequired behavior.While the mock includes the emailRequired property, consider adding specific test cases to validate the behavior when emailRequired is true vs false.
public function testGetScopeReturnsThinnedScope(): void { // Arrange $expectedScope = $this->createMockThinnedScope(); $scopeId = 123; $queryParams = ['scopeId' => (string)$scopeId]; $this->createMockValidationService([]); $this->createMockFacade($expectedScope); // Act $result = $this->service->getScope($queryParams); // Assert $this->assertInstanceOf(ThinnedScope::class, $result); $this->assertEquals($expectedScope, $result); + $this->assertFalse($result->getEmailRequired()); } +public function testGetScopeWithEmailRequired(): void +{ + // Arrange + $expectedScope = new ThinnedScope( + id: 123, + provider: null, + shortName: 'Test Scope', + emailRequired: true, + telephoneActivated: false, + telephoneRequired: false, + customTextfieldActivated: false, + customTextfieldRequired: false, + customTextfieldLabel: null, + captchaActivatedRequired: false, + displayInfo: null + ); + $scopeId = 123; + $queryParams = ['scopeId' => (string)$scopeId]; + + $this->createMockValidationService([]); + $this->createMockFacade($expectedScope); + + // Act + $result = $this->service->getScope($queryParams); + + // Assert + $this->assertInstanceOf(ThinnedScope::class, $result); + $this->assertTrue($result->getEmailRequired()); +}zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopeByIdControllerTest.php (1)
59-59: Consider adding test case for emailRequired=true scenario.While the test covers the emailRequired=false case, consider adding a test case with scopeId=2 to validate the behavior when emailRequired is true.
+public function testRenderingWithEmailRequired() +{ + $this->setApiCalls([ + [ + 'function' => 'readGetResult', + 'url' => '/source/unittest/', + 'parameters' => [ + 'resolveReferences' => 2, + ], + 'response' => $this->readFixture("GET_SourceGet_dldb.json"), + ] + ]); + $response = $this->render([], [ + 'scopeId' => '2' + ], []); + $expectedResponse = [ + "id" => 2, + "provider" => [ + "id" => 9999999, + "name" => "Unittest Source Dienstleister 2", + "lat" => 48.12750898398659, + "lon" => 11.604317899956524, + "source" => "unittest", + "contact" => [ + "city" => "Berlin", + "country" => "Germany", + "name" => "Unittest Source Dienstleister 2", + "postalCode" => "10178", + "region" => "Berlin", + "street" => "Alte Jakobstraße", + "streetNumber" => "106" + ] + ], + "shortName" => "Scope 2", + "emailFrom" => "no-reply@muenchen.de", + 'emailRequired' => true, + "telephoneActivated" => false, + "telephoneRequired" => true, + "customTextfieldActivated" => false, + "customTextfieldRequired" => true, + "customTextfieldLabel" => "", + "captchaActivatedRequired" => false, + "displayInfo" => null + ]; + $responseBody = json_decode((string)$response->getBody(), true); + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEqualsCanonicalizing($expectedResponse, $responseBody); +}zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsControllerTest.php (1)
73-73: Consider reducing test data duplication.The expected response structure is duplicated between
testRenderingandtestDisplayNotPublicRequests. Consider extracting the common test data into a shared fixture or helper method.Example refactor:
private function getExpectedOfficesResponse() { return [ "offices" => [ [ // ... other fields ... "scope" => [ // ... other fields ... "emailRequired" => false, // ... other fields ... ] ], [ // ... other fields ... "scope" => [ // ... other fields ... "emailRequired" => true, // ... other fields ... ] ] ], // ... services and relations ... ]; }Then use it in both test methods:
-$expectedResponse = [ - "offices" => [ - // ... duplicated structure ... - ] -]; +$expectedResponse = $this->getExpectedOfficesResponse();Also applies to: 116-116, 209-209, 252-252
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (1)
559-560: Consider using null as the default value for emailRequired.In
getThinnedProcessById, the default value foremailRequiredis set tofalse. This differs from other methods where null is used as the default value. Consider using null for consistency.Apply this diff to maintain consistency:
- emailRequired: (bool) $process->scope->getEmailRequired() ?? false, + emailRequired: (bool) $process->scope->getEmailRequired() ?? null,zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php (1)
124-144: Consider consolidating API call mocking setup.The API call mocking setup is duplicated across multiple test methods. Consider extracting it to a helper method to reduce code duplication.
Example implementation:
private function setupDefaultApiCalls(): void { $this->setApiCalls([ [ 'function' => 'readGetResult', 'url' => '/process/101002/fb43/', 'parameters' => [ 'resolveReferences' => 2, ], 'response' => $this->readFixture("GET_process.json") ], [ 'function' => 'readGetResult', 'url' => '/source/unittest/', 'parameters' => [ 'resolveReferences' => 2, ], 'response' => $this->readFixture("GET_SourceGet_dldb.json") ] ]); }zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_reserve_SourceGet_dldb.json (1)
62-66: New 'emailRequired' Property Added in Client Preferences
The addition of"emailRequired": "0"under thepreferences.clientobject clarifies that, for this fixture, an email is not mandatory. One suggestion: consider aligning the data type with the new nullable boolean property in the domain model. Using a Boolean value (e.g.falseinstead of"0") may prevent type conversion issues later on.zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_SourceGet_dldb.json (2)
105-109: Verify Email Requirement Consistency for Scope 1
For the first scope, the"client"object now includes"emailRequired": "0". Please confirm that downstream mapping and validation correctly interpret"0"as indicating that an email is not required, and consider using native Boolean values if feasible.
132-136: Confirm Email Requirement Flag for Scope 2
In the second scope, the"client"object includes"emailRequired": "1", signaling that an email is required. Ensure that this value is handled properly by the service layer and that any tests expecting a Boolean reflect the intended behavior. Similar to the previous comment, you might consider using an actual Boolean value (true) for clarity and consistency.zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_process.json (1)
127-130: Updated 'emailRequired' Flag in Process Fixture
The change to"emailRequired": "1"within the client's preferences in the process data ensures that email is now mandatory. Confirm that all related services (e.g. validation and mapping) and test cases reflect this updated requirement. Additionally, consider if representing this flag as a Boolean (true) might reduce potential type conversion issues.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
phpmd.rules.xml(1 hunks)zmscitizenapi/src/Zmscitizenapi/Models/ThinnedScope.php(4 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Appointment/AppointmentUpdateService.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php(3 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Core/ValidationService.php(2 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php(6 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(17 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficeListByServiceControllerTest.php(3 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesListControllerTest.php(2 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsControllerTest.php(4 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopeByIdControllerTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopesListControllerTest.php(2 hunks)zmscitizenapi/tests/Zmscitizenapi/Services/Appointment/AppointmentUpdateServiceTest.php(2 hunks)zmscitizenapi/tests/Zmscitizenapi/Services/Core/MapperServiceTest.php(7 hunks)zmscitizenapi/tests/Zmscitizenapi/Services/Core/ValidationServiceTest.php(2 hunks)zmscitizenapi/tests/Zmscitizenapi/Services/Core/ZmsApiFacadeServiceTest.php(2 hunks)zmscitizenapi/tests/Zmscitizenapi/Services/Scope/ScopeByIdServiceTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_SourceGet_dldb.json(2 hunks)zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_process.json(1 hunks)zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_reserve_SourceGet_dldb.json(1 hunks)zmsdb/migrations/91740151321-set-email-required-for-all-locations.sql(1 hunks)zmsentities/src/Zmsentities/Scope.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- zmsdb/migrations/91740151321-set-email-required-for-all-locations.sql
🧰 Additional context used
🧠 Learnings (3)
zmscitizenapi/tests/Zmscitizenapi/Services/Core/ValidationServiceTest.php (1)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-02-21T10:24:04.000Z
Learning: In zmscitizenapi's tests, when validation logic is moved from private service methods to ValidationService, update tests to use ValidationService methods directly instead of using invokePrivateMethod. This ensures tests reflect the actual validation flow and dependencies.
zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (1)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-02-14T16:58:25.118Z
Learning: In zmscitizenapi's MapperService::thinnedProcessToProcess, preferences must be set as array structure (not objects) with client.emailFrom and client.adminMailOnDeleted for email functionality to work correctly. The Scope class uses getPreference() method to access these values.
zmscitizenapi/src/Zmscitizenapi/Services/Appointment/AppointmentUpdateService.php (3)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Controllers/AppointmentUpdate.php:0-0
Timestamp: 2025-01-08T10:08:39.350Z
Learning: The getThinnedProcessById method handles null process scenarios through ValidationService::validateGetProcessNotFound which returns the 'appointmentNotAvailable' error message, making additional null checks redundant.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-02-21T10:26:32.712Z
Learning: In zmscitizenapi's AppointmentUpdateService, wrap all validation logic (basic auth, process fetching, field validation) in a validateClientData method to maintain consistency with other controllers and encapsulate validation logic in one place.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Controllers/AppointmentUpdate.php:0-0
Timestamp: 2025-01-08T10:08:39.350Z
Learning: The getThinnedProcessById method in ZmsApiFacadeService already implements comprehensive error handling for process not found scenarios through ZmsApiClientService and ValidationService, making additional null checks in controllers redundant.
🔇 Additional comments (37)
zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficeListByServiceControllerTest.php (3)
71-71: LGTM! Test case added for required email configuration.The test case correctly verifies that the scope response includes the
emailRequiredflag set to true for service ID 2.
134-134: LGTM! Test case added for optional email configuration.The test case correctly verifies that the scope response includes the
emailRequiredflag set to false for the first scope of service ID 1.
174-174: LGTM! Comprehensive test coverage for mixed email configurations.The test case effectively verifies that different scopes within the same service can have different email requirements (required vs optional), providing good coverage for this feature.
zmscitizenapi/tests/Zmscitizenapi/Services/Core/ValidationServiceTest.php (2)
7-7: LGTM!The import of
ThinnedScopemodel is correctly added to support the new validation tests.
209-225: LGTM! Well-structured test cases.The test method has been correctly renamed and restructured to focus on authentication validation. The implementation follows best practices by:
- Using direct validation service methods
- Testing both valid and invalid scenarios
- Having clear assertions with descriptive error messages
zmscitizenapi/src/Zmscitizenapi/Services/Appointment/AppointmentUpdateService.php (2)
17-17: Use ofextractClientDatais well-structured.
Extracting client input early in theprocessUpdatemethod clarifies where data conversion/cleaning occurs before validation.
32-61: Validation approach is consistent with retrieved learnings.
Wrapping all validation logic invalidateClientDatais a best practice aligned with the user feedback. The granular steps to combine errors from each stage are coherent. However, notice thatvalidateAppointmentUpdateFieldsis always invoked, even if earlier validations fail. If you plan to skip further checks once critical errors have been found, ensure to short-circuit to optimize performance. Otherwise, this design is acceptable.zmscitizenapi/tests/Zmscitizenapi/Services/Appointment/AppointmentUpdateServiceTest.php (3)
2-16: Migration toTestCaseand new static facade property looks good.
Switching toPHPUnit\Framework\TestCasepromotes standard testing, and introducing$originalFacadeis a practical way to safeguard global state changes during tests. Ensure that any further static property usage in tests remains thread-safe if you ever run parallel tests.
30-38: Properly restoring the facade intearDown.
Restoring the global state intearDownavoids cross-test side effects. This is a good practice to maintain isolation.
100-269: Thorough mocking of external HTTP calls.
The test uses a well-structured mock setup for/processand/sourceendpoints, confirming correct validation logic forvalidateClientData. Including afinallyblock to restore the global HTTP client is prudent. Be cautious with parallel test execution, as shared static variables can still cause race conditions.zmscitizenapi/src/Zmscitizenapi/Services/Core/ValidationService.php (4)
8-8: Newuse BO\Zmscitizenapi\Models\ThinnedScopedependency is appropriate.
UsingThinnedScopefor conditional field checks consolidates domain logic. Ensuring any references toemailRequiredor other scope-based properties remain consistent with the model’s definition.
190-200: Clear separation of process/auth key validation.
validateAppointmentUpdateAuthneatly isolates process ID and auth key validation. This improves readability and maintainability compared to a single monolithic validation method.
202-217: Granular field validations enhance flexibility.
validateAppointmentUpdateFieldsdelegates to specialized methods for each field. This modular structure is easier to maintain, especially if you need to expand checks (e.g., new required fields).
219-268: Scope-aware validation logic is consistent and modular.
Each private validator (validateFamilyNameField,validateEmailField, etc.) neatly checks one field in the context ofThinnedScope. This design is straightforward and aligns with best practices. Be mindful of any performance impact if many fields keep cross-referencing the same scope; however, this is typically not an issue.zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopesListControllerTest.php (1)
60-60: LGTM! Comprehensive test coverage for email requirements.The test cases effectively validate both enabled and disabled email requirement scenarios across different scopes.
Also applies to: 89-89
zmscitizenapi/src/Zmscitizenapi/Models/ThinnedScope.php (1)
24-24: LGTM! Well-structured implementation of emailRequired property.The implementation follows best practices:
- Proper PHP type hints
- Consistent with existing properties
- Complete with getter method and serialization
Also applies to: 39-39, 45-45, 78-81, 125-125
zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesListControllerTest.php (1)
73-73: LGTM! Test updates for email requirement validation.The test has been correctly updated to validate the new
emailRequiredfield in the response structure, with appropriate test cases for both enabled and disabled states.Also applies to: 116-116
zmsentities/src/Zmsentities/Scope.php (1)
39-42: LGTM! Well-implemented getter for email requirement.The implementation follows the established pattern for preference getters and correctly handles the nullable state.
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentCancelControllerTest.php (1)
90-90: LGTM! Test update for cancelled appointment scope.The test has been correctly updated to include the
emailRequiredfield with an appropriatenullvalue for a cancelled appointment's scope.zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentPreconfirmControllerTest.php (3)
85-85: AddemailRequiredto expected response.The addition of
emailRequiredfield withnullvalue aligns with the PR's objective of managing email requirements for appointments.
321-362: Test case aligns with PR objective.The test case
testTooManyEmailsAtLocationdirectly addresses the PR's objective of handling the issue with too many appointments associated with the same email at a location.
364-405: Test case validates email requirement.The test case
testEmailRequiredensures proper validation when email is required, complementing the PR's email management functionality.zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (3)
89-89: Consistent handling of emailRequired in office mapping.The mapping correctly handles the nullable boolean
emailRequiredfield, maintaining type safety with explicit boolean casting.
194-194: Consistent handling of emailRequired in scope mapping.The mapping correctly handles the nullable boolean
emailRequiredfield from scope data, maintaining type safety with explicit boolean casting.
287-287:❓ Verification inconclusive
Verify default value for emailRequired in process mapping.
The mapping sets
emailRequiredtofalseby default, which differs from thenulldefault in other mappings. This might need verification to ensure consistency.Run this script to check the default value usage across the codebase:
🏁 Script executed:
#!/bin/bash # Search for emailRequired default value assignments rg -A 2 "emailRequired.*=.*(?:false|null)" --type phpLength of output: 6919
Email Required Default Consistency Verification
In
MapperService.php(line 287) the mapping sets:'emailRequired' => $thinnedProcess->scope->getEmailRequired() ?? false,However, in other parts of the code (e.g., in
ZmsApiFacadeService.phpand some tests) the default foremailRequiredis set to or expected asnull. Please verify whether defaulting tofalsehere is intentional or if it should be made consistent with thenulldefault used elsewhere.zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentReserveControllerTest.php (1)
90-90:❓ Verification inconclusive
Verify emailRequired default value consistency.
The test expects
emailRequiredto befalseby default, which should be verified against the actual business logic requirements.Run this script to check the expected default value across test files:
🏁 Script executed:
#!/bin/bash # Search for emailRequired assertions in tests rg -A 2 "'emailRequired'.*=>.*(?:false|null|true)" --type php testLength of output: 119
Action Required: Verify
emailRequiredDefault Value ConsistencyIn the test file
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentReserveControllerTest.php(line 90), the default value is set as:'emailRequired' => false,The intent is to ensure that this default remains consistent with the business logic expectations. However, the initial shell command failed to produce output because it referenced a non-existent directory (
test). Please re-run the verification using the correct tests directory.For example, try running the following shell script:
#!/bin/bash # Re-run search for emailRequired assertions in the tests directory rg -A 2 "emailRequired.*=>.*(?:false|null|true)" --type php zmscitizenapi/testsAfter manually verifying that all test files (and the business logic) consistently use
falseas the default value foremailRequired, please confirm that this value is correct. If you find discrepancies, update the tests or business logic implementation accordingly.zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentConfirmControllerTest.php (1)
84-86: LGTM!The addition of
emailRequiredto the expected response structure aligns with the broader changes in the codebase to support email requirement validation.zmscitizenapi/tests/Zmscitizenapi/Services/Core/MapperServiceTest.php (4)
207-208: LGTM!The addition of
emailRequiredassertion intestMinimalProcessToThinnedProcessensures proper testing of the default value.Also applies to: 240-241
470-471: LGTM!The test case in
testScopeToThinnedScopeproperly validates thatemailRequiredis correctly mapped when set to true.Also applies to: 484-485
609-610: LGTM!The test case in
testScopeToThinnedScopeWithMissingProviderproperly validates thatemailRequiredis correctly mapped when set to false.Also applies to: 620-621
635-636: LGTM!The test case in
testScopeToThinnedScopeWithEmptyDataproperly validates thatemailRequiredis null when scope data is empty.zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (4)
83-84: LGTM!The addition of
emailRequiredin thegetOfficesmethod is consistent with the model changes.
121-122: LGTM!The addition of
emailRequiredin thegetScopesmethod is consistent with the model changes.
203-204: LGTM!The addition of
emailRequiredin thegetScopeByOfficeIdmethod is consistent with the model changes.Also applies to: 217-218
312-313: LGTM!The addition of
emailRequiredin thegetScopeByIdmethod is consistent with the model changes.zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php (1)
84-85: LGTM!The addition of
emailRequiredto the expected response structure is consistent with the model changes.phpmd.rules.xml (1)
71-73: Increased maxmethods Threshold
The update increases the"maxmethods"value from 20 to 25. This change will allow classes to have more public methods without triggering a PHPMD warning. Ensure that this new threshold is well documented in your team’s coding guidelines to avoid masking overly complex classes.
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.php
Show resolved
Hide resolved
ThomasAFink
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
zmsentities/schema/citizenapi/office.json (1)
113-115: Schema Reference Update: Enhanced Modularity and ConsistencyThe update to the
"scope"property, replacing an empty object with a reference to"./ThinnedScope.json", improves maintainability by centralizing the schema definition. This promotes consistency across the system and facilitates future adjustments, like the introduction of the newemailRequiredproperty.Please verify:
- That the
ThinnedScope.jsonfile exists in the expected relative location.- The referenced schema properly defines all required properties, including
emailRequired, to align with the intended behavior across services and tests.zmsentities/schema/citizenapi/service.json (1)
2-7: Review "title" and "type" DefinitionsThe schema now defines the Service entity’s type as an array, object, or null. Please confirm that this level of flexibility is intended. If the Service is conceptually an object, you might simplify the type to "object" for stricter validation.
zmsentities/schema/citizenapi/thinnedProcess.json (2)
23-29: Review 'timestamp' Field TypeThe
"timestamp"is defined as a union of"string"and"null", while its description indicates it represents seconds since epoch. Consider whether a numerical type (e.g.,"integer") might be more appropriate for representing epoch seconds. If a string is required for specific reasons (such as formatting or legacy constraints), it would be beneficial to add a brief note explaining the rationale.
72-74: Externalize 'scope' via $ref to ThinnedScope.jsonReplacing the detailed inline definition of
"scope"with a"$ref"to"./ThinnedScope.json"enhances modularity and maintainability. Please ensure that the external schema file is versioned and well-documented—especially since it now includes the newemailRequiredproperty—and verify that all integrations referencing"scope"have been updated accordingly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
zmscitizenapi/src/Zmscitizenapi/Models/ProcessFreeSlots.php(0 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php(7 hunks)zmsentities/schema/citizenapi/availableAppointmentsByOffice.json(1 hunks)zmsentities/schema/citizenapi/collections/officeList.json(1 hunks)zmsentities/schema/citizenapi/collections/officeServiceAndRelationList.json(1 hunks)zmsentities/schema/citizenapi/collections/officeServiceRelationList.json(1 hunks)zmsentities/schema/citizenapi/collections/serviceList.json(1 hunks)zmsentities/schema/citizenapi/collections/thinnedScopeList.json(1 hunks)zmsentities/schema/citizenapi/office.json(1 hunks)zmsentities/schema/citizenapi/processFreeSlots.json(0 hunks)zmsentities/schema/citizenapi/service.json(1 hunks)zmsentities/schema/citizenapi/thinnedProcess.json(2 hunks)zmsentities/schema/citizenapi/thinnedProvider.json(1 hunks)zmsentities/schema/citizenapi/thinnedScope.json(2 hunks)
💤 Files with no reviewable changes (2)
- zmsentities/schema/citizenapi/processFreeSlots.json
- zmscitizenapi/src/Zmscitizenapi/Models/ProcessFreeSlots.php
✅ Files skipped from review due to trivial changes (4)
- zmsentities/schema/citizenapi/collections/officeList.json
- zmsentities/schema/citizenapi/thinnedProvider.json
- zmsentities/schema/citizenapi/collections/thinnedScopeList.json
- zmsentities/schema/citizenapi/collections/officeServiceAndRelationList.json
🚧 Files skipped from review as they are similar to previous changes (2)
- zmsentities/schema/citizenapi/availableAppointmentsByOffice.json
- zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: call-owasp-security-checks / security-scan (zmsclient, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsdb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsslim, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsentities, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsdldb, 8.0)
- GitHub Check: call-php-code-quality / module-code-quality (zmsentities, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsmessaging, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmscitizenapi, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmscalldisplay, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsapi, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsadmin, 8.0)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (10)
zmsentities/schema/citizenapi/collections/serviceList.json (1)
7-7: Modular Schema Reference UpdateReplacing the inline schema definition for the "services" items with an external reference (
"$ref": "../service.json") streamlines the schema by delegating the detailed structure to a dedicated file. Ensure that the referencedservice.jsonis maintained consistently with the rest of the project’s schema definitions.zmsentities/schema/citizenapi/collections/officeServiceRelationList.json (1)
7-7: Externalizing Office Service Relation SchemaSwitching from an inline schema definition to a reference (
"$ref": "../officeServiceRelation.json") for the "relations" items reinforces modularity and consistency across schemas. Please confirm that the external file exists at the correct relative path and follows the expected structure.zmsentities/schema/citizenapi/service.json (1)
30-32: Simplified "combinable" PropertyThe "combinable" property now uses a JSON Reference (
"$ref": "./combinable.json") to delegate its definition externally. This enhances modularity and reusability. Ensure that the externalcombinable.jsonstays consistent with your overall schema expectations.zmsentities/schema/citizenapi/thinnedScope.json (2)
17-19: Refactored "provider" PropertyThe detailed inline definition for "provider" has been replaced with a reference to
./thinnedProvider.json. This change streamlines the schema and promotes reuse. Make sure that the external schema accurately represents all required provider attributes.
41-44: Addition of "emailRequired" PropertyIntroducing the new "emailRequired" boolean property clearly indicates whether an email is required. Verify that all corresponding service components, mappings, and tests (as noted in the PR objectives) have been updated to handle this new field appropriately.
zmsentities/schema/citizenapi/thinnedProcess.json (5)
3-7: Clarify Schema 'type' Union UsageThe top-level
"type"property now accepts an array of types:"array","object", and"null". Please verify that this union is intentional. In many cases, a JSON schema is expected to represent a concrete structure (typically an object), so if an array is truly a possible format for a ThinnedProcess, it would be helpful to document this decision clearly to avoid confusion for schema consumers.
10-14: Validate 'processId' NullabilityThe
"processId"property is marked as required but its type permits"null". Confirm that this design is intentional—i.e. that the key must be present even if its value is null—and ensure that all consumers of this schema properly handle a null value for a required field.
17-22: Confirm 'authKey' NullabilitySimilar to
"processId", the"authKey"property is required yet allows the"null"value. Please double-check that allowing null here is the desired behavior and that downstream systems can unambiguously differentiate between a missing key and one that is present with a null value.
75-100: Review 'status' Enum ValuesThe
"status"property now includes a comprehensive enumeration of allowed statuses. This explicit list is useful for validation. As the application evolves, please ensure that any additions or deprecations to the process status are also reflected in this enum and accompanying documentation to avoid mismatches between the schema and runtime behavior.
129-133: Ensure Consistency in Required FieldsThe schema mandates
"processId"and"authKey"as required fields while allowing their values to be null. Confirm that this approach—requiring the key's presence regardless of its value—matches the overall data contract and that all interfacing systems can properly differentiate between an omitted field and one explicitly set to null.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
zmscitizenapi/tests/Zmscitizenapi/Services/Appointment/AppointmentUpdateServiceTest.php (2)
98-221: Consider moving test data to fixtures.The test data is quite extensive and could be moved to separate JSON fixture files to improve maintainability and reusability.
Consider creating fixture files and loading them like this:
- $processJson = [ - // ... large JSON structure ... - ]; - $sourceJson = [ - // ... large JSON structure ... - ]; + $processJson = json_decode(file_get_contents(__DIR__ . '/../fixtures/process.json'), true); + $sourceJson = json_decode(file_get_contents(__DIR__ . '/../fixtures/source.json'), true);
223-247: Consider extracting mock setup to a helper method.The mock setup logic could be extracted to improve readability and reusability across tests.
Consider refactoring like this:
+ private function createMockHttpClient($processJson, $sourceJson) + { + $processResponse = $this->createMock(\Psr\Http\Message\ResponseInterface::class); + $processResponse->method('getBody') + ->willReturn(\GuzzleHttp\Psr7\Utils::streamFor(json_encode($processJson))); + $processResponse->method('getStatusCode') + ->willReturn(200); + + $sourceResponse = $this->createMock(\Psr\Http\Message\ResponseInterface::class); + $sourceResponse->method('getBody') + ->willReturn(\GuzzleHttp\Psr7\Utils::streamFor(json_encode($sourceJson))); + $sourceResponse->method('getStatusCode') + ->willReturn(200); + + $mockHttpClient = $this->createMock(\BO\Zmsclient\Http::class); + $mockHttpClient->expects($this->exactly(2)) + ->method('readGetResult') + ->willReturnCallback(function($url, $params) use ($processResponse, $sourceResponse) { + if (strpos($url, '/process/101002/fb43/') !== false) { + return new \BO\Zmsclient\Result($processResponse); + } + if (strpos($url, '/source/unittest/') !== false) { + return new \BO\Zmsclient\Result($sourceResponse); + } + throw new \RuntimeException("Unexpected URL: " . $url); + }); + return $mockHttpClient; + } - $processResponse = $this->createMock(\Psr\Http\Message\ResponseInterface::class); - // ... mock setup code ... + $mockHttpClient = $this->createMockHttpClient($processJson, $sourceJson);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php(7 hunks)zmscitizenapi/tests/Zmscitizenapi/Services/Appointment/AppointmentUpdateServiceTest.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: call-owasp-security-checks / security-scan (zmsclient, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsdb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsslim, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsentities, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsdldb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsticketprinter, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsstatistic, 8.0)
- GitHub Check: call-php-code-quality / module-code-quality (zmsclient, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsmessaging, 8.0)
- GitHub Check: call-php-code-quality / module-code-quality (zmscitizenapi, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmscitizenapi, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmscalldisplay, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsapi, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsadmin, 8.0)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsapi-test
- GitHub Check: call-php-unit-tests / zmsdb-test
🔇 Additional comments (4)
zmscitizenapi/tests/Zmscitizenapi/Services/Appointment/AppointmentUpdateServiceTest.php (4)
7-7: LGTM! Good improvement in test isolation.The change from
MiddlewareTestCaseto PHPUnit'sTestCasereduces coupling and improves test isolation.Also applies to: 12-12
16-16: LGTM! Good practice in managing test state.The addition of
$originalFacadeand its initialization insetUpensures proper state management of static properties.Also applies to: 27-27
29-36: LGTM! Proper cleanup in tearDown.The
tearDownmethod correctly restores the original facade state, preventing test pollution.
248-266: LGTM! Good practice in resource cleanup.The use of try/finally ensures proper cleanup of the HTTP client state even if the test fails.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
zmsentities/schema/citizenapi/collections/thinnedScopeList.json (1)
10-11: Schema Refactoring for ModularityReplacing the detailed inline definition of the "scopes" items with a reference to an external schema (
../thinnedScope.json) increases modularity and reduces redundancy. Please verify that the referenced file fully captures all necessary properties (including the newemailRequiredfield) to ensure consistency across the system.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
zmsentities/schema/citizenapi/collections/thinnedScopeList.json(1 hunks)zmsentities/schema/citizenapi/office.json(1 hunks)zmsentities/schema/citizenapi/thinnedProcess.json(2 hunks)zmsentities/schema/citizenapi/thinnedScope.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- zmsentities/schema/citizenapi/office.json
- zmsentities/schema/citizenapi/thinnedScope.json
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: call-owasp-security-checks / security-scan (zmsclient, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsdb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsslim, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsentities, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsdldb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsticketprinter, 8.0)
- GitHub Check: call-php-code-quality / module-code-quality (zmsdb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsstatistic, 8.0)
- GitHub Check: call-php-code-quality / module-code-quality (zmsclient, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsmessaging, 8.0)
- GitHub Check: call-php-code-quality / module-code-quality (zmscitizenapi, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmscitizenapi, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmscalldisplay, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsapi, 8.0)
- GitHub Check: Analyze (python)
- GitHub Check: call-owasp-security-checks / security-scan (zmsadmin, 8.0)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (17)
zmsentities/schema/citizenapi/thinnedProcess.json (17)
3-7: Enhanced Root Type Formatting
The root"type"is now defined as a multi-line array including"array","object", and"null". This improved formatting enhances readability and clearly expresses that a process entity can be represented in multiple forms.
10-13: ConsistentprocessIdType Declaration
The"processId"property now explicitly allows values of type"integer"or"null", which improves clarity and consistency for handling optional numerical identifiers.
17-21: ClearauthKeyType Specification
The"authKey"property is now defined to accept either a"string"or"null", ensuring that the schema accurately reflects the possibility of an absent authentication key.
24-28: AccuratetimestampDefinition
The"timestamp"property now explicitly accepts"string"or"null"values. This provides a clear contract for how timestamps are represented (e.g., as a string in epoch seconds) and supports nullable entries.
31-35: Properly DefinedfamilyNameProperty
The"familyName"property is clearly defined to allow values of type"string"or"null". This is in line with the schema’s consistent handling of optional textual properties.
38-42: ClearcustomTextfieldProperty Types
The"customTextfield"property now properly declares its type as either a"string"or"null", which enhances the understandability and consistency of custom text input handling.
45-49: Consistent
The"email"property is now explicitly defined to accept a"string"or"null", ensuring that email entries are correctly typed and can be omitted where appropriate.
52-56: DefinedtelephoneField with Precision
The"telephone"property’s allowed types ("string"or"null") are now clearly delineated. This consistent treatment of optional contact fields boosts schema clarity.
59-63: Well-StructuredofficeNameDefinition
The"officeName"property has been updated to explicitly support both"string"and"null", maintaining a consistent pattern across similar fields.
66-70: ClearofficeIdProperty Specification
The"officeId"property now clearly restricts values to"integer"or"null", ensuring that the identifier is either a valid number or explicitly missing.
72-74: ModularizedscopeUsing External Reference
The"scope"property has been refactored to reference an external schema via"$ref": "./thinnedScope.json". This promotes modularity and reuse while aligning with the new design that incorporates theemailRequiredproperty. Please verify that the external schema correctly defines the newemailRequiredfield.
76-79: ComprehensivestatusField Definition
The"status"property continues to enforce a strict enumeration of allowed string values. The multi-line type formatting enhances clarity, and no functional changes are observed here.
102-112: DetailedsubRequestCountsStructure
The"subRequestCounts"property now clearly defines its type as an array ornulland further specifies that its items must be either an"integer"or"null". This explicit detailing improves both documentation and validation precision.
115-120: ExplicitserviceIdType Definition
The"serviceId"property now explicitly supports"integer"or"null", which fits well with the schema’s overall approach to handling optional numerical fields.
122-127: ClearserviceCountDeclaration
The"serviceCount"property is now unambiguously defined to allow either an"integer"or"null", ensuring consistency with similarly structured properties across the schema.
129-132: ConsistentrequiredArray Formatting
The"required"property now lists the mandatory fields ("processId"and"authKey") in a clear, structured format. This reformatting enhances readability without altering the enforced constraints.
134-134: Schema Closure and Overall Consistency
The schema is properly closed with a clear and well-formatted structure. The overall changes streamline the definitions and support a modular design with the external reference for"scope".
zmscitizenapi/src/Zmscitizenapi/Services/Appointment/AppointmentUpdateService.php
Outdated
Show resolved
Hide resolved
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php
Outdated
Show resolved
Hide resolved
…-email-at-location
…-email-at-location
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (1)
201-201:?? nullovershadowed by(bool)cast
(bool) $matchingScope->getEmailRequired() ?? nullwill always yieldtrueorfalse, making?? nullredundant. Consider removing?? nullor adjust the logic if you actually need to propagate anullvalue whengetEmailRequired()isnull.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.php`: Flag any usage of error_log() as it should be re...
**/*.php: Flag any usage of error_log() as it should be replaced with proper logging mechanisms:
- For error handling: Use a proper logging framework with error levels (PSR-3 LoggerInterface)
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- For application events: Use structured logging with proper log levels
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $logger->error("Import failed", ['error' => $e->getMessage()]);
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php
🧠 Learnings (1)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (3)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-02-21T16:46:00.197Z
Learning: In ZmsApiFacadeService.php's processFreeSlots method, when collecting appointment timestamps, use array_unique() to deduplicate timestamps and return them as a simple sorted array to avoid duplicate entries in the response.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-02-21T16:47:04.952Z
Learning: In ZmsApiFacadeService.php's processFreeSlots method, when collecting appointment timestamps from multiple providers, collect them in a flat array and apply array_unique() at the final step to ensure each timestamp appears only once in the response.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-02-21T16:47:30.875Z
Learning: In ZmsApiFacadeService.php's processFreeSlots method, collect timestamps in a flat array without provider grouping and apply array_unique() unconditionally to ensure each timestamp appears exactly once in the response. Return the result with 'appointmentTimestamps' key.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: call-owasp-security-checks / security-scan (zmsclient, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsdb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsslim, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsentities, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsdldb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsticketprinter, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsstatistic, 8.0)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-owasp-security-checks / security-scan (zmsmessaging, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmscitizenapi, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmscalldisplay, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsapi, 8.0)
- GitHub Check: call-php-unit-tests / zmsapi-test
- GitHub Check: call-owasp-security-checks / security-scan (zmsadmin, 8.0)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (9)
81-81: Ensure consistent null handling forgetEmailRequired()Here,
(bool) $matchingScope->getEmailRequired()may be sufficient ifgetEmailRequired()always returns a boolean. If it can returnnull, then casting toboolwill silently convertnulltofalse. Double-check if that is the intended behavior.
119-119: Same note for casting to bool
This is the same pattern of casting tobool. If the method is guaranteed to return a truthy/falsy value, that’s fine; otherwise, ensure the conversion is intended.
215-215: Consistent assignment toemailRequiredAssigning
$result['emailRequired']directly is consistent if$result['emailRequired']is always set to a boolean. No immediate issues, but verify the possibility of null values to ensure no type mismatch.
310-310:?? nullovershadowed by(bool)cast (duplicate)
This line mirrors the earlier pattern.(bool)casting will convert anynulltofalse, making?? nullirrelevant.
449-481: Comprehensive approach to collect and deduplicate future timestampsThis new function correctly:
- Validates incoming
ProcessListfor errors.- Accumulates only timestamps in the future.
- Deduplicates them and sorts.
- Validates final timestamps list.
It has clear logic, good readability, and handles error returns properly.
506-509: Valid error handling flowThe check
if (isset($timestamps['errors']))correctly follows the structure returned byprocessFreeSlots(), ensuring errors are returned early if needed.
511-511: Explicit structure forAvailableAppointmentsByOfficeReturning
['appointmentTimestamps' => $timestamps]matches this class’s expected constructor. If you decide to unify the approach withAvailableAppointments, consider updating the constructor signature accordingly.
513-514: Direct array provided toAvailableAppointmentsPassing
$timestampsdirectly aligns withAvailableAppointments’s constructor, avoiding extra array nesting. This is consistent with the rest of the method’s logic.
555-555:?? falseovershadowing (duplicate)
Again,(bool) $process->scope->getEmailRequired()will never yieldnull;?? falseis effectively redundant.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
New Features
Refactor