Fix/XSUP-70513/ManageEngine check response status#44548
Conversation
MosheEichler
added
release-notes-only
🤖 AI-Powered Code Review AvailableYou can leverage AI-powered code review to assist with this PR! Available Commands:
|
Coverage Report
|
||||||||||||||||||||||||||||||
marketplace-ai-reviewer
removed
the
ready-for-ai-review
|
🤖 Analysis started. Please wait for results... |
|
Validate summary Verdict: PR can be force merged from validate perspective? ✅ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
tcarmeli1
added
the
ready-for-ai-review
marketplace-ai-reviewer
removed
the
ready-for-ai-review
|
🤖 Analysis started. Please wait for results... |
🤖 AI Review DisclaimerThis review was generated by an AI-powered tool and may contain inaccuracies. Please be advised, and we extend our sincere apologies for any inconvenience this may cause. |
marketplace-ai-reviewer
left a comment
marketplace-ai-reviewer
left a comment
There was a problem hiding this comment.
Hi! Thanks for your contribution to the ServiceDeskPlus integration. I've left a few notes to help improve the robustness of the code, particularly around safely handling null values and missing fields in the API responses to prevent unexpected errors. I also suggested centralizing the response status checks within the HTTP client and fixing a minor formatting issue in the Release Notes. Great work overall!
Additionally, please address the following file-level notes:
Packs/ServiceDeskPlus/Integrations/ServiceDeskPlus/ServiceDeskPlus.py: Consider callingcheck_response_statuscentrally insideClient.http_request.
@DeanArbel, @marketplace-ai-reviewer, @kamalq97, @MosheEichler please review and approve the results generated by the AI Reviewer by responding 👍 on this comment.
| messages = response_status.get("messages", []) | ||
| message_texts = [] | ||
| for msg in messages: | ||
| text = msg.get("message") or ", ".join(str(f) for f in msg.get("fields", [])) |
There was a problem hiding this comment.
Prevent potential TypeError and AttributeError if API returns null for fields or type.
fields = msg.get("fields") or []
text = msg.get("message") or ", ".join(str(f) for f in fields)
if text:
msg_type = msg.get("type") or "error"
message_texts.append(f"[{msg_type.upper()}] {text}")| success_response = {"response_status": {"status": "success", "status_code": 2000, "messages": []}} | ||
| # Should not raise | ||
| check_response_status(success_response) | ||
|
|
There was a problem hiding this comment.
Consider adding a test case for a response where response_status is missing entirely.
| if not response_status: | ||
| return | ||
|
|
||
| status = response_status.get("status", "success") |
There was a problem hiding this comment.
Ensure robust handling of null values and type mismatches in the API response.
status = response_status.get("status") or "success"
status_code = response_status.get("status_code") or 2000
# 2000 / "success" means the operation completed without issues
if status == "success" and str(status_code) == "2000":
return| @@ -0,0 +1,5 @@ | |||
| #### Integrations | |||
|
|
|||
| ##### Manage Engine Service Desk Plus | |||
There was a problem hiding this comment.
The content item header must match the display name of the integration as defined in its YAML file.
| ##### Manage Engine Service Desk Plus | |
| ##### Service Desk Plus |
🔍 AI Triage Report AvailableAn automated triage report has been generated for this pipeline. Status: 📋 Triage Report
|
6 participants
Status
Related Issues
https://jira-dc.paloaltonetworks.com/browse/XSUP-70513
Description
The integration always returns HTTP 200, even when an operation is rejected by the server.
Root Cause
When mandatory fields such as
TechnicianorResolutionare not populated, ManageEngine rejects the status change and returns:{"response_status": {"status": "warning", "status_code": 3000, "messages": [{"fields": ["technician", "resolution"], "status_code": 4003, "type": "warning"}]}}The integration was not inspecting this object and reported success regardless.
Fixed
Added check_response_status() helper that inspects response_status in the API JSON body and raises a DemistoException with the server's message when the operation was not successful.
For more details: https://www.manageengine.com/products/service-desk/sdpod-v3-api/getting-started/common-error-code.html
Must have