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

Remove location origin #161

Merged
merged 4 commits into from
Jun 6, 2024
Merged

Remove location origin #161

merged 4 commits into from
Jun 6, 2024

Conversation

jamalsoueidan
Copy link
Owner

@jamalsoueidan jamalsoueidan commented Jun 6, 2024

PR Type

Enhancement, Tests, Documentation


Description

  • Removed LocationOriginTypes and originType fields from various files, including schemas, services, and tests.
  • Updated location types to include HOME, SALON, and VIRTUAL.
  • Simplified tag generation logic in update-article.ts and update-product.ts.
  • Adjusted tests and helper functions to reflect the updated location types.
  • Updated OpenAPI documentation to reflect the removal of originType and the new location types.

Changes walkthrough 📝

Relevant files
Enhancement
14 files
update-article.ts
Remove `LocationOriginTypes` and simplify tag generation.

src/functions/customer/orchestrations/customer/update/update-article.ts

  • Removed LocationOriginTypes and related logic.
  • Simplified tag generation logic.
  • +4/-20   
    create-location-metafield.ts
    Remove `originType` from location metafield creation.       

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

    • Removed originType field from metafield creation.
    +0/-4     
    update-location-metafield.ts
    Remove `originType` from location metafield update.           

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

    • Removed originType field from metafield update.
    +0/-4     
    update-product.ts
    Refactor product update logic and tag generation.               

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

  • Removed CustomerProductServiceGet import.
  • Updated product fetching logic.
  • Added new tag generation logic.
  • +48/-10 
    aggregation.ts
    Remove `originType` from booking aggregation.                       

    src/functions/customer/services/booking/aggregation.ts

    • Removed originType from booking aggregation.
    +0/-1     
    range.ts
    Remove `originType` from booking range type.                         

    src/functions/customer/services/booking/range.ts

    • Removed originType from booking range type.
    +1/-4     
    get.ts
    Remove `originType` from order service.                                   

    src/functions/customer/services/order/get.ts

    • Removed originType from order service.
    +1/-5     
    location.schema.ts
    Update location schema to remove `originType`.                     

    src/functions/location/location.schema.ts

  • Removed originType field from location schema.
  • Updated location type enum with new values.
  • +4/-9     
    location.types.ts
    Update location types to remove `LocationOriginTypes`.     

    src/functions/location/location.types.ts

  • Removed LocationOriginTypes enum.
  • Updated LocationTypes enum with new values.
  • +3/-8     
    schedule.types.ts
    Remove `LocationOriginTypes` from schedule types.               

    src/functions/schedule/schedule.types.ts

    • Removed LocationOriginTypes from schedule types.
    +1/-2     
    product.schema.ts
    Update product schema to remove `originType`.                       

    src/functions/schedule/schemas/product.schema.ts

  • Removed originType field from product schema.
  • Updated location type enum with new values.
  • +7/-11   
    search.ts
    Remove `originType` from user search schema.                         

    src/functions/user/controllers/user/search.ts

    • Removed originType from user search schema.
    +0/-1     
    filters.ts
    Remove `originType` from user filters.                                     

    src/functions/user/services/user/filters.ts

    • Removed originType from user filters.
    +1/-8     
    search.ts
    Remove `originType` from user search service.                       

    src/functions/user/services/user/search.ts

    • Removed originType from user search service.
    +1/-2     
    Tests
    14 files
    create-location-metafield.spec.ts
    Update tests to remove `LocationOriginTypes`.                       

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

  • Removed LocationOriginTypes from tests.
  • Updated test data to reflect new location types.
  • +2/-11   
    update-location-metafield.spec.ts
    Update tests to remove `LocationOriginTypes`.                       

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

  • Removed LocationOriginTypes from tests.
  • Updated test data to reflect new location types.
  • +2/-11   
    update-product.spec.ts
    Update product tests for new location types and day tags.

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

  • Updated tests to reflect new location types.
  • Added logic to handle new day tags.
  • +10/-1   
    create.spec.ts
    Update location creation tests for new location types.     

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

  • Removed LocationOriginTypes from tests.
  • Updated test data to reflect new location types.
  • +3/-5     
    get-products.spec.ts
    Remove `originType` from location product tests.                 

    src/functions/customer/services/location/get-products.spec.ts

    • Removed originType from location product tests.
    +0/-2     
    add.spec.ts
    Remove `LocationOriginTypes` from product add tests.         

    src/functions/customer/services/product/add.spec.ts

    • Removed LocationOriginTypes from product add tests.
    +1/-2     
    remove-location-from-all.spec.ts
    Update product removal tests for new location types.         

    src/functions/customer/services/product/remove-location-from-all.spec.ts

  • Removed LocationOriginTypes from product removal tests.
  • Updated test data to reflect new location types.
  • +6/-11   
    get.spec.ts
    Remove `LocationOriginTypes` from shipping service tests.

    src/functions/shipping/services/get.spec.ts

    • Removed LocationOriginTypes from shipping service tests.
    +2/-3     
    generate.spec.ts
    Update availability generation tests for new location types.

    src/functions/user/services/availability/generate.spec.ts

  • Removed LocationOriginTypes from availability generation tests.
  • Updated test data to reflect new location types.
  • +1/-3     
    get.spec.ts
    Update availability get tests for new location types.       

    src/functions/user/services/availability/get.spec.ts

  • Removed LocationOriginTypes from availability get tests.
  • Updated test data to reflect new location types.
  • +1/-1     
    filters.spec.ts
    Remove `originType` from user filters tests.                         

    src/functions/user/services/user/filters.spec.ts

    • Removed originType from user filters tests.
    +0/-1     
    search.spec.ts
    Remove `originType` from user search tests.                           

    src/functions/user/services/user/search.spec.ts

    • Removed originType from user search tests.
    +0/-1     
    location.ts
    Remove `LocationOriginTypes` from location helpers.           

    src/library/jest/helpers/location.ts

    • Removed LocationOriginTypes from location helpers.
    +3/-9     
    product.ts
    Remove `LocationOriginTypes` from product helpers.             

    src/library/jest/helpers/product.ts

    • Removed LocationOriginTypes from product helpers.
    +1/-2     
    Documentation
    7 files
    update.ts
    Add TODO for updating day tags in products.                           

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

    • Added TODO comment for updating day tags in products.
    +3/-0     
    base-location.yaml
    Update base location schema to remove `originType`.           

    openapi/paths/customer/location/_types/base-location.yaml

  • Removed originType field from location schema.
  • Updated location type enum with new values.
  • +1/-5     
    body.yaml
    Update location creation schema to remove `originType`.   

    openapi/paths/customer/location/create/body.yaml

  • Removed originType field from location creation schema.
  • Updated location type enum with new values.
  • +1/-5     
    body.yaml
    Update location update schema to remove `originType`.       

    openapi/paths/customer/location/update/body.yaml

    • Removed originType field from location update schema.
    +0/-4     
    product-locations.yaml
    Update product locations schema to remove `originType`.   

    openapi/paths/customer/schedule/_types/product-locations.yaml

  • Removed originType field from product locations schema.
  • Updated location type enum with new values.
  • +1/-5     
    payload.yaml
    Update user filters payload to remove `originType`.           

    openapi/paths/user/filters/payload.yaml

  • Removed originType field from user filters payload.
  • Updated location type enum with new values.
  • +1/-5     
    body.yaml
    Update user search body to remove `originType`.                   

    openapi/paths/user/search/body.yaml

  • Removed originType field from user search body.
  • Updated location type enum with new values.
  • +1/-3     

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

     - Remove LocationOriginTypes imports and usage across various files
     - Simplify tag generation logic in update-article.ts
     - Adjust tests and metafield creation/update to reflect removal of origin_type
     - Update product orchestration to handle changes in locationType and day tags
     - Remove LocationOriginTypes enum and its usage across various schemas and types.
     - Introduce new location types: HOME, SALON, and VIRTUAL.
     - Replace ORIGIN and ONLINE types with the new location types.
     - Adjust tests and helper functions to reflect the updated location types.
    @github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request tests labels Jun 6, 2024
    Copy link

    github-actions bot commented Jun 6, 2024

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files and changes related to the removal of a data field across various services and tests. The logic changes in tag generation and data handling need careful review to ensure that all references are correctly updated and that no residual or broken code remains.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The removal of originType and changes in location types could lead to issues if not all instances are properly updated or if other parts of the system rely on the old data structure.

    🔒 Security concerns

    No

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

    Consider adding error handling for the .map() function used in tag generation to ensure that the properties accessed on locations are not undefined, which could cause runtime errors. [important]

    relevant line.map((l) => l.locationType)

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

    Ensure that the product variable is not null after the find() operation before accessing its properties to avoid potential runtime errors. [important]

    relevant lineconst product = schedule.products.find((p) => p.productId === productId);

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

    Since origin_type key is removed, ensure that no external integrations or database queries depend on this metafield to prevent breaking changes. [medium]

    relevant line- key: "origin_type",

    relevant filesrc/functions/customer/orchestrations/customer/update/update-article.ts
    suggestion      

    Refactor the tag generation logic to a separate function for better modularity and readability. [medium]

    relevant linetags.push(

    Copy link

    github-actions bot commented Jun 6, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Prevent duplicate location ID tags by using a Set to ensure uniqueness

    The current implementation may add duplicate locationid tags if the same location appears
    multiple times in the product.locations array. To prevent this, you can use a Set to
    ensure all tags are unique before converting them back to an array.

    src/functions/customer/orchestrations/product/update/update-product.ts [199]

    -.concat(product.locations.map((l) => `locationid-${l.location}`))
    +.concat(Array.from(new Set(product.locations.map((l) => `locationid-${l.location}`))))
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential issue with duplicate tags, which can lead to incorrect or redundant data. Using a Set to ensure uniqueness is a good enhancement for data integrity.

    9
    Expand the enum list for location types to cover all use cases

    Update the enum for locationType to include all possible types such as 'office' or
    'warehouse' to cover all business cases.

    openapi/paths/customer/location/_types/base-location.yaml [7]

    -enum: [home, commercial, destination, virtual]
    +enum: [home, commercial, destination, virtual, office, warehouse]
     
    Suggestion importance[1-10]: 6

    Why: Expanding the enum list for location types is a good enhancement to cover more use cases, but it is not critical and depends on the actual business requirements.

    6
    Possible bug
    Add a safeguard against undefined locationType properties in tag generation

    Ensure that the locationType property is always defined before using it in the tag
    generation. This can prevent runtime errors if some locations do not have this property
    set. You can use optional chaining (?.) and provide a default value if needed.

    src/functions/customer/orchestrations/customer/update/update-article.ts [55-57]

     `location_type-${locations
    -  .map((l) => l.locationType)
    +  .map((l) => l.locationType ?? 'unknown')
       .join(", location_type-")}`
     
    Suggestion importance[1-10]: 8

    Why: This suggestion adds a safeguard against potential runtime errors, which is important for robustness. It ensures that the code does not break if locationType is undefined.

    8
    Add checks to ensure day values are defined before processing to avoid errors

    When generating the day tags from schedule.slots, consider handling potential undefined
    values in slot.day to avoid runtime errors. Implement a filter to exclude undefined or
    null values before converting them to lowercase and joining.

    src/functions/customer/orchestrations/product/update/update-product.ts [125]

    -const days = schedule.slots.map((slot) => slot.day.toLowerCase());
    +const days = schedule.slots
    +  .filter((slot) => slot.day)
    +  .map((slot) => slot.day.toLowerCase());
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves the robustness of the code by ensuring that undefined values are handled properly, preventing potential runtime errors. It adds necessary checks to ensure data integrity.

    8
    Best practice
    Use a neutral default location type in test helpers

    Ensure that the locationType in getLocationObject function defaults to a more neutral type
    like LocationTypes.DESTINATION instead of LocationTypes.HOME to avoid biased test data.

    src/library/jest/helpers/location.ts [28]

    -locationType: LocationTypes.HOME,
    +locationType: LocationTypes.DESTINATION,
     
    Suggestion importance[1-10]: 8

    Why: Using a neutral default location type in test helpers is a best practice that ensures unbiased test data, which is important for accurate testing.

    8
    Performance
    Optimize the generation of location type tags by reducing array iterations

    Consider using a more efficient method for generating tags related to locationType.
    Currently, the code maps over locations twice: once for converting locationType to a
    string and again for joining them. This can be optimized by chaining these operations
    together in a single map function, reducing the overhead of multiple iterations over the
    array.

    src/functions/customer/orchestrations/customer/update/update-article.ts [55-57]

    -`location_type-${locations
    -  .map((l) => l.locationType)
    -  .join(", location_type-")}`
    +`location_type-${locations.map((l) => `location_type-${l.locationType}`).join(", ")}`
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves performance by reducing the number of iterations over the array, which is a minor but useful optimization. However, it does not address any critical issues or bugs.

    7
    Maintainability
    Replace hardcoded location type with a variable for flexibility

    Replace the hardcoded LocationTypes.HOME with a variable or a function parameter to
    increase flexibility and maintainability of the test setup.

    src/functions/shipping/services/get.spec.ts [26]

    -locationType: LocationTypes.HOME,
    +locationType: testLocationType,
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves maintainability by making the test setup more flexible. However, it is not crucial and does not address a major bug or issue.

    7
    Consistency
    Review and update type definitions for consistency post-removal of origin types

    Ensure consistency in the UserAggregationResult type by possibly including other relevant
    location-related properties that might have been overlooked due to the removal of
    LocationOriginTypes.

    src/functions/user/services/user/filters.ts [8]

     locationType: LocationTypes;
    +additionalProperty: string;  # Replace or modify as needed based on actual use case
     
    Suggestion importance[1-10]: 5

    Why: While ensuring consistency in type definitions is important, the suggestion is somewhat vague and speculative without specific context on what additional properties might be needed.

    5

    @jamalsoueidan jamalsoueidan merged commit 0d3151c into main Jun 6, 2024
    3 checks passed
    @jamalsoueidan jamalsoueidan deleted the remove-location-origin branch June 6, 2024 23:54
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    documentation Improvements or additions to documentation enhancement New feature or request Review effort [1-5]: 3 tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant