-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix broken image urls in Settings > Profile and Invite To Workspace Email #8942
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,9 @@ | ||
export const getImageAbsoluteURI = ( | ||
imageUrl?: string | null, | ||
serverUrl?: string, | ||
) => { | ||
if (!imageUrl) { | ||
return null; | ||
} | ||
|
||
if (imageUrl?.startsWith('https:')) { | ||
export const getImageAbsoluteURI = (imageUrl: string, serverUrl: string) => { | ||
if (imageUrl.startsWith('https:') || imageUrl.startsWith('http:')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Removing optional parameters could break existing code that passes undefined/null. Consider keeping parameters optional with default values. |
||
return imageUrl; | ||
} | ||
|
||
return serverUrl?.endsWith('/') | ||
return serverUrl.endsWith('/') | ||
? `${serverUrl.substring(0, serverUrl.length - 1)}/files/${imageUrl}` | ||
: `${serverUrl || ''}/files/${imageUrl}`; | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Using |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,7 +107,7 @@ export const SettingsAdminFeatureFlags = () => { | |
title: workspace.name, | ||
logo: | ||
getImageAbsoluteURI( | ||
workspace.logo === null ? DEFAULT_WORKSPACE_LOGO : workspace.logo, | ||
isDefined(workspace.logo) ? workspace.logo : DEFAULT_WORKSPACE_LOGO, | ||
) ?? '', | ||
Comment on lines
109
to
111
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: This logic could still result in undefined being passed to getImageAbsoluteURI if workspace.logo is null. Consider using |
||
})) ?? []; | ||
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Removing this file removes null/undefined handling and proper URL path construction. The twenty-ui version may need these features added back to maintain full functionality. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: The removed version handled leading slashes in image URLs differently than the twenty-ui version. This could cause broken image URLs if image paths start with '/'. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: The URL construction using URL class was more robust than string concatenation. Consider keeping this approach for better URL handling. |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ export class FilePathGuard implements CanActivate { | |
async canActivate(context: ExecutionContext): Promise<boolean> { | ||
const request = context.switchToHttp().getRequest(); | ||
const query = request.query; | ||
const urlParamsV2 = new URLSearchParams(request.originalUrl); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: urlParamsV2 is declared but never used |
||
|
||
if (!query || !query['token']) { | ||
return false; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,16 @@ | ||
import { UseGuards } from '@nestjs/common'; | ||
import { Args, Mutation, Query, Resolver } from '@nestjs/graphql'; | ||
|
||
import { AuthWorkspace } from 'src/engine/decorators/auth/auth-workspace.decorator'; | ||
import { WorkspaceAuthGuard } from 'src/engine/guards/workspace-auth.guard'; | ||
import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity'; | ||
import { WorkspaceInvitationService } from 'src/engine/core-modules/workspace-invitation/services/workspace-invitation.service'; | ||
import { WorkspaceInvitation } from 'src/engine/core-modules/workspace-invitation/dtos/workspace-invitation.dto'; | ||
import { AuthUser } from 'src/engine/decorators/auth/auth-user.decorator'; | ||
import { FileService } from 'src/engine/core-modules/file/services/file.service'; | ||
import { User } from 'src/engine/core-modules/user/user.entity'; | ||
import { SendInvitationsOutput } from 'src/engine/core-modules/workspace-invitation/dtos/send-invitations.output'; | ||
import { WorkspaceInvitation } from 'src/engine/core-modules/workspace-invitation/dtos/workspace-invitation.dto'; | ||
import { WorkspaceInvitationService } from 'src/engine/core-modules/workspace-invitation/services/workspace-invitation.service'; | ||
import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity'; | ||
import { AuthUser } from 'src/engine/decorators/auth/auth-user.decorator'; | ||
import { AuthWorkspace } from 'src/engine/decorators/auth/auth-workspace.decorator'; | ||
import { UserAuthGuard } from 'src/engine/guards/user-auth.guard'; | ||
import { WorkspaceAuthGuard } from 'src/engine/guards/workspace-auth.guard'; | ||
|
||
import { SendInvitationsInput } from './dtos/send-invitations.input'; | ||
|
||
|
@@ -18,6 +19,7 @@ import { SendInvitationsInput } from './dtos/send-invitations.input'; | |
export class WorkspaceInvitationResolver { | ||
constructor( | ||
private readonly workspaceInvitationService: WorkspaceInvitationService, | ||
private readonly fileService: FileService, | ||
) {} | ||
|
||
@Mutation(() => String) | ||
|
@@ -57,9 +59,19 @@ export class WorkspaceInvitationResolver { | |
@AuthUser() user: User, | ||
@AuthWorkspace() workspace: Workspace, | ||
): Promise<SendInvitationsOutput> { | ||
let workspaceLogoWithToken = ''; | ||
|
||
if (workspace.logo) { | ||
const workspaceLogoToken = await this.fileService.encodeFileToken({ | ||
workspaceId: workspace.id, | ||
}); | ||
|
||
workspaceLogoWithToken = `${workspace.logo}?token=${workspaceLogoToken}`; | ||
} | ||
Comment on lines
+64
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: token generation should be wrapped in try/catch to handle potential encoding errors gracefully |
||
|
||
return await this.workspaceInvitationService.sendInvitations( | ||
sendInviteLinkInput.emails, | ||
workspace, | ||
{ ...workspace, logo: workspaceLogoWithToken }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: spreading workspace and overriding logo could cause issues if workspace is frozen/sealed object |
||
user, | ||
); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,14 @@ | ||
import { getImageAbsoluteURI } from '../getImageAbsoluteURI'; | ||
|
||
describe('getImageAbsoluteURI', () => { | ||
it('should return null if imageUrl is null', () => { | ||
const imageUrl = null; | ||
it('should return absolute url if the imageUrl is an absolute url', () => { | ||
const imageUrl = 'https://XXX'; | ||
const result = getImageAbsoluteURI(imageUrl); | ||
expect(result).toBeNull(); | ||
expect(result).toBe(imageUrl); | ||
}); | ||
|
||
it('should return absolute url if the imageUrl is an absolute url', () => { | ||
const imageUrl = 'https://XXX'; | ||
it('should return absolute url if the imageUrl is an absolute unsecure url', () => { | ||
const imageUrl = 'http://XXX'; | ||
const result = getImageAbsoluteURI(imageUrl); | ||
expect(result).toBe(imageUrl); | ||
}); | ||
Comment on lines
+4
to
14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: test cases for https and http are redundant since they test the same logic branch in the implementation |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,7 @@ | ||
import { REACT_APP_SERVER_BASE_URL } from '@ui/utilities/config'; | ||
|
||
// TODO: this is a code smell trying to guess whether it's a relative path or not | ||
// We should instead put the meaning onto our variables and parameters | ||
// imageUrl should be either imageAbsoluteURL or imageRelativeServerPath | ||
// But we need to refactor the chain of calls to this function | ||
export const getImageAbsoluteURI = (imageUrl?: string | null) => { | ||
if (!imageUrl) { | ||
return null; | ||
} | ||
|
||
if (imageUrl?.startsWith('http')) { | ||
export const getImageAbsoluteURI = (imageUrl: string) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Removing null/undefined handling could break existing code that passes undefined. Consider keeping parameter optional or adding runtime validation. |
||
if (imageUrl.startsWith('https:') || imageUrl.startsWith('http:')) { | ||
return imageUrl; | ||
} | ||
|
||
|
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.
Overall, I think we should stop authorizing undefined in our typings unless this is needed. The caller should handle the case. This will reduce code complexity