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-1778] Add singleUseCodes filter to GetClaims query. #71

Merged
merged 3 commits into from
May 30, 2024

Conversation

v16Studios
Copy link
Contributor

@v16Studios v16Studios commented May 27, 2024

PR Type

Enhancement, Tests


Description

  • Added singleUseCodes argument to GetClaims query and implemented filtering logic.
  • Created HasSingleUseCodeScope trait for filtering by single use code and integrated it into the BeamClaim model.
  • Added translation for single_use_code argument in claim_beam.
  • Added test for singleUseCodes filter in GetClaims query.
  • Added fix script for PHP CS Fixer in composer.json.
  • Added singleUseCodes argument to GetClaims GraphQL query.

Changes walkthrough 📝

Relevant files
Enhancement
mutation.php
Add translation for single use code argument.                       

lang/en/mutation.php

  • Added translation for single_use_code argument in claim_beam.
+1/-0     
GetClaimsQuery.php
Add singleUseCodes filter to GetClaims query.                       

src/GraphQL/Queries/GetClaimsQuery.php

  • Added singleUseCodes argument to GetClaims query.
  • Implemented filtering logic for singleUseCodes.
  • +6/-0     
    BeamClaim.php
    Integrate HasSingleUseCodeScope trait in BeamClaim model.

    src/Models/Laravel/BeamClaim.php

    • Added HasSingleUseCodeScope trait to BeamClaim model.
    +6/-5     
    HasSingleUseCodeScope.php
    Create HasSingleUseCodeScope trait for single use code filtering.

    src/Models/Laravel/Traits/HasSingleUseCodeScope.php

  • Created HasSingleUseCodeScope trait for filtering by single use code.
  • +29/-0   
    composer.json
    Add fix script for PHP CS Fixer.                                                 

    composer.json

    • Added fix script for PHP CS Fixer.
    +1/-0     
    GetClaims.graphql
    Add singleUseCodes argument to GetClaims GraphQL query.   

    tests/Feature/GraphQL/Resources/GetClaims.graphql

    • Added singleUseCodes argument to GetClaims GraphQL query.
    +2/-0     
    Tests
    GetClaimsTest.php
    Add test for singleUseCodes filter in GetClaims query.     

    tests/Feature/GraphQL/Queries/GetClaimsTest.php

    • Added test for singleUseCodes filter in GetClaims query.
    +9/-0     

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

    @github-actions github-actions bot added enhancement New feature or request tests labels May 27, 2024
    Copy link

    PR Description updated to latest commit (9650bdd)

    Copy link

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the PR involves a moderate amount of changes across multiple files, including backend logic, GraphQL schema updates, and tests. The changes are straightforward and well-documented, which simplifies the review process.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The test_it_can_get_claims_with_single_use_codes in GetClaimsTest.php seems to use the wrong parameter. It uses codes instead of singleUseCodes, which might not test the new functionality correctly.

    🔒 Security concerns

    No

    Code feedback:
    relevant filetests/Feature/GraphQL/Queries/GetClaimsTest.php
    suggestion      

    Update the test case test_it_can_get_claims_with_single_use_codes to use the singleUseCodes parameter instead of codes. This change ensures that the test accurately checks the functionality of filtering claims by single use codes. [important]

    relevant line$response = $this->graphql($this->method, ['codes' => [$this->beam->claims[0]->singleUseCode]]);

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Improve error handling by logging decryption failures

    Consider handling the case where decryption fails more explicitly. Currently, if
    decryption fails, the query simply returns without any specific action or logging. It
    might be beneficial to log these exceptions or handle them in a way that does not silently
    fail.

    src/Models/Laravel/Traits/HasSingleUseCodeScope.php [15-27]

     try {
         if (is_array($code)) {
             $singleUseCode = array_map(function ($item) {
                 return explode(':', decrypt($item))[0];
             }, $code);
         } else {
             $singleUseCode = explode(':', decrypt($code))[0];
         }
     
         return $query->whereIn('code', Arr::wrap($singleUseCode));
     } catch (\Throwable $exception) {
    +    Log::error("Decryption failed: " . $exception->getMessage());
         return $query;
     }
     
    Suggestion importance[1-10]: 9

    Why: This suggestion improves error handling by logging decryption failures, which is crucial for debugging and maintaining the robustness of the application. It addresses a significant issue without altering the core functionality.

    9
    Enhance the test to verify that the correct claims are returned for 'singleUseCodes'

    Add assertions to check for the correct handling of 'singleUseCodes' in the response,
    ensuring that the claims returned actually match the single use codes provided in the
    test.

    tests/Feature/GraphQL/Queries/GetClaimsTest.php [57-61]

     public function test_it_can_get_claims_with_single_use_codes(): void
     {
    -    $response = $this->graphql($this->method, ['codes' => [$this->beam->claims[0]->singleUseCode]]);
    +    $response = $this->graphql($this->method, ['singleUseCodes' => [$this->beam->claims[0]->singleUseCode]]);
         $this->assertNotEmpty($response['totalCount']);
    +    $this->assertEquals($this->beam->claims[0]->singleUseCode, $response['claims'][0]['singleUseCode']);
     }
     
    Suggestion importance[1-10]: 7

    Why: Adding assertions to check for the correct handling of 'singleUseCodes' ensures the accuracy and reliability of the test. This is a useful enhancement for verifying that the implementation works as intended.

    7
    Possible issue
    Add validation rules to handle edge cases for 'singleUseCodes'

    Ensure that the 'singleUseCodes' filter is properly tested for edge cases such as empty
    arrays, null values, and invalid formats. The current implementation may not handle these
    cases, which could lead to unexpected behavior or errors.

    src/GraphQL/Queries/GetClaimsQuery.php [59-62]

     'singleUseCodes' => [
         'type' => GraphQL::type('[String]'),
         'description' => __('enjin-platform-beam::mutation.claim_beam.args.single_use_code'),
    -    'rules' => ['prohibits:ids'],
    +    'rules' => ['prohibits:ids', 'nullable', 'array'],
     ],
     
    Suggestion importance[1-10]: 8

    Why: Adding validation rules for edge cases such as empty arrays, null values, and invalid formats enhances the reliability and stability of the code. This is important for preventing unexpected behavior and errors.

    8
    Maintainability
    Reorder trait usage for better organization and readability

    Reorder the use of traits in the BeamClaim class to maintain consistency and readability.
    Group related traits together and separate them from unrelated ones.

    src/Models/Laravel/BeamClaim.php [29-37]

    -use HasEagerLimit;
     use HasFactory;
    -use HasSingleUseCodeScope;
    -use MassPrunable;
     use SoftDeletes;
    -use Traits\EagerLoadSelectFields;
     use Traits\HasBeamQr;
     use Traits\HasClaimable;
     use Traits\HasCodeScope;
    +use HasSingleUseCodeScope;
    +use MassPrunable;
    +use Traits\EagerLoadSelectFields;
    +use HasEagerLimit;
     
    Suggestion importance[1-10]: 5

    Why: Reordering traits improves code readability and maintainability, but it does not affect the functionality. This is a minor improvement focused on code organization.

    5

    'singleUseCodes' => [
    'type' => GraphQL::type('[String]'),
    'description' => __('enjin-platform-beam::mutation.claim_beam.args.single_use_code'),
    'rules' => ['prohibits:ids'],
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    We need to add string limit here just like the code validation, also we need to limit the array

    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This query has been overridden in the internal platform, you need to update the code there as well.

    @v16Studios v16Studios merged commit 91347ae into master May 30, 2024
    7 checks passed
    @v16Studios v16Studios deleted the update/pla-1778/filter-single-use-codes branch May 30, 2024 13:05
    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