Issue #3845 fix : auto-store failure going to done state#3999
Conversation
Signed-off-by: Vijay Soni <vijaysoni@sonivijay.com>
…ore-failure-handling
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #3845 by preventing auto-store failures from incorrectly moving v2.0 credential exchanges to the done state. The fix differentiates between recoverable storage errors (which leave the credential in credential-received state for manual retry) and unrecoverable errors (which abandon the exchange).
Key changes:
- Added a
cred_store_succeededflag to track successful credential storage - Separated StorageError handling to allow retries by keeping credentials in
credential-receivedstate - Only send credential acknowledgment when storage actually succeeds
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| acapy_agent/protocols/issue_credential/v2_0/handlers/cred_issue_handler.py | Implements differentiated error handling for auto-store failures, with StorageError allowing retry and other errors triggering abandonment |
| acapy_agent/protocols/issue_credential/v2_0/handlers/tests/test_cred_issue_handler.py | Updates tests to verify error handling behavior with proper mock setup and assertions for both error paths |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
acapy_agent/protocols/issue_credential/v2_0/handlers/tests/test_cred_issue_handler.py
Show resolved
Hide resolved
acapy_agent/protocols/issue_credential/v2_0/handlers/tests/test_cred_issue_handler.py
Show resolved
Hide resolved
| mock_cred_ex = mock.MagicMock( | ||
| save_error_state=mock.CoroutineMock(), | ||
| save=mock.CoroutineMock(), | ||
| ) |
There was a problem hiding this comment.
The mock object mock_cred_ex needs to include the error_msg attribute because the handler code sets cred_ex_record.error_msg = err.roll_up when a StorageError occurs. Without this attribute in the mock, the test may not properly validate that the error message is being set correctly on the credential exchange record.
| mock_cred_ex = mock.MagicMock( | ||
| save_error_state=mock.CoroutineMock(), | ||
| save=mock.CoroutineMock(), | ||
| ) |
There was a problem hiding this comment.
The mock object mock_cred_ex needs to include the error_msg attribute because the handler code sets cred_ex_record.error_msg = err.roll_up when a StorageError occurs. Without this attribute in the mock, the test may not properly validate that the error message is being set correctly on the credential exchange record.
| store_credential=mock.CoroutineMock( | ||
| side_effect=[ | ||
| test_module.IndyHolderError, |
There was a problem hiding this comment.
The StorageError() instantiation should provide a message or have a mocked roll_up property. In the handler code at line 81, err.roll_up is accessed when a StorageError occurs. Without a proper message or mocked roll_up attribute, this will cause the roll_up property to return "StorageError." which may not provide meaningful testing of the error message handling. Consider passing a message like StorageError("Database connection failed") to make the test more realistic.
| store_credential=mock.CoroutineMock( | ||
| side_effect=[ | ||
| test_module.AnonCredsHolderError, |
There was a problem hiding this comment.
The StorageError() instantiation should provide a message or have a mocked roll_up property. In the handler code at line 81, err.roll_up is accessed when a StorageError occurs. Without a proper message or mocked roll_up attribute, this will cause the roll_up property to return "StorageError." which may not provide meaningful testing of the error message handling. Consider passing a message like StorageError("Database connection failed") to make the test more realistic.
|
There is some co-pilot suggestions. If you could go over them quickly and decide if they are worth addressing, please. The fix looks good to me. |
…ore-failure-handling
|
Issue Summary
donecredential-receivedand savingerror_msgTesting