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

[PLA-1740] bad request 400 error handling #102

Merged
merged 2 commits into from
Apr 26, 2024

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented Apr 26, 2024

Type

enhancement, bug_fix


Description

  • Enhanced error handling and user feedback mechanisms in password reset functionalities.
  • Refactored API error handling to provide clearer and more specific error messages.
  • Simplified the RequestPasswordReset mutation by removing unnecessary parameters.
  • Improved code readability and structure across various components and store configurations.

Changes walkthrough

Relevant files
Enhancement
RequestResetPassword.vue
Enhance Error Handling and Feedback in Password Reset       

resources/js/components/pages/auth/RequestResetPassword.vue

  • Improved error handling for excessive requests during password reset.
  • Added user feedback for successful password reset request initiation.
  • +13/-7   
    index.ts
    Refactor API Error Handling and Improve Response Management

    resources/js/api/index.ts

  • Refactored error handling for API responses to include more specific
    error messages.
  • Improved handling of 204 and 401 HTTP status codes.
  • +33/-23 
    RequestPasswordReset.ts
    Simplify RequestPasswordReset Mutation by Removing Recaptcha

    resources/js/graphql/mutation/auth/RequestPasswordReset.ts

  • Removed the recaptcha parameter from the RequestPasswordReset
    mutation.
  • +2/-2     
    index.ts
    Code Refactoring and Optional Chaining in App Store           

    resources/js/store/index.ts

  • Updated import order and added optional chaining for network
    configuration.
  • +5/-5     
    Settings.vue
    Refactor and Enhance Readability of Settings Page Components

    resources/js/components/pages/Settings.vue

  • Improved readability and structure of Vue components in settings page.

  • +9/-2     
    SettingsResetPassword.vue
    Improve Form Formatting in Settings Reset Password Page   

    resources/js/components/pages/SettingsResetPassword.vue

    • Reformatted the password reset form for better readability.
    +7/-1     
    Collections.vue
    Improve HTML Table Layout Readability in Collections Page

    resources/js/components/pages/Collections.vue

    • Enhanced readability by restructuring the HTML table layout.
    +5/-1     
    Formatting
    BatchMint.vue
    Minor Formatting Adjustment in Batch Mint Component           

    resources/js/components/batch/BatchMint.vue

    • Minor formatting adjustment for the Batch Mint button.
    +3/-1     

    PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @zlayine zlayine self-assigned this Apr 26, 2024
    @zlayine zlayine removed the request for review from Bradez April 26, 2024 07:22
    @github-actions github-actions bot added enhancement New feature or request bug_fix labels Apr 26, 2024
    Copy link

    PR Description updated to latest commit (b93ef67)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files with changes related to both front-end components and API error handling logic. The changes are moderate in complexity, involving error handling, UI updates, and API response management.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The error handling in resources/js/api/index.ts might not correctly handle non-JSON responses leading to unhandled promise rejections.

    Error Handling Consistency: Different error handling strategies in various parts of the application could lead to inconsistent user experiences.

    🔒 Security concerns

    No

    Code feedback:
    relevant fileresources/js/api/index.ts
    suggestion      

    Consider adding a check for JSON content type in responses before attempting to parse them as JSON to avoid errors in cases where the response is not in JSON format. This is important to prevent runtime errors. [important]

    relevant linethrow resp;

    relevant fileresources/js/components/pages/auth/RequestResetPassword.vue
    suggestion      

    It's recommended to abstract the error handling logic into a separate function or use a more generic approach to handle different types of errors. This can improve code modularity and readability. [medium]

    relevant lineif (e.message.includes('Too many requests')) {

    relevant fileresources/js/components/pages/auth/ResetPassword.vue
    suggestion      

    Refactor the error handling to ensure that all potential errors are managed consistently, possibly using a centralized error handling mechanism. This could enhance maintainability and consistency across different modules. [important]

    relevant lineif (snackbarErrors(e)) {

    relevant fileresources/js/graphql/mutation/auth/RequestPasswordReset.ts
    suggestion      

    Ensure that removing the recaptcha parameter from the mutation does not impact security, especially for operations like password reset that are sensitive to brute force attacks. [important]

    relevant lineexport default `mutation RequestPasswordReset($email: String!) {


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Best practice
    Improve error handling by using error types or status codes instead of message string checking.

    Instead of using a broad catch block that checks for a specific error message, consider
    implementing a more robust error handling strategy that categorizes errors based on HTTP
    status codes or error types. This will make the error handling more predictable and
    maintainable.

    resources/js/components/pages/auth/RequestResetPassword.vue [135-139]

    -if (e.message.includes('Too many requests')) {
    +if (e instanceof TooManyRequestsError) {
         snackbar.error({
    -        title: 'Error',
    -        text: e.message,
    +        title: 'Too Many Requests',
    +        text: 'You have reached the limit for password reset requests. Please try again later.',
         });
     }
     
    Refactor error handling into a dedicated function to improve code reuse and clarity.

    Refactor the error handling to use a dedicated function for checking and handling
    different types of errors, improving code reuse and clarity.

    resources/js/components/pages/SettingsResetPassword.vue [107-110]

    -if (snackbarErrors(e)) {
    -    return;
    +if (!handleErrors(e)) {
    +    snackbar.error({ title: 'Error resetting password.' });
     }
    -snackbar.error({ title: 'Error resetting password.' });
     
    Enhancement
    Replace throwing raw response with throwing a custom error class for better error handling.

    Use a more specific error type or status code check instead of throwing the raw response
    object. This will help in handling errors more gracefully and provide clearer error
    messages to the user.

    resources/js/api/index.ts [82-83]

     if (resp.status >= 400 && resp.status < 600) {
    -    throw resp;
    +    throw new ApiError('Server responded with an error', resp.status);
     }
     
    Maintainability
    Improve the readability of the button component by properly formatting the closing tags.

    Ensure that the button text and actions are clearly separated in the template for better
    readability and maintainability.

    resources/js/components/pages/Settings.vue [64-66]

    -<Btn dusk="deleteBtn" v-if="appStore.isMultiTenant" @click="confirmModal = true"
    -    >Delete account</Btn
    ->
    +<Btn dusk="deleteBtn" v-if="appStore.isMultiTenant" @click="confirmModal = true">
    +    Delete account
    +</Btn>
     
    Bug
    Ensure the table is rendered only when there are items by checking the array length.

    Ensure that the table is only rendered when there are items in the collection by checking
    the length properly before rendering the table.

    resources/js/components/pages/Collections.vue [28-31]

     <table
         id="collectionsTable"
         class="min-w-full divide-y divide-gray-300"
    -    v-if="collections.items?.length"
    +    v-if="collections.items && collections.items.length > 0"
     >
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @zlayine zlayine merged commit efa4f31 into master Apr 26, 2024
    4 checks passed
    @zlayine zlayine deleted the feature/pla-1740/request-400 branch April 26, 2024 10:16
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    2 participants