Feature zms 3429 show not public providers and services #809
Conversation
WalkthroughThis pull request introduces a new static property Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (5)
🪧 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
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsControllerTest.php (1)
147-148: Add descriptive assertion messages.Adding descriptive messages to assertions helps in quickly identifying test failures.
-$this->assertEquals(200, $response->getStatusCode()); -$this->assertEqualsCanonicalizing($expectedResponse, $responseBody); +$this->assertEquals(200, $response->getStatusCode(), 'Response should return HTTP 200'); +$this->assertEqualsCanonicalizing($expectedResponse, $responseBody, 'Response structure should match expected format');zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (2)
50-50: LGTM! Consider using null coalescing operator for cleaner code.The filtering logic for non-public providers is correct. However, the condition can be simplified using the null coalescing operator.
- if (! $showUnpublished && isset($provider->data['public']) && ! (bool) $provider->data['public']) { + if (! $showUnpublished && ! (bool) ($provider->data['public'] ?? true)) {Also applies to: 61-64
105-108: LGTM! Consider using null coalescing operator for cleaner code.The filtering logic for non-public services is correct. However, the condition can be simplified using the null coalescing operator.
- if (! $showUnpublished - && isset($service->getAdditionalData()['public']) - && !$service->getAdditionalData()['public']) - { + if (! $showUnpublished && ! ($service->getAdditionalData()['public'] ?? true)) {Also applies to: 128-133
zmscitizenapi/src/Zmscitizenapi/Application.php (1)
247-250: Improve getter method documentation and implementation.The getter method needs documentation and can be simplified:
+ /** + * Get the domain name where unpublished content should be accessible. + * + * @return string|null The domain name or null if unpublished content should not be accessible + */ public static function getAccessUnpublishedOnDomain(): ?string { - return self::$ACCESS_UNPUBLISHED_ON_DOMAIN ?: null; + return self::$ACCESS_UNPUBLISHED_ON_DOMAIN; }zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (2)
142-153: Consider adding parameter validation and documentation.The
$showUnpublishedparameter lacks validation and PHPDoc documentation explaining its purpose and impact.Add parameter validation and documentation:
+ /** + * Get services and offices with optional visibility of unpublished entities. + * + * @param bool $showUnpublished When true, includes unpublished offices and services in the results + * @return OfficeServiceAndRelationList|array + */ public static function getServicesAndOffices(bool $showUnpublished = false): OfficeServiceAndRelationList|array { + if (!is_bool($showUnpublished)) { + throw new \InvalidArgumentException('showUnpublished must be a boolean'); + }
142-153: Consider implementing access control for unpublished entities.The current implementation lacks access control mechanisms for viewing unpublished entities, which could lead to security concerns.
Consider:
- Adding role-based access control for viewing unpublished entities
- Implementing audit logging when unpublished entities are accessed
- Adding configuration validation to ensure the feature is properly secured
zmscitizenapi/src/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsController.php (1)
39-47: Consider documenting the response structure.The method returns different response structures based on the result type. This should be documented for API consumers.
Add PHPDoc to describe the response format:
/** * @param RequestInterface $request * @param ResponseInterface $response * @param array $args * @return ResponseInterface * * @throws \RuntimeException When service encounters an error * * @api {get} /offices-services Get services and offices list * @apiSuccess {Array} data List of services and offices * @apiError {Array} errors Error details with status codes */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
zmscitizenapi/src/Zmscitizenapi/Application.php(3 hunks)zmscitizenapi/src/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsController.php(2 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php(4 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Office/OfficesServicesRelationsService.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsControllerTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Services/Core/MapperServiceTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_SourceGet_dldb.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_SourceGet_dldb.json (1)
82-83: LGTM! The test fixture is correctly updated.The addition of the
"public": falseproperty to the second request object is consistent with the test case and properly structured within the JSON.zmscitizenapi/src/Zmscitizenapi/Application.php (1)
Line range hint
71-250: Consider security implications of unpublished content access.The feature introduces potential security risks that should be addressed:
- Add logging when unpublished content is accessed
- Consider additional rate limiting for unpublished content
- Implement audit logging for configuration changes
- Document the security implications in the README
Run this script to check for existing security measures:
zmscitizenapi/src/Zmscitizenapi/Services/Office/OfficesServicesRelationsService.php (2)
11-13: Consider standardizing the return type.The union return type
OfficeServiceAndRelationList|arraysuggests inconsistent return values. Consider standardizing to always returnOfficeServiceAndRelationListfor better type safety and predictability.Let's verify how this return type is handled by consumers:
16-18: Verify the facade service implementation.The parameter propagation looks correct, but we should verify that
ZmsApiFacadeService::getServicesAndOfficesproperly handles the$showUnpublishedparameter.Let's verify the facade service implementation:
✅ Verification successful
Parameter handling in facade service is correct
The
ZmsApiFacadeService::getServicesAndOfficesmethod properly handles the$showUnpublishedparameter by propagating it through the mapping chain to filter offices and services accordingly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the facade service implementation ast-grep --pattern 'class ZmsApiFacadeService { $$$ public static function getServicesAndOffices($$$) { $$$ } $$$ }'Length of output: 130
Script:
#!/bin/bash # Try multiple search strategies to find the facade service echo "=== Searching for class name ===" rg -l "ZmsApiFacadeService" echo -e "\n=== Searching for method name ===" rg "getServicesAndOffices" echo -e "\n=== Searching for facade files ===" fd -e php facade -iLength of output: 4344
Script:
#!/bin/bash # Get the implementation of getServicesAndOffices method rg -A 10 "getServicesAndOffices\(bool \$showUnpublished" zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.phpLength of output: 127
Script:
#!/bin/bash # Get the implementation with a simpler pattern echo "=== Method implementation ===" rg -A 15 "public static function getServicesAndOffices" zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.phpLength of output: 1057
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (1)
142-142: Method signature change requires careful consideration.The addition of the
$showUnpublishedparameter is a breaking change that could affect code using reflection or strict parameter matching. Consider adding a changelog entry and incrementing the major version number.zmscitizenapi/src/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsController.php (4)
6-6: LGTM!The App class import is correctly placed and follows PSR-4 conventions.
17-17: LGTM!The property is well-named and correctly typed as a nullable string with appropriate visibility.
30-37: LGTM!The request validation and error handling logic is well-structured and follows consistent error response patterns.
22-22: Consider adding error handling for the configuration retrieval.The call to
App::getAccessUnpublishedOnDomain()could potentially fail. Consider adding try-catch block to handle any configuration retrieval errors gracefully.Let's verify if the App class method has proper error handling:
| "services" => [ | ||
| [ | ||
| "id" => 1, | ||
| "name" => "Unittest Source Dienstleistung", | ||
| "maxQuantity" => 1, | ||
| "combinable" => [] | ||
| ] | ||
| ], |
There was a problem hiding this comment.
Fix inconsistency in test data: Missing service definition.
The relations array references serviceId 2, but this service is not defined in the services array. This could lead to false positives in testing.
Add the missing service definition:
"services" => [
[
"id" => 1,
"name" => "Unittest Source Dienstleistung",
"maxQuantity" => 1,
"combinable" => []
+ ],
+ [
+ "id" => 2,
+ "name" => "Unittest Source Dienstleistung 2",
+ "maxQuantity" => 1,
+ "combinable" => []
]
],Also applies to: 141-144
| "slotTimeInMinutes" => null, | ||
| "geo" => [ | ||
| "lat" => "48.12750898398659", | ||
| "lon" => "11.604317899956524" | ||
| ], | ||
| "scope" => [ | ||
| "id" => 1, | ||
| "provider" => [ | ||
| "id" => 9999998, | ||
| "name" => "Unittest Source Dienstleister", | ||
| "lat" => 48.12750898398659, | ||
| "lon" => 11.604317899956524, | ||
| "source" => "unittest", | ||
| "contact" => [ | ||
| "city" => "Berlin", | ||
| "country" => "Germany", | ||
| "name" => "Unittest Source Dienstleister", | ||
| "postalCode" => "10178", | ||
| "region" => "Berlin", | ||
| "street" => "Alte Jakobstraße", | ||
| "streetNumber" => "105" | ||
| ] | ||
| ], | ||
| "shortName" => "Scope 1", | ||
| "telephoneActivated" => true, | ||
| "telephoneRequired" => false, | ||
| "customTextfieldActivated" => true, | ||
| "customTextfieldRequired" => false, | ||
| "customTextfieldLabel" => "Custom Label", | ||
| "captchaActivatedRequired" => true, | ||
| "displayInfo" => null | ||
| ] | ||
| ], | ||
| [ | ||
| "id" => 9999999, | ||
| "name" => "Unittest Source Dienstleister 2", | ||
| "address" => null, | ||
| "displayNameAlternatives" => [], | ||
| "organization" => null, | ||
| "organizationUnit" => null, | ||
| "slotTimeInMinutes" => null, | ||
| "geo" => [ | ||
| "lat" => "48.12750898398659", | ||
| "lon" => "11.604317899956524" | ||
| ], | ||
| "scope" => [ | ||
| "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", | ||
| "telephoneActivated" => false, | ||
| "telephoneRequired" => true, | ||
| "customTextfieldActivated" => false, | ||
| "customTextfieldRequired" => true, | ||
| "customTextfieldLabel" => "", | ||
| "captchaActivatedRequired" => false, | ||
| "displayInfo" => null | ||
| ] | ||
| ] | ||
| ], | ||
| "services" => [ | ||
| [ | ||
| "id" => 1, | ||
| "name" => "Unittest Source Dienstleistung", | ||
| "maxQuantity" => 1, | ||
| "combinable" => [] | ||
| ] | ||
| ], | ||
| "relations" => [ | ||
| [ | ||
| "officeId" => 9999998, | ||
| "serviceId" => 1, | ||
| "slots" => 2 | ||
| ], | ||
| [ | ||
| "officeId" => 9999999, | ||
| "serviceId" => 1, | ||
| "slots" => 1 | ||
| ], | ||
| [ | ||
| "officeId" => 9999999, | ||
| "serviceId" => 2, | ||
| "slots" => 1 | ||
| ] | ||
| ] | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce test data duplication using data providers.
The test data structure is largely duplicated between testRendering and testDisplayNotPublicRequests. Consider using PHPUnit data providers to reduce duplication and improve maintainability.
Example refactoring:
/**
* @return array<string,array<string,mixed>>
*/
public function provideTestData(): array
{
return [
'basic_rendering' => [
'unpublished_domain' => null,
'expected_services' => [
[
'id' => 1,
'name' => 'Unittest Source Dienstleistung',
'maxQuantity' => 1,
'combinable' => []
]
]
],
'with_unpublished' => [
'unpublished_domain' => 'localhost',
'expected_services' => [
[
'id' => 1,
'name' => 'Unittest Source Dienstleistung',
'maxQuantity' => 1,
'combinable' => []
],
[
'id' => 2,
'name' => 'Unittest Source Dienstleistung 2',
'maxQuantity' => 1,
'combinable' => [
'1' => [9999999],
'2' => [9999999]
]
]
]
]
];
}
/**
* @dataProvider provideTestData
*/
public function testRendering(?string $unpublishedDomain, array $expectedServices): void
{
if ($unpublishedDomain !== null) {
\App::$ACCESS_UNPUBLISHED_ON_DOMAIN = $unpublishedDomain;
}
// ... rest of the test
}| public function testDisplayNotPublicRequests() | ||
| { | ||
| \App::$ACCESS_UNPUBLISHED_ON_DOMAIN = 'localhost'; | ||
| $this->setApiCalls([ | ||
| [ | ||
| 'function' => 'readGetResult', | ||
| 'url' => '/source/unittest/', | ||
| 'parameters' => [ | ||
| 'resolveReferences' => 2, | ||
| ], | ||
| 'response' => $this->readFixture("GET_SourceGet_dldb.json") | ||
| ] | ||
| ]); |
There was a problem hiding this comment.
Clean up global state after test execution.
Setting App::$ACCESS_UNPUBLISHED_ON_DOMAIN modifies global state which could affect other tests. This should be cleaned up in tearDown().
Add cleanup in tearDown:
public function setUp(): void
{
parent::setUp();
\App::$source_name = 'unittest';
if (\App::$cache) {
\App::$cache->clear();
}
}
+public function tearDown(): void
+{
+ parent::tearDown();
+ \App::$ACCESS_UNPUBLISHED_ON_DOMAIN = null;
+}Committable suggestion skipped: line range outside the PR's diff.
| public function testDontReturnNotPublicServices() | ||
| { | ||
| $request1 = new Request(); | ||
| $request1->id = 1; | ||
| $request1->name = 'Service 111'; | ||
| $request1->data = [ | ||
| 'maxQuantity' => 22, | ||
| 'public' => false | ||
| ]; | ||
|
|
||
| $request2 = new Request(); | ||
| $request2->id = 2; | ||
| $request2->name = 'Service 2'; | ||
| $request2->data = ['maxQuantity' => 1]; | ||
|
|
||
| $requestList = new RequestList([$request1, $request2]); | ||
|
|
||
| $provider = new Provider(); | ||
| $provider->id = 100; | ||
|
|
||
| $relation1 = new RequestRelation(); | ||
| $relation1->request = $request1; | ||
| $relation1->provider = $provider; | ||
| $relation1->slots = 5; | ||
|
|
||
| $relation2 = new RequestRelation(); | ||
| $relation2->request = $request2; | ||
| $relation2->provider = $provider; | ||
| $relation2->slots = 3; | ||
|
|
||
| $expectedResponse = [ | ||
| "services" => [ | ||
| [ | ||
| "id" => 2, | ||
| "name" => "Service 2", | ||
| "maxQuantity" => 1, | ||
| "combinable" => new Combinable(), | ||
| ] | ||
| ] | ||
| ]; | ||
|
|
||
| $relationList = new RequestRelationList([$relation1, $relation2]); | ||
| $result = MapperService::mapServicesWithCombinations($requestList, $relationList); | ||
| $resultArray = $result->toArray(); | ||
|
|
||
| $this->assertEquals($expectedResponse, $resultArray); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test case for $showUnpublished = true.
The test verifies that non-public services are filtered out when $showUnpublished is false (default). However, it's important to also verify that non-public services are included when $showUnpublished is true.
Add this test case:
public function testReturnNotPublicServicesWhenShowUnpublishedIsTrue()
{
$request1 = new Request();
$request1->id = 1;
$request1->name = 'Service 111';
$request1->data = [
'maxQuantity' => 22,
'public' => false
];
$request2 = new Request();
$request2->id = 2;
$request2->name = 'Service 2';
$request2->data = ['maxQuantity' => 1];
$requestList = new RequestList([$request1, $request2]);
$provider = new Provider();
$provider->id = 100;
$relation1 = new RequestRelation();
$relation1->request = $request1;
$relation1->provider = $provider;
$relation1->slots = 5;
$relation2 = new RequestRelation();
$relation2->request = $request2;
$relation2->provider = $provider;
$relation2->slots = 3;
$expectedResponse = [
"services" => [
[
"id" => 1,
"name" => "Service 111",
"maxQuantity" => 22,
"combinable" => new Combinable(),
],
[
"id" => 2,
"name" => "Service 2",
"maxQuantity" => 1,
"combinable" => new Combinable(),
]
]
];
$relationList = new RequestRelationList([$relation1, $relation2]);
$result = MapperService::mapServicesWithCombinations($requestList, $relationList, true);
$resultArray = $result->toArray();
$this->assertEquals($expectedResponse, $resultArray);
}| @@ -68,6 +68,8 @@ class Application extends \BO\Slim\Application | |||
| // IP Filter config | |||
| public static string $IP_BLACKLIST; | |||
|
|
|||
| public static string $ACCESS_UNPUBLISHED_ON_DOMAIN; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and documentation for the unpublished access property.
Consider these improvements:
- Make the property nullable since it can be empty:
public static ?string - Consider making it private with a public getter to prevent external modifications
- Add PHPDoc to document the property's purpose and expected values
+ /**
+ * Domain name where unpublished content should be accessible.
+ * When empty or null, unpublished content is not accessible anywhere.
+ */
- public static string $ACCESS_UNPUBLISHED_ON_DOMAIN;
+ private static ?string $ACCESS_UNPUBLISHED_ON_DOMAIN = null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static string $ACCESS_UNPUBLISHED_ON_DOMAIN; | |
| /** | |
| * Domain name where unpublished content should be accessible. | |
| * When empty or null, unpublished content is not accessible anywhere. | |
| */ | |
| private static ?string $ACCESS_UNPUBLISHED_ON_DOMAIN = null; |
| @@ -153,6 +155,8 @@ | |||
|
|
|||
| // IP Filter | |||
| self::$IP_BLACKLIST = getenv('IP_BLACKLIST') ?: ''; | |||
|
|
|||
| self::$ACCESS_UNPUBLISHED_ON_DOMAIN = getenv('ACCESS_UNPUBLISHED_ON_DOMAIN') ?: ''; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve initialization logic and placement.
Several architectural improvements are recommended:
- Move this initialization to a new dedicated method like
initializeAccessControl() - Add domain validation to ensure the value is a valid domain name when provided
- Use null as fallback instead of empty string
- self::$ACCESS_UNPUBLISHED_ON_DOMAIN = getenv('ACCESS_UNPUBLISHED_ON_DOMAIN') ?: '';
+ private static function initializeAccessControl(): void
+ {
+ $domain = getenv('ACCESS_UNPUBLISHED_ON_DOMAIN');
+ if ($domain && !filter_var($domain, FILTER_VALIDATE_DOMAIN)) {
+ throw new \RuntimeException(
+ sprintf('Invalid domain "%s" provided for ACCESS_UNPUBLISHED_ON_DOMAIN', $domain)
+ );
+ }
+ self::$ACCESS_UNPUBLISHED_ON_DOMAIN = $domain ?: null;
+ }Don't forget to call this method in the initialize() method.
Committable suggestion skipped: line range outside the PR's diff.
| $offices = MapperService::mapOfficesWithScope($providerList, $showUnpublished) ?? new OfficeList; | ||
| $services = MapperService::mapServicesWithCombinations( | ||
| $requestList, | ||
| $relationList, | ||
| $showUnpublished | ||
| ) ?? new ServiceList(); |
There was a problem hiding this comment.
Inconsistent visibility filtering across the service.
While getServicesAndOffices now supports showing unpublished entities, other methods like getOfficesThatProvideService still filter by public status. This inconsistency could lead to confusion and bugs.
Consider either:
- Propagating the
$showUnpublishedparameter to all relevant methods, or - Moving the visibility filtering logic to a central location
Example implementation for option 1:
public static function getOfficesThatProvideService(int $serviceId): OfficeList|array
{
$providerList = ZmsApiClientService::getOffices() ?? new ProviderList();
$requestRelationList = ZmsApiClientService::getRequestRelationList() ?? new RequestRelationList();
+ $showUnpublished = \App::ACCESS_UNPUBLISHED_ON_DOMAIN ?? false;
$providerIds = [];
foreach ($requestRelationList as $relation) {
if ((int) $relation->request->id === $serviceId) {
$providerIds[] = $relation->provider->id;
}
}
$offices = [];
foreach ($providerList as $provider) {
if (
in_array($provider->id, $providerIds) &&
- isset($provider->data['public']) &&
- $provider->data['public'] === true
+ ($showUnpublished || !isset($provider->data['public']) || $provider->data['public'] === true)
) {Committable suggestion skipped: line range outside the PR's diff.
| $domain = $request->getUri()->getHost(); | ||
| $showUnpublished = !empty($this->showUnpublishedOnDomain) | ||
| && strpos($domain, $this->showUnpublishedOnDomain) !== false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve domain matching logic.
The current domain matching has several potential issues:
- The
strposcheck is case-sensitive, which could cause issues with different domain cases - Using
strposfor domain matching could lead to false positives (e.g., "example.com" would match "example.com.malicious.com")
Consider using this more robust approach:
- $domain = $request->getUri()->getHost();
- $showUnpublished = !empty($this->showUnpublishedOnDomain)
- && strpos($domain, $this->showUnpublishedOnDomain) !== false;
+ $domain = strtolower($request->getUri()->getHost());
+ $showUnpublished = !empty($this->showUnpublishedOnDomain)
+ && fnmatch("*.$this->showUnpublishedOnDomain", $domain);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $domain = $request->getUri()->getHost(); | |
| $showUnpublished = !empty($this->showUnpublishedOnDomain) | |
| && strpos($domain, $this->showUnpublishedOnDomain) !== false; | |
| $domain = strtolower($request->getUri()->getHost()); | |
| $showUnpublished = !empty($this->showUnpublishedOnDomain) | |
| && fnmatch("*.$this->showUnpublishedOnDomain", $domain); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
zmscitizenapi/src/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsController.php (1)
17-17: Add PHPDoc comments for the new property.The new property's purpose and expected values should be documented for better code maintainability.
Add this documentation above the property:
+ /** + * Domain suffix for which unpublished services/providers should be shown. + * When null or empty, only published content is shown for all domains. + */ private ?string $showUnpublishedOnDomain;Also applies to: 22-22
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmscitizenapi/src/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsController.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: call-unit-tests / zmsdb-test
- GitHub Check: call-unit-tests / zmsapi-test
🔇 Additional comments (3)
zmscitizenapi/src/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsController.php (3)
6-6: LGTM!The App import is correctly added and follows PSR-4 autoloading standards.
28-29: Improve domain matching logic.The current domain matching has several potential issues:
- The
strposcheck is case-sensitive, which could cause issues with different domain cases- Using
strposfor domain matching could lead to false positives (e.g., "example.com" would match "example.com.malicious.com")Consider using this more robust approach:
- $domain = $_SERVER['HTTP_HOST'] ?? ''; - $showUnpublished = !empty($this->showUnpublishedOnDomain) - && strpos($domain, $this->showUnpublishedOnDomain) !== false; + $domain = strtolower($request->getUri()->getHost()); + $showUnpublished = !empty($this->showUnpublishedOnDomain) + && fnmatch("*.$this->showUnpublishedOnDomain", $domain);
39-39: LGTM!The service method call is correctly updated to pass the showUnpublished parameter.
| ErrorMessages::getHighestStatusCode($result['errors']) | ||
| ) | ||
| : $this->createJsonResponse($response, $result->toArray(), 200); | ||
| $domain = $_SERVER['HTTP_HOST'] ?? ''; |
There was a problem hiding this comment.
Replace $_SERVER['HTTP_HOST'] with PSR-7 Request method.
Using $_SERVER['HTTP_HOST'] directly is a security risk as it can be spoofed. Since you already have access to PSR-7 RequestInterface, use its methods instead.
Apply this change:
- $domain = $_SERVER['HTTP_HOST'] ?? '';
+ $domain = $request->getUri()->getHost();Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
zmscitizenapi/src/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsController.php (1)
29-30:⚠️ Potential issueFix domain matching logic.
The current implementation has several issues:
- Using URI scheme instead of host for domain matching
- Using strpos for domain matching which could lead to false positives
Apply this diff to fix the domain matching:
- $showUnpublished = !empty($this->showUnpublishedOnDomain) - && strpos($uri, $this->showUnpublishedOnDomain) !== false; + $domain = strtolower($request->getUri()->getHost()); + $showUnpublished = !empty($this->showUnpublishedOnDomain) + && fnmatch("*.$this->showUnpublishedOnDomain", $domain);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmscitizenapi/src/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsController.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: call-unit-tests / zmsdb-test
- GitHub Check: call-unit-tests / zmsapi-test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
zmscitizenapi/src/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsController.php (2)
6-6: LGTM!The App import and the nullable string property for domain configuration are well-defined.
Also applies to: 17-17
22-22: LGTM!The domain configuration is properly initialized in the constructor.
| ); | ||
| } | ||
|
|
||
| $result = $this->service->getServicesAndOfficesList($showUnpublished); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for service call.
The service call result should be validated before using it.
- $result = $this->service->getServicesAndOfficesList($showUnpublished);
+ try {
+ $result = $this->service->getServicesAndOfficesList($showUnpublished);
+ } catch (\Exception $e) {
+ return $this->createJsonResponse(
+ $response,
+ ['errors' => [['message' => $e->getMessage()]]],
+ 500
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $result = $this->service->getServicesAndOfficesList($showUnpublished); | |
| try { | |
| $result = $this->service->getServicesAndOfficesList($showUnpublished); | |
| } catch (\Exception $e) { | |
| return $this->createJsonResponse( | |
| $response, | |
| ['errors' => [['message' => $e->getMessage()]]], | |
| 500 | |
| ); | |
| } |
| $uri = $request->getUri()->getScheme(); | ||
| var_dump($uri);exit; |
There was a problem hiding this comment.
Remove debug code.
The var_dump($uri) and exit statements should be removed as they were likely used for debugging.
- $uri = $request->getUri()->getScheme();
- var_dump($uri);exit;
+ $domain = $request->getUri()->getHost();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $uri = $request->getUri()->getScheme(); | |
| var_dump($uri);exit; | |
| $domain = $request->getUri()->getHost(); |
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
New Features
Tests
Improvements