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

Add durable client input and orchestration for customer location upd… #160

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

jamalsoueidan
Copy link
Owner

@jamalsoueidan jamalsoueidan commented Jun 5, 2024

User description

…ates

  • Include df.input.durableClient() in customerLocationUpdate HTTP trigger
  • Introduce InvocationContext and orchestration call in CustomerLocationControllerUpdate
  • Implement new orchestration CustomerLocationUpdateOrchestration for location updates
  • Add geo_location field handling in createLocationMetafield and corresponding tests
  • Create new activity and orchestration for updating location metafields
  • Refactor CustomerLocationServiceUpdate to remove direct Shopify metafield update logic

PR Type

Enhancement, Tests


Description

  • Added df.input.durableClient() to customerLocationUpdate HTTP trigger.
  • Introduced InvocationContext and orchestration call in CustomerLocationControllerUpdate.
  • Implemented new orchestration CustomerLocationUpdateOrchestration for location updates.
  • Added geo_location field handling in createLocationMetafield and corresponding tests.
  • Created new activity and orchestration for updating location metafields.
  • Refactored CustomerLocationServiceUpdate to remove direct Shopify metafield update logic.

Changes walkthrough 📝

Relevant files
Enhancement
6 files
customer-location.function.ts
Add durable client input to HTTP trigger                                 

src/functions/customer-location.function.ts

  • Added df.input.durableClient() to customerLocationUpdate HTTP trigger.

  • +1/-0     
    update.ts
    Add orchestration call and context to controller                 

    src/functions/customer/controllers/location/update.ts

  • Introduced InvocationContext to the request type.
  • Added orchestration call to CustomerLocationControllerUpdate.
  • +15/-3   
    create-location-metafield.ts
    Handle geo_location field in location metafield creation 

    src/functions/customer/orchestrations/location/create/create-location-metafield.ts

    • Added geo_location field handling in createLocationMetafield.
    +4/-0     
    update.ts
    Implement orchestration for updating location metafields 

    src/functions/customer/orchestrations/location/update.ts

  • Implemented new orchestration for updating location metafields.
  • Added durable functions orchestration and activity handler.
  • +50/-0   
    update-location-metafield.ts
    Implement updateLocationMetafield function and GraphQL mutation

    src/functions/customer/orchestrations/location/update/update-location-metafield.ts

  • Implemented updateLocationMetafield function.
  • Added GraphQL mutation for updating location metafields.
  • +97/-0   
    update.ts
    Refactor location update service                                                 

    src/functions/customer/services/location/update.ts

    • Removed direct Shopify metafield update logic from service.
    +1/-75   
    Tests
    3 files
    create-location-metafield.spec.ts
    Update tests for location metafield creation                         

    src/functions/customer/orchestrations/location/create/create-location-metafield.spec.ts

  • Added geo_location field handling in tests.
  • Renamed test descriptions for clarity.
  • +10/-2   
    update-location-metafield.spec.ts
    Add tests for updating location metafields                             

    src/functions/customer/orchestrations/location/update/update-location-metafield.spec.ts

  • Added tests for updateLocationMetafield function.
  • Mocked Shopify admin requests.
  • +176/-0 
    update.spec.ts
    Refactor location update service tests                                     

    src/functions/customer/services/location/update.spec.ts

    • Removed direct Shopify metafield update logic from tests.
    +0/-69   

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

    …ates
    
     - Include `df.input.durableClient()` in `customerLocationUpdate` HTTP trigger
     - Introduce `InvocationContext` and orchestration call in `CustomerLocationControllerUpdate`
     - Implement new orchestration `CustomerLocationUpdateOrchestration` for location updates
     - Add `geo_location` field handling in `createLocationMetafield` and corresponding tests
     - Create new activity and orchestration for updating location metafields
     - Refactor `CustomerLocationServiceUpdate` to remove direct Shopify metafield update logic
    @github-actions github-actions bot added enhancement New feature or request tests labels Jun 5, 2024
    Copy link

    github-actions bot commented Jun 5, 2024

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files with significant changes including new features, refactoring, and updates to existing functionality. The changes impact core functionality such as HTTP triggers, service logic, and orchestration which requires careful review to ensure system integrity and performance.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The CustomerLocationServiceUpdate function now returns directly from a MongoDB update operation. This change might lead to unexpected behavior if the consumer of this function expects the previous behavior where additional processing was done after the update.

    Data Consistency: The removal of direct Shopify metafield update logic from CustomerLocationServiceUpdate and its distribution across multiple new functions and orchestrations increases the complexity of the system, which might lead to issues with data consistency if not handled properly.

    🔒 Security concerns

    No

    Code feedback:
    relevant filesrc/functions/customer-location.function.ts
    suggestion      

    Consider validating df.input.durableClient() before using it to ensure it is correctly configured and can prevent runtime errors. [important]

    relevant lineextraInputs: [df.input.durableClient()],

    relevant filesrc/functions/customer/controllers/location/update.ts
    suggestion      

    Add error handling for the orchestration call in CustomerLocationControllerUpdate to manage failures gracefully. [important]

    relevant lineawait CustomerLocationUpdateOrchestration(

    relevant filesrc/functions/customer/orchestrations/location/update.ts
    suggestion      

    Ensure that input is validated before starting the orchestration to prevent issues related to invalid data being processed. [important]

    relevant lineconst input = context.df.getInput() as Input;

    relevant filesrc/functions/customer/orchestrations/location/update/update-location-metafield.ts
    suggestion      

    Implement a retry mechanism or transaction-like handling in updateLocationMetafield to ensure data integrity during update failures. [important]

    relevant lineconst { data } = await shopifyAdmin().request(UPDATE_LOCATION_METAOBJECT, {

    Copy link

    github-actions bot commented Jun 5, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Correct the input parameter structure for starting a new orchestration

    Ensure that the startNew method's input parameters are correctly provided. The second
    parameter should be the input for the orchestration, not an object containing the input.
    This change ensures that the orchestration receives the correct data structure.

    src/functions/customer/orchestrations/location/update.ts [40-44]

     const instanceId: string = await client.startNew(
       "CustomerLocationUpdateOrchestration",
    -  {
    -    input,
    -  }
    +  input
     );
     
    Suggestion importance[1-10]: 10

    Why: This suggestion corrects a potential bug that could prevent the orchestration from receiving the correct data structure, ensuring the functionality works as intended.

    10
    Add error handling for data validation to prevent server crashes

    It's recommended to handle potential exceptions when parsing the query and body with Zod
    schemas. This can prevent the server from crashing due to unexpected inputs. Wrap the
    parsing logic in a try-catch block and handle the errors appropriately.

    src/functions/customer/controllers/location/update.ts [36-39]

    -const validateData =
    -  CustomerLocationControllerUpdateQuerySchema.parse(query);
    -const validateBody =
    -  CustomerLocationControllerUpdateBodySchema.parse(body) || {};
    +let validateData, validateBody;
    +try {
    +  validateData = CustomerLocationControllerUpdateQuerySchema.parse(query);
    +  validateBody = CustomerLocationControllerUpdateBodySchema.parse(body) || {};
    +} catch (error) {
    +  // Handle parsing error, e.g., return a 400 Bad Request response
    +  return { error: "Invalid input format", details: error };
    +}
     
    Suggestion importance[1-10]: 8

    Why: Adding error handling for data validation is crucial to prevent server crashes due to unexpected inputs, enhancing the robustness of the application.

    8
    Security
    Add authentication to protect the customer location update endpoint

    Consider adding authentication and authorization checks for the customerLocationUpdate
    endpoint. Currently, the authLevel is set to "anonymous", which might expose sensitive
    customer location data to unauthorized users. Implementing proper authentication will
    ensure that only authenticated users can access or modify customer location data.

    src/functions/customer-location.function.ts [38]

    -authLevel: "anonymous",
    +authLevel: "function",
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a significant security concern by recommending the addition of authentication to protect sensitive customer location data from unauthorized access.

    9
    Possible issue
    Implement error handling for Shopify API requests to enhance reliability

    Add error handling for the shopifyAdmin().request call to manage scenarios where the
    Shopify API might fail or return an unexpected response. This can improve the reliability
    of the metafield update process.

    src/functions/customer/orchestrations/location/update/update-location-metafield.ts [19-20]

    -const { data } = await shopifyAdmin().request(UPDATE_LOCATION_METAOBJECT, {
    -  variables: {
    -    id: location.metafieldId,
    -    fields: [
    -      {
    -        key: "location_type",
    -        value: location.locationType,
    -      },
    -      ...
    -    ],
    -  },
    -});
    +let data;
    +try {
    +  const response = await shopifyAdmin().request(UPDATE_LOCATION_METAOBJECT, {
    +    variables: {
    +      id: location.metafieldId,
    +      fields: [
    +        {
    +          key: "location_type",
    +          value: location.locationType,
    +        },
    +        ...
    +      ],
    +    },
    +  });
    +  data = response.data;
    +} catch (error) {
    +  throw new Error(`Shopify API request failed: ${error}`);
    +}
     
    Suggestion importance[1-10]: 8

    Why: Implementing error handling for external API requests is important to manage failures gracefully and improve the reliability of the process.

    8

    @jamalsoueidan jamalsoueidan merged commit 05daf73 into main Jun 5, 2024
    3 checks passed
    @jamalsoueidan jamalsoueidan deleted the add-geo-location-metaobject branch June 5, 2024 23:37
    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