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-1896] Fix validation create beam on mint on demand #86

Merged

Conversation

enjinabner
Copy link
Contributor

@enjinabner enjinabner commented Jul 17, 2024

PR Type

Bug fix, Tests


Description

  • Removed validation rules TokensDoNotExistInCollection and TokenUploadNotExistInCollection from CreateBeamMutation.
  • Updated conditional logic to handle cases where these validation rules were previously applied.
  • Removed corresponding tests for token existence in collection validation from CreateBeamTest.
  • Simplified the test for invalid file upload in CreateBeamTest.

Changes walkthrough 📝

Relevant files
Bug fix
CreateBeamMutation.php
Remove redundant token existence validation rules               

src/GraphQL/Mutations/CreateBeamMutation.php

  • Removed validation rules TokensDoNotExistInCollection and
    TokenUploadNotExistInCollection.
  • Updated conditional logic to handle empty validation rules.
  • +2/-4     
    Tests
    CreateBeamTest.php
    Remove tests for redundant token existence validation       

    tests/Feature/GraphQL/Mutations/CreateBeamTest.php

  • Removed tests related to token existence in collection validation.
  • Simplified test for invalid file upload.
  • +0/-38   

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

    @enjinabner enjinabner added the bug Something isn't working label Jul 17, 2024
    @enjinabner enjinabner self-assigned this Jul 17, 2024
    Copy link

    PR Reviewer Guide 🔍

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

    Logic Change
    The conditional logic in lines 132 and 143 has been changed to return an empty string instead of a validation rule. This might lead to unexpected behavior if the system expects a validation rule object instead of an empty string. It's important to ensure that the system can handle this new return type without issues.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Replace the empty string with an appropriate validation rule

    Replace the empty string with a validation rule or remove the conditional operator
    if no alternative rule is required. An empty string as a validation rule can lead to
    unexpected behavior or bugs.

    src/GraphQL/Mutations/CreateBeamMutation.php [133]

    -: '',
    +: new SomeValidationRule($args['collectionId']),
     
    Suggestion importance[1-10]: 9

    Why: Replacing the empty string with a proper validation rule is crucial to avoid unexpected behavior or bugs. This suggestion addresses a potential bug effectively.

    9
    Possible issue
    Ensure that the removal of certain validation rules does not compromise functionality

    Consider adding a validation rule or handling for the removed
    TokensDoNotExistInCollection and TokenUploadNotExistInCollection to ensure that the
    removal does not affect the intended functionality.

    src/GraphQL/Mutations/CreateBeamMutation.php [12-13]

     use Enjin\Platform\Beam\Rules\TokensExistInCollection;
    +use Enjin\Platform\Beam\Rules\TokensDoNotExistInCollection;
     use Enjin\Platform\Beam\Rules\TokenUploadExistInCollection;
    +use Enjin\Platform\Beam\Rules\TokenUploadNotExistInCollection;
     
    Suggestion importance[1-10]: 8

    Why: Adding back the removed validation rules or ensuring their removal does not affect functionality is important for maintaining the integrity of the code. This suggestion addresses a possible issue.

    8
    Enhancement
    Ensure conditional validation logic is complete and aligns with requirements

    Ensure that the conditional validation logic aligns with the business requirements,
    especially after removing specific rules like TokensDoNotExistInCollection.

    src/GraphQL/Mutations/CreateBeamMutation.php [132-133]

     BeamType::getEnumCase(Arr::get($args, str_replace('tokenIds', 'type', $attribute))) == BeamType::TRANSFER_TOKEN
         ? new TokensExistInCollection($args['collectionId'])
    -    : '',
    +    : new DefaultValidationRule($args['collectionId']),
     
    Suggestion importance[1-10]: 7

    Why: Ensuring that the conditional validation logic is complete and aligns with business requirements is important for the correctness of the code. This suggestion provides a reasonable enhancement.

    7

    @leonardocustodio leonardocustodio merged commit 05c3a33 into master Jul 17, 2024
    5 checks passed
    @leonardocustodio leonardocustodio deleted the feature/pla-1898/remove-token-exist-validation branch July 17, 2024 12:10
    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