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-1878] Mutation support for v1010 #57

Merged
merged 4 commits into from
Jul 5, 2024
Merged

[PLA-1878] Mutation support for v1010 #57

merged 4 commits into from
Jul 5, 2024

Conversation

leonardocustodio
Copy link
Member

@leonardocustodio leonardocustodio commented Jul 4, 2024

PR Type

Enhancement, Bug fix, Tests


Description

  • Added Rector configuration file for code quality improvements.
  • Introduced listingData support in GraphQL mutations and adjusted method naming based on version checks.
  • Refactored resolve functions in MarketplaceListingType to use arrow functions.
  • Replaced switch-case with match expression in EagerLoadSelectFields trait and added type hints.
  • Fixed type casting issue in minimum price validation.
  • Added support for CreateListingV1010 in call index keys.
  • Used self::class instead of __CLASS__ in exception messages.
  • Utilized property promotion for marketplaceService in Parser.
  • Added return type hints to anonymous functions in tests.
  • Updated composer.json to support PHP 8.3 and added Rector as a development dependency.

Changes walkthrough 📝

Relevant files
Enhancement
7 files
rector.php
Add Rector configuration for code quality                               

rector.php

  • Added Rector configuration file.
  • Configured paths and rules for Rector.
  • +17/-0   
    CreateListingMutation.php
    Add listingData support and version-based method naming   

    src/GraphQL/Mutations/CreateListingMutation.php

  • Added support for listingData in mutation arguments.
  • Adjusted method name based on version check.
  • Updated encodable parameters to include listingData.
  • +21/-4   
    MarketplaceListingType.php
    Refactor resolve functions to use arrow functions               

    src/GraphQL/Types/MarketplaceListingType.php

    • Refactored resolve functions to use arrow functions.
    +26/-34 
    EagerLoadSelectFields.php
    Refactor switch-case to match expression and add type hints

    src/Models/Laravel/Traits/EagerLoadSelectFields.php

  • Replaced switch-case with match expression.
  • Added return type hints to anonymous functions.
  • +27/-38 
    Encoder.php
    Add support for CreateListingV1010 in call index keys       

    src/Services/Processor/Substrate/Codec/Encoder.php

    • Added support for CreateListingV1010 in call index keys.
    +1/-0     
    MarketplaceSubstrateEvent.php
    Use self::class in exception message                                         

    src/Services/Processor/Substrate/Events/Implementations/MarketplaceSubstrateEvent.php

    • Replaced __CLASS__ with self::class in exception message.
    +1/-1     
    Parser.php
    Use property promotion for marketplaceService                       

    src/Services/Processor/Substrate/Parser.php

    • Used property promotion for marketplaceService.
    +1/-4     
    Bug fix
    1 files
    MinimumPrice.php
    Fix type casting issue in minimum price validation             

    src/Rules/MinimumPrice.php

    • Cast value to string before comparison.
    +1/-1     
    Tests
    1 files
    TestCaseGraphQL.php
    Add return type hints to anonymous functions in tests       

    tests/Feature/GraphQL/TestCaseGraphQL.php

    • Added return type hints to anonymous functions.
    +1/-1     
    Dependencies
    1 files
    composer.json
    Add PHP 8.3 support and Rector dependency                               

    composer.json

  • Added support for PHP 8.3.
  • Added Rector as a development dependency.
  • Updated scripts to include Rector commands.
  • +4/-2     

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

    @leonardocustodio leonardocustodio self-assigned this Jul 4, 2024
    @github-actions github-actions bot added enhancement New feature or request Bug fix tests labels Jul 4, 2024
    Copy link

    github-actions bot commented Jul 4, 2024

    PR Reviewer Guide 🔍

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

    Possible Bug:
    The CreateListingV1010 call index key is duplicated with the same value as CreateListing. This might be an oversight or could cause unexpected behavior if different behaviors are expected for these versions.

    Refactoring Concern:
    The switch to arrow functions in GraphQL resolvers and other parts of the codebase should be thoroughly tested to ensure that the context ($this) and variable scoping behave as expected, especially in closures.

    Data Handling:
    The changes in how auction data is handled based on version checks (isRunningLatest()) need careful review to ensure that data integrity and application logic are maintained across different versions.

    Type Casting:
    The explicit casting of $value to string in MinimumPrice.php during the bccomp comparison should be checked to ensure it handles all expected input formats correctly without data loss or type issues.

    Copy link

    github-actions bot commented Jul 4, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add null checks to lambda functions in resolve to prevent potential errors

    Ensure that the lambda functions used in the resolve fields do not lead to unexpected
    behavior or performance issues, especially considering the use of $listing which might be
    null.

    src/GraphQL/Types/MarketplaceListingType.php [45-48]

    -'resolve' => fn ($listing) => [
    +'resolve' => fn ($listing) => $listing ? [
         'collectionId' => $listing->make_collection_chain_id,
         'tokenId' => $listing->make_token_chain_id,
    -],
    +] : null,
     
    Suggestion importance[1-10]: 9

    Why: Adding null checks in lambda functions is crucial to prevent potential runtime errors, ensuring the code handles edge cases gracefully.

    9
    Maintainability
    Refactor the conditional assignment of extra to a separate method for clarity

    Refactor the conditional structure for setting extra to improve readability and
    maintainability. Consider using a separate method to handle the conditional logic.

    src/GraphQL/Mutations/CreateListingMutation.php [147-155]

    -$extra = isRunningLatest() ? [
    -    'listingData' => $auctionData ? [
    -        'Auction' => $auctionData->toEncodable(),
    -    ] : [
    -        'FixedPrice' => null,
    -    ],
    -] : [
    -    'auctionData' => $auctionData?->toEncodable(),
    -];
    +$extra = $this->prepareExtraData($auctionData);
     
    Suggestion importance[1-10]: 8

    Why: Refactoring the conditional assignment to a separate method enhances readability and maintainability, making the code cleaner and easier to manage.

    8
    Remove or address the TODO comment in the code

    Replace the commented TODO with an actionable implementation or remove it if not needed.
    Leaving TODO comments can lead to confusion and untracked technical debt.

    src/GraphQL/Mutations/CreateListingMutation.php [88]

    -// TODO: We should remove `auctionData` and replace it with `listingData`
    +// Removed the TODO after implementation
     
    Suggestion importance[1-10]: 7

    Why: Removing or addressing TODO comments is important for maintainability, but it is not a critical issue. It helps in reducing technical debt and avoiding confusion.

    7
    Best practice
    Clarify and encapsulate the version checking logic in a method

    Ensure that the method isRunningLatest() is well-defined and its impact on the mutation
    logic is clear, especially in how it affects the encoding of auctionData.

    src/GraphQL/Mutations/CreateListingMutation.php [114]

    -$method = isRunningLatest() ? $this->getMutationName() . 'V1010' : $this->getMutationName();
    +$method = $this->determineMutationMethod();
     
    Suggestion importance[1-10]: 8

    Why: Encapsulating the version checking logic in a method improves code readability and maintainability, making the codebase easier to understand and modify in the future.

    8

    @leonardocustodio leonardocustodio requested review from v16Studios and enjinabner and removed request for v16Studios July 4, 2024 21:49
    @leonardocustodio leonardocustodio merged commit 39e08ee into master Jul 5, 2024
    7 checks passed
    @leonardocustodio leonardocustodio deleted the PLA-1878 branch July 5, 2024 00:54
    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