feat: Add support for skip expense post import of expenses #712
Conversation
ashwin1111
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThis pull request modifies the Fyle application by updating the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/fyle/actions.py (1)
Line range hint
127-161: Update the docstring to reflect the return value.The docstring currently states “:return: None” but the method returns a list of skipped expenses. Keeping the docstring accurate helps maintain code clarity.
- :return: None + :return: List[Expense]
🧹 Nitpick comments (1)
tests/test_fyle/test_tasks.py (1)
214-239: Consider verifying skipped outcomes in your new test.This test successfully demonstrates the workflow for filtering and attempting to skip expenses. However, there are no final assertions confirming that the skipped expenses are indeed marked as skipped or removed from their groups.
You might add simple post-call assertions similar to:
assert Expense.objects.filter(is_skipped=True).exists()to confirm the result.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/fyle/actions.py(2 hunks)apps/fyle/tasks.py(2 hunks)apps/quickbooks_online/queue.py(2 hunks)tests/test_fyle/test_tasks.py(2 hunks)
🔇 Additional comments (6)
tests/test_fyle/test_tasks.py (2)
11-11: Import forExpenseFilterlooks good.This addition is aligned with the new test scenario for skipping expenses. The usage here appears consistent with the rest of the file.
17-18: Newly importedskip_expenses_pre_exportis consistent with usage.It's logical to include this in the import list because the new test function will verify its behavior.
apps/fyle/tasks.py (2)
21-21: Import ofErroris suitably placed.The new import aligns with the usage in the
skip_expenses_pre_exportfunction.
322-356: Overall logic for skipping expenses is solid.The function persistently marks matching expenses as skipped, manages associated logs/errors, and removes empty groups. This appears coherent for the intended workflow.
apps/quickbooks_online/queue.py (2)
9-9: Import ofExpenseFilteris consistent with usage.This import is needed to detect and skip qualified expense groups before export.
95-99: Pre-export skip in chain tasks is correctly placed.Calling
skip_expenses_pre_exportbefore running the main export tasks ensures that only valid, unskipped expenses proceed. This placement is logical and should not disrupt existing flows.
anishfyle
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
anishfyle
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
anishfyle
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/fyle/tasks.py (2)
322-356: Add transaction handling and optimize bulk operations.Consider the following improvements to ensure data consistency and better performance:
- Wrap operations in a transaction to ensure atomicity
- Use bulk operations for removing expenses from groups
def skip_expenses_pre_export(workspace_id: int, expense_group_ids: List[int]) -> None: + from django.db import transaction + expense_filters = ExpenseFilter.objects.filter(workspace_id=workspace_id).order_by('rank') if expense_filters: - workspace = Workspace.objects.get(pk=workspace_id) - expense_ids_queued = Expense.objects.filter(expensegroup__id__in=expense_group_ids).values_list('id', flat=True) - filtered_expense_query = construct_expense_filter_query(expense_filters) - skipped_expenses = mark_expenses_as_skipped(filtered_expense_query, expense_ids_queued, workspace) + with transaction.atomic(): + workspace = Workspace.objects.get(pk=workspace_id) + expense_ids_queued = Expense.objects.filter(expensegroup__id__in=expense_group_ids).values_list('id', flat=True) + filtered_expense_query = construct_expense_filter_query(expense_filters) + skipped_expenses = mark_expenses_as_skipped(filtered_expense_query, expense_ids_queued, workspace) if skipped_expenses: skipped_expense_ids = [expense.id for expense in skipped_expenses] logger.info('Skipping expenses before export %s', skipped_expense_ids) - expense_groups = ExpenseGroup.objects.filter(expenses__id__in=skipped_expense_ids) + # Bulk delete related records + TaskLog.objects.filter( + workspace_id=workspace_id, + expense_group_id__in=ExpenseGroup.objects.filter( + expenses__id__in=skipped_expense_ids + ).values_list('id', flat=True) + ).delete() + + Error.objects.filter( + workspace_id=workspace_id, + expense_group_id__in=ExpenseGroup.objects.filter( + expenses__id__in=skipped_expense_ids + ).values_list('id', flat=True) + ).delete()
336-338: Consider adding post-skip notification.The function logs skipped expense IDs but doesn't notify the user. Consider adding a notification mechanism to inform users about skipped expenses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/fyle/actions.py(2 hunks)apps/fyle/tasks.py(2 hunks)apps/quickbooks_online/queue.py(2 hunks)tests/test_fyle/test_tasks.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/quickbooks_online/queue.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pytest
🔇 Additional comments (2)
apps/fyle/actions.py (1)
Line range hint
124-159: LGTM! Return type change improves function reusability.The modification to return skipped expenses and removal of the
expensegroup__isnull=Truecondition makes the function more versatile, allowing it to be used in both pre-export and post-import scenarios.apps/fyle/tasks.py (1)
322-356: Verify integration with existing export workflow.Please ensure that this new pre-export skipping functionality integrates properly with the existing export workflow, particularly in:
- QuickBooks Online queue processing
- Export status tracking
- Error handling and reporting
✅ Verification successful
Integration with export workflow verified successfully
The
skip_expenses_pre_exportfunction is properly integrated into the QuickBooks Online export workflow:
- Correctly placed in the task chain before expense updates
- Followed by appropriate sync operations
- Covered by test cases
- Protected by the chain task system's error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for proper integration with export workflow # Look for export workflow integration points rg -A 5 "skip_expenses_pre_export" --type python # Check for proper error handling in export workflow ast-grep --pattern 'try { $$$ skip_expenses_pre_export($_, $_) $$$ } catch ($err) { $$$ }'Length of output: 180
Script:
#!/bin/bash # Find all occurrences and usages of skip_expenses_pre_export rg "skip_expenses_pre_export" -B 2 -A 5 # Look for try-except blocks around this function ast-grep --pattern 'try: $$$ skip_expenses_pre_export($$$) $$$ except $$$: $$$' # Find related export workflow files rg "export.*workflow|export.*queue" -B 2 -A 2Length of output: 2459
anishfyle
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
anishfyle
had a problem deploying
to
CI Environment
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/test_fyle/test_tasks.py (1)
Line range hint
1-211: Add missing test function for skip_expenses_pre_export.The AI summary indicates that a new test function
test_skip_expenses_pre_exportshould be present to test the new functionality, but it's missing from the code.Would you like me to help you generate the test function? Based on the past review comments, I can help create a comprehensive test that:
- Sets up expense filters
- Creates test expenses
- Verifies the initial state
- Calls
skip_expenses_pre_export- Verifies the final state
🧰 Tools
🪛 Ruff (0.8.2)
12-12:
apps.fyle.models.ExpenseFilterimported but unusedRemove unused import:
apps.fyle.models.ExpenseFilter(F401)
19-19:
apps.fyle.tasks.skip_expenses_pre_exportimported but unusedRemove unused import:
apps.fyle.tasks.skip_expenses_pre_export(F401)
🪛 GitHub Actions: Continuous Integration
[error] 12-12: Import 'apps.fyle.models.ExpenseFilter' is unused
[error] 13-13: Import 'apps.fyle.tasks.skip_expenses_pre_export' is unused
🧹 Nitpick comments (1)
tests/test_fyle/test_tasks.py (1)
211-211: Remove extra blank line at the end of file.Remove the extra blank line at the end of the file.
🧰 Tools
🪛 GitHub Actions: Continuous Integration
[warning] 211-211: Blank line found at end of file
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_fyle/test_tasks.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_fyle/test_tasks.py
12-12: apps.fyle.models.ExpenseFilter imported but unused
Remove unused import: apps.fyle.models.ExpenseFilter
(F401)
19-19: apps.fyle.tasks.skip_expenses_pre_export imported but unused
Remove unused import: apps.fyle.tasks.skip_expenses_pre_export
(F401)
🪛 GitHub Actions: Continuous Integration
tests/test_fyle/test_tasks.py
[error] 12-12: Import 'apps.fyle.models.ExpenseFilter' is unused
[error] 13-13: Import 'apps.fyle.tasks.skip_expenses_pre_export' is unused
[warning] 211-211: Blank line found at end of file
Description
Please add PR description here, add screenshots if needed
Clickup
https://app.clickup.com/
Summary by CodeRabbit
New Features
Tests
Improvements