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.

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: expense migration #176

Merged
merged 5 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
18 changes: 18 additions & 0 deletions apps/fyle/migrations/0004_expense_is_posted_at_null.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 4.1.2 on 2024-11-22 10:15

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('fyle', '0003_remove_expense_settlement_id'),
]

operations = [
migrations.AddField(
model_name='expense',
name='is_posted_at_null',
field=models.BooleanField(default=False, help_text='Flag check if posted at is null or not'),
),
]
Comment on lines +12 to +18
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add data migration for existing records.

The migration adds the field with a default False value, but existing records should have this field populated based on their actual posted_at values. Consider adding a data migration to set the correct values.

Would you like me to help create a data migration that:

  1. Queries all existing expenses
  2. Sets is_posted_at_null=True for records where posted_at IS NULL
  3. Sets is_posted_at_null=False for records where posted_at IS NOT NULL

2 changes: 2 additions & 0 deletions apps/fyle/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ def create_expense_objects(expenses: List[Dict], workspace_id: int, skip_update:

# Create an empty list to store expense objects
expense_objects = []
print(expenses)
Copy link
Contributor

@Hrishabh17 Hrishabh17 Nov 22, 2024

Choose a reason for hiding this comment

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

print pls remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed sir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


for expense in expenses:
# Iterate through custom property fields and handle empty values
Expand Down Expand Up @@ -145,6 +146,7 @@ def create_expense_objects(expenses: List[Dict], workspace_id: int, skip_update:
'report_id': expense['report_id'],
'spent_at': expense['spent_at'],
'posted_at': expense['posted_at'],
'is_posted_at_null': expense['is_posted_at_null'],
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 input validation for is_posted_at_null

The method assumes is_posted_at_null will be present in the input dictionary. Consider adding validation or a default value to handle missing or invalid data.

 defaults = {
-    'is_posted_at_null': expense['is_posted_at_null'],
+    'is_posted_at_null': bool(expense.get('is_posted_at_null', False)),
     'fund_source': SOURCE_ACCOUNT_MAP[expense['source_account_type']],
📝 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
'is_posted_at_null': expense['is_posted_at_null'],
'is_posted_at_null': bool(expense.get('is_posted_at_null', False)),

'fund_source': SOURCE_ACCOUNT_MAP[expense['source_account_type']],
'verified_at': expense['verified_at'],
'custom_properties': expense['custom_properties'],
Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ ms-dynamics-business-central-sdk==1.5.2
# Reusable Fyle Packages
fyle-rest-auth==1.7.2
fyle-accounting-mappings==1.34.8
fyle-integrations-platform-connector==1.38.4
fyle-integrations-platform-connector==1.39.3

# Postgres Dependincies
psycopg2-binary==2.9.9
Expand Down
1 change: 1 addition & 0 deletions tests/test_business_central/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@
'spent_at':'2022-05-13T17:00:00Z',
'approved_at':'2022-05-13T09:30:13.484000Z',
'posted_at': '2021-12-22T07:30:26.289842+00:00',
'is_posted_at_null': False,
'expense_created_at':'2022-05-13T09:29:43.535468Z',
'expense_updated_at':'2022-05-13T09:32:06.643941Z',
'created_at':'2022-05-23T11:11:28.241406Z',
Expand Down
3 changes: 3 additions & 0 deletions tests/test_fyle/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@
'expense_rule_data': None,
'expense_rule_id': None,
'extracted_data': None,
'is_posted_at_null': True,
'file_ids': [],
'files': [],
'foreign_amount': None,
Expand Down Expand Up @@ -223,6 +224,7 @@
'spent_at': '2024-05-10 17:00:00',
'approved_at': '2024-05-10 07:53:25',
'posted_at': None,
'is_posted_at_null': True,
'is_skipped': False,
'expense_created_at': '2024-05-10 07:52:10',
'expense_updated_at': '2024-05-13 05:53:25',
Expand Down Expand Up @@ -589,6 +591,7 @@
'spent_at':'2022-05-13T17:00:00Z',
'approved_at':'2022-05-13T09:30:13.484000Z',
'posted_at': '2021-12-22T07:30:26.289842+00:00',
'is_posted_at_null': False,
'expense_created_at':'2022-05-13T09:29:43.535468Z',
'expense_updated_at':'2022-05-13T09:32:06.643941Z',
'created_at':'2022-05-23T11:11:28.241406Z',
Expand Down
11 changes: 5 additions & 6 deletions tests/test_fyle/test_tasks.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
from .fixtures import fixtures as data
from django.urls import reverse
from rest_framework.exceptions import ValidationError
from rest_framework import status
from apps.fyle.tasks import (
update_non_exported_expenses
)
from rest_framework.exceptions import ValidationError

from apps.accounting_exports.models import AccountingExport
from apps.fyle.models import Expense
from apps.fyle.tasks import update_non_exported_expenses
from apps.workspaces.models import Workspace
from apps.accounting_exports.models import AccountingExport
from tests.test_fyle.fixtures import fixtures as data


def test_update_non_exported_expenses(db, create_temp_workspace, mocker, api_client):
Expand Down
Loading