-
Notifications
You must be signed in to change notification settings - Fork 13k
fix: Smarsh integration compatibility with multiple files #38338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: f9b0265 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis PR updates the Smarsh integration backend to support multiple file attachments per message by introducing a helper function to process files and unifying the file attachment handling logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38338 +/- ##
===========================================
+ Coverage 70.74% 70.78% +0.03%
===========================================
Files 3159 3159
Lines 109384 109384
Branches 19676 19666 -10
===========================================
+ Hits 77383 77423 +40
+ Misses 29963 29930 -33
+ Partials 2038 2031 -7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 2 files
| const fileLinks = | ||
| attachments | ||
| ?.filter((a): a is MessageAttachment & { title: string } => 'title' in a && a.title !== undefined) | ||
| .map((a) => `${a.title} (${_getLink({ title_link: 'title_link' in a ? a.title_link || '' : '' })})`) || []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .map((a) => `${a.title} (${_getLink({ title_link: 'title_link' in a ? a.title_link || '' : '' })})`) || []; | |
| .map((a) => `${a.title} (${_getLink({ title_link: a.title_link || '' })})`) || []; |
:ape_think: was there a reason for the ternary?
| if (rows.length !== 0) { | ||
| const result = start + rows.join('') + end; | ||
|
|
||
| await SmarshHistory.updateOne( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would u b so kind to move this to the model? :please: (or create a task for doing it later)
Proposed changes (including videos or screenshots)
This PR updates the Smarsh integration to be compatible with the new multiple file attachment format introduced in the multi-file upload feature on #32703.
Issue(s)
Steps to test or reproduce
1. Set up email testing environment
Create a docker-compose.mail-test.yml file:
Start the containers:
docker-compose -f docker-compose.mail-test.yml up -d2. Configure Smarsh in Rocket.Chat
smarsh@localhostrocketchat@localhost3. Test single file message (baseline)
smarsh@localhost/smarsh4. Test multiple file attachments (new feature)
Further comments
When a message contains both text (
message.msg) and files, only the file information is included in the Smarsh export - the message text is not displayed. This is pre-existing behavior from the original implementation and should be addressed in a separate task.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.