Skip to content
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: Setting avatar save prompt error(#2523) #2535

Merged
merged 1 commit into from
Mar 11, 2025
Merged

Conversation

wangdan-fit2cloud
Copy link
Contributor

What this PR does / why we need it?

Summary of your change

Please indicate you've done the following:

  • Made sure tests are passing and test coverage is added if needed.
  • Made sure commit message follow the rule of Conventional Commits specification.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed.

Copy link

f2c-ci-robot bot commented Mar 11, 2025

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link

f2c-ci-robot bot commented Mar 11, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@wangdan-fit2cloud wangdan-fit2cloud merged commit 175a801 into main Mar 11, 2025
4 of 5 checks passed
@wangdan-fit2cloud wangdan-fit2cloud deleted the pr@main/fix-tip branch March 11, 2025 07:55
dialogVisible.value = false
})
} else if (radioType.value === 'custom' && iconFile.value) {
let fd = new FormData()
fd.append('file', iconFile.value.raw)
overviewApi.putAppIcon(id as string, fd, loading).then((res: any) => {
emit('refresh')
MsgSuccess(t('views.applicationOverview.appInfo.EditAvatarDialog.setSuccess'))
MsgSuccess(t('common.saveSuccess'))
dialogVisible.value = false
})
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code you provided has several potential issues that could be addressed:

  1. Lack of Consistent Error Handling: There is no error handling in case something goes wrong during API requests (asyncPutApplication or overviewApi.putAppIcon). This can result in unexpected behavior if the server responds with errors.

  2. Use of Raw File Data: The line fd.append('file', iconFile.value.raw) assumes that iconFile.value.raw already contains the raw data from the file input element, which might not always be reliable depending on the browser's implementation. It would be safer to work directly with the file object without assuming its raw property exists.

  3. Redundant Code Blocks: Both conditions for setting an avatar use similar logic, including appending a form data entry and updating dialogVisible. These blocks could be extracted into a separate function to reduce redundancy.

  4. Variable Naming: Variable names like defaultIcon, radioType, etc., should be more descriptive if possible, especially since they refer to user-facing elements.

Here’s a modified version of the code addressing some of these points:

function saveAvatar() {
    const id = id as string; // Assuming id is defined somewhere in scope

    const uploadImage = async () => {
        try {
            await appApi.uploadAppIcon(id, iconFile.value);
            emit('refresh');
            MsgSuccess(t('common.saveSuccess'));
            dialogVisible.value = false;
        } catch (error) {
            console.error("Failed to set Avatar:", error);
        }
    };

    if (radioType.value === 'default') {
        uploadImage();
    } else if (radioType.value === 'custom' && iconFile.value) {
        uploadImage();
    }
}

// In your main application component, call this function when you want to save the avatar

Key Changes:

  • Function Extraction: The upload logic is now inside the saveAvatar function to prevent duplication.
  • Error Handling: Added a generic error handler using a promise catch block to ensure any failed uploads are logged.
  • Consistent Error Message: Replaced the success message specific to the Avatar Dialog with a common "Save Success" message.

This refactored version improves maintainability and robustness by encapsulating the core functionality in one place and providing better feedback on potential failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants