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-1979] Fix mutate fuel tank #61

Merged

Conversation

enjinabner
Copy link
Contributor

@enjinabner enjinabner commented Sep 6, 2024

PR Type

Bug fix


Description

  • Fixed an encoding issue in the MutateFuelTankMutation class, specifically in the getEncodableParams method.
  • Adjusted the logic for determining the coveragePolicy and providesDeposit parameters based on the isRunningLatest condition.
  • Improved the conditional structure for parameter assignment to ensure correct encoding.

Changes walkthrough 📝

Relevant files
Bug fix
MutateFuelTankMutation.php
Fix encoding issue in mutate fuel tank mutation                   

src/GraphQL/Mutations/MutateFuelTankMutation.php

  • Fixed encoding issue in getEncodableParams method.
  • Adjusted logic for coveragePolicy and providesDeposit parameters.
  • Updated conditional structure for parameter assignment.
  • +3/-1     

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

    Copy link

    github-actions bot commented Sep 6, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug
    The spread operator is used to merge arrays, but the conditional structure might introduce keys dynamically, which could lead to unexpected behavior if isRunningLatest() changes during runtime or if there are other side effects. It's recommended to ensure that the keys and their values are managed consistently.

    Copy link

    github-actions bot commented Sep 6, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation for the $providesDeposit variable to ensure reliable mutation logic

    Ensure that the $providesDeposit variable is properly validated or sanitized before
    use, as it directly influences the mutation logic and might be susceptible to
    unexpected values affecting the logic flow.

    src/GraphQL/Mutations/MutateFuelTankMutation.php [116-118]

     ...(isRunningLatest() ?
    -    ['coveragePolicy' => $providesDeposit ? 'Fees' : 'FeesAndDeposit'] :
    -    ['providesDeposit' => $providesDeposit]),
    +    ['coveragePolicy' => isset($providesDeposit) && $providesDeposit ? 'Fees' : 'FeesAndDeposit'] :
    +    ['providesDeposit' => isset($providesDeposit) ? $providesDeposit : false]),
     
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a potential issue with the $providesDeposit variable not being validated, which could lead to unexpected behavior. Adding validation improves the reliability of the mutation logic.

    8
    Best practice
    Refactor mutation parameter determination into a separate method for better modularity

    Refactor the spread operator usage to a separate method to encapsulate the logic for
    determining the mutation parameters based on the version, enhancing modularity and
    testability.

    src/GraphQL/Mutations/MutateFuelTankMutation.php [116-118]

    -...(isRunningLatest() ?
    -    ['coveragePolicy' => $providesDeposit ? 'Fees' : 'FeesAndDeposit'] :
    -    ['providesDeposit' => $providesDeposit]),
    +...$this->determineMutationParameters($providesDeposit)
     
    Suggestion importance[1-10]: 7

    Why: Refactoring the logic into a separate method enhances modularity and testability, which is a good practice for maintaining clean and manageable code.

    7
    Enhancement
    Enhance the clarity of 'coveragePolicy' values by using more descriptive terms

    Consider using a more descriptive key than 'Fees' and 'FeesAndDeposit' to clarify
    the intent of these values in the context of 'coveragePolicy'.

    src/GraphQL/Mutations/MutateFuelTankMutation.php [117]

    -['coveragePolicy' => $providesDeposit ? 'Fees' : 'FeesAndDeposit']
    +['coveragePolicy' => $providesDeposit ? 'BasicFees' : 'FullCoverageIncludingDeposit']
     
    Suggestion importance[1-10]: 6

    Why: The suggestion improves code clarity by using more descriptive terms for 'coveragePolicy' values, which can help future developers understand the intent of these values better.

    6

    @enjinabner enjinabner merged commit a791d63 into master Sep 6, 2024
    7 checks passed
    @enjinabner enjinabner deleted the feature/pla-1979/mutate-fuel-tank-encoding-issue branch September 6, 2024 12:09
    enjinabner added a commit that referenced this pull request Sep 6, 2024
    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