add support for skipped expenses api #53
Conversation
NileshPant1999
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe latest changes introduce a new feature allowing the application to track skipped expenses. This enhancement spans multiple areas of the codebase, including the model, serializer, URL configuration, and views. The Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (4)
- apps/fyle/models.py (1 hunks)
- apps/fyle/serializers.py (2 hunks)
- apps/fyle/urls.py (2 hunks)
- apps/fyle/views.py (2 hunks)
Additional comments: 2
apps/fyle/urls.py (2)
29-29: The addition of
SkippedExpenseViewto theviewsimport list is consistent with the PR's objective to support skipped expenses.42-42: The URL pattern for
SkippedExpenseViewis correctly defined and follows Django's convention for class-based views.
| 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'] |
There was a problem hiding this comment.
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.
| 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'] |
| @@ -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') | |||
There was a problem hiding this comment.
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.
| 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') | ||
| 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') |
There was a problem hiding this comment.
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),
apps/fyle/urls.py
Outdated
| @@ -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('skipped_expenses/', SkippedExpenseView.as_view(), name='expenses'), | |||
There was a problem hiding this comment.
The URL name 'expenses' for SkippedExpenseView is generic and could lead to confusion or conflicts with other expense-related views. Consider renaming it to something more specific, like 'skipped-expenses', to clearly indicate its purpose.
| 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 |
There was a problem hiding this comment.
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.
| 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 | |
| ) |
| if start_date and end_date: | ||
| filters['updated_at__range'] = [start_date, end_date] |
There was a problem hiding this comment.
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.
| 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] |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #53 +/- ##
==========================================
- Coverage 91.98% 91.71% -0.28%
==========================================
Files 51 51
Lines 2434 2451 +17
==========================================
+ Hits 2239 2248 +9
- Misses 195 203 +8 ☔ View full report in Codecov by Sentry. |
NileshPant1999
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (3)
- apps/fyle/helpers.py (1 hunks)
- apps/fyle/tasks.py (3 hunks)
- apps/fyle/urls.py (2 hunks)
Additional comments: 3
apps/fyle/helpers.py (1)
- 3-5: The addition of imports for
ListandQis appropriate as they are used in the newly added functions.apps/fyle/tasks.py (2)
9-10: The imports of
Expense,ExpenseFilter, andconstruct_expense_filter_queryalign with the new functionality for handling skipped expenses.79-90: The integration of the new
get_filtered_expensesfunction withinimport_expensesis consistent with the PR objectives and appears to be correctly implemented, assuming that the potential issue with an emptyexpense_objectslist is handled.
| 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 |
There was a problem hiding this comment.
The construct_expense_filter function correctly handles the construction of filters for different custom field types and operators. However, consider the following points:
- For the
BOOLEANtype, the conversion from string to boolean should be case-insensitive. - Ensure that the
expense_filter.valueslist always contains the expected data types before performing operations like integer conversion or boolean checks. - Validate that the
expense_filter.conditionandexpense_filter.operatorvalues 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 |
There was a problem hiding this comment.
The construct_expense_filter_query function aggregates filters correctly. However, ensure that:
- The
final_filteris initialized properly before being used. The current logic assumes thatexpense_filter.rank == 1will always be present, which might not be the case. - The
join_byvariable is set correctly before it's used in the loop. Consider initializing it outside of the loop to a default value if necessary. - Error handling is in place in case the
expense_filterslist is empty or if any of the filters are malformed.
| 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 |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
| @@ -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 = [ | |||
There was a problem hiding this comment.
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_pathsCommittable 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.
| fyle_dimension_paths = [ | |
| urlpatterns = accounting_exports_path + fyle_dimension_paths + other_paths |
Summary by CodeRabbit
New Features
/skipped_expenses/URL.Enhancements
ExpenseSerializerto facilitate the representation of expense data.Bug Fixes
Documentation
Expensemodel to improve clarity.