feat(ui): add system configuration settings page with logo upload#265
feat(ui): add system configuration settings page with logo upload#265
Conversation
Summary of ChangesHello @l1ttps, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces comprehensive system configuration management capabilities, allowing administrators to personalize the application's branding by setting a custom system name and uploading or removing a logo. The changes span both the user interface, with a new settings page, and the backend, with new API endpoints and services to handle the configuration data and file storage securely and efficiently. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new system configuration page allowing administrators to update the system name and upload a custom logo. The changes span across the frontend UI, backend API, and database schema. While the new feature is a great addition, I've identified a critical bug in the backend logic that could lead to data loss, where uploading a logo resets the system name. Additionally, I've noticed that some existing tests were removed, which is a significant concern for maintaining code quality and preventing regressions. I've also included a suggestion to refactor some of the new frontend code for better clarity and maintainability. Please address the critical issues before merging.
| mockConfig, | ||
| ); | ||
| (mockSystemConfigRepository.save as jest.Mock).mockResolvedValue({ | ||
| ...mockConfig, | ||
| logoPath: null, | ||
| }); | ||
| expect(mockRepository.findOne).toHaveBeenCalledWith({ where: {} }); | ||
| }); | ||
|
|
||
| it('should create and return default config if none exists', async () => { | ||
| (mockRepository.findOne as jest.Mock).mockResolvedValue(null); | ||
| (mockRepository.create as jest.Mock).mockReturnValue(mockConfig); | ||
| (mockRepository.save as jest.Mock).mockResolvedValue(mockConfig); | ||
|
|
||
| const result = await service.getConfig(); | ||
| const result = await service.removeLogo(); | ||
|
|
||
| expect(mockRepository.create).toHaveBeenCalled(); | ||
| expect(mockRepository.save).toHaveBeenCalled(); | ||
| expect(mockSystemConfigRepository.findOne).toHaveBeenCalled(); | ||
| expect(mockSystemConfigRepository.save).toHaveBeenCalledWith({ | ||
| ...mockConfig, | ||
| logoPath: null, | ||
| }); | ||
| expect(result).toEqual({ | ||
| name: mockConfig.name, | ||
| logoPath: mockConfig.logoPath, | ||
| message: 'System logo removed successfully', | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('updateConfig', () => { | ||
| it('should update config with provided name', async () => { | ||
| const updatedConfig = { ...mockConfig, name: 'New Name' }; | ||
| (mockRepository.findOne as jest.Mock).mockResolvedValue(mockConfig); | ||
| (mockRepository.save as jest.Mock).mockResolvedValue(updatedConfig); | ||
|
|
||
| const result = await service.updateConfig({ name: 'New Name' }); | ||
| it('should create default config if none exists and set logoPath to null', async () => { | ||
| const mockConfig = { | ||
| id: 1, | ||
| name: 'OASM', | ||
| logoPath: null, | ||
| }; | ||
|
|
||
| expect(mockRepository.save).toHaveBeenCalledWith( | ||
| expect.objectContaining({ name: 'New Name' }), | ||
| (mockSystemConfigRepository.findOne as jest.Mock).mockResolvedValue(null); | ||
| (mockSystemConfigRepository.create as jest.Mock).mockReturnValue( | ||
| mockConfig, | ||
| ); | ||
| expect(result.message).toBe('System configuration updated successfully'); | ||
| }); | ||
|
|
||
| it('should update config with provided logoPath', async () => { | ||
| const updatedConfig = { ...mockConfig, logoPath: '/new-logo.png' }; | ||
| (mockRepository.findOne as jest.Mock).mockResolvedValue(mockConfig); | ||
| (mockRepository.save as jest.Mock).mockResolvedValue(updatedConfig); | ||
|
|
||
| const result = await service.updateConfig({ logoPath: '/new-logo.png' }); | ||
|
|
||
| expect(mockRepository.save).toHaveBeenCalledWith( | ||
| expect.objectContaining({ logoPath: '/new-logo.png' }), | ||
| (mockSystemConfigRepository.save as jest.Mock).mockResolvedValue( | ||
| mockConfig, | ||
| ); | ||
| expect(result.message).toBe('System configuration updated successfully'); | ||
| }); | ||
|
|
||
| it('should update config with both name and logoPath', async () => { | ||
| const updatedConfig = { | ||
| ...mockConfig, | ||
| name: 'New Name', | ||
| logoPath: '/new-logo.png', | ||
| }; | ||
| (mockRepository.findOne as jest.Mock).mockResolvedValue(mockConfig); | ||
| (mockRepository.save as jest.Mock).mockResolvedValue(updatedConfig); | ||
| const result = await service.removeLogo(); | ||
|
|
||
| const result = await service.updateConfig({ | ||
| name: 'New Name', | ||
| logoPath: '/new-logo.png', | ||
| expect(mockSystemConfigRepository.findOne).toHaveBeenCalled(); | ||
| expect(mockSystemConfigRepository.create).toHaveBeenCalledWith({ | ||
| name: 'OASM', | ||
| logoPath: undefined, | ||
| }); | ||
| expect(mockSystemConfigRepository.save).toHaveBeenCalledWith(mockConfig); | ||
| expect(result).toEqual({ | ||
| message: 'System logo removed successfully', | ||
| }); | ||
|
|
||
| expect(mockRepository.save).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| name: 'New Name', | ||
| logoPath: '/new-logo.png', | ||
| }), | ||
| ); | ||
| expect(result.message).toBe('System configuration updated successfully'); | ||
| }); | ||
|
|
||
| it('should not update fields when dto values are undefined', async () => { | ||
| (mockRepository.findOne as jest.Mock).mockResolvedValue(mockConfig); | ||
| (mockRepository.save as jest.Mock).mockResolvedValue(mockConfig); | ||
|
|
||
| await service.updateConfig({}); | ||
|
|
||
| expect(mockRepository.save).toHaveBeenCalledWith(mockConfig); | ||
| }); | ||
|
|
||
| it('should create default config if none exists before updating', async () => { | ||
| (mockRepository.findOne as jest.Mock) | ||
| .mockResolvedValueOnce(null) | ||
| .mockResolvedValue(mockConfig); | ||
| (mockRepository.create as jest.Mock).mockReturnValue(mockConfig); | ||
| (mockRepository.save as jest.Mock).mockResolvedValue(mockConfig); | ||
|
|
||
| await service.updateConfig({ name: 'New Name' }); | ||
|
|
||
| expect(mockRepository.create).toHaveBeenCalled(); | ||
| expect(mockRepository.save).toHaveBeenCalledTimes(2); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
It appears that existing tests for getConfig and updateConfig have been removed. Removing tests, especially when the logic they cover is being changed, is a dangerous practice that can lead to regressions. The tests for updateConfig would likely have caught the bug introduced in system-configs.service.ts where the system name is unintentionally reset.
Please restore and update these tests to ensure the new logic is fully covered, including edge cases.
| dto: UpdateSystemConfigDto, | ||
| ): Promise<DefaultMessageResponseDto> { | ||
| const config = await this.findOrCreateConfig(); | ||
| config.name = dto.name || SystemConfigsService.DEFAULT_SYSTEM_NAME; |
There was a problem hiding this comment.
This line introduces a critical bug that can lead to data loss. When updateConfig is called without a name in the DTO (e.g., from storage.controller.ts during a logo upload), dto.name is undefined. This causes the expression dto.name || SystemConfigsService.DEFAULT_SYSTEM_NAME to evaluate to the default system name, resetting any custom name the user has configured.
The previous implementation correctly checked if dto.name was defined before attempting to update it. Please restore that conditional logic to prevent accidental data overwrites.
| config.name = dto.name || SystemConfigsService.DEFAULT_SYSTEM_NAME; | |
| if (dto.name !== undefined) { | |
| config.name = dto.name; | |
| } |
| } else if (shouldRemoveLogo) { | ||
| // If logo should be removed, update the config without logoPath | ||
| updateConfigMutation.mutate({ | ||
| data: { | ||
| name: values.name, | ||
| // Don't include logoPath to remove it (assuming the API handles this) | ||
| }, | ||
| }); | ||
| } else { | ||
| // If no new logo and no logo removal, just update the config | ||
| updateConfigMutation.mutate({ | ||
| data: { | ||
| name: values.name, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The logic in this onSubmit function can be simplified. The else if (shouldRemoveLogo) block is redundant, as it performs the exact same action as the final else block. This redundancy can make the code harder to read and maintain.
Since the handleRemoveLogo function already performs an immediate API call to remove the logo, the shouldRemoveLogo state is not needed to stage the removal. The form submission should only be concerned with saving the system name and handling a new logo upload.
I suggest removing the else if block and the shouldRemoveLogo state entirely for a cleaner implementation.
} else {
// If no new logo is being uploaded, just update the config name.
// The logo removal is handled by a separate mutation in `handleRemoveLogo`.
updateConfigMutation.mutate({
data: {
name: values.name,
},
});
}
No description provided.