Communication: Fix missing length check when forwarding messages#12516
Communication: Fix missing length check when forwarding messages#12516
Communication: Fix missing length check when forwarding messages#12516Conversation
github-actions
bot
added
client
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
…heck-for-forwarding
This comment was marked as duplicate.
This comment was marked as duplicate.
|
@anian03 Test coverage has been automatically updated in the PR description. |
End-to-End Test Results
Test Strategy: Two-phase execution
Overall: ❌ Phase 2 (remaining tests) failed 🔗 Workflow Run · 📊 Test Report |
|
@anian03 Test coverage has been automatically updated in the PR description. |
WalkthroughAdds client-side character-length validation to the forward message dialog: exports Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/webapp/app/communication/course-conversations-components/forward-message-dialog/forward-message-dialog.component.html`:
- Around line 124-130: Replace the Bootstrap button with PrimeNG's pButton in
the template: update the button element in forward-message-dialog.component.html
to use the pButton directive and class="p-button-primary" instead of class="btn
btn-primary", keeping the same type, [disabled], (click)="send()" and
jhiTranslate attributes; also add ButtonModule to the component's NgModule
imports (or the component's imports array) so the pButton directive is available
for the ForwardMessageDialogComponent.
In
`@src/main/webapp/app/communication/course-conversations-components/forward-message-dialog/forward-message-dialog.component.ts`:
- Around line 287-291: The isMessageValid() function currently rejects messages
whose length equals maxContentLength because it uses '<'; update
isMessageValid() (referencing this.newPost.content and this.maxContentLength) to
allow lengths up to and including the limit by using '<=' for the comparison so
a 5000-character message is accepted. Ensure the null-coalescing to '' remains
and the return type stays boolean.
In
`@src/main/webapp/app/communication/course-conversations-components/forward-message-dialog/forward-messages-dialog.component.spec.ts`:
- Around line 312-323: Add an exact-boundary unit test that uses
component.maxContentLength (not maxContentLength - 1) to ensure the allowed
limit is accepted: create longText = 'a'.repeat(component.maxContentLength),
call component.updateField(longText), set component.selectedChannels and
component.selectedUsers as in the existing test, call fixture.detectChanges(),
then assert component.isMessageValid() is true and the send button is enabled
(query the button via
fixture.debugElement.query(By.css('button.btn-primary')).nativeElement and
assert disabled is false). Ensure the new spec references
component.maxContentLength, updateField, isMessageValid, selectedChannels,
selectedUsers, and the same fixture/button query so it runs alongside the
existing test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d328ceaa-aedd-46c3-8328-43f29b442c2e
📒 Files selected for processing (4)
src/main/webapp/app/communication/course-conversations-components/forward-message-dialog/forward-message-dialog.component.htmlsrc/main/webapp/app/communication/course-conversations-components/forward-message-dialog/forward-message-dialog.component.tssrc/main/webapp/app/communication/course-conversations-components/forward-message-dialog/forward-messages-dialog.component.spec.tssrc/main/webapp/app/communication/directive/posting-create-edit.directive.ts
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
|
@anian03 Test coverage has been automatically updated in the PR description. |
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@anian03 Clean, well-scoped bugfix. The length validation logic is correct, boundary tests are solid, and reusing the existing MAX_CONTENT_LENGTH constant is the right call. Nice work.
anian03
dismissed stale reviews from Claudia-Anthropica and coderabbitai[bot]
via
688115c
|
@anian03 Test coverage has been automatically updated in the PR description. |
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@anian03 Clean, well-scoped fix. All previous feedback addressed — the off-by-one is fixed, boundary test is in place, and the ngStyle cleanup looks good. Nice work.
…heck-for-forwarding
|
@anian03 Test coverage has been automatically updated in the PR description. |
SedaOran
left a comment
SedaOran
left a comment
There was a problem hiding this comment.
Nice addition, the client-side validation and UX improvements look good to me, and the boundary tests are solid.
One question: should we also enforce this limit on the backend? Relying only on client-side validation might not be sufficient (e.g. direct API calls), so it could be safer to have a corresponding server-side check as well.
Otherwise looks good 👍 Thanks.
The server already does not accept messages longer than 5000 characters, which is the reason for this PR :) |
SedaOran
left a comment
SedaOran
left a comment
There was a problem hiding this comment.
Code looks good. Approving.
3 participants
Summary
Resolves #12309. There was no client-side check for the length of a message during forwarding. This could result in strange internal server errors.
Checklist
General
Client
Motivation and Context
A missing client side check for the message length when forwarding a message could result in strange errors when sending long messages.
Description
Adds a length check to the forward message dialog and disables the send button if the max length is exceeded.
Steps for Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Test Coverage
Client
Last updated: 2026-04-19 20:36:46 UTC
Screenshots
Summary by CodeRabbit
New Features
Tests