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-1907] Fixes Beam transactions #88

Merged
merged 1 commit into from
Jul 22, 2024
Merged

[PLA-1907] Fixes Beam transactions #88

merged 1 commit into from
Jul 22, 2024

Conversation

leonardocustodio
Copy link
Member

@leonardocustodio leonardocustodio commented Jul 22, 2024

PR Type

Bug fix, Enhancement


Description

  • Fixed a bug in BatchProcess.php by conditionally appending 'V1010' to the method name in the encoded_data field based on the isRunningLatest() function.
  • Updated composer.json to:
    • Include PHP version ^8.3.
    • Downgrade rebing/graphql-laravel dependency version.
    • Adjust versions for laravel/pint and nunomaduro/collision in require-dev.

Changes walkthrough 📝

Relevant files
Bug fix
BatchProcess.php
Conditional method name modification in transaction encoding

src/Commands/BatchProcess.php

  • Modified the encoded_data field to append 'V1010' to the method name
    if isRunningLatest() returns true.
  • +1/-1     
    Dependencies
    composer.json
    Update dependencies and PHP version requirements                 

    composer.json

  • Updated PHP version requirement to include ^8.3.
  • Downgraded rebing/graphql-laravel dependency version.
  • Adjusted versions for laravel/pint and nunomaduro/collision in
    require-dev.
  • +4/-4     

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

    Copy link

    PR Reviewer Guide 🔍

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

    Conditional Logic
    Ensure that the conditional logic in line 209 correctly appends 'V1010' only when isRunningLatest() returns true. Verify that this does not affect other transaction encodings unexpectedly.

    Copy link

    github-actions bot commented Jul 22, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add checks for the existence of keys in the $param array to prevent errors

    Consider checking the existence of the 'collectionId' and 'recipients' keys in the
    $param array before accessing them. This will prevent potential issues if these keys
    are missing in some cases.

    src/Commands/BatchProcess.php [210-211]

    -'collectionId' => $param['collectionId'],
    -'recipients' => $param['recipients'],
    +'collectionId' => $param['collectionId'] ?? null,
    +'recipients' => $param['recipients'] ?? [],
     
    Suggestion importance[1-10]: 9

    Why: This suggestion is crucial as it prevents potential runtime errors if the 'collectionId' or 'recipients' keys are missing in the $param array, enhancing the robustness of the code.

    9
    Best practice
    Specify a more precise version constraint for the PHP dependency

    Consider specifying a more constrained version range for the "php" dependency to
    avoid potential compatibility issues with future minor versions.

    composer.json [22]

    -"php": "^8.2|^8.3",
    +"php": "^8.2.0, <8.4",
     
    Suggestion importance[1-10]: 8

    Why: Constraining the PHP version range more precisely helps avoid potential compatibility issues with future minor versions, ensuring more stable and predictable behavior.

    8
    Maintainability
    Improve readability by refactoring the ternary operation into a separate variable

    Refactor the ternary operation inside the encode method call to improve readability.
    Extract the conditional logic to a separate variable before the method call.

    src/Commands/BatchProcess.php [209]

    -'encoded_data' => $this->serialize->encode(isRunningLatest() ? $method . 'V1010' : $method, [
    +$encodedMethod = isRunningLatest() ? $method . 'V1010' : $method;
    +'encoded_data' => $this->serialize->encode($encodedMethod, [
     
    Suggestion importance[1-10]: 7

    Why: Refactoring the ternary operation into a separate variable improves code readability and maintainability, making the code easier to understand and modify in the future.

    7
    Enhancement
    Update the version constraint for better compatibility and access to new features

    Update the version constraint for "nunomaduro/collision" to include the latest minor
    versions for better compatibility and features.

    composer.json [76]

    -"nunomaduro/collision": "^8.0",
    +"nunomaduro/collision": "^8.0, <8.2",
     
    Suggestion importance[1-10]: 6

    Why: Updating the version constraint for "nunomaduro/collision" to include the latest minor versions ensures better compatibility and access to new features, though it is a minor enhancement.

    6

    @leonardocustodio leonardocustodio merged commit 085a650 into master Jul 22, 2024
    5 checks passed
    @leonardocustodio leonardocustodio deleted the fix-tx branch July 22, 2024 16:24
    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