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

Update .gitignore and .graphqlrc.ts for admin-2024-04 schema, adjust … #159

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

jamalsoueidan
Copy link
Owner

@jamalsoueidan jamalsoueidan commented Jun 4, 2024

User description

…updatePrice for location-based shipping


PR Type

enhancement, configuration changes


Description

  • Updated the GraphQL schema URL and API version to 2024-04 in .graphqlrc.ts.
  • Enhanced the updatePrice function to include location-based shipping logic by importing LocationTypes and checking if the product's location type is DESTINATION.

Changes walkthrough 📝

Relevant files
Configuration changes
.graphqlrc.ts
Update GraphQL schema and API version to 2024-04.               

.graphqlrc.ts

  • Updated the GraphQL schema URL to the 2024-04 version.
  • Changed the API version to 2024-04.
  • +2/-2     
    Enhancement
    update-price.ts
    Enhance `updatePrice` function with location-based shipping logic.

    src/functions/customer/orchestrations/product/update/update-price.ts

  • Imported LocationTypes from the location module.
  • Added location-based logic to determine if a product requires
    shipping.
  • Adjusted the updatePrice function to handle location-based shipping
    requirements.
  • +17/-4   

    💡 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 configuration changes labels Jun 4, 2024
    Copy link

    github-actions bot commented Jun 4, 2024

    PR Description updated to latest commit (e7faa7c)

    Copy link

    github-actions bot commented Jun 4, 2024

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the PR involves straightforward changes in two files with clear intentions and limited complexity. The changes are mostly updates to configurations and enhancements to an existing function.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The logic for setting requiresShipping to false when isDestination is true might be incorrect. Typically, if a product is shipped to a destination, it should require shipping rather than not.

    🔒 Security concerns

    No

    Code feedback:
    relevant filesrc/functions/customer/orchestrations/product/update/update-price.ts
    suggestion      

    Consider revising the logic for requiresShipping. If isDestination is true, it generally implies that the item should require shipping. This might be a logical error. [important]

    relevant linerequiresShipping: false,

    relevant filesrc/functions/customer/orchestrations/product/update/update-price.ts
    suggestion      

    Add error handling for the asynchronous calls within updatePrice. For instance, wrap the await CustomerProductServiceGet and await shopifyAdmin().request calls in a try-catch block to handle potential exceptions and provide a fallback or error message. [important]

    relevant lineawait CustomerProductServiceGet({

    relevant filesrc/functions/customer/orchestrations/product/update/update-price.ts
    suggestion      

    Consider adding a check to ensure locations is not undefined before calling locations.some(...). This will prevent runtime errors if locations is undefined. [medium]

    relevant lineconst isDestination = locations.some(

    relevant filesrc/functions/customer/orchestrations/product/update/update-price.ts
    suggestion      

    Optimize the destructuring of price and compareAtPrice to directly access the amount property in the initial destructuring step, which can make the code cleaner and reduce redundancy. [medium]

    relevant lineconst { variantId, price, compareAtPrice, locations } =

    Copy link

    github-actions bot commented Jun 4, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add error handling for the asynchronous operation to improve robustness

    It's recommended to handle potential errors from asynchronous operations to avoid
    unhandled promise rejections. Consider wrapping the CustomerProductServiceGet call in a
    try-catch block.

    src/functions/customer/orchestrations/product/update/update-price.ts [13-17]

    -const { variantId, price, compareAtPrice, locations } =
    -  await CustomerProductServiceGet({
    +let variantId, price, compareAtPrice, locations;
    +try {
    +  const response = await CustomerProductServiceGet({
         customerId,
         productId,
       });
    +  variantId = response.variantId;
    +  price = response.price;
    +  compareAtPrice = response.compareAtPrice;
    +  locations = response.locations;
    +} catch (error) {
    +  console.error("Failed to fetch product details:", error);
    +  // Handle error appropriately
    +}
     
    Suggestion importance[1-10]: 10

    Why: Adding error handling for the asynchronous operation is crucial for preventing unhandled promise rejections and ensuring the robustness of the code. This addresses a potential bug and is highly important.

    10
    Maintainability
    Use a constant for the API version to ensure consistency across the configuration

    Consider using a constant for the API version to ensure consistency and maintainability.
    This will help avoid manual errors when updating the version in multiple places.

    .graphqlrc.ts [4-9]

    -schema: "https://shopify.dev/admin-graphql-direct-proxy/2024-04",
    -apiVersion: "2024-04",
    +const API_VERSION = "2024-04";
    +schema: `https://shopify.dev/admin-graphql-direct-proxy/${API_VERSION}`,
    +apiVersion: API_VERSION,
     
    Suggestion importance[1-10]: 8

    Why: Using a constant for the API version enhances maintainability and reduces the risk of errors when updating the version in multiple places. This is a significant improvement for code consistency.

    8
    Best practice
    Improve variable naming for clarity and maintainability

    Consider using a more descriptive variable name than l for better code readability and
    maintainability.

    src/functions/customer/orchestrations/product/update/update-price.ts [19-20]

     const isDestination = locations.some(
    -  (l) => l.locationType === LocationTypes.DESTINATION
    +  (location) => location.locationType === LocationTypes.DESTINATION
     );
     
    Suggestion importance[1-10]: 7

    Why: Using a more descriptive variable name improves code readability and maintainability. This is a best practice that enhances the clarity of the code, though it is a minor improvement.

    7
    Enhancement
    Use direct destructuring for price and compareAtPrice to enhance code clarity

    To improve code readability and maintainability, consider destructuring the price and
    compareAtPrice objects directly in the function parameter.

    src/functions/customer/orchestrations/product/update/update-price.ts [29-30]

    -price: price.amount,
    -compareAtPrice: compareAtPrice.amount,
    +price: { amount: priceAmount },
    +compareAtPrice: { amount: compareAtPriceAmount },
     
    Suggestion importance[1-10]: 6

    Why: While destructuring can improve readability, the current suggestion does not align perfectly with the existing code structure and might introduce unnecessary complexity. It is a minor enhancement.

    6

    @jamalsoueidan jamalsoueidan merged commit 1977ab2 into main Jun 4, 2024
    3 checks passed
    @jamalsoueidan jamalsoueidan deleted the update-api-version branch June 4, 2024 02:45
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant