fix(ZMSKVR-1056): optional textfield in admin #1808
Conversation
📝 WalkthroughWalkthroughTwo targeted changes to process data handling and display: unconditional assignment of amendment and custom text field values during data population in a query method, paired with CSS styling to constrain and wrap these fields in queue table cells. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
zmsadmin/templates/block/appointment/form.twig(2 hunks)zmsdb/src/Zmsdb/Query/Process.php(1 hunks)zmsentities/src/Zmsentities/Process.php(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/src/Zmsdb/Query/Process.phpzmsadmin/templates/block/appointment/form.twigzmsentities/src/Zmsentities/Process.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Query/Process.phpzmsentities/src/Zmsentities/Process.php
🧬 Code graph analysis (1)
zmsdb/src/Zmsdb/Query/Process.php (1)
zmsentities/src/Zmsentities/Process.php (3)
getAmendment(351-354)getCustomTextfield(395-398)getCustomTextfield2(409-412)
🔇 Additional comments (5)
zmsadmin/templates/block/appointment/form.twig (1)
396-406: LGTM - textarea conversion properly implemented.The conversion from input to textarea for customTextfield2 is correctly implemented with appropriate constraints preserved (maxlength, value binding, accesskey).
zmsentities/src/Zmsentities/Process.php (3)
391-391: LGTM - trimmed value properly stored.The amendment is now correctly trimmed and the result stored back to the property, preventing unwanted leading/trailing whitespace.
416-419: The code currently uses direct assignment (not concatenation) for customTextfield2, matching the identical pattern used in customTextfield. While the current behavior cannot be confirmed as a recent change based on available git history, the assessment of the current implementation is accurate: both methods directly assign the input value rather than concatenating it.
402-405: Remove or clarify the customTextfield assignment comment; the code shows replacement behavior (not a change from concatenation).The customTextfield implementation uses direct replacement via assignment, not concatenation. The method is called only once during process initialization (line 186), and tests consistently show single-value usage. No evidence exists of prior concatenation behavior or code expecting multiple sequential calls to append values.
Likely an incorrect or invalid review comment.
zmsdb/src/Zmsdb/Query/Process.php (1)
894-896: Verify if unconditional assignment of amendment and custom text fields is intentional.These three fields lack the conditional checks applied to other client data in this method. While the database columns allow NULL and the system has
customTextfieldActivatedpreferences, the code does not check activation status before assigning values. Confirm whether this unconditional assignment—which will store values regardless of field activation status—is the intended behavior or if these assignments should be gated by scope preferences.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zmsadmin/templates/block/queue/table.twig (1)
161-172: Use explicite('html_attr')for HTML attribute context escaping.The
titleattribute receives decoded entities viadecodeEntities|, which is not marked as a safe filter. While Twig's default autoescape for.twigfiles provides baseline protection, explicit attribute-context escaping is a defense-in-depth best practice and follows OWASP guidelines.Apply this to all instances in the three tables (parked, queue, and missed lists):
Apply to customTextfield title attributes
- <td title="{{ item.customTextfield|decodeEntities }}" class="limited-width"> + <td title="{{ item.customTextfield|decodeEntities|e('html_attr') }}" class="limited-width">Apply to customTextfield2 title attributes
- <td title="{{ item.customTextfield2|decodeEntities }}" class="limited-width"> + <td title="{{ item.customTextfield2|decodeEntities|e('html_attr') }}" class="limited-width">
🧹 Nitpick comments (1)
zmsadmin/templates/block/queue/table.twig (1)
722-726: Verify.limited-widthactually constrains<td>width in your table layout.Depending on existing table CSS,
max-widthon<td>may not bite unless the table uses a compatible layout (oftentable-layout: fixedand/or explicit column widths). Please verify in-browser for all three tables + responsive wrapper behavior.Optional follow-up: consider moving this rule out of the Twig template into the project’s main stylesheet to keep presentation concerns centralized (Clean Code separation). Based on coding guidelines, separate concerns.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmsadmin/templates/block/queue/table.twig
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsadmin/templates/block/queue/table.twig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-zmsautomation-tests / zmsapiautomation
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (2)
zmsadmin/templates/block/queue/table.twig (2)
424-435: Good: consistent width limiting + truncation for queue custom text fields.This should prevent the table from being “blown up” by long values while still giving full content via tooltip. (See the escaping note on
titlefrom above.)
555-566: Good: missed-list custom text fields also constrained.Nice to apply the same behavior consistently across parked/queued/missed lists.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
Style
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.