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

add support for skipped expenses api #53

Merged
merged 2 commits into from
Dec 18, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
92 changes: 91 additions & 1 deletion apps/fyle/helpers.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,104 @@
import json

import requests
from typing import List
from django.conf import settings
from django.db.models import Q

from fyle_integrations_platform_connector import PlatformConnector

from apps.accounting_exports.models import AccountingExport
from apps.fyle.constants import DEFAULT_FYLE_CONDITIONS
from apps.fyle.models import ExpenseFilter
from apps.workspaces.models import ExportSetting, FyleCredential


def construct_expense_filter(expense_filter):
constructed_expense_filter = {}
# If the expense filter is a custom field
if expense_filter.is_custom:
# If the operator is not isnull
if expense_filter.operator != 'isnull':
# If the custom field is of type SELECT and the operator is not_in
if expense_filter.custom_field_type == 'SELECT' and expense_filter.operator == 'not_in':
# Construct the filter for the custom property
filter1 = {
f'custom_properties__{expense_filter.condition}__in': expense_filter.values
}
# Invert the filter using the ~Q operator and assign it to the constructed expense filter
constructed_expense_filter = ~Q(**filter1)
else:
# If the custom field is of type NUMBER, convert the values to integers
if expense_filter.custom_field_type == 'NUMBER':
expense_filter.values = [int(value) for value in expense_filter.values]
# If the expense filter is a custom field and the operator is yes or no(checkbox)
if expense_filter.custom_field_type == 'BOOLEAN':
expense_filter.values[0] = True if expense_filter.values[0] == 'true' else False
# Construct the filter for the custom property
filter1 = {
f'custom_properties__{expense_filter.condition}__{expense_filter.operator}':
expense_filter.values[0] if len(expense_filter.values) == 1 and expense_filter.operator != 'in'
else expense_filter.values
}
# Assign the constructed filter to the constructed expense filter
constructed_expense_filter = Q(**filter1)

# If the expense filter is a custom field and the operator is isnull
elif expense_filter.operator == 'isnull':
# Determine the value for the isnull filter based on the first value in the values list
expense_filter_value: bool = True if expense_filter.values[0].lower() == 'true' else False
# Construct the isnull filter for the custom property
filter1 = {
f'custom_properties__{expense_filter.condition}__isnull': expense_filter_value
}
# Construct the exact filter for the custom property
filter2 = {
f'custom_properties__{expense_filter.condition}__exact': None
}
if expense_filter_value:
# If the isnull filter value is True, combine the two filters using the | operator and assign it to the constructed expense filter
constructed_expense_filter = Q(**filter1) | Q(**filter2)
else:
# If the isnull filter value is False, invert the exact filter using the ~Q operator and assign it to the constructed expense filter
constructed_expense_filter = ~Q(**filter2)

# For all non-custom fields
else:
# Construct the filter for the non-custom field
filter1 = {
f'{expense_filter.condition}__{expense_filter.operator}':
expense_filter.values[0] if len(expense_filter.values) == 1 and expense_filter.operator != 'in'
else expense_filter.values
}
# Assign the constructed filter to the constructed expense filter
constructed_expense_filter = Q(**filter1)

# Return the constructed expense filter
return constructed_expense_filter
Comment on lines +15 to +76
Copy link

Choose a reason for hiding this comment

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

The construct_expense_filter function correctly handles the construction of filters for different custom field types and operators. However, consider the following points:

  1. For the BOOLEAN type, the conversion from string to boolean should be case-insensitive.
  2. Ensure that the expense_filter.values list always contains the expected data types before performing operations like integer conversion or boolean checks.
  3. Validate that the expense_filter.condition and expense_filter.operator values are sanitized and do not introduce any security vulnerabilities such as SQL injection.



def construct_expense_filter_query(expense_filters: List[ExpenseFilter]):
final_filter = None
join_by = None
for expense_filter in expense_filters:
constructed_expense_filter = construct_expense_filter(expense_filter)

# If this is the first filter, set it as the final filter
if expense_filter.rank == 1:
final_filter = (constructed_expense_filter)

# If join by is AND, OR
elif expense_filter.rank != 1:
if join_by == 'AND':
final_filter = final_filter & (constructed_expense_filter)
else:
final_filter = final_filter | (constructed_expense_filter)

# Set the join type for the additonal filter
join_by = expense_filter.join_by

return final_filter
Comment on lines +79 to +99
Copy link

Choose a reason for hiding this comment

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

The construct_expense_filter_query function aggregates filters correctly. However, ensure that:

  1. The final_filter is initialized properly before being used. The current logic assumes that expense_filter.rank == 1 will always be present, which might not be the case.
  2. The join_by variable is set correctly before it's used in the loop. Consider initializing it outside of the loop to a default value if necessary.
  3. Error handling is in place in case the expense_filters list is empty or if any of the filters are malformed.



def post_request(url, body, refresh_token=None):
"""
Create a HTTP post request.
Expand Down
1 change: 1 addition & 0 deletions apps/fyle/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ class Expense(BaseForeignWorkspaceModel):
spent_at = CustomDateTimeField(help_text='Expense spent at')
approved_at = CustomDateTimeField(help_text='Expense approved at')
posted_at = CustomDateTimeField(help_text='Date when the money is taken from the bank')
is_skipped = models.BooleanField(null=True, default=False, help_text='Expense is skipped or not')
Copy link

Choose a reason for hiding this comment

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

The addition of the is_skipped field to the Expense model is consistent with the PR's objective to track skipped expenses. However, consider whether the field should allow null values. If an expense can only be either skipped or not, it might be more appropriate to use a non-nullable BooleanField with default=False.

expense_created_at = CustomDateTimeField(help_text='Expense created at')
expense_updated_at = CustomDateTimeField(help_text='Expense created at')
fund_source = StringNotNullField(help_text='Expense fund source')
Comment on lines 79 to 85
Copy link

Choose a reason for hiding this comment

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

The create_expense_objects method needs to be updated to handle the is_skipped field when creating or updating Expense objects. Currently, the is_skipped field is not included in the defaults dictionary passed to update_or_create, which means it will not be set when expenses are created or updated.

# Inside the defaults dictionary of update_or_create method, add:
'is_skipped': expense.get('is_skipped', False),

Expand Down
12 changes: 11 additions & 1 deletion apps/fyle/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from rest_framework.views import status

from apps.fyle.helpers import get_expense_fields
from apps.fyle.models import ExpenseFilter
from apps.fyle.models import ExpenseFilter, Expense
from apps.workspaces.models import FyleCredential, Workspace

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -124,3 +124,13 @@ def get_expense_fields(self, workspace_id:int):
expense_fields = get_expense_fields(workspace_id=workspace_id)

return expense_fields


class ExpenseSerializer(serializers.ModelSerializer):
"""
Expense serializer
"""

class Meta:
model = Expense
fields = ['updated_at', 'claim_number', 'employee_email', 'employee_name', 'fund_source', 'expense_number', 'payment_number', 'vendor', 'category', 'amount', 'report_id', 'settlement_id', 'expense_id']
Comment on lines +135 to +136
Copy link

Choose a reason for hiding this comment

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

The ExpenseSerializer fields list is missing the is_skipped field, which is crucial for the skipped expenses feature. It should be added to ensure the new functionality is properly serialized.

class Meta:
    model = Expense
    fields = [
        'updated_at', 'claim_number', 'employee_email', 'employee_name', 
        'fund_source', 'expense_number', 'payment_number', 'vendor', 
        'category', 'amount', 'report_id', 'settlement_id', 'expense_id',
+       'is_skipped'
    ]

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.

Suggested change
model = Expense
fields = ['updated_at', 'claim_number', 'employee_email', 'employee_name', 'fund_source', 'expense_number', 'payment_number', 'vendor', 'category', 'amount', 'report_id', 'settlement_id', 'expense_id']
model = Expense
fields = ['updated_at', 'claim_number', 'employee_email', 'employee_name', 'fund_source', 'expense_number', 'payment_number', 'vendor', 'category', 'amount', 'report_id', 'settlement_id', 'expense_id', 'is_skipped']

35 changes: 32 additions & 3 deletions apps/fyle/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

from apps.accounting_exports.models import AccountingExport
from apps.fyle.exceptions import handle_exceptions
from apps.fyle.models import Expense
from apps.fyle.models import Expense, ExpenseFilter
from apps.fyle.helpers import construct_expense_filter_query
from apps.workspaces.models import ExportSetting, FyleCredential, Workspace

SOURCE_ACCOUNT_MAP = {
Expand All @@ -18,6 +19,31 @@
logger.level = logging.INFO


def get_filtered_expenses(workspace: int, expense_objects: list, expense_filters: list):
"""
function to get filtered expense objects
"""

expenses_object_ids = [expense_object.id for expense_object in expense_objects]
final_query = construct_expense_filter_query(expense_filters)

Expense.objects.filter(
final_query,
id__in=expenses_object_ids,
accountingexport__isnull=True,
org_id=workspace.org_id
).update(is_skipped=True)

filtered_expenses = Expense.objects.filter(
is_skipped=False,
id__in=expenses_object_ids,
accountingexport__isnull=True,
org_id=workspace.org_id
)

return filtered_expenses
Comment on lines +22 to +44
Copy link

Choose a reason for hiding this comment

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

Consider combining the update and retrieval of Expense objects into a single query to improve performance and reduce database round trips. The current implementation updates the is_skipped field and then retrieves the expenses in two separate queries, which could be optimized.



@handle_exceptions
def import_expenses(workspace_id, accounting_export: AccountingExport, source_account_type, fund_source_key):
"""
Expand Down Expand Up @@ -53,9 +79,12 @@ def import_expenses(workspace_id, accounting_export: AccountingExport, source_ac
workspace.save()

with transaction.atomic():
expenses_object = Expense.create_expense_objects(expenses, workspace_id)
expense_objects = Expense.create_expense_objects(expenses, workspace_id)
expense_filters = ExpenseFilter.objects.filter(workspace_id=workspace_id).order_by('rank')
if expense_filters:
expense_objects = get_filtered_expenses(workspace, expense_objects, expense_filters)
Comment on lines +82 to +85
Copy link

Choose a reason for hiding this comment

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

Verify that overwriting expense_objects with the result from get_filtered_expenses does not lead to issues when expense_objects is empty after filtering. If there are no expenses after filtering, the subsequent call to AccountingExport.create_accounting_export might fail or behave unexpectedly.

AccountingExport.create_accounting_export(
expenses_object,
expense_objects,
fund_source=fund_source_key,
workspace_id=workspace_id
)
Expand Down
2 changes: 2 additions & 0 deletions apps/fyle/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
ExportableExpenseGroupsView,
FyleFieldsView,
ImportFyleAttributesView,
SkippedExpenseView
)

accounting_exports_path = [
Expand All @@ -38,6 +39,7 @@
path('expense_filters/', ExpenseFilterView.as_view(), name='expense-filters'),
path('fields/', FyleFieldsView.as_view(), name='fyle-fields'),
path('expense_fields/', CustomFieldView.as_view(), name='fyle-expense-fields'),
path('expenses/', SkippedExpenseView.as_view(), name='expenses'),
]

fyle_dimension_paths = [
Copy link

Choose a reason for hiding this comment

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

The use of itertools.chain to combine URL patterns is unconventional in Django. Consider using simple list concatenation or extension, which is more idiomatic and easier to read.

- urlpatterns = list(itertools.chain(accounting_exports_path, fyle_dimension_paths, other_paths))
+ urlpatterns = accounting_exports_path + fyle_dimension_paths + other_paths

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.

Suggested change
fyle_dimension_paths = [
urlpatterns = accounting_exports_path + fyle_dimension_paths + other_paths

Expand Down
27 changes: 26 additions & 1 deletion apps/fyle/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,16 @@
from rest_framework.views import status

from apps.fyle.helpers import get_exportable_accounting_exports_ids
from apps.fyle.models import ExpenseFilter
from apps.fyle.models import ExpenseFilter, Expense
from apps.fyle.queue import queue_import_credit_card_expenses, queue_import_reimbursable_expenses
from apps.fyle.serializers import (
ExpenseFieldSerializer,
ExpenseFilterSerializer,
FyleFieldsSerializer,
ImportFyleAttributesSerializer,
ExpenseSerializer
)
from apps.workspaces.models import Workspace
from ms_business_central_api.utils import LookupFieldMixin

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -96,3 +98,26 @@ def post(self, request, *args, **kwargs):
return Response(
status=status.HTTP_200_OK
)


class SkippedExpenseView(generics.ListAPIView):
"""
List Skipped Expenses
"""
serializer_class = ExpenseSerializer

def get_queryset(self):
start_date = self.request.query_params.get('start_date', None)
end_date = self.request.query_params.get('end_date', None)
org_id = Workspace.objects.get(id=self.kwargs['workspace_id']).org_id
Copy link

Choose a reason for hiding this comment

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

Consider adding error handling for the case where Workspace.objects.get(id=self.kwargs['workspace_id']) does not find a matching workspace, which would raise a Workspace.DoesNotExist exception.

+        try:
+            org_id = Workspace.objects.get(id=self.kwargs['workspace_id']).org_id
+        except Workspace.DoesNotExist:
+            return Response(
+                data={'error': 'Workspace not found.'},
+                status=status.HTTP_404_NOT_FOUND
+            )

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.

Suggested change
org_id = Workspace.objects.get(id=self.kwargs['workspace_id']).org_id
try:
org_id = Workspace.objects.get(id=self.kwargs['workspace_id']).org_id
except Workspace.DoesNotExist:
return Response(
data={'error': 'Workspace not found.'},
status=status.HTTP_404_NOT_FOUND
)


filters = {
'org_id': org_id,
'is_skipped': True
}

if start_date and end_date:
filters['updated_at__range'] = [start_date, end_date]
Comment on lines +119 to +120
Copy link

Choose a reason for hiding this comment

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

Ensure that the start_date and end_date query parameters are validated and converted into the correct date format before using them in the updated_at__range filter.

+        from django.core.exceptions import ValidationError
+        from django.utils.dateparse import parse_date
+        if start_date and end_date:
+            try:
+                start_date = parse_date(start_date)
+                end_date = parse_date(end_date)
+                if not start_date or not end_date:
+                    raise ValidationError
+            except ValidationError:
+                return Response(
+                    data={'error': 'Invalid date format for start_date or end_date.'},
+                    status=status.HTTP_400_BAD_REQUEST
+                )
+            filters['updated_at__range'] = [start_date, end_date]

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.

Suggested change
if start_date and end_date:
filters['updated_at__range'] = [start_date, end_date]
from django.core.exceptions import ValidationError
from django.utils.dateparse import parse_date
if start_date and end_date:
try:
start_date = parse_date(start_date)
end_date = parse_date(end_date)
if not start_date or not end_date:
raise ValidationError
except ValidationError:
return Response(
data={'error': 'Invalid date format for start_date or end_date.'},
status=status.HTTP_400_BAD_REQUEST
)
filters['updated_at__range'] = [start_date, end_date]


queryset = Expense.objects.filter(**filters).order_by('-updated_at')
return queryset
Loading