feat: PATCH /integrations on export errors and token expiry
#442
Conversation
* feat: PATCH `/integrations` on export errors * feat: patch `/integrations` on token expiry (#436) * feat: patch `/integrations` on token expiry * fix: move `patch_integration_settings` to `apps/workspaces/actions.py` Avoids a circular import between actions.py and tasks.py * test: unit test `/integrations` patch calls (#437) * test: unit test `/integrations` patch calls * fix: token expiry logic (#440) * fix: token expiry logic * fix: rename invalidate_token to invalidate_xero_credentials * test: unit test updates for comment fixes (#441)
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThis pull request enhances error handling across multiple modules by adding calls to invalidate stale Xero credentials and update integration settings when specific authentication errors occur. New functions such as Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CX as connect_xero()
participant P as PatchIntegrationSettings()
participant I as InvalidateXeroCredentials()
participant X as Xero API
U->>CX: Initiate Xero connection
CX->>CX: Generate refresh token
CX->>P: Call patch_integration_settings(workspace_id, is_token_expired=False)
P-->>CX: Integration settings updated
alt Exception Occurs
CX->>X: Request company details
X-->>CX: Error response (e.g., UnsuccessfulAuthentication)
CX->>I: Call invalidate_xero_credentials(workspace_id)
I-->>CX: Credentials marked as expired
end
sequenceDiagram
participant V as View Decorator
participant F as Decorated Function
participant I as InvalidateXeroCredentials()
V->>F: Invoke function with workspace_id
F-->>V: Raise UnsuccessfulAuthentication exception
V->>I: Catch exception and call invalidate_xero_credentials(workspace_id)
I-->>V: Credentials invalidated
V-->>V: Return error response
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (3)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
tests/test_xero/test_exceptions.py (2)
6-21: Improve test function implementation.A few suggestions to enhance the test:
- The
dbfixture is imported but not used.- The test function name could be more descriptive (e.g.,
test_handle_xero_exceptions_invalidates_credentials).- Consider verifying that the exception is propagated to ensure the decorator's behavior is fully tested.
Apply this diff to improve the test:
-def test_handle_view_exceptions(mocker, db): +def test_handle_xero_exceptions_invalidates_credentials(mocker): workspace_id = 1 task_log_id = 1 mocked_invalidate_call = MagicMock() mocker.patch('apps.xero.exceptions.invalidate_xero_credentials', side_effect=mocked_invalidate_call) @handle_xero_exceptions(payment=False) def func(expense_group_id: int, task_log_id: int, xero_connection): raise NoPrivilegeError('Invalid Token') - func(workspace_id, task_log_id, MagicMock()) + with pytest.raises(NoPrivilegeError, match='Invalid Token'): + func(workspace_id, task_log_id, MagicMock()) args, _ = mocked_invalidate_call.call_args assert args[0] == workspace_id
23-31: Consider parameterizing similar test cases.The two test cases are very similar and could be parameterized using
pytest.mark.parametrizeto reduce code duplication and make it easier to add more test cases.Here's how you could refactor the tests:
+import pytest + +@pytest.mark.parametrize('payment,func_params', [ + (False, {'expense_group_id': 1, 'task_log_id': 1, 'xero_connection': MagicMock()}), + (True, {'bill': MagicMock(), 'workspace_id': 1, 'task_log': MagicMock()}), +]) +def test_handle_xero_exceptions_invalidates_credentials(mocker, payment, func_params): + workspace_id = func_params.get('workspace_id', func_params['expense_group_id']) + + mocked_invalidate_call = MagicMock() + mocker.patch('apps.xero.exceptions.invalidate_xero_credentials', side_effect=mocked_invalidate_call) + + @handle_xero_exceptions(payment=payment) + def func(**kwargs): + raise NoPrivilegeError('Invalid Token') + + with pytest.raises(NoPrivilegeError, match='Invalid Token'): + func(**func_params) + + args, _ = mocked_invalidate_call.call_args + assert args[0] == workspace_idapps/workspaces/helpers.py (1)
10-31: Consider adding type hints and improving error handling.The function implementation is good, but could benefit from:
- Type hints for the
is_token_expiredparameter- More specific exception handling
Apply this diff to add type hints and improve error handling:
-def patch_integration_settings(workspace_id: int, errors: int = None, is_token_expired = None): +def patch_integration_settings(workspace_id: int, errors: int = None, is_token_expired: bool = None): try: patch_request(url, payload, refresh_token) - except Exception as error: + except (ConnectionError, TimeoutError) as error: logger.error(error, exc_info=True) + except Exception as error: + logger.error(f'Unexpected error while patching integration settings for {workspace_id = }') + logger.error(error, exc_info=True)tests/test_workspaces/test_actions.py (1)
9-52: Consider adding negative test cases and improving test structure.While the test covers the main scenarios well, consider:
- Adding negative test cases (e.g., API failures)
- Using pytest parametrize for cleaner test structure
Here's how you could refactor using parametrize:
import pytest from unittest.mock import MagicMock @pytest.mark.django_db(databases=['default']) @pytest.mark.parametrize('errors,is_token_expired,expected_payload', [ (7, None, {'errors_count': 7, 'tpa_name': 'Fyle Xero Integration'}), (None, True, {'is_token_expired': True, 'tpa_name': 'Fyle Xero Integration'}), (241, True, { 'errors_count': 241, 'is_token_expired': True, 'tpa_name': 'Fyle Xero Integration' }) ]) def test_patch_integration_settings(mocker, errors, is_token_expired, expected_payload): workspace_id = 1 mocked_patch = MagicMock() mocker.patch('apps.fyle.helpers.requests.patch', side_effect=mocked_patch) patch_integration_settings( workspace_id, errors=errors, is_token_expired=is_token_expired ) _, kwargs = mocked_patch.call_args actual_payload = json.loads(kwargs['data']) assert actual_payload == expected_payloadapps/fyle/helpers.py (1)
41-60: Add type hints for consistency.The new
patch_requestfunction should include type hints to match the style of other functions in the file.Apply this diff to add type hints:
-def patch_request(url, body, refresh_token=None): +def patch_request(url: str, body: dict, refresh_token: Union[str, None] = None) -> dict:tests/test_workspaces/test_views.py (1)
131-134: Fix boolean comparison in assertion.The assertion uses an equality comparison with
Falsewhich is not Pythonic.- assert kwargs['is_token_expired'] == False + assert not kwargs['is_token_expired']🧰 Tools
🪛 Ruff (0.8.2)
134-134: Avoid equality comparisons to
False; useif not kwargs['is_token_expired']:for false checksReplace with
not kwargs['is_token_expired'](E712)
apps/xero/tasks.py (1)
834-834: Consider consolidating error handling logic.The same error handling pattern (logging + credential invalidation) is repeated in three locations. Consider extracting this into a helper function to maintain DRY principles.
def handle_xero_auth_error(workspace_id: int, context: str): """ Handle Xero authentication error :param workspace_id: Workspace ID :param context: Context where the error occurred """ logger.info( "Xero refresh token expired for workspace_id %s during %s", workspace_id, context ) invalidate_xero_credentials(workspace_id)Also applies to: 932-932, 963-963
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/exceptions.py(3 hunks)apps/fyle/helpers.py(1 hunks)apps/mappings/actions.py(2 hunks)apps/mappings/exceptions.py(3 hunks)apps/workspaces/actions.py(3 hunks)apps/workspaces/helpers.py(1 hunks)apps/xero/exceptions.py(3 hunks)apps/xero/queue.py(3 hunks)apps/xero/tasks.py(4 hunks)tests/test_exceptions.py(1 hunks)tests/test_workspaces/test_actions.py(1 hunks)tests/test_workspaces/test_views.py(2 hunks)tests/test_xero/test_exceptions.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_workspaces/test_views.py
134-134: Avoid equality comparisons to False; use if not kwargs['is_token_expired']: for false checks
Replace with not kwargs['is_token_expired']
(E712)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pytest
🔇 Additional comments (17)
tests/test_xero/test_exceptions.py (1)
1-4: Add test case for UnsuccessfulAuthentication exception.The AI summary mentions testing for
UnsuccessfulAuthenticationexception, but this test case is missing from the implementation.Would you like me to generate the additional test case for handling the
UnsuccessfulAuthenticationexception?tests/test_exceptions.py (1)
6-20: LGTM! Well-structured test for exception handling.The test effectively validates that the
handle_view_exceptionsdecorator properly handlesUnsuccessfulAuthenticationand callsinvalidate_xero_credentialswith the correct workspace ID.apps/mappings/actions.py (1)
49-51: LGTM! Good error handling for authentication failures.The addition of
invalidate_xero_credentialscall in theUnsuccessfulAuthenticationexception handler aligns well with the PR's objective of handling token expiry.apps/exceptions.py (2)
47-57: LGTM! Proper handling of Xero token expiry.The code correctly invalidates Xero credentials when token-related exceptions occur, ensuring proper cleanup of expired tokens.
71-81: LGTM! Comprehensive handling of authentication failures.The code properly handles various authentication-related exceptions by invalidating Xero credentials and returning appropriate error messages.
apps/mappings/exceptions.py (2)
59-62: LGTM! Proper handling of Xero token expiry in import exceptions.The code correctly invalidates Xero credentials when token-related exceptions occur during imports.
118-130: LGTM! Comprehensive error handling for import exceptions v2.The code properly handles various Xero authentication failures by invalidating credentials and updating import logs.
apps/workspaces/actions.py (2)
99-100: LGTM! Proper update of integration settings.The code correctly updates integration settings after refreshing the Xero token.
123-128: LGTM! Proper error handling during company info retrieval.The code correctly invalidates Xero credentials when authentication fails during company information retrieval.
apps/xero/exceptions.py (3)
20-20: LGTM!The new imports are correctly added and align with the PR objectives for handling token expiry and integration settings updates.
48-48: LGTM!The
patch_integration_settingscall is correctly placed after updating the last export details, ensuring the integration settings are updated with the latest error count.
194-201:❓ Verification inconclusive
Verify the order of operations in error handling.
The code correctly invalidates credentials first, but consider whether setting
countrytoNoneshould happen before or after invalidating credentials.
🏁 Script executed:
#!/bin/bash # Search for similar patterns in error handling rg -A 5 "invalidate_xero_credentials.*\n.*country\s*=\s*None" || rg -A 5 "country\s*=\s*None.*\n.*invalidate_xero_credentials"Length of output: 511
Review: Confirm the Order of Credential Invalidation vs. Country Update
The error handler currently calls
invalidate_xero_credentials(workspace_id)before retrieving and modifying theXeroCredentials(settingcountrytoNoneand saving the change). Please double-check whether updating thecountryfield should occur before invalidating the credentials—in case the invalidation process resets or supersedes that update—or if the present ordering correctly reflects the intended business logic.
- The snippet in
apps/xero/exceptions.py(lines 194–201) shows:
- First: Invalidate credentials.
- Then: Retrieve, modify (
country = None), and save the credentials.- Finally: Log the exception message.
If the intention is to have a clean slate before updating, consider reordering those operations; otherwise, if this sequence is deliberate, please confirm that it aligns with the overall error-handling strategy.
tests/test_workspaces/test_views.py (1)
118-119: LGTM!The mock setup for
patch_integration_settingsis correctly implemented usingmock.MagicMock().apps/xero/queue.py (3)
6-6: LGTM!The import for
invalidate_xero_credentialsis correctly added.
185-185: LGTM!The credential invalidation is correctly placed in the error handling block for bill creation.
263-263: LGTM!The credential invalidation is correctly placed in the error handling block for bank transaction creation.
apps/xero/tasks.py (1)
22-22: LGTM!The import for
invalidate_xero_credentialsis correctly added.
| def invalidate_xero_credentials(workspace_id): | ||
| try: | ||
| xero_credentials = XeroCredentials.get_active_xero_credentials(workspace_id) | ||
| if xero_credentials: | ||
| if not xero_credentials.is_expired: | ||
| patch_integration_settings(workspace_id, is_token_expired=True) | ||
| xero_credentials.refresh_token = None | ||
| xero_credentials.is_expired = True | ||
| xero_credentials.save() | ||
| except XeroCredentials.DoesNotExist as error: | ||
| logger.error(f'Xero credentials not found for {workspace_id = }:', ) | ||
| logger.error(error, exc_info=True) |
There was a problem hiding this comment.
Fix error message formatting and improve error handling.
The f-string in the error message has an invalid syntax, and the error handling could be more specific.
Apply this diff to fix the issues:
def invalidate_xero_credentials(workspace_id):
try:
xero_credentials = XeroCredentials.get_active_xero_credentials(workspace_id)
if xero_credentials:
if not xero_credentials.is_expired:
patch_integration_settings(workspace_id, is_token_expired=True)
xero_credentials.refresh_token = None
xero_credentials.is_expired = True
xero_credentials.save()
except XeroCredentials.DoesNotExist as error:
- logger.error(f'Xero credentials not found for {workspace_id = }:', )
+ logger.error(f'Xero credentials not found for workspace_id: {workspace_id}')
logger.error(error, exc_info=True)
+ except Exception as error:
+ logger.error(f'Unexpected error while invalidating Xero credentials for workspace_id: {workspace_id}')
+ logger.error(error, exc_info=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def invalidate_xero_credentials(workspace_id): | |
| try: | |
| xero_credentials = XeroCredentials.get_active_xero_credentials(workspace_id) | |
| if xero_credentials: | |
| if not xero_credentials.is_expired: | |
| patch_integration_settings(workspace_id, is_token_expired=True) | |
| xero_credentials.refresh_token = None | |
| xero_credentials.is_expired = True | |
| xero_credentials.save() | |
| except XeroCredentials.DoesNotExist as error: | |
| logger.error(f'Xero credentials not found for {workspace_id = }:', ) | |
| logger.error(error, exc_info=True) | |
| def invalidate_xero_credentials(workspace_id): | |
| try: | |
| xero_credentials = XeroCredentials.get_active_xero_credentials(workspace_id) | |
| if xero_credentials: | |
| if not xero_credentials.is_expired: | |
| patch_integration_settings(workspace_id, is_token_expired=True) | |
| xero_credentials.refresh_token = None | |
| xero_credentials.is_expired = True | |
| xero_credentials.save() | |
| except XeroCredentials.DoesNotExist as error: | |
| logger.error(f'Xero credentials not found for workspace_id: {workspace_id}') | |
| logger.error(error, exc_info=True) | |
| except Exception as error: | |
| logger.error(f'Unexpected error while invalidating Xero credentials for workspace_id: {workspace_id}') | |
| logger.error(error, exc_info=True) |
JustARatherRidiculouslyLongUsername
deployed
to
CI Environment
GitHub Actions
Active
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
feat: PATCH
/integrationson export errorsfeat: patch
/integrationson token expiry (feat: patch/integrationson token expiry #436)feat: patch
/integrationson token expiryfix: move
patch_integration_settingstoapps/workspaces/actions.pyAvoids a circular import between actions.py and tasks.py
test: unit test
/integrationspatch calls (test: unit test/integrationspatch calls #437)test: unit test
/integrationspatch callsfix: token expiry logic (fix: token expiry logic #440)
fix: token expiry logic
fix: rename invalidate_token to invalidate_xero_credentials
test: unit test updates for comment fixes (test: unit test updates for comment fixes #441)