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

SD-508-Shipper and consignee po reference #239

Merged
merged 4 commits into from
Dec 6, 2024

Conversation

palatsangeetha
Copy link
Collaborator

@palatsangeetha palatsangeetha commented Dec 5, 2024

PR Type

enhancement, tests


Description

  • Introduced validation for consignment items reference types to ensure they match known dataset keywords.
  • Refactored the code to replace string literals with constants for better maintainability and consistency.
  • Added a new dataset for consignment items reference types in EblDatasets.
  • Enhanced test coverage by adding tests for the new consignment items reference types validation.
  • Refactored test setup in EBLChecksTest to use a helper method for creating root nodes.

Changes walkthrough 📝

Relevant files
Enhancement
EBLChecks.java
Enhance validation and refactor constants usage                   

ebl/src/main/java/org/dcsa/conformance/standards/ebl/checks/EBLChecks.java

  • Added new constants for various fields.
  • Introduced validation for consignment items reference types.
  • Replaced string literals with constants for consistency.
  • Updated existing checks to use new constants.
  • +34/-24 
    EblDatasets.java
    Add consignment items reference type dataset                         

    ebl/src/main/java/org/dcsa/conformance/standards/ebl/checks/EblDatasets.java

  • Added a new dataset for consignment items reference types.
  • Made the class non-instantiable by adding a private constructor.
  • +8/-0     
    Tests
    EBLChecksTest.java
    Add tests for consignment items reference types                   

    ebl/src/test/java/org/dcsa/conformance/standards/ebl/checks/EBLChecksTest.java

  • Added tests for consignment items reference types validation.
  • Refactored test setup to use a helper method for root node creation.
  • +37/-14 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    qodo-merge-pro bot commented Dec 5, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Duplication
    The validation logic for reference types is split between VALID_REFERENCE_TYPES and VALID_CONSIGMENT_ITEMS_REFERENCE_TYPES checks. Consider consolidating these checks to reduce duplication and improve maintainability.

    Naming Consistency
    The constant name CONSIGNMENT_ITEMS_REFERENCE_TYPE is inconsistent with other constant names in the file that use plural form (e.g. REFERENCE_TYPE vs REFERENCE_TYPES)

    Copy link

    qodo-merge-pro bot commented Dec 5, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Remove unnecessary parentheses that affect condition evaluation logic

    The LOCATION_NAME_REQUIRED predicate has an extra set of parentheses around the
    second condition that changes the logic. Remove the extra parentheses to fix the
    condition evaluation.

    ebl/src/main/java/org/dcsa/conformance/standards/ebl/checks/EBLChecks.java [551-554]

     private static final Predicate<JsonNode> LOCATION_NAME_REQUIRED =
        place ->
    -       (place.path("UNLocationCode").isMissingNode()
    -          && (place.path(LOCATION_NAME).isMissingNode()));
    +       place.path("UNLocationCode").isMissingNode()
    +          && place.path(LOCATION_NAME).isMissingNode();
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly identifies redundant parentheses that could make the logic harder to read and maintain. While the logic would work the same way, removing unnecessary parentheses improves code clarity.

    7

    💡 Need additional feedback ? start a PR chat

    Copy link
    Collaborator

    @jkosternl jkosternl left a comment

    Choose a reason for hiding this comment

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

    Added some remarks, but looks fine for the rest, so you already get the approval.

    @palatsangeetha palatsangeetha merged commit afa5cfb into dev Dec 6, 2024
    1 check passed
    @palatsangeetha palatsangeetha deleted the SD-508-Shipper-and-consignee-PO-reference branch December 6, 2024 12:51
    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.

    3 participants