From 8cefa566da6d792097dedaecb348d35dda828322 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 11 Aug 2025 11:50:46 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=93=B8=20fix:=20Avatar=20Handling=20for?= =?UTF-8?q?=20Social=20Login=20(#8993)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Updated `handleExistingUser` function to improve avatar handling logic, including checks for manual flags and null/undefined avatars. - Introduced a new test suite for `handleExistingUser` covering various scenarios, ensuring robust functionality for avatar updates in both local and non-local storage contexts. --- api/strategies/process.js | 7 +- api/strategies/process.test.js | 164 +++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+), 2 deletions(-) create mode 100644 api/strategies/process.test.js diff --git a/api/strategies/process.js b/api/strategies/process.js index 1f7e7c81d..49f99c1f4 100644 --- a/api/strategies/process.js +++ b/api/strategies/process.js @@ -22,9 +22,12 @@ const handleExistingUser = async (oldUser, avatarUrl) => { const isLocal = fileStrategy === FileSources.local; let updatedAvatar = false; - if (isLocal && (oldUser.avatar === null || !oldUser.avatar.includes('?manual=true'))) { + const hasManualFlag = + typeof oldUser?.avatar === 'string' && oldUser.avatar.includes('?manual=true'); + + if (isLocal && (!oldUser?.avatar || !hasManualFlag)) { updatedAvatar = avatarUrl; - } else if (!isLocal && (oldUser.avatar === null || !oldUser.avatar.includes('?manual=true'))) { + } else if (!isLocal && (!oldUser?.avatar || !hasManualFlag)) { const userId = oldUser._id; const resizedBuffer = await resizeAvatar({ userId, diff --git a/api/strategies/process.test.js b/api/strategies/process.test.js new file mode 100644 index 000000000..729552c86 --- /dev/null +++ b/api/strategies/process.test.js @@ -0,0 +1,164 @@ +const { FileSources } = require('librechat-data-provider'); +const { handleExistingUser } = require('./process'); + +jest.mock('~/server/services/Files/strategies', () => ({ + getStrategyFunctions: jest.fn(), +})); + +jest.mock('~/server/services/Files/images/avatar', () => ({ + resizeAvatar: jest.fn(), +})); + +jest.mock('~/models', () => ({ + updateUser: jest.fn(), + createUser: jest.fn(), + getUserById: jest.fn(), +})); + +jest.mock('~/server/services/Config', () => ({ + getBalanceConfig: jest.fn(), +})); + +const { getStrategyFunctions } = require('~/server/services/Files/strategies'); +const { resizeAvatar } = require('~/server/services/Files/images/avatar'); +const { updateUser } = require('~/models'); + +describe('handleExistingUser', () => { + beforeEach(() => { + jest.clearAllMocks(); + process.env.CDN_PROVIDER = FileSources.local; + }); + + it('should handle null avatar without throwing error', async () => { + const oldUser = { + _id: 'user123', + avatar: null, + }; + const avatarUrl = 'https://example.com/avatar.png'; + + await handleExistingUser(oldUser, avatarUrl); + + expect(updateUser).toHaveBeenCalledWith('user123', { avatar: avatarUrl }); + }); + + it('should handle undefined avatar without throwing error', async () => { + const oldUser = { + _id: 'user123', + // avatar is undefined + }; + const avatarUrl = 'https://example.com/avatar.png'; + + await handleExistingUser(oldUser, avatarUrl); + + expect(updateUser).toHaveBeenCalledWith('user123', { avatar: avatarUrl }); + }); + + it('should not update avatar if it has manual=true flag', async () => { + const oldUser = { + _id: 'user123', + avatar: 'https://example.com/avatar.png?manual=true', + }; + const avatarUrl = 'https://example.com/new-avatar.png'; + + await handleExistingUser(oldUser, avatarUrl); + + expect(updateUser).not.toHaveBeenCalled(); + }); + + it('should update avatar for local storage when avatar has no manual flag', async () => { + const oldUser = { + _id: 'user123', + avatar: 'https://example.com/old-avatar.png', + }; + const avatarUrl = 'https://example.com/new-avatar.png'; + + await handleExistingUser(oldUser, avatarUrl); + + expect(updateUser).toHaveBeenCalledWith('user123', { avatar: avatarUrl }); + }); + + it('should process avatar for non-local storage', async () => { + process.env.CDN_PROVIDER = 's3'; + + const mockProcessAvatar = jest.fn().mockResolvedValue('processed-avatar-url'); + getStrategyFunctions.mockReturnValue({ processAvatar: mockProcessAvatar }); + resizeAvatar.mockResolvedValue(Buffer.from('resized-image')); + + const oldUser = { + _id: 'user123', + avatar: null, + }; + const avatarUrl = 'https://example.com/avatar.png'; + + await handleExistingUser(oldUser, avatarUrl); + + expect(resizeAvatar).toHaveBeenCalledWith({ + userId: 'user123', + input: avatarUrl, + }); + expect(mockProcessAvatar).toHaveBeenCalledWith({ + buffer: Buffer.from('resized-image'), + userId: 'user123', + manual: 'false', + }); + expect(updateUser).toHaveBeenCalledWith('user123', { avatar: 'processed-avatar-url' }); + }); + + it('should not update if avatar already has manual flag in non-local storage', async () => { + process.env.CDN_PROVIDER = 's3'; + + const oldUser = { + _id: 'user123', + avatar: 'https://cdn.example.com/avatar.png?manual=true', + }; + const avatarUrl = 'https://example.com/new-avatar.png'; + + await handleExistingUser(oldUser, avatarUrl); + + expect(resizeAvatar).not.toHaveBeenCalled(); + expect(updateUser).not.toHaveBeenCalled(); + }); + + it('should handle avatar with query parameters but without manual flag', async () => { + const oldUser = { + _id: 'user123', + avatar: 'https://example.com/avatar.png?size=large&format=webp', + }; + const avatarUrl = 'https://example.com/new-avatar.png'; + + await handleExistingUser(oldUser, avatarUrl); + + expect(updateUser).toHaveBeenCalledWith('user123', { avatar: avatarUrl }); + }); + + it('should handle empty string avatar', async () => { + const oldUser = { + _id: 'user123', + avatar: '', + }; + const avatarUrl = 'https://example.com/avatar.png'; + + await handleExistingUser(oldUser, avatarUrl); + + expect(updateUser).toHaveBeenCalledWith('user123', { avatar: avatarUrl }); + }); + + it('should handle avatar with manual=false parameter', async () => { + const oldUser = { + _id: 'user123', + avatar: 'https://example.com/avatar.png?manual=false', + }; + const avatarUrl = 'https://example.com/new-avatar.png'; + + await handleExistingUser(oldUser, avatarUrl); + + expect(updateUser).toHaveBeenCalledWith('user123', { avatar: avatarUrl }); + }); + + it('should handle oldUser being null gracefully', async () => { + const avatarUrl = 'https://example.com/avatar.png'; + + // This should throw an error when trying to access oldUser._id + await expect(handleExistingUser(null, avatarUrl)).rejects.toThrow(); + }); +});