feat(ZMS-3503): added logic to end emergency with checkbox #805
Conversation
WalkthroughThis pull request introduces a new JavaScript class Changes
Sequence DiagramsequenceDiagram
participant User
participant EmergencyEndView
participant Backend
User->>EmergencyEndView: Click Emergency End Button
EmergencyEndView->>EmergencyEndView: endEmergency()
EmergencyEndView->>Backend: sendEmergencyCancel()
Backend-->>EmergencyEndView: Request Response
EmergencyEndView->>User: Update UI
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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 (3)
zmsadmin/js/index.js (1)
147-147: Remove debug console.log statement.This appears to be a debug statement that should be removed before production deployment.
-console.log("Hello")zmsadmin/js/block/scope/emergencyend.js (2)
12-13: Remove debug console.log statements.These appear to be debug statements that should be removed before production deployment.
- console.log("Emergency Options received:", options); this.includeUrl = options.includeurl this.returnTarget = options.returnTarget this.workstationName = ""+options.workstationname this.scope = options.scope - console.log("Emergency Scope received:", this.scope);Also applies to: 17-18
48-58: Enhance error handling in loadData method.The error handling in the AJAX call could be improved by:
- Adding specific error types/messages
- Implementing retry logic for transient failures
- Providing user feedback on errors
return new Promise((resolve, reject) => { $.ajax(url, { method: 'GET' }).done(data => { const emergencyData = deepGet(tryJson(data), ['workstation', 'scope', 'status', 'emergency']) resolve(emergencyData) }).fail(err => { - console.log('XHR error', url, err) + const error = new Error(`Failed to load emergency data: ${err.statusText}`); + error.originalError = err; + reject(error); }) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
zmsadmin/js/block/emergency/index.js(1 hunks)zmsadmin/js/block/scope/emergencyend.js(1 hunks)zmsadmin/js/index.js(3 hunks)zmsadmin/templates/block/scope/form.twig(2 hunks)zmsentities/src/Zmsentities/Schema/Entity.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- zmsadmin/js/block/emergency/index.js
🔇 Additional comments (5)
zmsadmin/js/index.js (2)
29-29: LGTM!The import follows the established pattern in the file.
112-114: LGTM!The initialization of EmergencyEnd follows the consistent pattern used for other view components.
zmsadmin/js/block/scope/emergencyend.js (1)
126-159: Improve accessibility in focus trap implementation.The focus trap implementation could be enhanced:
- Add ARIA attributes for better screen reader support
- Handle cases where no focusable elements exist
- Consider using established libraries like focus-trap
Additionally, verify if the focus trap is being properly removed when the emergency overlay is closed.
✅ Verification successful
Focus trap is properly removed, but accessibility can be improved
The focus trap is correctly removed when the emergency overlay is closed, with proper cleanup in all exit paths. However, the original accessibility improvement suggestions remain valid:
- Add ARIA attributes for better screen reader support
- Handle cases where no focusable elements exist
- Consider using established libraries like focus-trap
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for focus trap removal calls rg -A 5 "removeFocusTrap" .Length of output: 5739
zmsentities/src/Zmsentities/Schema/Entity.php (1)
311-313: LGTM!Good defensive programming practice to check if the key exists before unsetting it.
zmsadmin/templates/block/scope/form.twig (1)
630-635: LGTM!The template changes properly integrate with the new EmergencyEnd JavaScript functionality:
- Added required data attributes for state management
- Added appropriate classes for event handling
Also applies to: 647-647
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
zmsadmin/js/block/scope/emergencyend.js (5)
1-4: Remove or clarify commented-out import.
You have some commented-out code (// import { playSound } from '../../lib/audio'). It's a best practice to remove unused code to maintain clarity, or add a comment explaining its future purpose if you plan to use it.-// import { playSound } from '../../lib/audio' +// import { playSound } from '../../lib/audio'} // Future use for emergency alert audio
6-7: Consider making the refresh interval configurable.
Having a hard-coded constantDEFAULT_REFRESH_INTERVAL = 15might be limiting. Consider making it configurable through an environment variable or configuration object to adapt to different environments or user requirements without changing the code.
10-29: Avoid excessive console logging in production code.
Whileconsole.logcalls help with debugging, they can clutter the console in production. Consider using a logging utility that can be toggled off or replaced with debug-level logging in non-production environments.- console.log("Emergency Options received:", options); - console.log("Emergency Scope received:", this.scope); + // console.debug("Emergency Options received:", options); + // console.debug("Emergency Scope received:", this.scope);
38-49: Use newer fetch or axios for AJAX calls.
While jQuery’s$.ajaxis valid, modern web applications often use the nativefetchAPI or a library like axios for readability and simpler promise handling. Replacing$.ajaxwithfetchor axios can improve maintainability.- $.ajax(url, { - method: 'GET' - }).done(data => { - ... - }).fail(err => { - ... - }) + fetch(url) + .then(res => res.json()) + .then(data => { + // handle data + }) + .catch(err => { + // handle error + })Also applies to: 58-67
104-108: Consider i18n for displayed strings.
Labels like"Tresen"and"Platz"are hard-coded in German. If the application needs a multilingual audience, consider introducing an i18n library or a language file for easy translations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
zmsadmin/js/block/scope/emergencyend.js(1 hunks)zmsadmin/js/index.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- zmsadmin/js/index.js
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-unit-tests / zmsdb-test
- GitHub Check: call-unit-tests / zmsapi-test
🔇 Additional comments (2)
zmsadmin/js/block/scope/emergencyend.js (2)
79-82: Confirm parseInt usage and default fallback.
You useparseIntforactivatedandacceptedByWorkstation. Ensure these values are always convertible to a valid number in your data flow. Otherwise, consider providing a default fallback to avoidNaN.✅ Verification successful
parseInt usage is safe, no fallback needed
The values foractivatedandacceptedByWorkstationare always valid numeric strings controlled by the system through schema validation and explicit assignments. The current implementation is correct and safe.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verifying if the properties are always numeric rg -A 4 'activated' | rg 'acceptedByWorkstation'Length of output: 27954
23-24: Ensure correct selector for the end-emergency button.
Double-check that.emergency__button-endis indeed the correct DOM element, as no default text or visible label is provided. Use a more descriptive class name or a data attribute if multiple buttons might exist in the future.✅ Verification successful
The
.emergency__button-endselector is correctly implemented
- Follows consistent BEM naming convention with other emergency buttons
- Has clear visible text label "Notruf beenden"
- Used consistently in templates and JavaScript components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # search for class usage in the entire codebase to confirm correctness fd -E node_modules -E dist -e js -e html -e twig | xargs rg '\.emergency__button-end'Length of output: 318
Script:
#!/bin/bash # Search for emergency-related button classes and HTML structure echo "=== Emergency-related classes ===" rg -i 'emergency.*button' --type html --type twig echo -e "\n=== Emergency button in HTML/template structure ===" rg -i '<[^>]*emergency' --type html --type twig echo -e "\n=== Module initialization ===" rg -A 5 'new.*Emergency' --type jsLength of output: 4544
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
zmsadmin/js/block/scope/emergencyend.js (2)
5-13: Add input validation and improve code quality.Several improvements can be made to the constructor:
- Add validation for required options
- Use
String()instead of string concatenation- Use arrow function for event binding to avoid explicit
bind(this)constructor(element, options) { + if (!element || !options || !options.includeurl || !options.scope || !options.workstationname) { + throw new Error('Required options missing'); + } this.includeUrl = options.includeurl this.scope = options.scope - this.workstationName = "" + options.workstationname + this.workstationName = String(options.workstationname) this.data = {} this.$ = $(element) - this.$.find('.emergency__button-end').on('click', this.endEmergency.bind(this)) + this.$.find('.emergency__button-end').on('click', () => this.endEmergency()) }
40-40: Remove trailing whitespace.There's an extra whitespace at the end of the export statement.
-export default EndEmergencyView; +export default EndEmergencyView;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmsadmin/js/block/scope/emergencyend.js(1 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 (1)
zmsadmin/js/block/scope/emergencyend.js (1)
1-3: LGTM!The import statement and class declaration follow best practices.
| endEmergency() { | ||
| this.update({ activated: "0", calledByWorkstation: "-1", acceptedByWorkstation: "-1" }) | ||
| this.sendEmergencyCancel() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and user feedback.
The endEmergency method should handle errors from sendEmergencyCancel and provide feedback to the user.
endEmergency() {
this.update({ activated: "0", calledByWorkstation: "-1", acceptedByWorkstation: "-1" })
- this.sendEmergencyCancel()
+ this.sendEmergencyCancel()
+ .then(() => {
+ // Add success feedback (e.g., toast notification)
+ })
+ .catch(error => {
+ // Add error feedback
+ this.update({ activated: "1" }) // Revert state on error
+ })
}Committable suggestion skipped: line range outside the PR's diff.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores