-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Fix attachment body not being loaded #12770
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
Conversation
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.
PR Summary
Added testing coverage and improved file attachment handling in activities, with significant refactoring of the ActivityQueryResultGetterHandler.
- Added comprehensive test suite in
packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-result-getters/handlers/__tests__/activity-query-result-getter.handler.spec.ts
covering key URL transformation scenarios - Simplified rich text handling by removing FeatureFlagService dependency and standardizing on bodyV2 format
- Enhanced security for file URLs by implementing robust URL pattern matching and reconstruction for
/files/attachment/
paths - Added proper dependency injection through
@Injectable()
decorator for NestJS integration - Improved error handling with better JSON parsing error logging
2 files reviewed, no comments
Edit PR Review Bot Settings | Greptile
expect(result).toEqual(note); | ||
}); | ||
|
||
it('when link are external and activity is a task', async () => { |
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.
Nitpick: would rename test ?
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:9699 This environment will automatically shut down when the PR is closed or after 5 hours. |
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.
Nice catch ! left few comments !
}, | ||
}; | ||
|
||
const result = await handler.handle(note, '1'); |
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.
nitpick: renaming 1 by workspace id
|
||
const signedPath = this.fileService.signFileUrl({ | ||
url: imageProps.url.toString(), | ||
url: `attachment/${fileName}`, |
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.
Question: always attachement ?
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.
yes
|
||
const pathname = url.pathname; | ||
|
||
if (!pathname.startsWith('/files/attachment/')) { |
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.
Nitpick: Looks quite duplicated assertions
Remark: So we consider that an external link is any link whom path does not start from files/attachement/
https://google.com/files/attachement` would imply generating a token
Closes #12756