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

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 1 commit
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
4 changes: 2 additions & 2 deletions apps/accounting_exports/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,8 @@ def create_accounting_export(expense_objects: List[Expense], fund_source: str, w
for accounting_export in accounting_exports:
# 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 and date_field not in ['current_date', 'last_spent_at']:
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic seems wrong
For example if date_field is 'current_date' and accounting_export[date_field] will be None, then it should be set to current date, but this way it will never reach to else block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay will revert this back, the previous code is good not issue then

if 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
4 changes: 2 additions & 2 deletions apps/workspaces/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,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 +75,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
Loading