fix(ZMSKVR-125): cancel email not sending to client in zmscitizenapi #880
Conversation
WalkthroughThis pull request introduces a new command for auto-fixing PHPCS code style issues and adds several new validation and exception classes while enhancing existing ones. Updates include the addition of an email-related field (“emailFrom”) across models, JSON schemas, API responses, and tests; significant improvements to logging with rate limiting and simplified methods; several formatting and type-hint refinements; and renaming corrections for consistency throughout the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LoggerService
participant Cache
participant App_Log
Client->>LoggerService: logError(exception, request, response, context)
LoggerService->>Cache: checkRateLimit()
Cache-->>LoggerService: rate limit result
LoggerService->>App_Log: log error details
LoggerService-->>Client: error logged
sequenceDiagram
participant Client
participant Validator
participant InputStream
Client->>Validator: getInput(name)
Validator->>InputStream: file_get_contents("php://input")
InputStream-->>Validator: raw input data
Validator-->>Client: returns Unvalidated object with input
Suggested reviewers
Poem
🪧 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: 3
🔭 Outside diff range comments (1)
mellon/src/Mellon/Collection.php (1)
96-96:⚠️ Potential issueFix incorrect return type hint.
The method returns a Valid instance but is typed as Parameter.
- public function getValid($parameterName): Parameter + public function getValid($parameterName): Valid
🧹 Nitpick comments (18)
README.md (1)
125-125: Ensure Consistent Markdown List StyleThe addition of the
phpcbfcommand on line 125 improves developer workflow by enabling automatic code style fixes. However, according to our markdownlint rules (MD004), unordered list items should use asterisks (*) rather than dashes (-) for consistency across the document.Proposed diff:
- - `ddev exec "./cli modules loop 'vendor/bin/phpcbf --standard=psr12 src/'"` + * `ddev exec "./cli modules loop 'vendor/bin/phpcbf --standard=psr12 src/'"`🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
125-125: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php (2)
85-114: Ensure exception details meet data privacy requirements.
Storing detailed exception data (stack traces, file paths) can reveal internal information. Make sure this approach aligns with your logging/privacy policies, especially in production environments.
188-189: Dynamic log level for HTTP requests.
Switching to'error'for status >= 400 is a practical approach. Confirm there's no need for a dedicated'warning'threshold (e.g., 300-399) if you do care about redirection or other conditions.mellon/src/Mellon/ValidationException.php (1)
2-27: Provide context on validation failures.
The newValidationExceptionclass integrates with a validator but does not expose how the validator’s results are consumed downstream. Consider adding a getter for$validatoror a more descriptive exception message for easier debugging.mellon/src/Mellon/Failure/Exception.php (1)
2-36: Enhance clarity of validator-based exception.
Attaching validator info to the exception message is helpful, but mixing user input and shell-escape methods may cause confusion. If you only need safe logging, consider a more straightforward sanitization method.mellon/src/Mellon/ValidDate.php (2)
10-13: Fix incorrect class documentation.The class documentation states "Validation of URLs" but this class is for date validation.
- * Validation of URLs + * Validation of dates
23-37: Enhance date validation logic.The current implementation has several potential issues:
- Using
getTimestamp() > 0might not catch all invalid dates.- The commented-out timezone setting suggests incomplete timezone handling.
Consider this improved implementation:
public function isDate($format = 'U', $message = 'no valid date') { $this->validated = true; if ($this->value) { $selectedDate = \DateTime::createFromFormat($format, $this->value); - //$selectedDate->setTimezone(new \DateTimeZone(\App::TIMEZONE)); - $isDate = $selectedDate->getTimestamp() > 0; - if (false === $isDate) { + $errors = \DateTime::getLastErrors(); + if ($selectedDate === false || ($errors && ($errors['warning_count'] > 0 || $errors['error_count'] > 0))) { $this->setFailure($message); } } else { $this->setFailure($message); } return $this; }mellon/src/Mellon/ValidMail.php (2)
10-13: Fix incorrect class documentation.The class documentation states "Validation of URLs" but this class is for email validation.
- * Validation of URLs + * Validation of email addresses
43-49: Consider caching DNS check results.The
checkDnsAnymethod performs multiple DNS queries which could be expensive. Consider caching the results for performance optimization.Consider implementing a simple caching mechanism to store DNS check results temporarily, reducing the load on DNS servers and improving response times.
mellon/src/Mellon/Unvalidated.php (2)
76-77: Fix typo in error message.There's a grammatical error in the error message.
- throw new Exception("Validation class $class does not exists"); + throw new Exception("Validation class $class does not exist");
70-73: Consider adding error handling for callback invocation.The callback invocation could fail silently if the callback throws an exception.
if ($this->setValid) { $callback = $this->setValid; + try { $callback($newClass); + } catch (\Exception $e) { + throw new Exception("Callback execution failed: " . $e->getMessage()); + } }mellon/src/Mellon/Collection.php (2)
28-29: Consider using a more specific exception class.Instead of using a generic Exception, create a specific exception class for validation errors to improve error handling and maintainability.
- throw new Exception("No Valid value for $key"); + throw new ValidationException("No Valid value for $key");
104-111: Consider adding return type declarations.The callable parameter type could benefit from a more specific type declaration using PHP 8's callable syntax.
- public function validatedAction(Valid $valid, callable $action): self + public function validatedAction(Valid $valid, callable(mixed): void $action): selfmellon/src/Mellon/Validator.php (1)
166-168: Consider stream handling improvements for large inputs.Reading the entire input stream into memory could be problematic for large requests. Consider using stream functions or implementing chunked reading for better memory management.
- $this->input = file_get_contents('php://input'); + $stream = fopen('php://input', 'r'); + $this->input = stream_get_contents($stream, -1, 0); + fclose($stream);Also, consider adding input size validation to prevent potential memory exhaustion attacks.
zmscitizenapi/src/Zmscitizenapi/Models/ThinnedScope.php (2)
37-37: Consider reordering constructor parameters.The emailFrom parameter is inserted in the middle of existing parameters. Consider moving it to the end to maintain better backward compatibility.
- public function __construct(int $id = 0, ?ThinnedProvider $provider = null, ?string $shortName = null, ?string $emailFrom = null, ?bool $telephoneActivated = null, ?bool $telephoneRequired = null, ?bool $customTextfieldActivated = null, ?bool $customTextfieldRequired = null, ?string $customTextfieldLabel = null, ?bool $captchaActivatedRequired = null, ?string $displayInfo = null) + public function __construct(int $id = 0, ?ThinnedProvider $provider = null, ?string $shortName = null, ?bool $telephoneActivated = null, ?bool $telephoneRequired = null, ?bool $customTextfieldActivated = null, ?bool $customTextfieldRequired = null, ?string $customTextfieldLabel = null, ?bool $captchaActivatedRequired = null, ?string $displayInfo = null, ?string $emailFrom = null)
53-58: Consider enhancing validation error message.The current error message doesn't provide details about which schema validation rules failed.
- throw new InvalidArgumentException("The provided data is invalid according to the schema."); + $errors = $this->getValidationErrors(); + throw new InvalidArgumentException( + "Schema validation failed: " . implode(", ", $errors) + );zmsentities/src/Zmsentities/Scope.php (1)
249-253: Consider simplifying the boolean return.The boolean conversion can be simplified.
- return ($emailFrom) ? true : false; + return (bool) $emailFrom;zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php (1)
24-99: Consider adding positive test cases for emailFrom validation.While the test covers the basic success case, consider adding specific test cases to validate the
emailFromfield's format and behavior, such as:
- Valid email formats
- Invalid email formats
- Empty email value
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (56)
README.md(1 hunks)mellon/src/Mellon/Collection.php(1 hunks)mellon/src/Mellon/Condition.php(0 hunks)mellon/src/Mellon/Exception.php(1 hunks)mellon/src/Mellon/Failure/Exception.php(1 hunks)mellon/src/Mellon/Failure/Message.php(1 hunks)mellon/src/Mellon/Failure/MessageList.php(1 hunks)mellon/src/Mellon/Parameter.php(1 hunks)mellon/src/Mellon/Unvalidated.php(1 hunks)mellon/src/Mellon/Valid.php(1 hunks)mellon/src/Mellon/ValidArray.php(2 hunks)mellon/src/Mellon/ValidBool.php(1 hunks)mellon/src/Mellon/ValidDate.php(1 hunks)mellon/src/Mellon/ValidDatetime.php(1 hunks)mellon/src/Mellon/ValidFile.php(2 hunks)mellon/src/Mellon/ValidJson.php(1 hunks)mellon/src/Mellon/ValidMail.php(1 hunks)mellon/src/Mellon/ValidNumber.php(1 hunks)mellon/src/Mellon/ValidPath.php(1 hunks)mellon/src/Mellon/ValidString.php(1 hunks)mellon/src/Mellon/ValidUrl.php(1 hunks)mellon/src/Mellon/ValidationException.php(1 hunks)mellon/src/Mellon/Validator.php(1 hunks)zmsapi/src/Zmsapi/ProcessConfirmationMail.php(1 hunks)zmsapi/src/Zmsapi/ProcessDeleteMail.php(1 hunks)zmsapi/src/Zmsapi/ProcessPreconfirmationMail.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Models/ThinnedScope.php(3 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Appointment/AppointmentCancelService.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php(2 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php(9 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiClientService.php(15 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php(10 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/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/Core/LoggerServiceTest.php(0 hunks)zmscitizenapi/tests/Zmscitizenapi/Services/Core/MapperServiceTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Services/Core/ZmsApiClientServiceTest.php(4 hunks)zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_SourceGet_dldb.json(2 hunks)zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_appointments_empty.json(1 hunks)zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_process.json(1 hunks)zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_reserve_SourceGet_dldb.json(1 hunks)zmsclient/src/Zmsclient/Http.php(9 hunks)zmsentities/schema/citizenapi/collections/thinnedScopeList.json(3 hunks)zmsentities/schema/citizenapi/thinnedProcess.json(1 hunks)zmsentities/schema/citizenapi/thinnedScope.json(1 hunks)zmsentities/src/Zmsentities/Scope.php(1 hunks)zmsslim/src/Slim/Application.php(1 hunks)
💤 Files with no reviewable changes (2)
- mellon/src/Mellon/Condition.php
- zmscitizenapi/tests/Zmscitizenapi/Services/Core/LoggerServiceTest.php
✅ Files skipped from review due to trivial changes (18)
- mellon/src/Mellon/Failure/MessageList.php
- mellon/src/Mellon/Exception.php
- mellon/src/Mellon/ValidNumber.php
- mellon/src/Mellon/Parameter.php
- mellon/src/Mellon/ValidJson.php
- mellon/src/Mellon/ValidDatetime.php
- mellon/src/Mellon/ValidFile.php
- mellon/src/Mellon/ValidArray.php
- mellon/src/Mellon/ValidPath.php
- zmsapi/src/Zmsapi/ProcessDeleteMail.php
- mellon/src/Mellon/Failure/Message.php
- mellon/src/Mellon/ValidString.php
- mellon/src/Mellon/ValidBool.php
- zmsapi/src/Zmsapi/ProcessPreconfirmationMail.php
- zmsapi/src/Zmsapi/ProcessConfirmationMail.php
- mellon/src/Mellon/ValidUrl.php
- mellon/src/Mellon/Valid.php
- zmscitizenapi/src/Zmscitizenapi/Services/Appointment/AppointmentCancelService.php
🧰 Additional context used
🧠 Learnings (2)
zmscitizenapi/src/Zmscitizenapi/Models/ThinnedScope.php (1)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-02-14T17:08:30.231Z
Learning: In zmscitizenapi's ThinnedScope model, only include emailFrom field for email functionality. Other email settings (emailEnabled, adminMailOnDeleted, emailRequired) should be handled by the Process class through its preferences structure.
zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php (2)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:0-0
Timestamp: 2025-01-10T09:02:08.840Z
Learning: Rate limiting in the application is primarily handled by RateLimitingMiddleware for API requests, while LoggerService implements a secondary rate limit specifically for logging to prevent log flooding.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:168-174
Timestamp: 2025-01-07T13:45:46.077Z
Learning: The LoggerService should implement rate limiting, persistent syslog connections, and error handling for syslog operations to ensure robust logging in production environments. It should also include safeguards like message size limits, timestamp caching, and proper handling of sensitive data.
🪛 markdownlint-cli2 (0.17.2)
README.md
125-125: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-owasp-security-checks / security-scan (zmsticketprinter, 8.0)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-owasp-security-checks / security-scan (zmsstatistic, 8.0)
- GitHub Check: call-php-build-images / build (zmsadmin, 8.0)
🔇 Additional comments (39)
zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php (6)
26-28: Use consistent naming for cache keys.
The introduction ofCACHE_KEY_PREFIXandCACHE_COUNTER_KEYis logical for organizing cache entries. Just ensure broader naming conventions remain consistent if you add more counters or metrics in the future.
29-81: Consider edge cases and concurrency aspects incheckRateLimit.
The rate-limiting mechanism is straightforward but can fail open if the lock is lost or not acquired after multiple attempts. This may lead to log flooding in highly concurrent scenarios if caching or locks fail. Evaluate whether to enforce a stricter fallback or exponential backoff.
121-121: Good addition of rate-limit checks inlogWarning.
This ensures that warning logs do not overwhelm the system.
129-129: Consistent approach to rate limiting forlogInfo.
Having the same mechanism for info-level messages helps maintain uniform logging behavior.
141-145: Validate the path-based query filter.
This custom filter excludes query params that look like paths. Confirm whether filtering them out might hinder debugging or inadvertently skip important data.
157-157: Path construction logic is clear.
Appending query parts with proper encoding ensures safer logs. Keep in mind potential query parameter length constraints.zmsslim/src/Slim/Application.php (1)
21-21:❓ Verification inconclusive
Verify impact of reduced logging on email troubleshooting.
Changing
DEBUGLEVELfrom 'DEBUG' to 'INFO' might make it harder to diagnose email-related issues, which is particularly relevant given that this PR aims to fix email sending problems.Consider keeping DEBUG level logging specifically for email-related operations. Run this script to check email-related logging calls that might be affected:
🏁 Script executed:
#!/bin/bash # Search for email-related logging calls that might be affected by the log level change rg -i "log.*mail|mail.*log" -g "*.php"Length of output: 5240
Review: Potential Impact of Lowering Log Verbosity on Email Troubleshooting
The change in
zmsslim/src/Slim/Application.phpfrom:const DEBUGLEVEL = 'DEBUG';to
const DEBUGLEVEL = 'INFO';reduces log verbosity globally. Our search confirms that multiple email-related operations (e.g., in
zmsapi/src/Zmsapi/ProcessPreconfirmationMail.php,zmsapi/src/Zmsapi/ProcessConfirmationMail.php, andzmsmessaging/src/Zmsmessaging/Mail.php) rely ondebug-level logging for diagnostic details.
- Impact: Suppressing these debug logs may hinder diagnosing email sending issues, which is critical given the PR’s objective.
- Recommendation: Consider either maintaining or conditionally enabling
DEBUG-level logging for email-specific operations so that crucial troubleshooting information isn’t lost.zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopesListControllerTest.php (1)
59-59: LGTM!The addition of emailFrom field in the test data is consistent with the model changes.
Also applies to: 87-87
zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopeByIdControllerTest.php (1)
58-58: LGTM!The addition of the
emailFromfield with a default sender address aligns with the PR objective to fix email sending functionality.zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesListControllerTest.php (1)
72-72: LGTM!The addition of the
emailFromfield with a consistent sender address for both office entries maintains test consistency.Also applies to: 114-114
zmsclient/src/Zmsclient/Http.php (2)
7-7: LGTM!Good addition of the
ClientInterfaceimport to improve type safety.
110-110: LGTM!Good simplification of the type hints by using the imported interface directly.
Also applies to: 136-136, 232-232
zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficeListByServiceControllerTest.php (1)
70-70: LGTM!The addition of the
emailFromfield with a consistent sender address across all office entries maintains test consistency.Also applies to: 132-132, 171-171
zmsentities/src/Zmsentities/Scope.php (1)
34-37: LGTM! Method follows consistent pattern.The
getEmailFrom()method follows the established pattern of other preference getters in the class.zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiClientService.php (2)
271-271: LGTM! Method name spelling corrected.The method name has been corrected from
sendCancelationEmailtosendCancellationEmail, improving consistency and correctness.
230-230: LGTM! Parameter type made more explicit.Changed from
nullto[]for better clarity and consistency.zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentCancelControllerTest.php (1)
89-89: LGTM! Test updated for new email field.Test correctly includes the new
emailFromfield in the expected response structure.zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsControllerTest.php (1)
72-72: LGTM! Tests updated with consistent email values.The
emailFromfield is consistently added across all test cases with the appropriate default value.Also applies to: 114-114, 206-206, 248-248
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentPreconfirmControllerTest.php (1)
84-84: LGTM!The addition of the
emailFromfield to the scope array in the test response is consistent with the changes across the codebase.zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (1)
188-200: LGTM! Well-structured email configuration handling.The changes properly handle the
emailFromfield:
- Added to
ThinnedScopecreation with proper null handling- Structured in preferences array with proper grouping under client settings
Also applies to: 276-295
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentReserveControllerTest.php (1)
89-89: LGTM!The addition of the
emailFromfield with the default email address is consistent with the changes across other test files.zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.php (1)
82-82: LGTM!The addition of the
emailFromfield with the default email address is consistent with the changes across other test files.zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentConfirmControllerTest.php (1)
84-84: LGTM!The addition of the
emailFromfield with an empty string default value is consistent with the changes across other test files.zmscitizenapi/tests/Zmscitizenapi/Services/Core/MapperServiceTest.php (1)
206-206: LGTM!The addition of the
emailFromfield with an empty string default value in the expected response is consistent with the system-wide changes.zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (3)
68-91: LGTM!The multi-line formatting of constructor calls improves code readability while maintaining functionality.
209-221: LGTM!The consistent addition of the
emailFromfield across allThinnedScopeinstantiations ensures proper email sender information handling throughout the system.Also applies to: 303-315, 549-561
622-629: LGTM!Fixed spelling of "cancellation" in method name for consistency.
zmscitizenapi/tests/Zmscitizenapi/Services/Core/ZmsApiClientServiceTest.php (2)
697-697: LGTM!Removed unnecessary null parameter from
readDeleteResultcall, improving code cleanliness.
847-847: LGTM!Fixed spelling of "cancellation" in test method names for consistency with the implementation.
Also applies to: 864-864, 879-879
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php (1)
83-83: LGTM: The emailFrom field is correctly added to the expected response structure.The addition of the
emailFromfield in the scope object's expected response structure aligns with the schema changes.zmsentities/schema/citizenapi/collections/thinnedScopeList.json (2)
38-51: LGTM: The emailFrom field is well-defined with proper validation.The schema definition includes:
- Clear description
- Email format validation pattern
- Proper localization
- Default value
78-78: LGTM: The emailFrom field is correctly marked as required.Adding emailFrom to the required fields ensures proper validation of this essential field.
zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_reserve_SourceGet_dldb.json (1)
63-63: LGTM: The emailFrom field is properly configured in the test fixture.The default value "[email protected]" is appropriate for testing purposes.
zmsentities/schema/citizenapi/thinnedScope.json (1)
99-112: LGTM: The emailFrom field definition is consistent with other schemas.The schema properly defines:
- Email format validation
- Default value
- Localization support
- Clear description
zmsentities/schema/citizenapi/thinnedProcess.json (1)
103-116: Schema Update: NewemailFromProperty
The addition of theemailFromproperty is well executed. It specifies a string type with a default value, includes a regex pattern to enforce email format, and even provides localized error messaging via thex-localeobject. Please double-check that the regular expression covers all valid email cases needed by your application.zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_appointments_empty.json (1)
49-49: Fixture Enhancement: AddedemailFromField
The inclusion of"emailFrom": "[email protected]"in the client preferences is appropriate and aligns with the updated schema. Ensure that any tests consuming this fixture verify that the new field is processed as expected.zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_SourceGet_dldb.json (2)
105-107: Fixture Update:emailFromin Scope 1 Client Preferences
The new"emailFrom": "[email protected]"entry in Scope 1’s client preferences is correctly integrated. This consistency helps ensure that email communications use the designated sender address.
132-139: Fixture Consistency:emailFromin Scope 2 Client Preferences
Similarly, adding"emailFrom": "[email protected]"in Scope 2 confirms that both scopes share the same email configuration. This uniformity reduces potential misconfigurations in client notifications.zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_process.json (1)
127-127: Client Preference Update: AdjustedemailFromValue
Changing"emailFrom"to"[email protected]"aligns with the new email sender policy. Verify that all components and tests expecting this value are updated accordingly so that cancellation and other communication emails use the correct sender address.
zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (1)
70-70: Improve boolean expression readability.The nested boolean expression could be simplified for better readability.
-if (!$showUnpublished && isset($provider->data['public']) && !(bool) $provider->data['public']) { +if (!$showUnpublished && isset($provider->data['public']) && !$provider->data['public']) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php(8 hunks)zmsclient/src/Zmsclient/Http.php(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- zmsclient/src/Zmsclient/Http.php
🧰 Additional context used
🧠 Learnings (1)
zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (2)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-02-14T15:34:11.636Z
Learning: In zmscitizenapi's MapperService::thinnedProcessToProcess, when mapping a ThinnedProcess to Process, ensure all required fields are copied including scope ID, email settings, client data, appointments, queue status, and requests. This is crucial for maintaining functionality across the system.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-02-14T16:29:45.142Z
Learning: In zmscitizenapi's MapperService::thinnedProcessToProcess, when mapping scope data, ensure scope.contact.email is set to '[email protected]' and email settings are in scope.preferences.notifications structure, not in scope.data. This is required for email functionality to work correctly.
⏰ 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-php-unit-tests / module-test (zmsticketprinter, 8.0)
- GitHub Check: call-php-unit-tests / module-test (zmsstatistic, 8.0)
- GitHub Check: call-php-unit-tests / module-test (zmsslim, 8.0)
- GitHub Check: call-php-unit-tests / module-test (zmsmessaging, 8.0)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / module-test (zmsentities, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmscitizenapi, 8.0)
- GitHub Check: call-php-unit-tests / zmsclient-test
- GitHub Check: call-php-unit-tests / module-test (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 / module-test (zmsadmin, 8.0)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsapi-test
- GitHub Check: call-owasp-security-checks / security-scan (zmsadmin, 8.0)
- GitHub Check: call-php-unit-tests / zmsapi-test
- GitHub Check: call-php-unit-tests / zmsdb-test
🔇 Additional comments (4)
zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (4)
31-34: Class complexity issue needs to be addressed.The class has exceeded the complexity threshold (101 vs 100) and contains multiple methods with high cyclomatic complexity.
188-200: LGTM! Proper handling of emailFrom field.The addition of emailFrom field in scopeToThinnedScope is well-implemented with proper null handling.
238-253: LGTM! Proper handling of process fields.The processToThinnedProcess implementation correctly maps all required fields including email from clients array.
360-360: LGTM! Improved formatting.The providerToThinnedProvider method has been properly formatted with named parameters.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Mellon(just ignore it)Summary by CodeRabbit