Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for editing expenses #87

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Add support for editing expenses #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions apps/fyle/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from fyle.platform.exceptions import NoPrivilegeError, RetryException, InvalidTokenError as FyleInvalidTokenError
from rest_framework.response import Response
from rest_framework.views import status
from rest_framework.exceptions import ValidationError

from apps.workspaces.models import FyleCredential, Workspace, ExportSettings, AdvancedSetting
from apps.tasks.models import AccountingExport
Expand Down Expand Up @@ -43,6 +44,10 @@ def new_fn(*args, **kwargs):
except ExportSettings.DoesNotExist:
return Response({'message': 'Export Settings does not exist in workspace'}, status=status.HTTP_400_BAD_REQUEST)

except ValidationError as e:
logger.exception(e)
return Response({"message": e.detail}, status=status.HTTP_400_BAD_REQUEST)
Comment on lines +47 to +49
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)


except Exception as exception:
logger.exception(exception)
return Response(data={'message': 'An unhandled error has occurred, please re-try later'}, status=status.HTTP_400_BAD_REQUEST)
Expand Down
13 changes: 12 additions & 1 deletion apps/fyle/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@

from django.conf import settings
from fyle.platform import Platform
from rest_framework.exceptions import ValidationError

from apps.workspaces.models import FyleCredential
from apps.workspaces.models import Workspace, FyleCredential


def post_request(url, body, refresh_token=None):
Expand Down Expand Up @@ -142,3 +143,13 @@ def download_iif_file(file_id: str, workspace_id: int):
)['data'][0]['download_url']

return download_url


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')
Comment on lines +148 to +155
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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}")

17 changes: 14 additions & 3 deletions apps/fyle/queue.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,18 @@
* User Triggered Async Tasks
* Schedule Triggered Async Tasks
"""
import logging
from django_q.tasks import async_task
from apps.fyle.tasks import (
import_credit_card_expenses,
import_reimbursable_expenses
)
from apps.workspaces.models import Workspace
from apps.tasks.models import AccountingExport
from apps.fyle.helpers import assert_valid_request

logger = logging.getLogger(__name__)
logger.level = logging.INFO


def queue_import_reimbursable_expenses(workspace_id: int, synchronous: bool = False):
Expand Down Expand Up @@ -60,14 +65,20 @@ def queue_import_credit_card_expenses(workspace_id: int, synchronous: bool = Fal
import_credit_card_expenses(workspace_id, accounting_export)


def async_handle_webhook_callback(body: dict) -> None:
def async_handle_webhook_callback(body: dict, workspace_id: int) -> None:
"""
Async'ly import and export expenses
:param body: bodys
:param body: body
:return: None
"""
if body.get('action') == 'ACCOUNTING_EXPORT_INITIATED' and body.get('data'):
org_id = body['data']['org_id']

assert_valid_request(workspace_id=workspace_id, org_id=org_id)
workspace = Workspace.objects.get(org_id=org_id)
async_task('apps.workspaces.tasks.run_import_export', 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move out

async_task('apps.fyle.tasks.update_non_exported_expenses', body['data'])
20 changes: 20 additions & 0 deletions apps/fyle/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@
2. Import Credit Card Expenses from Fyle
"""
import logging
from typing import Dict
from datetime import datetime
import traceback

from django.db import transaction

from fyle_integrations_platform_connector import PlatformConnector
from fyle.platform.exceptions import RetryException, NoPrivilegeError
from fyle_integrations_platform_connector.apis.expenses import Expenses as FyleExpenses

from apps.tasks.models import AccountingExport
from apps.workspaces.models import Workspace, ExportSettings, FyleCredential
Expand Down Expand Up @@ -151,3 +153,21 @@ def import_credit_card_expenses(workspace_id, accounting_export: AccountingExpor
accounting_export.status = 'FATAL'
accounting_export.save()
logger.exception('Something unexpected happened workspace_id: %s %s', accounting_export.workspace_id, accounting_export.errors)


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
)
Comment on lines +158 to +173
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
+     return
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.

Suggested change
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
)

2 changes: 1 addition & 1 deletion apps/fyle/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return Response(data={}, status=status.HTTP_200_OK)
6 changes: 3 additions & 3 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ gevent==23.9.1
gunicorn==20.1.0

# Platform SDK
fyle==0.36.1
fyle==0.37.0

# Reusable Fyle Packages
fyle-rest-auth==1.7.2
fyle-accounting-mappings==1.32.3
fyle-integrations-platform-connector==1.37.4
fyle-accounting-mappings==1.33.1
fyle-integrations-platform-connector==1.38.1

# Postgres Dependincies
psycopg2-binary==2.8.4
Expand Down
Loading
Loading