fix(ZMSKVR-1275): mail subject character limits#2041
Open
ChristianVierthaler wants to merge 4 commits intonextfrom
Open
fix(ZMSKVR-1275): mail subject character limits#2041ChristianVierthaler wants to merge 4 commits intonextfrom
ChristianVierthaler wants to merge 4 commits intonextfrom
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors the mail-send flow: Changes
Sequence DiagramsequenceDiagram
actor User
participant Trigger as Trigger Element
participant Dialog as Mail Dialog
participant Binder as bindMailForm
participant Backend as Server
participant UI as UI/Spinner
User->>Trigger: click "Send custom mail"
Trigger->>Dialog: load mail dialog HTML
Dialog->>Binder: bindMailForm($container, triggerElement)
User->>Dialog: submit form (native submit)
Binder->>UI: show spinner (spinner on)
Binder->>Backend: POST serialized form data
Backend-->>Binder: response (contains form OR success message)
alt response contains form
Binder->>Dialog: refresh dialog content
Dialog->>Binder: re-bind mail form with triggerElement
Binder->>UI: hide spinner
else success message
Binder->>UI: show success message
Binder->>UI: hide spinner
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
zmsadmin/templates/element/helper/messages.twig (1)
319-319: Minor: Semantic inconsistency in button class name.Using
button-abortclass on a success confirmation button seems semantically inconsistent—the user is acknowledging success, not aborting an action. Consider whetherbutton-okor a more neutral class would better reflect the button's purpose.This is a minor styling concern and doesn't affect functionality since
data-action-okis preserved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zmsadmin/templates/element/helper/messages.twig` at line 319, The OK confirmation button currently uses the semantic class "button-abort" which is misleading; update the element with data-action-ok (the <button> that currently has class "button button-abort btn right") to use a more appropriate class such as "button-ok" or "button-confirm" (e.g., replace "button-abort" with "button-ok") so the class name matches the button's purpose while leaving data-action-ok unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@zmsadmin/js/page/workstation/index.js`:
- Line 488: The call in bindMailForm uses the outer-scope variable event which
may be undefined; change the call to use the local parameter triggerElement
instead: in bindMailForm (and where onSendCustomMail supplies triggerElement),
replace the fourth argument event.currentTarget with triggerElement so
loadMessage(...) receives the passed-in trigger element rather than relying on
the closure-bound event.
- Around line 471-489: Wrap the async sequence in the submit handler for
form[name="mail"] in a try/finally so hideSpinner($container) is always called:
after ev.preventDefault() and showSpinner($container), perform the await
this.loadCall(...) and subsequent response handling (including this.loadDialog,
this.bindMailForm, this.loadMessage) inside a try block and call
hideSpinner($container) in the finally block; optionally handle errors in a
catch to show a user message via this.loadMessage or rethrow, but ensure
hideSpinner is guaranteed to run.
---
Nitpick comments:
In `@zmsadmin/templates/element/helper/messages.twig`:
- Line 319: The OK confirmation button currently uses the semantic class
"button-abort" which is misleading; update the element with data-action-ok (the
<button> that currently has class "button button-abort btn right") to use a more
appropriate class such as "button-ok" or "button-confirm" (e.g., replace
"button-abort" with "button-ok") so the class name matches the button's purpose
while leaving data-action-ok unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
zmsadmin/js/page/workstation/index.jszmsadmin/src/Zmsadmin/Mail.phpzmsadmin/templates/block/mail/form.twigzmsadmin/templates/element/helper/messages.twig
Comment on lines
+471
to
+489
| $dialog.on('submit.mail', 'form[name="mail"]', async (ev) => { | ||
| ev.preventDefault(); | ||
| showSpinner($container); | ||
| const formData = | ||
| $(ev.currentTarget).serialize() + '&submit=form&dialog=1'; | ||
| const response = await this.loadCall( | ||
| `${this.includeUrl}/mail/`, | ||
| 'POST', | ||
| formData, | ||
| false, | ||
| $container | ||
| ); | ||
| hideSpinner($container); | ||
| if (response.includes('<form')) { | ||
| this.loadDialog(response, null, null, triggerElement); | ||
| this.bindMailForm($container, triggerElement); | ||
| } else { | ||
| this.loadMessage(response, () => { }, null, event.currentTarget); | ||
| } |
Contributor
There was a problem hiding this comment.
Add error handling to prevent spinner from staying visible on failure.
If loadCall throws an exception, hideSpinner($container) will never be called, leaving the UI in a stuck state. Wrap the async operations in try/finally.
🛡️ Proposed fix
$dialog.on('submit.mail', 'form[name="mail"]', async (ev) => {
ev.preventDefault();
showSpinner($container);
- const formData =
- $(ev.currentTarget).serialize() + '&submit=form&dialog=1';
- const response = await this.loadCall(
- `${this.includeUrl}/mail/`,
- 'POST',
- formData,
- false,
- $container
- );
- hideSpinner($container);
- if (response.includes('<form')) {
- this.loadDialog(response, null, null, triggerElement);
- this.bindMailForm($container, triggerElement);
- } else {
- this.loadMessage(response, () => { }, null, triggerElement);
+ try {
+ const formData =
+ $(ev.currentTarget).serialize() + '&submit=form&dialog=1';
+ const response = await this.loadCall(
+ `${this.includeUrl}/mail/`,
+ 'POST',
+ formData,
+ false,
+ $container
+ );
+ if (response.includes('<form')) {
+ this.loadDialog(response, null, null, triggerElement);
+ this.bindMailForm($container, triggerElement);
+ } else {
+ this.loadMessage(response, () => { }, null, triggerElement);
+ }
+ } finally {
+ hideSpinner($container);
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@zmsadmin/js/page/workstation/index.js` around lines 471 - 489, Wrap the async
sequence in the submit handler for form[name="mail"] in a try/finally so
hideSpinner($container) is always called: after ev.preventDefault() and
showSpinner($container), perform the await this.loadCall(...) and subsequent
response handling (including this.loadDialog, this.bindMailForm,
this.loadMessage) inside a try block and call hideSpinner($container) in the
finally block; optionally handle errors in a catch to show a user message via
this.loadMessage or rethrow, but ensure hideSpinner is guaranteed to run.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
zmsadmin/js/page/workstation/index.js (1)
471-490:⚠️ Potential issue | 🟡 MinorAdd error handling to prevent spinner from staying visible on failure.
If
loadCallthrows an exception,hideSpinner($container)will never be called, leaving the UI in a stuck state. Wrap the async operations in try/finally.🛡️ Proposed fix
$dialog.on('submit.mail', 'form[name="mail"]', async (ev) => { ev.preventDefault(); showSpinner($container); - const formData = - $(ev.currentTarget).serialize() + '&submit=form&dialog=1'; - const response = await this.loadCall( - `${this.includeUrl}/mail/`, - 'POST', - formData, - false, - $container - ); - hideSpinner($container); - if (response.includes('<form')) { - this.loadDialog(response, null, null, triggerElement); - this.bindMailForm($container, triggerElement); - } else { - this.loadMessage(response, () => { }, null, triggerElement); + try { + const formData = + $(ev.currentTarget).serialize() + '&submit=form&dialog=1'; + const response = await this.loadCall( + `${this.includeUrl}/mail/`, + 'POST', + formData, + false, + $container + ); + if (response.includes('<form')) { + this.loadDialog(response, null, null, triggerElement); + this.bindMailForm($container, triggerElement); + } else { + this.loadMessage(response, () => { }, null, triggerElement); + } + } finally { + hideSpinner($container); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zmsadmin/js/page/workstation/index.js` around lines 471 - 490, The submit handler registered on $dialog (the function starting with $dialog.on('submit.mail', 'form[name="mail"]', async (ev) => { ... })) should wrap the async call to this.loadCall and subsequent response handling in a try/finally so showSpinner($container) is always paired with hideSpinner($container); move showSpinner before the try, perform await this.loadCall(...) and the response logic inside try, and call hideSpinner($container) in finally; optionally handle or rethrow the error after hiding the spinner so the UI cannot remain stuck (leave existing calls to this.loadDialog, this.bindMailForm, and this.loadMessage unchanged inside the try).
🧹 Nitpick comments (1)
zmsadmin/js/page/workstation/index.js (1)
457-466: Consider adding error handling for the async dialog load.If
loadCallfails (e.g., network error), the promise rejection will propagate unhandled. While no spinner is shown here, the user won't receive feedback about the failure.🛡️ Proposed fix
async onSendCustomMail($container, event) { stopEvent(event); const processId = $(event.currentTarget).data('process'); const triggerElement = event.currentTarget; - const html = await this.loadCall( - `${this.includeUrl}/mail/?selectedprocess=${processId}&dialog=1` - ); - this.loadDialog(html, null, null, triggerElement); - this.bindMailForm($container, triggerElement); + try { + const html = await this.loadCall( + `${this.includeUrl}/mail/?selectedprocess=${processId}&dialog=1` + ); + this.loadDialog(html, null, null, triggerElement); + this.bindMailForm($container, triggerElement); + } catch (error) { + console.error('Failed to load mail dialog:', error); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zmsadmin/js/page/workstation/index.js` around lines 457 - 466, The onSendCustomMail handler awaits loadCall which can reject; wrap the await this.loadCall(...) in a try/catch inside onSendCustomMail, and on failure call a user-visible error path (e.g., show a toast/alert or this.showError) and log the error (console.error or process logger) while returning early so you do not call this.loadDialog or this.bindMailForm; include the original error message in the log and a concise user message so failures to load the mail dialog are handled gracefully.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@zmsadmin/js/page/workstation/index.js`:
- Around line 471-490: The submit handler registered on $dialog (the function
starting with $dialog.on('submit.mail', 'form[name="mail"]', async (ev) => { ...
})) should wrap the async call to this.loadCall and subsequent response handling
in a try/finally so showSpinner($container) is always paired with
hideSpinner($container); move showSpinner before the try, perform await
this.loadCall(...) and the response logic inside try, and call
hideSpinner($container) in finally; optionally handle or rethrow the error after
hiding the spinner so the UI cannot remain stuck (leave existing calls to
this.loadDialog, this.bindMailForm, and this.loadMessage unchanged inside the
try).
---
Nitpick comments:
In `@zmsadmin/js/page/workstation/index.js`:
- Around line 457-466: The onSendCustomMail handler awaits loadCall which can
reject; wrap the await this.loadCall(...) in a try/catch inside
onSendCustomMail, and on failure call a user-visible error path (e.g., show a
toast/alert or this.showError) and log the error (console.error or process
logger) while returning early so you do not call this.loadDialog or
this.bindMailForm; include the original error message in the log and a concise
user message so failures to load the mail dialog are handled gracefully.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
New Features
Improvements
Style