Add support for editing expenses #87
Conversation
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe recent changes improve exception handling, request validation, and logging for the Fyle application components. New validation functions and error handling mechanisms were added, and logging was enhanced across many files. Additionally, a new function, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebhookCallbackView
participant Helpers
participant Queue
participant Tasks
Client->>WebhookCallbackView: POST /webhook_callback
WebhookCallbackView->>Helpers: assert_valid_request(workspace_id, org_id)
Helpers-->>WebhookCallbackView: Validation Status
WebhookCallbackView->>Queue: async_handle_webhook_callback(body, workspace_id)
Queue->>Tasks: update_non_exported_expenses(data)
Tasks-->>Queue: Update Status
Queue-->>WebhookCallbackView: None
WebhookCallbackView-->>Client: Response Status
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 5
Outside diff range and nitpick comments (2)
apps/fyle/helpers.py (1)
Line range hint
82-82: Use modern Python string formatting.Convert the
formatmethod to an f-string for better readability and performance.- logger.info("| Updating non-exported expenses through webhook | Content: {{WORKSPACE_ID: {} Payload: {}}}".format(workspace_id, body.get('data'))) + logger.info(f"| Updating non-exported expenses through webhook | Content: {{WORKSPACE_ID: {workspace_id} Payload: {body.get('data')}}}")tests/test_fyle/fixtures.py (1)
Line range hint
1479-1482: Security Issue: Potential exposure of AWS credentials in URLs.The download and upload URLs contain AWS access credentials, which should not be exposed in the codebase or test fixtures.
- 'download_url': 'https://fyle-storage-mumbai-3.s3.amazonaws.com/2022-12-02/orFcbTXP4Nzl/integrations/fimNloQVF0D8.bills.iif?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIA54Z3LIXTX6CFH4VG%2F20221202%2Fap-south-1%2Fs3%2Faws4_request&X-Amz-Date=20221202T090102Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host&X-Amz-Signature=049855502d50779c694eb1df93f19a3c2e5386410cce9f337dfa127beddd2140', - 'upload_url': 'https://fyle-storage-mumbai-3.s3.amazonaws.com/2022-12-02/orFcbTXP4Nzl/integrations/fimNloQVF0D8.bills.iif?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIA54Z3LIXTX6CFH4VG%2F20221202%2Fap-south-1%2Fs3%2Faws4_request&X-Amz-Date=20221202T090102Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host&X-Amz-Signature=b9e0943a61356cb8e85b2eaeee0df5464e3450d820387c19a984a132e6a5bd83' + 'download_url': 'https://<URL_REMOVED_FOR_SECURITY>', + 'upload_url': 'https://<URL_REMOVED_FOR_SECURITY>'
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- apps/fyle/exceptions.py (2 hunks)
- apps/fyle/helpers.py (2 hunks)
- apps/fyle/queue.py (2 hunks)
- apps/fyle/tasks.py (2 hunks)
- apps/fyle/views.py (1 hunks)
- requirements.txt (1 hunks)
- tests/test_fyle/fixtures.py (1 hunks)
- tests/test_fyle/test_tasks.py (2 hunks)
Files skipped from review due to trivial changes (1)
- requirements.txt
Additional context used
Ruff
apps/fyle/helpers.py
23-23: Use implicit references for positional format fields (UP030)
Remove explicit positional indices
23-23: Use f-string instead of
formatcall (UP032)Convert to f-string
57-57: Use implicit references for positional format fields (UP030)
Remove explicit positional indices
57-57: Use f-string instead of
formatcall (UP032)Convert to f-string
72-72: Use f-string instead of
formatcall (UP032)Convert to f-string
104-104: Use context handler for opening files (SIM115)
126-126: Use f-string instead of
formatcall (UP032)Convert to f-string
Gitleaks
tests/test_fyle/fixtures.py
1479-1479: Identified a pattern that may indicate AWS credentials, risking unauthorized cloud resource access and data breaches on AWS platforms. (aws-access-token)
1482-1482: Identified a pattern that may indicate AWS credentials, risking unauthorized cloud resource access and data breaches on AWS platforms. (aws-access-token)
Additional comments not posted (4)
apps/fyle/queue.py (2)
6-6: Review new imports and logger setup.The new imports and logger setup are appropriate for the added functionalities. Ensure that the logger configuration is consistent across the application and that sensitive information is not logged.
Also applies to: 14-14, 16-16
68-84: Check the handling ofworkspace_idandorg_idin webhook callbacks.The function now validates requests and logs information about the operation. Ensure that
workspace_idandorg_idare validated against possible SQL injection or incorrect data types before being used in database queries.tests/test_fyle/fixtures.py (2)
2-226: Well-structured test data forraw_expense.The dictionary covers a broad range of fields, which should be beneficial for thorough testing of expense handling functionalities.
227-262: Good representation of default expense data indefault_raw_expense.This should aid in testing scenarios with default or expected values effectively.
| def test_update_non_exported_expenses(db, create_temp_workspace, mocker, api_client): | ||
| expense = fixtures['raw_expense'] | ||
| default_raw_expense = fixtures['default_raw_expense'] | ||
| org_id = expense['org_id'] | ||
| payload = { | ||
| "resource": "EXPENSE", | ||
| "action": 'UPDATED_AFTER_APPROVAL', | ||
| "data": expense, | ||
| "reason": 'expense update testing', | ||
| } | ||
|
|
||
| expense_created, _ = Expense.objects.update_or_create( | ||
| org_id=org_id, | ||
| expense_id='txhJLOSKs1iN', | ||
| workspace_id=1, | ||
| defaults=default_raw_expense | ||
| ) | ||
| expense_created.exported = False | ||
| expense_created.save() | ||
|
|
||
| workspace = Workspace.objects.filter(id=1).first() | ||
| workspace.org_id = org_id | ||
| workspace.save() | ||
|
|
||
| assert expense_created.category == 'Old Category' | ||
|
|
||
| update_non_exported_expenses(payload['data']) | ||
|
|
||
| expense = Expense.objects.get(expense_id='txhJLOSKs1iN', org_id=org_id) | ||
| assert expense.category == 'ABN Withholding' | ||
|
|
||
| expense.exported = True | ||
| expense.category = 'Old Category' | ||
| expense.save() | ||
|
|
||
| update_non_exported_expenses(payload['data']) | ||
| expense = Expense.objects.get(expense_id='txhJLOSKs1iN', org_id=org_id) | ||
| assert expense.category == 'Old Category' | ||
|
|
||
| try: | ||
| update_non_exported_expenses(payload['data']) | ||
| except ValidationError as e: | ||
| assert e.detail[0] == 'Workspace mismatch' | ||
|
|
||
| url = reverse('webhook-callback', kwargs={'workspace_id': 1}) | ||
| response = api_client.post(url, data=payload, format='json') | ||
| assert response.status_code == status.HTTP_200_OK | ||
|
|
||
| url = reverse('webhook-callback', kwargs={'workspace_id': 2}) | ||
| response = api_client.post(url, data=payload, format='json') | ||
| assert response.status_code == status.HTTP_400_BAD_REQUEST |
There was a problem hiding this comment.
Review of test_update_non_exported_expenses
-
Data Isolation: The test seems to use a fixed
workspace_idandexpense_id. Using hardcoded IDs can lead to tests that are not isolated and might interfere with each other if the database state isn't properly reset between tests.- workspace_id=1 - expense_id='txhJLOSKs1iN' + workspace_id=create_temp_workspace.id + expense_id=create_random_id() # Assuming there's a helper to generate random IDs
-
Exception Handling: The test captures a
ValidationErrorand asserts on the error message. This is good as it tests the error handling capabilities of the function. -
HTTP Response Codes: The test checks the HTTP response codes after POST requests to the webhook callback. This is crucial for ensuring that the API behaves correctly under different conditions.
-
Use of Fixtures: The test uses
fixtures['raw_expense']andfixtures['default_raw_expense']effectively to simulate real-world data scenarios. -
Improvement Suggestion: Consider adding more scenarios to test other possible actions and edge cases, for example, what happens when the
expense_iddoes not exist in the database.
[REFACTOR_SUGGESTion]# Add a test scenario for non-existent expense_id def test_update_non_existing_expense(): non_existing_id = 'non_existing_id' payload = { "data": {"expense_id": non_existing_id}, ... } with pytest.raises(ValidationError): update_non_exported_expenses(payload['data'])
-
Consistency in Test Naming: The test function names should consistently reflect the functionality they are testing. The name
test_update_non_exported_expensesis appropriate.
Overall, the test function is well-structured but could benefit from enhanced data isolation and additional test scenarios.
| @@ -31,6 +31,6 @@ class WebhookCallbackView(generics.CreateAPIView): | |||
|
|
|||
| @handle_view_exceptions() | |||
| def post(self, request, *args, **kwargs): | |||
| async_handle_webhook_callback(request.data) | |||
| async_handle_webhook_callback(request.data, int(kwargs['workspace_id'])) | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure proper error handling and logging in webhook callback.
The async_handle_webhook_callback function relies on assert_valid_request to raise a ValidationError for invalid requests. However, there are no explicit try-except blocks in async_handle_webhook_callback to catch and handle these or other potential exceptions. This could lead to unhandled exceptions.
- apps/fyle/queue.py:
- Add try-except blocks around critical sections to handle potential exceptions gracefully.
- Ensure that exceptions are logged appropriately to aid in debugging and monitoring.
Analysis chain
Ensure proper error handling and logging in webhook callback.
The async_handle_webhook_callback function is now receiving workspace_id as an integer. Ensure that this function handles potential errors gracefully, especially with type conversion and when workspace_id is not provided or invalid.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the error handling in `async_handle_webhook_callback`.
# Test: Search for the function definition. Expect: Proper exception handling.
rg --type python 'def async_handle_webhook_callback' -A 20
Length of output: 91
Script:
#!/bin/bash
# Description: Verify the error handling in `async_handle_webhook_callback`.
# Correct the file type and search for the function definition. Expect: Proper exception handling.
rg 'def async_handle_webhook_callback' -A 50
Length of output: 1328
Script:
#!/bin/bash
# Description: Verify the error handling in `assert_valid_request`.
# Search for the function definition of `assert_valid_request`. Expect: Proper exception handling.
rg 'def assert_valid_request' -A 20
Length of output: 517
| except ValidationError as e: | ||
| logger.exception(e) | ||
| return Response({"message": e.detail}, status=status.HTTP_400_BAD_REQUEST) |
There was a problem hiding this comment.
Ensure detailed logging and user-friendly error messages for ValidationError.
The new exception handling for ValidationError logs the exception and returns a detailed message. Ensure that the log level is appropriate (e.g., ERROR rather than INFO) and that the message is clear and user-friendly.
- logger.exception(e)
+ logger.error("Validation error occurred", 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.
| except ValidationError as e: | |
| logger.exception(e) | |
| return Response({"message": e.detail}, status=status.HTTP_400_BAD_REQUEST) | |
| except ValidationError as e: | |
| logger.error("Validation error occurred", exc_info=True) | |
| return Response({"message": e.detail}, status=status.HTTP_400_BAD_REQUEST) |
| def assert_valid_request(workspace_id:int, org_id:str): | ||
| """ | ||
| Assert if the request is valid by checking | ||
| the url_workspace_id and fyle_org_id workspace | ||
| """ | ||
| workspace = Workspace.objects.get(org_id=org_id) | ||
| if workspace.id != workspace_id: | ||
| raise ValidationError('Workspace mismatch') |
There was a problem hiding this comment.
Validate workspace and org ID matching logic in assert_valid_request.
Ensure that the function assert_valid_request correctly checks workspace and organization IDs. Consider adding logging for failed validations to aid in troubleshooting.
- raise ValidationError('Workspace mismatch')
+ logger.warning("Workspace mismatch - attempted access with org_id: %s", org_id)
+ raise ValidationError(f"Workspace mismatch for org_id: {org_id}")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 assert_valid_request(workspace_id:int, org_id:str): | |
| """ | |
| Assert if the request is valid by checking | |
| the url_workspace_id and fyle_org_id workspace | |
| """ | |
| workspace = Workspace.objects.get(org_id=org_id) | |
| if workspace.id != workspace_id: | |
| raise ValidationError('Workspace mismatch') | |
| def assert_valid_request(workspace_id:int, org_id:str): | |
| """ | |
| Assert if the request is valid by checking | |
| the url_workspace_id and fyle_org_id workspace | |
| """ | |
| workspace = Workspace.objects.get(org_id=org_id) | |
| if workspace.id != workspace_id: | |
| logger.warning("Workspace mismatch - attempted access with org_id: %s", org_id) | |
| raise ValidationError(f"Workspace mismatch for org_id: {org_id}") |
| def update_non_exported_expenses(data: Dict) -> None: | ||
| """ | ||
| To update expenses not in COMPLETE, IN_PROGRESS state | ||
| """ | ||
| org_id = data['org_id'] | ||
| expense_id = data['id'] | ||
| workspace = Workspace.objects.get(org_id=org_id) | ||
| expense = Expense.objects.filter(workspace_id=workspace.id, expense_id=expense_id).first() | ||
|
|
||
| if expense and not expense.exported: | ||
| expense_obj = [] | ||
| expense_obj.append(data) | ||
| expense_objects = FyleExpenses().construct_expense_object(expense_obj, expense.workspace_id) | ||
| Expense.create_expense_objects( | ||
| expense_objects, expense.workspace_id | ||
| ) |
There was a problem hiding this comment.
Ensure robust handling in update_non_exported_expenses.
The function should handle cases where the expense does not exist or has already been exported more robustly. Consider adding more detailed logging and error handling.
- if expense and not expense.exported:
+ if not expense:
+ logger.error("Expense with ID %s not found in workspace %s", expense_id, workspace.id)
+ return
+ elif expense.exported:
+ logger.info("Expense with ID %s already exported from workspace %s", expense_id, workspace.id)
+ returnCommittable 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 update_non_exported_expenses(data: Dict) -> None: | |
| """ | |
| To update expenses not in COMPLETE, IN_PROGRESS state | |
| """ | |
| org_id = data['org_id'] | |
| expense_id = data['id'] | |
| workspace = Workspace.objects.get(org_id=org_id) | |
| expense = Expense.objects.filter(workspace_id=workspace.id, expense_id=expense_id).first() | |
| if expense and not expense.exported: | |
| expense_obj = [] | |
| expense_obj.append(data) | |
| expense_objects = FyleExpenses().construct_expense_object(expense_obj, expense.workspace_id) | |
| Expense.create_expense_objects( | |
| expense_objects, expense.workspace_id | |
| ) | |
| def update_non_exported_expenses(data: Dict) -> None: | |
| """ | |
| To update expenses not in COMPLETE, IN_PROGRESS state | |
| """ | |
| org_id = data['org_id'] | |
| expense_id = data['id'] | |
| workspace = Workspace.objects.get(org_id=org_id) | |
| expense = Expense.objects.filter(workspace_id=workspace.id, expense_id=expense_id).first() | |
| if not expense: | |
| logger.error("Expense with ID %s not found in workspace %s", expense_id, workspace.id) | |
| return | |
| elif expense.exported: | |
| logger.info("Expense with ID %s already exported from workspace %s", expense_id, workspace.id) | |
| return | |
| expense_obj = [] | |
| expense_obj.append(data) | |
| expense_objects = FyleExpenses().construct_expense_object(expense_obj, expense.workspace_id) | |
| Expense.create_expense_objects( | |
| expense_objects, expense.workspace_id | |
| ) |
| elif body.get('action') == 'UPDATED_AFTER_APPROVAL' and body.get('data') and body.get('resource') == 'EXPENSE': | ||
| org_id = body['data']['org_id'] | ||
| logger.info("| Updating non-exported expenses through webhook | Content: {{WORKSPACE_ID: {} Payload: {}}}".format(workspace_id, body.get('data'))) | ||
| assert_valid_request(workspace_id=workspace_id, org_id=org_id) |
Summary by CodeRabbit
New Features
Updates
Tests
update_non_exported_expensesto ensure proper handling of expense updates and validation.