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

Fix and cleanup IEC.postman_collection.json #113

Merged
merged 7 commits into from
Jun 14, 2024
Merged

Fix and cleanup IEC.postman_collection.json #113

merged 7 commits into from
Jun 14, 2024

Conversation

maorcc
Copy link
Contributor

@maorcc maorcc commented Jun 12, 2024

User description

What changes do you are proposing?

I have done the following fixes and cleanups. The first one fixes a bug, the rest are mostly cleanups. Each fix is its own commit.

  1. Remove the duplicate Content-Type header in step 5
    Fix Auth step 5 remove Content-Type: application/json. Postman adds auto generates the correct Content-Type of application/x-www-form-urlencoded instead. Having the content type twice in Postman causes it to behave inconsistently, and sometimes works but not always. This change should fix it.

  2. Remove type "text" from content-type parameters. 
    This was done automatically by the export of the latest version of Postman.

  3. Remove Content-type of application/json from step 4
    Done because this step is a GET request that should not have a Content-Type header. It was disabled anyway, but I removed it as it was wrong.

  4. Sort APIs list
    Moved the Devices api upwards to its correct location in the list of APIs, because it must run before the RemoteReadingRange API.

  5. Clean double slash in URL
    Postman removes the double slash ("//") from all URLs on import.
    This empty path element makes no difference, and the API works with and without it.
    This extra slash is probably a bug in the IEC front-end, and was not intended to be there by their IEC API backend developers.

How did you test these changes?

To test I cleared my Postman workspace. Re-imported the file, and tested that all APIs are working and returning valid data.

Closing issues


PR Type

enhancement, bug fix


Description

  • Removed duplicate Content-Type headers in multiple steps to ensure consistent behavior in Postman.
  • Removed type: "text" from Content-Type and Accept headers as per the latest Postman export.
  • Corrected Content-Type for GET requests, ensuring they do not have a Content-Type header.
  • Sorted APIs list, moving the Devices API upwards to its correct location.
  • Cleaned double slashes in URLs, which were removed by Postman on import.

Changes walkthrough 📝

Relevant files
Enhancement
IEC.postman_collection.json
Fix headers, clean URLs, and reorder APIs in Postman collection.

IEC.postman_collection.json

  • Removed duplicate Content-Type headers in multiple steps.
  • Removed type: "text" from Content-Type and Accept headers.
  • Corrected Content-Type for GET requests.
  • Sorted APIs list and moved Devices API upwards.
  • Cleaned double slashes in URLs.
  • +117/-146

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

    maorcc and others added 7 commits June 9, 2024 18:54
    …uto generates the correct Content-Type of application/x-www-form-urlencoded instead. Having the content type twice in Postman causes it to behave inconsistently, and sometimes work but not allways. This change should fix it.
    …tically by the export of the latest version of Postman.
    …ET request that should not have a Content-Type. It was disabled anyway, but I removed it becuase it was also wrong.
    
     Also fixed the value of the disabled Accept header to be text/html because this is the returned content type of this request. I left it disabled because the api also works without it.
    …APIs, because it must run before the RemoteReadingRange api.
    …path param looks like a double slash in the url "//".
    
    This empty path element makes no difference, and the API works with and without it. The latest version of Postman removes it automatically.
    I am pretty sure that this extra slash is actually a bug in the IEC front-end code, and shoult not be used in the API.
    Copy link

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

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5]

    2

    🧪 Relevant tests

    No

    🔒 Security concerns

    No

    ⚡ Key issues to review

    Possible Bug:
    The removal of the "type" attribute from the headers might affect how the headers are processed by the receiving server or by Postman itself. It's important to ensure that this change does not affect the expected behavior of the API calls.

    Cleanup Verification:
    Ensure that the removal of double slashes in URLs does not affect the routing or accessibility of the endpoints. Although it's mentioned that the API works with and without the double slash, this should be explicitly tested across all environments.

    Copy link

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

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add error handling for empty or unexpected JSON responses in the Devices request script

    Verify that the script section under the Devices request correctly handles cases where
    jsonData might be empty or not contain the expected keys. This will prevent runtime
    errors.

    IEC.postman_collection.json [831-835]

    -"let jsonData = JSON.parse(pm.response.text());"
    +"let jsonData = JSON.parse(pm.response.text() || '[]'); if (jsonData.length > 0) { pm.collectionVariables.set(\"device_id\", jsonData[0].deviceNumber); pm.collectionVariables.set(\"device_code\", jsonData[0].deviceCode); }"
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding error handling for JSON parsing is crucial to prevent runtime errors and ensure the robustness of the script, especially in production environments.

    9
    Possible issue
    Remove double slashes from URL paths

    Ensure that the url paths do not contain double slashes (//). This can cause issues with
    some HTTP clients and servers.

    IEC.postman_collection.json [612]

    -"raw": "https://iecapi.iec.co.il//api/outages/accounts"
    +"raw": "https://iecapi.iec.co.il/api/outages/accounts"
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Double slashes in URL paths can lead to routing issues or unexpected behavior, making this a significant improvement.

    8
    Best practice
    Standardize the Accept header value across all requests

    Ensure that the value for the Accept header is consistent across all requests. Currently,
    some requests use application/json while others use application/json, text/plain, /.
    Standardizing this can help avoid unexpected behavior.

    IEC.postman_collection.json [46]

    -"value": "application/json"
    +"value": "application/json, text/plain, */*"
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Standardizing the Accept header value can improve consistency and prevent unexpected behavior, but it's a moderate improvement rather than a critical fix.

    6
    Maintainability
    Remove unnecessary disabled attributes from headers

    Remove the disabled attribute from headers that are not actually disabled. This will
    reduce confusion and ensure that only the necessary headers are marked as disabled.

    IEC.postman_collection.json [277]

    -"disabled": true
    +// Remove the "disabled": true attribute from headers that are not disabled
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Removing unnecessary disabled attributes can enhance code clarity and maintainability, but it's not a critical issue.

    5

    @GuyKh
    Copy link
    Owner

    GuyKh commented Jun 13, 2024

    @maorcc I have to praise you for the clear and VERY informative PR.
    I wish all devs were working this way.
    Kudos! 🎉

    @GuyKh
    Copy link
    Owner

    GuyKh commented Jun 13, 2024

    @maorcc The only part I'm concerned about is the double-slash. From tests I've done when I wrote this, IEC API failed on single slash and worked on double, for some reason.

    I want to make sure you've tested this

    @maorcc
    Copy link
    Contributor Author

    maorcc commented Jun 13, 2024

    Yes, after doing all the changes, including the removal of double slashes I tested it as I described above:

    To test I cleared my Postman workspace. Re-imported the file, and tested that all APIs are working and returning valid data.

    @GuyKh
    Copy link
    Owner

    GuyKh commented Jun 14, 2024

    Thanks again for your work

    @GuyKh GuyKh merged commit 9b22fee into GuyKh:main Jun 14, 2024
    1 check failed
    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.

    None yet

    2 participants