feat: patch integrations-settings-api on export errors #712
Conversation
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThis pull request introduces new patch functionalities. A new function, Changes
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ 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 (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/fyle/helpers.py (1)
100-122: LGTM! Consider refactoring to reduce code duplication.The patch_request function is well implemented, but there's significant code duplication with the post_request function. Consider refactoring both functions to use a common helper.
Here's a suggested refactoring:
+def _make_request(method, url, body, refresh_token=None): + """ + Create a HTTP request. + """ + access_token = None + api_headers = { + 'Content-Type': 'application/json', + } + if refresh_token: + access_token = get_access_token(refresh_token) + api_headers['Authorization'] = 'Bearer {0}'.format(access_token) + + response = getattr(requests, method)( + url, + headers=api_headers, + data=json.dumps(body) + ) + + if response.status_code in [200, 201]: + return json.loads(response.text) + else: + raise Exception(response.text) + def post_request(url, body, refresh_token=None): - """ - Create a HTTP post request. - """ - access_token = None - api_headers = { - 'Content-Type': 'application/json', - } - if refresh_token: - access_token = get_access_token(refresh_token) - - api_headers['Authorization'] = 'Bearer {0}'.format(access_token) - - response = requests.post( - url, - headers=api_headers, - data=json.dumps(body) - ) - - if response.status_code in [200, 201]: - return json.loads(response.text) - else: - raise Exception(response.text) + return _make_request('post', url, body, refresh_token) def patch_request(url, body, refresh_token=None): - """ - Create a HTTP patch request. - """ - access_token = None - api_headers = { - 'Content-Type': 'application/json', - } - if refresh_token: - access_token = get_access_token(refresh_token) - - api_headers['Authorization'] = 'Bearer {0}'.format(access_token) - - response = requests.patch( - url, - headers=api_headers, - data=json.dumps(body) - ) - - if response.status_code in [200, 201]: - return json.loads(response.text) - else: - raise Exception(response.text) + return _make_request('patch', url, body, refresh_token)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/fyle/helpers.py(1 hunks)apps/netsuite/actions.py(2 hunks)apps/workspaces/tasks.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pytest
🔇 Additional comments (2)
apps/netsuite/actions.py (1)
5-5: LGTM! The integration of error logging is well placed.The patch_integration_settings call is appropriately placed after saving the last_export_detail, ensuring that error counts are updated after processing is complete.
Also applies to: 26-26
apps/workspaces/tasks.py (1)
10-10: LGTM! Well-structured error logging implementation.The patch_integration_settings function is well implemented with proper error handling and logging. The function correctly retrieves the refresh token and constructs the payload for updating integration errors.
Also applies to: 231-245
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/test_netsuite/test_actions.py (3)
3-3: Remove unused import.The
ANYimport fromunittest.mockis not used in the test file.-from unittest.mock import ANY, MagicMock +from unittest.mock import MagicMock🧰 Tools
🪛 Ruff (0.8.2)
3-3:
unittest.mock.ANYimported but unusedRemove unused import:
unittest.mock.ANY(F401)
9-14: LGTM! Clear and descriptive docstring.The docstring effectively explains the test's purpose and expected behavior.
Remove trailing whitespace in the docstring:
- should do a patch request to integrations settings api to + should do a patch request to integrations settings api to
16-34: Consider enhancing test coverage.While the test effectively verifies the payload, consider adding these assertions to make it more comprehensive:
- Verify the URL and headers of the patch request
- Assert that LastExportDetail was updated correctly
- Verify that all TaskLogs were processed
Here's how you could enhance the test:
update_last_export_details(workspace_id) failed_count = len([i for i in mock_task_logs if i['status'] in ('FAILED', 'FATAL')]) expected_payload = {'errors_count': failed_count, 'tpa_name': 'Fyle Netsuite Integration'} _, kwargs = mocked_patch.call_args +url = kwargs['url'] actual_payload = json.loads(kwargs['data']) assert actual_payload == expected_payload +assert url == 'https://integrations-settings.fyle.tech/api/workspaces/1/integrations' +assert kwargs['headers'] == {'Content-Type': 'application/json'} + +# Verify LastExportDetail was updated +last_export = LastExportDetail.objects.get(workspace_id=workspace_id) +assert last_export.last_exported_at is not None + +# Verify all TaskLogs were processed +assert TaskLog.objects.filter(workspace_id=workspace_id).count() == len(mock_task_logs)tests/test_netsuite/fixtures.py (1)
7666-7687: LGTM! The task logs data structure is well-organized.The task logs mock data covers key scenarios including both successful and failed operations.
Consider adding more metadata fields that could be useful for debugging like:
- timestamp
- error message/details
- user/context info
- correlation IDs
'task_logs': [ { 'type': 'CREATING_BILL', - 'status': 'FAILED' + 'status': 'FAILED', + 'timestamp': '2025-02-10T10:00:00Z', + 'error_details': 'Connection timeout', + 'user': 'test_user', + 'correlation_id': 'abc-123' }, ... ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_netsuite/fixtures.py(1 hunks)tests/test_netsuite/test_actions.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_netsuite/test_actions.py
3-3: unittest.mock.ANY imported but unused
Remove unused import: unittest.mock.ANY
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pytest
Description
Send a PATCH request to integrations-settings-api and update the
integrationstable to record all integration errors. This will be used in the admin tasks page to show a task to resolve these errorsClickup
https://app.clickup.com/t/86cxhftgj
Summary by CodeRabbit
New Features
Tests