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-1783] add internal beams query #126

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented Jun 11, 2024

PR Type

Enhancement, Bug fix


Description

  • Added new internal queries for beams and single-use codes with claimConditions.
  • Removed internal parameter from existing queries and adjusted logic accordingly.
  • Refactored button elements in BeamsList.vue for better readability.

Changes walkthrough 📝

Relevant files
Enhancement
9 files
BeamsList.vue
Refactor button elements for readability.                               

resources/js/components/beam/BeamsList.vue

  • Reformatted button elements for better readability.
+10/-2   
beam.ts
Add conditional queries for internal beams.                           

resources/js/api/beam.ts

  • Added conditional queries for internal beams.
  • Removed redundant internal variable from query variables.
  • +3/-5     
    queries.ts
    Import internal beam queries.                                                       

    resources/js/api/queries.ts

    • Imported new internal beam queries.
    +6/-0     
    GetBeam.ts
    Simplify GetBeam query by removing internal conditions.   

    resources/js/graphql/query/beam/GetBeam.ts

    • Removed internal parameter and claimConditions field.
    +1/-5     
    GetBeamInternal.ts
    Add GetBeamInternal query with claim conditions.                 

    resources/js/graphql/query/beam/GetBeamInternal.ts

    • Added new query for internal beams with claimConditions.
    +24/-0   
    GetBeams.ts
    Simplify GetBeams query by removing internal conditions. 

    resources/js/graphql/query/beam/GetBeams.ts

    • Removed internal parameter and claimConditions field.
    +1/-5     
    GetBeamsInternal.ts
    Add GetBeamsInternal query with claim conditions.               

    resources/js/graphql/query/beam/GetBeamsInternal.ts

    • Added new query for internal beams with claimConditions.
    +33/-0   
    GetSingleUseCodes.ts
    Simplify GetSingleUseCodes query by removing internal conditions.

    resources/js/graphql/query/beam/GetSingleUseCodes.ts

    • Removed internal parameter and claimConditions field.
    +1/-5     
    GetSingleUseCodesInternal.ts
    Add GetSingleUseCodesInternal query with claim conditions.

    resources/js/graphql/query/beam/GetSingleUseCodesInternal.ts

  • Added new query for internal single-use codes with claimConditions.
  • +47/-0   

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

    @zlayine zlayine requested a review from enjinabner June 11, 2024 01:38
    @github-actions github-actions bot added enhancement New feature or request Bug fix labels Jun 11, 2024
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5]

    3

    🧪 Relevant tests

    No

    🔒 Security concerns

    No

    ⚡ Key issues to review

    Possible Bug:
    The removal of the internal parameter in API calls without corresponding updates in the backend could lead to incorrect data retrieval or errors. Ensure backend compatibility or provide a fallback mechanism.

    Refactoring Scope:
    The refactoring in BeamsList.vue for button elements should be reviewed to ensure it does not affect functionality, especially with event bindings and dynamic classes.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add error handling to asynchronous methods to manage exceptions

    Consider adding error handling for the asynchronous operations in the getBeam, getBeams,
    and getSingleUseCodes methods to manage exceptions and provide feedback to the user or
    system in case of failure.

    resources/js/api/beam.ts [10-17]

     static async getBeam(getBeamData: Record<string, unknown>) {
    -    const data = {
    -        query: getBeamData.internal ? queries.GetBeamInternal : queries.GetBeam,
    -        variables: {
    -            code: getBeamData.code,
    -            account: getBeamData.account,
    -        },
    -    };
    +    try {
    +        const data = {
    +            query: getBeamData.internal ? queries.GetBeamInternal : queries.GetBeam,
    +            variables: {
    +                code: getBeamData.code,
    +                account: getBeamData.account,
    +            },
    +        };
    +        // Perform API call here
    +    } catch (error) {
    +        console.error("Failed to get beam data:", error);
    +        throw new Error("Error fetching beam data");
    +    }
     
    Suggestion importance[1-10]: 9

    Why: Adding error handling is crucial for managing exceptions and providing feedback, which improves the robustness and reliability of the application. This suggestion addresses a significant issue.

    9
    Possible issue
    Add checks to handle empty selections before performing actions

    Ensure that the expireSelectedBeams and exportSelectedBeams methods handle cases where
    selectedBeams is empty to prevent unnecessary operations or errors.

    resources/js/components/beam/BeamsList.vue [61-71]

     <Btn
         dusk="expireAllBtn"
         class="!px-2 !py-1 text-sm animate-fade-in"
    -    @click="expireSelectedBeams"
    +    @click="selectedBeams.length > 0 ? expireSelectedBeams() : null"
         >{{ `Expire ${selectedBeams.length > 1 ? 'All' : ''} ` }}
     </Btn>
     <Btn
         dusk="exportBtn"
         class="!px-2 !py-1 text-sm animate-fade-in"
    -    @click="exportSelectedBeams"
    +    @click="selectedBeams.length > 0 ? exportSelectedBeams() : null"
         >Export</Btn
     >
     
    Suggestion importance[1-10]: 8

    Why: Adding checks to handle empty selections before performing actions is important to prevent unnecessary operations and potential errors, enhancing the user experience and reliability of the application.

    8
    Maintainability
    Rename GraphQL queries to match their specific internal or external usage

    Consider using more descriptive names for GraphQL queries to avoid confusion with
    similarly named queries and improve maintainability.

    resources/js/graphql/query/beam/GetBeamInternal.ts [1-24]

    -export default `query GetBeam($code: String!, $account: String) {
    -    GetBeam(code: $code, account: $account) {
    +export default `query GetBeamInternal($code: String!, $account: String) {
    +    GetBeamInternal(code: $code, account: $account) {
             id
             code
             name
             description
             image
             start
             end
             collection {
                 collectionId
             }
             isClaimable
             flags
             qr {
                 url
                 payload
             }
             claimConditions {
                 type
                 value
             }
         }
     }`;
     
    Suggestion importance[1-10]: 7

    Why: Using more descriptive names for GraphQL queries can help avoid confusion and improve maintainability, but it is not as critical as error handling or preventing unnecessary operations.

    7
    Refactor repeated query data construction into a reusable function

    Refactor the repeated code pattern for constructing query data in getBeam, getBeams, and
    getSingleUseCodes methods into a reusable function to reduce code duplication and improve
    maintainability.

    resources/js/api/beam.ts [12-16]

    -const data = {
    -    query: getBeamData.internal ? queries.GetBeamInternal : queries.GetBeam,
    -    variables: {
    -        code: getBeamData.code,
    -        account: getBeamData.account,
    -    },
    -};
    +const data = createQueryData(getBeamData, 'Beam');
    +...
    +function createQueryData(data, type) {
    +    return {
    +        query: data.internal ? queries[`Get${type}Internal`] : queries[`Get${type}`],
    +        variables: {
    +            code: data.code,
    +            account: data.account,
    +        },
    +    };
    +}
     
    Suggestion importance[1-10]: 7

    Why: Refactoring repeated code into a reusable function improves maintainability and reduces code duplication, which is beneficial for long-term code management, though it is not as urgent as error handling.

    7

    @enjinabner
    Copy link
    Contributor

    Screenshot 2024-06-11 at 10 03 41 AM

    I was using your branch but still got this error

    Screenshot 2024-06-11 at 10 04 43 AM

    @enjinabner enjinabner merged commit 3b1aef7 into master Jun 26, 2024
    3 checks passed
    @enjinabner enjinabner deleted the bugfix/pla-1783/beam-claim-conditions-internal branch June 26, 2024 03:37
    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