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

Fix: All the bug fixes listed #170

Merged
merged 11 commits into from
Nov 14, 2024
Merged

Fix: All the bug fixes listed #170

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
24614c4
Fix: All the bug fixes listed
Ashutosh619-sudo Nov 5, 2024
ed15855
credentials expired issue resolved
Ashutosh619-sudo Nov 11, 2024
c12f413
pylint fix
Ashutosh619-sudo Nov 11, 2024
26edc76
test fail resolved
Ashutosh619-sudo Nov 11, 2024
5efb93d
pylint resolved
Ashutosh619-sudo Nov 11, 2024
3228379
comment resolved
Ashutosh619-sudo Nov 11, 2024
f5dc2eb
small change
Ashutosh619-sudo Nov 12, 2024
d419249
pylint fix
Ashutosh619-sudo Nov 12, 2024
6029b23
fix
Ashutosh619-sudo Nov 12, 2024
b82d962
remove intacct name
Ashutosh619-sudo Nov 12, 2024
25b8c66
Merge branch 'master' into bug-fix-beta
Ashutosh619-sudo Nov 13, 2024
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
2 changes: 1 addition & 1 deletion apps/accounting_exports/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ def create_accounting_export(expense_objects: List[Expense], fund_source: str, w
# Determine the date field based on fund_source
date_field = getattr(export_setting, f"{fund_source_map.get(fund_source)}_expense_date", None).lower()
if date_field and date_field != 'last_spent_at':
if date_field != 'current_date' and accounting_export[date_field]:
if date_field != 'current_date' and date_field in accounting_export and accounting_export[date_field]:
accounting_export[date_field] = accounting_export[date_field].strftime('%Y-%m-%d')
else:
accounting_export[date_field] = datetime.now().strftime('%Y-%m-%d')
Expand Down
1 change: 1 addition & 0 deletions apps/business_central/actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ def update_accounting_export_summary(workspace_id):
successful_exports = AccountingExport.objects.filter(
~Q(type__in=['FETCHING_REIMBURSABLE_EXPENSES', 'FETCHING_CREDIT_CARD_EXPENSES']),
workspace_id=workspace_id, status='COMPLETE',
updated_at__gte=accounting_export_summary.last_exported_at
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add null check for last_exported_at.

The code assumes last_exported_at is always set. If it's null, this could cause unexpected behavior.

Consider adding a null check:

+   last_exported_at = accounting_export_summary.last_exported_at or datetime.min
    successful_exports = AccountingExport.objects.filter(
        ~Q(type__in=['FETCHING_REIMBURSABLE_EXPENSES', 'FETCHING_CREDIT_CARD_EXPENSES']),
        workspace_id=workspace_id,
        status='COMPLETE',
-       updated_at__gte=accounting_export_summary.last_exported_at
+       updated_at__gte=last_exported_at
    ).count()

Don't forget to add the import:

from datetime import datetime

⚠️ Potential issue

Review the export counting logic for potential discrepancies.

The new time-based filter is only applied to successful exports but not to failed ones. This inconsistency could lead to:

  1. Inaccurate total counts since failed and successful exports are filtered differently
  2. Missing successful exports that were updated before last_exported_at

Consider applying consistent filtering:

    failed_exports = AccountingExport.objects.filter(
        ~Q(type__in=['FETCHING_REIMBURSABLE_EXPENSES', 'FETCHING_CREDIT_CARD_EXPENSES']),
-       workspace_id=workspace_id, status__in=['FAILED', 'FATAL']
+       workspace_id=workspace_id,
+       status__in=['FAILED', 'FATAL'],
+       updated_at__gte=accounting_export_summary.last_exported_at
    ).count()

Committable suggestion skipped: line range outside the PR's diff.

).count()

accounting_export_summary.failed_accounting_export_count = failed_exports
Expand Down
4 changes: 2 additions & 2 deletions apps/business_central/exports/base_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def get_expense_comment(workspace_id, lineitem: Expense, category: str, advance_
workspace.cluster_domain = cluster_domain
workspace.save()

expense_link = '{0}/app/main/#/enterprise/view_expense/{1}?org_id={2}'.format(
expense_link = '{0}/app/admin/#/enterprise/view_expense/{1}?org_id={2}'.format(
cluster_domain, lineitem.expense_id, org_id
)

Expand Down Expand Up @@ -169,7 +169,7 @@ def get_location_id(accounting_export: AccountingExport, lineitem: Expense):
elif location_setting.source_field == 'COST_CENTER':
source_value = lineitem.cost_center
else:
attribute = ExpenseAttribute.objects.filter(attribute_type=location_setting.source_field).first()
attribute = ExpenseAttribute.objects.filter(attribute_type=location_setting.source_field, workspace_id=accounting_export.workspace_id).first()
source_value = lineitem.custom_properties.get(attribute.display_name, None)

mapping: Mapping = Mapping.objects.filter(
Expand Down
15 changes: 12 additions & 3 deletions apps/workspaces/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from apps.business_central.exports.journal_entry.tasks import ExportJournalEntry
from apps.business_central.exports.purchase_invoice.tasks import ExportPurchaseInvoice
from apps.fyle.queue import queue_import_credit_card_expenses, queue_import_reimbursable_expenses
from apps.workspaces.models import AdvancedSetting, ExportSetting, FyleCredential
from apps.workspaces.models import AdvancedSetting, BusinessCentralCredentials, ExportSetting, FyleCredential

from fyle_integrations_platform_connector import PlatformConnector

Expand All @@ -31,6 +31,15 @@ def run_import_export(workspace_id: int, export_mode = None):
:param workspace_id: Workspace id
"""

if export_mode == 'AUTOMATIC':
business_central_creds = BusinessCentralCredentials.objects.filter(
workspace_id=workspace_id, is_expired=False, refresh_token__isnull=False
).first()

if not business_central_creds:
logger.info('Credentials have expired for workspace_id %s', workspace_id)
return

export_settings = ExportSetting.objects.get(workspace_id=workspace_id)
advance_settings = AdvancedSetting.objects.get(workspace_id=workspace_id)
Comment on lines 42 to 43
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Error handling for missing settings is not implemented

The .get calls for ExportSetting and AdvancedSetting are not wrapped in try-except blocks handling DoesNotExist exceptions.

🔗 Analysis chain

Verify error handling for missing settings

The code directly retrieves ExportSetting and AdvancedSetting without handling potential DoesNotExist exceptions. Consider adding proper error handling for cases where these settings don't exist.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for error handling patterns in the codebase
rg -A 2 "ExportSetting.objects.get|AdvancedSetting.objects.get" --type py

Length of output: 9159

Script:

#!/bin/bash
# Check for proper DoesNotExist exception handling around ExportSetting.objects.get and AdvancedSetting.objects.get
rg -A 4 "try:" --type py | rg "(ExportSetting\.objects\.get|AdvancedSetting\.objects\.get)\(workspace_id=workspace_id\)" -B 1 | rg "except .*DoesNotExist"

Length of output: 154

Script:

#!/bin/bash
# Use ast-grep to find ExportSetting.objects.get and AdvancedSetting.objects.get within try-except blocks handling DoesNotExist
ast-grep --pattern "
try:
    $$$
    (ExportSetting.objects.get($_) | AdvancedSetting.objects.get($_))
    $$$
except DoesNotExist:
    $$$
"

Length of output: 172

accounting_summary, _ = AccountingExportSummary.objects.update_or_create(
Expand Down Expand Up @@ -58,7 +67,7 @@ def run_import_export(workspace_id: int, export_mode = None):

if accounting_export.status == 'COMPLETE':
accounting_export_ids = AccountingExport.objects.filter(
fund_source='PERSONAL', exported_at__isnull=True).values_list('id', flat=True)
fund_source='PERSONAL', exported_at__isnull=True, workspace_id=workspace_id).values_list('id', flat=True)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical security fix: Proper workspace isolation added

Adding the workspace filter is essential for maintaining proper data isolation between different workspaces. This fix prevents potential data leakage across workspaces.


if len(accounting_export_ids):
is_expenses_exported = True
Expand All @@ -75,7 +84,7 @@ def run_import_export(workspace_id: int, export_mode = None):
)
if accounting_export.status == 'COMPLETE':
accounting_export_ids = AccountingExport.objects.filter(
fund_source='CCC', exported_at__isnull=True).values_list('id', flat=True)
fund_source='CCC', exported_at__isnull=True, workspace_id=workspace_id).values_list('id', flat=True)

if len(accounting_export_ids):
is_expenses_exported = True
Expand Down
10 changes: 9 additions & 1 deletion tests/test_workspaces/test_tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
async_create_admin_subcriptions
)
from apps.accounting_exports.models import AccountingExport, AccountingExportSummary
from apps.workspaces.models import FyleCredential, AdvancedSetting, ExportSetting
from apps.workspaces.models import BusinessCentralCredentials, FyleCredential, AdvancedSetting, ExportSetting
from django_q.models import Schedule
from django.conf import settings
from django.urls import reverse
Expand Down Expand Up @@ -64,6 +64,10 @@ def test_run_import_export_with_reimbursable_expense(
'trigger_export'
)

BusinessCentralCredentials.objects.create(
workspace_id=workspace_id, is_expired=False, refresh_token='bsajkdbasjb'
)
Comment on lines +67 to +69
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add test coverage for missing/expired credentials.

While adding the credential creation is good, we should also verify the behavior when:

  1. Credentials are missing
  2. Credentials are expired (is_expired=True)

This ensures we have complete test coverage for the new credential validation in run_import_export.

Example test cases to add:

def test_run_import_export_without_credentials(
    db,
    mocker,
    create_temp_workspace,
    add_fyle_credentials,
    create_export_settings,
    add_advanced_settings,
    create_accounting_export_expenses
):
    workspace_id = 1
    # Don't create credentials
    run_import_export(workspace_id=workspace_id)
    # Assert expected behavior

def test_run_import_export_with_expired_credentials(
    db,
    mocker,
    create_temp_workspace,
    add_fyle_credentials,
    create_export_settings,
    add_advanced_settings,
    create_accounting_export_expenses
):
    workspace_id = 1
    BusinessCentralCredentials.objects.create(
        workspace_id=workspace_id,
        is_expired=True,
        refresh_token='expired_token'
    )
    run_import_export(workspace_id=workspace_id)
    # Assert expected behavior


run_import_export(workspace_id=workspace_id)

accounting_summary = AccountingExportSummary.objects.get(workspace_id=workspace_id)
Expand Down Expand Up @@ -114,6 +118,10 @@ def test_run_import_export_with_credit_card_expenses(
'trigger_export'
)

BusinessCentralCredentials.objects.create(
workspace_id=workspace_id, is_expired=False, refresh_token='bsajkdbasjb'
)

run_import_export(workspace_id=workspace_id, export_mode='AUTOMATIC')

accounting_summary = AccountingExportSummary.objects.get(workspace_id=workspace_id)
Expand Down
Loading