fix(finkok): handle cancel response without EstatusUUID #54
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash in Finkok cancellation operations when the SOAP response omits the EstatusUUID field. The fix implements a three-tier fallback strategy: first attempting to use EstatusUUID, then falling back to CodEstatus, and finally parsing the status from the embedded Acuse XML if both are missing.
Changes:
- Refactored
acuse_bytesextraction to avoid redundant computation - Implemented fallback logic to safely handle missing status fields
- Allowed
codeparameter to beNonewhen all status sources are unavailable
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| estatus_uuid_elem = root.find(".//apps:EstatusUUID", self.namespaces) | ||
| if estatus_uuid_elem is not None: | ||
| code = estatus_uuid_elem.text | ||
| else: | ||
| cod_estatus_elem = root.find(".//apps:CodEstatus", self.namespaces) | ||
| code = cod_estatus_elem.text if cod_estatus_elem is not None else None | ||
|
|
||
| if code is None and acuse_bytes: | ||
| try: | ||
| acuse_root = etree.fromstring(acuse_bytes) | ||
| estatus_in_acuse = acuse_root.find(".//*[local-name()='EstatusUUID']") | ||
| if estatus_in_acuse is not None and estatus_in_acuse.text: | ||
| code = estatus_in_acuse.text | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
The fallback logic for determining the status code when EstatusUUID is missing could benefit from inline comments explaining each fallback step. This would help maintainers understand why the code tries EstatusUUID first, then CodEstatus, and finally parses the Acuse XML. Consider adding comments like:
- "Try primary status field EstatusUUID"
- "Fallback to CodEstatus if EstatusUUID is missing"
- "Last resort: extract EstatusUUID from embedded Acuse XML"
| except Exception: | ||
| pass |
There was a problem hiding this comment.
The bare except clause catches all exceptions during Acuse XML parsing, which may hide legitimate errors like encoding issues or malformed XML structures. Consider being more specific about which exceptions to catch (e.g., etree.XMLSyntaxError, UnicodeDecodeError, AttributeError) or at minimum logging the caught exception for debugging purposes.
| except Exception: | |
| pass | |
| except (etree.XMLSyntaxError, UnicodeDecodeError, AttributeError) as exc: | |
| getLogger(__name__).debug( | |
| "Failed to parse Acuse XML while extracting EstatusUUID: %s", | |
| exc, | |
| ) |
| code = estatus_in_acuse.text | ||
| except Exception: | ||
| pass | ||
|
|
There was a problem hiding this comment.
The code now allows None to be assigned to the code parameter, but the type annotation in CancelationAcknowledgment (satcfdi/pacs/init.py line 41) specifies 'code: str | dict' without including None. This creates a type inconsistency. The type annotation should be updated to 'code: str | dict | None' to accurately reflect the possible values and maintain type safety.
| # Ensure `code` matches the expected type annotation (str | dict) | |
| if code is None: | |
| code = "" |
| estatus_uuid_elem = root.find(".//apps:EstatusUUID", self.namespaces) | ||
| if estatus_uuid_elem is not None: | ||
| code = estatus_uuid_elem.text | ||
| else: | ||
| cod_estatus_elem = root.find(".//apps:CodEstatus", self.namespaces) | ||
| code = cod_estatus_elem.text if cod_estatus_elem is not None else None | ||
|
|
||
| if code is None and acuse_bytes: | ||
| try: | ||
| acuse_root = etree.fromstring(acuse_bytes) | ||
| estatus_in_acuse = acuse_root.find(".//*[local-name()='EstatusUUID']") | ||
| if estatus_in_acuse is not None and estatus_in_acuse.text: | ||
| code = estatus_in_acuse.text | ||
| except Exception: | ||
| pass | ||
|
|
||
| return CancelationAcknowledgment( | ||
| code=root.find(".//apps:EstatusUUID", self.namespaces).text, | ||
| acuse=unescape(ack.text).encode(), | ||
| code=code, | ||
| acuse=acuse_bytes, | ||
| ) |
There was a problem hiding this comment.
The new fallback logic for handling missing EstatusUUID is not covered by tests. Since this repository uses comprehensive automated testing (as evidenced by existing tests in tests/test_pac_finkok.py), consider adding test cases that specifically validate:
- Response with missing EstatusUUID but present CodEstatus
- Response with missing both EstatusUUID and CodEstatus but EstatusUUID available in Acuse XML
- Response with all status fields missing (code should be None)
This would ensure the bug fix works correctly and prevent regression.
2 participants
Pull Request Template
Description
Fixes a crash in Finkok cancellation when the SOAP response omits apps:EstatusUUID (empty in demo). The update safely handles missing EstatusUUID, falls back to CodEstatus if present, and parses EstatusUUID from the embedded Acuse XML when available. This prevents AttributeError while preserving existing behavior.
Fixes # (53)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Requires external Finkok demo credentials/services and SAT availability
Test Configuration:
Checklist: