Fix: All the bug fixes listed #170
Conversation
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe pull request introduces several modifications across multiple files related to accounting exports. Key changes include updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 your favorite social media? 🪧 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 (
|
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
|
PR description must contain a link to a ClickUp (case-insensitive) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
apps/business_central/actions.py (2)
Line range hint
6-21: Add error handling for database operations.The function assumes the AccountingExportSummary exists and all database operations will succeed. Consider adding error handling:
def update_accounting_export_summary(workspace_id): - accounting_export_summary = AccountingExportSummary.objects.get(workspace_id=workspace_id) + try: + accounting_export_summary = AccountingExportSummary.objects.get(workspace_id=workspace_id) + except AccountingExportSummary.DoesNotExist: + raise ValueError(f"AccountingExportSummary not found for workspace {workspace_id}") failed_exports = AccountingExport.objects.filter(~Q(type__in=['FETCHING_REIMBURSABLE_EXPENSES', 'FETCHING_CREDIT_CARD_EXPENSES']), workspace_id=workspace_id, status__in=['FAILED', 'FATAL']).count() 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 ).count() - accounting_export_summary.failed_accounting_export_count = failed_exports - accounting_export_summary.successful_accounting_export_count = successful_exports - accounting_export_summary.total_accounting_export_count = failed_exports + successful_exports - accounting_export_summary.save() + try: + accounting_export_summary.failed_accounting_export_count = failed_exports + accounting_export_summary.successful_accounting_export_count = successful_exports + accounting_export_summary.total_accounting_export_count = failed_exports + successful_exports + accounting_export_summary.save() + except Exception as e: + raise ValueError(f"Failed to update AccountingExportSummary: {str(e)}")
Line range hint
8-19: Consider using F expressions for atomic updates.The current implementation might be susceptible to race conditions when multiple processes update the summary simultaneously.
- failed_exports = AccountingExport.objects.filter(~Q(type__in=['FETCHING_REIMBURSABLE_EXPENSES', 'FETCHING_CREDIT_CARD_EXPENSES']), workspace_id=workspace_id, status__in=['FAILED', 'FATAL']).count() - - 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 - ).count() - - accounting_export_summary.failed_accounting_export_count = failed_exports - accounting_export_summary.successful_accounting_export_count = successful_exports - accounting_export_summary.total_accounting_export_count = failed_exports + successful_exports - accounting_export_summary.save() + from django.db.models import F + + AccountingExportSummary.objects.filter(workspace_id=workspace_id).update( + failed_accounting_export_count=AccountingExport.objects.filter( + ~Q(type__in=['FETCHING_REIMBURSABLE_EXPENSES', 'FETCHING_CREDIT_CARD_EXPENSES']), + workspace_id=workspace_id, + status__in=['FAILED', 'FATAL'] + ).count(), + successful_accounting_export_count=AccountingExport.objects.filter( + ~Q(type__in=['FETCHING_REIMBURSABLE_EXPENSES', 'FETCHING_CREDIT_CARD_EXPENSES']), + workspace_id=workspace_id, + status='COMPLETE', + updated_at__gte=F('last_exported_at') + ).count() + )apps/accounting_exports/models.py (1)
126-127: Consider enhancing date validation and error handling.The current implementation could benefit from more robust date handling:
if date_field and date_field not in ['current_date', 'last_spent_at']: - 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') + try: + if accounting_export[date_field]: + accounting_export[date_field] = accounting_export[date_field].strftime('%Y-%m-%d') + else: + from django.utils import timezone + from apps.workspaces.models import WorkspaceLog + current_date = timezone.now() + WorkspaceLog.objects.create( + workspace_id=workspace_id, + level='WARNING', + message=f'Empty date field {date_field} defaulted to current date' + ) + accounting_export[date_field] = current_date.strftime('%Y-%m-%d') + except (AttributeError, ValueError) as e: + raise ValueError(f"Invalid date format for {date_field}: {str(e)}")This enhancement:
- Uses timezone-aware datetime
- Adds logging for tracking empty date occurrences
- Improves error handling for invalid date formats
apps/business_central/exports/base_model.py (1)
172-173: Good security improvement with workspace-specific filtering.Adding the
workspace_idfilter to the ExpenseAttribute query is crucial for preventing potential data leaks between workspaces. Consider adding similar workspace-specific checks to other queries in this method for complete security.Consider adding workspace filters to the other queries in this method:
mapping: Mapping = Mapping.objects.filter( source_type=location_setting.source_field, destination_type='LOCATION', source__value=source_value, workspace_id=accounting_export.workspace_id ).first()apps/workspaces/tasks.py (1)
Line range hint
156-187: Similar workspace isolation fix needed in export_to_business_centralThe
export_to_business_centralfunction contains similar AccountingExport queries that are missing workspace filters:accounting_export_ids = AccountingExport.objects.filter( fund_source='PERSONAL', exported_at__isnull=True).values_list('id', flat=True)accounting_export_ids = AccountingExport.objects.filter( fund_source='CCC', exported_at__isnull=True).values_list('id', flat=True)These queries should also include the workspace filter for consistency and security.
Additionally, consider refactoring the common export logic between
run_import_exportandexport_to_business_centralto reduce code duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/accounting_exports/models.py(1 hunks)apps/business_central/actions.py(1 hunks)apps/business_central/exports/base_model.py(2 hunks)apps/workspaces/tasks.py(2 hunks)
🔇 Additional comments (4)
apps/business_central/actions.py (1)
14-14: Verify the business logic for export counting.
The new timestamp filter updated_at__gte=accounting_export_summary.last_exported_at could lead to inconsistent counts:
- Successful exports before
last_exported_atare excluded - The total count adds filtered successful exports to all failed exports, which could be misleading
✅ Verification successful
The timestamp filter is correctly implemented and won't cause inconsistencies.
The code analysis reveals that:
last_exported_atis properly initialized during export operations inworkspaces/tasks.py- The field is nullable and handled appropriately in the model definitions
- The filter is used correctly to count only new successful exports since the last export
- The total count is maintained correctly by adding failed exports (all) and successful exports (since last export)
The business logic is sound because:
- Failed exports are always counted regardless of timestamp to ensure no failures are missed
- Successful exports are counted only since last export to avoid double-counting
- The timestamp is updated after each export operation, maintaining the correct window for next count
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential discrepancies in export counts
# Test 1: Check if there are any successful exports before last_exported_at that would be excluded
echo "Checking for successful exports that might be excluded by the new filter..."
ast-grep --pattern 'class AccountingExportSummary:
$$$
last_exported_at = $_
$$$'
# Test 2: Check if there's proper handling for null last_exported_at
rg -A 5 "last_exported_at.*=.*None"
Length of output: 288
Script:
#!/bin/bash
# Let's try a different approach to understand the export counting logic
# Check the AccountingExportSummary model definition
echo "Checking AccountingExportSummary model definition..."
rg -l "class AccountingExportSummary"
# Once we find the file, let's examine its content
fd "models.py" --exec cat {}
# Look for any references to last_exported_at in the codebase
echo "Checking usage of last_exported_at..."
rg "last_exported_at" -A 3 -B 3
# Check the complete implementation of update_accounting_export_summary
echo "Checking the complete implementation..."
rg "def update_accounting_export_summary" -A 20 -B 2
Length of output: 69542
Script:
#!/bin/bash
# Let's check how last_exported_at is initialized and handled in the codebase
# Check for any null handling of last_exported_at in the codebase
echo "Checking for null handling of last_exported_at..."
rg "last_exported_at.*None|last_exported_at.*null|last_exported_at.*isnull" -A 3 -B 3
# Check for any initialization of last_exported_at
echo "Checking for initialization of last_exported_at..."
rg "last_exported_at.*=.*datetime" -A 3 -B 3
# Check for any filters using last_exported_at
echo "Checking for filters using last_exported_at..."
rg "updated_at.*gte.*last_exported_at|updated_at.*gt.*last_exported_at" -A 3 -B 3
Length of output: 13447
apps/accounting_exports/models.py (1)
126-127: Verify the date fallback behavior with business requirements.
The changes add a fallback to the current date when the date field is empty. While this prevents potential null date issues, it might mask underlying data problems.
Consider the following improvements:
- Add logging when falling back to current date to track such occurrences
- Validate if using current date as fallback aligns with accounting requirements
Let's check for any existing empty date fields that would trigger this fallback:
apps/business_central/exports/base_model.py (1)
32-34: Verify the expense URL path change across the codebase.
The URL path change from /app/main/ to /app/admin/ looks intentional. However, let's ensure this change is consistent across the codebase and verify that all users have the necessary admin access.
✅ Verification successful
URL path change to /app/admin/ appears to be an isolated update
The search results show that:
- The new URL pattern
/app/admin/#/enterprise/view_expense/exists only in thebase_model.pyfile - No other instances of the old path (
/app/main/) were found - No URL configurations contain this specific route pattern
This appears to be an isolated change in the expense link generation, likely part of a broader admin interface migration. Since there are no conflicting URL patterns or inconsistent implementations found in the codebase, the change is safe to proceed with.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining instances of the old URL path
# and any related configuration settings that might need updating
# Check for old URL path
echo "Checking for old URL path '/app/main/#/enterprise/view_expense/'..."
rg -l "/app/main/#/enterprise/view_expense"
# Check for any URL-related configuration files
echo "Checking for URL configuration files..."
fd -e json -e yaml -e yml -e ini -e conf -e config --exec grep -l "enterprise/view_expense"
Length of output: 375
Script:
#!/bin/bash
# Let's try a broader search for URL patterns and expense-related paths
# Search for any URLs containing 'view_expense'
echo "Searching for view_expense URLs..."
rg -i "view_expense"
# Search for any URLs containing app/main or app/admin
echo "Searching for app/main or app/admin patterns..."
rg -i "(app/main|app/admin).*enterprise"
# Search for any Django URL patterns or configurations
echo "Searching for Django URL patterns..."
ast-grep --pattern 'path($$$, include($$$))'
Length of output: 1421
apps/workspaces/tasks.py (1)
78-78: Consistent workspace isolation for credit card expenses
Good to see the same workspace isolation applied consistently to credit card expenses.
Let's verify if there are any other similar queries that might need the same workspace isolation:
✅ Verification successful
After analyzing the search results, I can see that all instances of AccountingExport.objects.filter() in the codebase properly include workspace_id in their filters, either directly or through related filters. The two instances in question from the review comment are part of a systematic pattern where workspace isolation is consistently maintained across the codebase.
All AccountingExport queries have proper workspace isolation
The workspace isolation is consistently implemented across all AccountingExport queries in the codebase, including test files and production code. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for AccountingExport queries that might be missing workspace filters
rg "AccountingExport.objects.filter\(" -A 5 -B 2
Length of output: 22326
| @@ -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) | |||
There was a problem hiding this comment.
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.
|
PR description must contain a link to a ClickUp (case-insensitive) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
apps/accounting_exports/models.py (1)
126-127: LGTM! Consider enhancing error handling and date format configuration.The improved date field validation logic correctly handles special cases. However, consider these enhancements:
- Add error handling for invalid date values
- Move the date format string to a configuration setting
Consider applying this improvement:
+ DATE_FORMAT = '%Y-%m-%d' # Add at module level def create_accounting_export(expense_objects: List[Expense], fund_source: str, workspace_id): ... if date_field and date_field not in ['current_date', 'last_spent_at']: - 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') + try: + if accounting_export[date_field]: + accounting_export[date_field] = accounting_export[date_field].strftime(DATE_FORMAT) + else: + accounting_export[date_field] = datetime.now().strftime(DATE_FORMAT) + except (ValueError, TypeError) as e: + raise ValueError(f"Invalid date value for field '{date_field}': {e}")apps/business_central/exports/base_model.py (1)
Line range hint
67-84: Consider using Decimal for financial calculations.The
get_total_amountmethod uses floating-point arithmetic which can lead to precision issues in financial calculations. Consider using Python'sdecimal.Decimalfor more accurate financial computations.def get_total_amount(accounting_export: AccountingExport): total_amount = accounting_export.expenses.aggregate(Sum('amount'))['amount__sum'] - return total_amount or 0.0 + from decimal import Decimal + return total_amount or Decimal('0.00')apps/workspaces/tasks.py (1)
61-61: Consider optimizing queries with select_related/prefetch_related.While the workspace filtering fixes are correct, these queries might benefit from optimization if they're used to fetch related data later in the code.
Consider modifying the queries to include necessary related fields:
-AccountingExport.objects.filter( +AccountingExport.objects.select_related( + 'expense_group', # Add relevant related fields +).filter(This would help prevent N+1 query issues if related data is accessed during the export process.
Also applies to: 78-78
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/accounting_exports/models.py(1 hunks)apps/business_central/actions.py(1 hunks)apps/business_central/exports/base_model.py(2 hunks)apps/workspaces/tasks.py(2 hunks)
🔇 Additional comments (5)
apps/accounting_exports/models.py (1)
126-127: Verify the impact of date field changes on existing queries.
Let's ensure that the modified date handling doesn't affect existing queries or reports.
✅ Verification successful
Let me gather more specific information about the date field usage in the accounting exports model.
Based on the gathered information, I can now provide the final response:
The date field changes are safely implemented and won't affect existing queries
The code changes are handling date fields appropriately because:
- The date field logic is contained within the accounting exports model and doesn't affect external queries
- The implementation includes proper fallbacks to 'current_date' and 'last_spent_at'
- All date-related filters in queries are using standard Django field lookups and aren't dependent on these specific date field changes
- The date formatting is consistently applied using the '%Y-%m-%d' format when needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for queries that might be affected by the date field changes
echo "Checking for date field usage in queries..."
rg -l "last_spent_at|current_date" --type py
echo "Checking for direct references to date formats..."
rg -l "strftime\('%Y-%m-%d'\)" --type py
echo "Checking for related model queries..."
ast-grep --pattern 'AccountingExport.objects.filter($$$)'
Length of output: 7214
Script:
#!/bin/bash
# Check the specific implementation of date fields in accounting_exports/models.py
echo "Checking date field implementations in accounting_exports model..."
rg -A 5 -B 5 "date_field" apps/accounting_exports/models.py
# Check for any date-related filters in queries
echo "Checking date-related filters in queries..."
rg "filter.*date|date.*filter" --type py
# Look for any date field references in the schema
echo "Checking date fields in model definitions..."
ast-grep --pattern 'class $_(models.Model):
$$$
$date = models.$_($$$)
$$$'
Length of output: 4835
apps/business_central/exports/base_model.py (2)
172-172: LGTM: Good security improvement with workspace filtering.
The addition of workspace_id filtering to the ExpenseAttribute query ensures proper data isolation between workspaces. This change aligns with the existing workspace-scoped queries in the method and follows multi-tenant security best practices.
32-34: Verify the URL path change impact on frontend navigation.
The expense link URL path has been changed from /app/main/ to /app/admin/. Please ensure this change aligns with the frontend application's routing structure.
apps/workspaces/tasks.py (2)
78-78: LGTM: Critical workspace filter added to credit card expenses query.
The addition of workspace_id filter is essential to prevent potential data leakage across workspaces.
61-61: LGTM: Critical workspace filter added to reimbursable expenses query.
The addition of workspace_id filter is essential to prevent potential data leakage across workspaces.
Let's verify there are no other similar queries missing workspace filters:
✅ Verification successful
All AccountingExport queries have proper workspace filtering
Based on the grep results, I've analyzed all occurrences of AccountingExport.objects.filter in the codebase:
- In
apps/workspaces/tasks.py, the two queries in question have been properly fixed withworkspace_idfilter - All other queries across the codebase either:
- Already include
workspace_idfilter - Are in test files which are isolated by nature
- Have workspace-specific filtering through related fields or query conditions
- Already include
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for AccountingExport queries that might be missing workspace filters
rg "AccountingExport.objects.filter\(" -A 3
Length of output: 13215
| @@ -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 | |||
There was a problem hiding this comment.
🛠️ 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 datetimeReview 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:
- Inaccurate total counts since failed and successful exports are filtered differently
- 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.
apps/accounting_exports/models.py
Outdated
| @@ -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']: | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
okay will revert this back, the previous code is good not issue then
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
PR description must contain a link to a ClickUp (case-insensitive) |
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Ashutosh619-sudo
had a problem deploying
to
Staging
GitHub Actions
Failure
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #170 +/- ##
==========================================
+ Coverage 96.38% 96.44% +0.06%
==========================================
Files 90 90
Lines 4981 5040 +59
==========================================
+ Hits 4801 4861 +60
+ Misses 180 179 -1 ☔ View full report in Codecov by Sentry. |
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Ashutosh619-sudo
added
deploy
Ashutosh619-sudo
had a problem deploying
to
Staging
GitHub Actions
Failure
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/workspaces/tasks.py (1)
Line range hint
69-91: Consider refactoring duplicate export logicThe export logic for reimbursable and credit card expenses is very similar and could be refactored into a shared helper function to improve maintainability and reduce code duplication.
Consider refactoring like this:
def _process_expense_export( workspace_id: int, fund_source: str, export_type: str, export_map: dict, is_auto_export: bool, interval_hours: int ) -> bool: accounting_export_ids = AccountingExport.objects.filter( fund_source=fund_source, exported_at__isnull=True, workspace_id=workspace_id ).values_list('id', flat=True) if len(accounting_export_ids): export = export_map.get(export_type, None) if export: export.trigger_export( workspace_id=workspace_id, accounting_export_ids=accounting_export_ids, is_auto_export=is_auto_export, interval_hours=interval_hours ) return True return FalseThen use it in both cases:
# For Reimbursable Expenses if export_settings.reimbursable_expenses_export_type: queue_import_reimbursable_expenses(workspace_id=workspace_id, synchronous=True) accounting_export = AccountingExport.objects.get( workspace_id=workspace_id, type='FETCHING_REIMBURSABLE_EXPENSES' ) if accounting_export.status == 'COMPLETE': is_expenses_exported = _process_expense_export( workspace_id, 'PERSONAL', export_settings.reimbursable_expenses_export_type, export_map, is_auto_export, interval_hours )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/workspaces/tasks.py(4 hunks)
🔇 Additional comments (3)
apps/workspaces/tasks.py (3)
12-12: LGTM: Import addition is appropriate
The addition of BusinessCentralCredentials to the imports is consistent with its new usage in the credential verification logic.
33-40: LGTM: Proper credential verification implemented
The credential verification is well-implemented with appropriate checks for:
- Valid workspace credentials
- Non-expired status
- Presence of refresh token
- Early return with proper logging
69-69: Skip comment: Security fix already reviewed
A previous review comment already addressed the importance of adding workspace filtering for proper data isolation.
Also applies to: 86-86
| export_settings = ExportSetting.objects.get(workspace_id=workspace_id) | ||
| advance_settings = AdvancedSetting.objects.get(workspace_id=workspace_id) |
There was a problem hiding this comment.
💡 Codebase verification
Error handling for missing settings is not implemented
The .get calls for ExportSetting and AdvancedSetting are not wrapped in try-except blocks handling DoesNotExist exceptions.
🔗 Analysis chain
Verify error handling for missing settings
The code directly retrieves ExportSetting and AdvancedSetting without handling potential DoesNotExist exceptions. Consider adding proper error handling for cases where these settings don't exist.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for error handling patterns in the codebase
rg -A 2 "ExportSetting.objects.get|AdvancedSetting.objects.get" --type py
Length of output: 9159
Script:
#!/bin/bash
# Check for proper DoesNotExist exception handling around ExportSetting.objects.get and AdvancedSetting.objects.get
rg -A 4 "try:" --type py | rg "(ExportSetting\.objects\.get|AdvancedSetting\.objects\.get)\(workspace_id=workspace_id\)" -B 1 | rg "except .*DoesNotExist"
Length of output: 154
Script:
#!/bin/bash
# Use ast-grep to find ExportSetting.objects.get and AdvancedSetting.objects.get within try-except blocks handling DoesNotExist
ast-grep --pattern "
try:
$$$
(ExportSetting.objects.get($_) | AdvancedSetting.objects.get($_))
$$$
except DoesNotExist:
$$$
"
Length of output: 172
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Ashutosh619-sudo
added
deploy
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Ashutosh619-sudo
temporarily deployed
to
Staging
GitHub Actions
Inactive
|
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/mappings/tasks.py (1)
Line range hint
106-113: Consider enhancing error logging.While the error messages are functional, consider these improvements for better debugging:
- Use WARNING level for credential/token errors as they indicate access issues
- Include more context in error messages (e.g., error details, timestamp)
Example improvement:
- logger.info('Invalid Token or Business Central Credentials does not exist - %s', workspace_id) + logger.warning('Invalid Token or Business Central Credentials does not exist - workspace_id: %s, timestamp: %s', + workspace_id, datetime.now().isoformat())
| @@ -104,7 +104,7 @@ def async_auto_map_employees(workspace_id: int): | |||
| destination_attribute_type=destination_type, | |||
| ) | |||
| except (BusinessCentralCredentials.DoesNotExist, InvalidTokenError): | |||
| logger.info('Invalid Token or Sage Intacct Credentials does not exist - %s', workspace_id) | |||
| logger.info('Invalid Token or Business Central Credentials does not exist - %s', workspace_id) | |||
There was a problem hiding this comment.
💡 Codebase verification
Found one remaining instance of "Sage Intacct" in test file that needs updating
The test file tests/test_mappings/test_tasks.py still contains the old "Sage Intacct" reference in both the error message and assertion:
- Line with
side_effect=InvalidTokenError(f'Invalid Token or Sage Intacct Credentials does not exist - {workspace_id}') - Line with
assert str(e) == 'Invalid Token or Sage Intacct Credentials does not exist - 1'
These test cases should be updated to match the new "Business Central" error message to maintain consistency with the implementation.
Other occurrences of "Sage" and "Intacct" are in test fixtures and unrelated message strings, which are not problematic.
🔗 Analysis chain
LGTM! Error message correctly updated.
The error message has been appropriately updated to reference "Business Central Credentials" instead of "Sage Intacct Credentials", fixing what appears to be a copy-paste error.
Let's verify there are no similar copy-paste errors in other error messages:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to "Sage" or "Intacct" in error messages
# that might indicate similar copy-paste errors
rg -i "sage|intacct" --type py
Length of output: 14160
* Fix: All the bug fixes listed * credentials expired issue resolved * pylint fix * test fail resolved * pylint resolved * comment resolved * small change * pylint fix * fix * remove intacct name
Description
Please add PR description here, add screenshots if needed
Clickup
Please add link here
https://app.clickup.com/1864988/v/l/li/901604887942
Bug fix listed here https://www.notion.so/fyleuniverse/Backend-1212ed8bfcb381d38cfff15659f16577?pvs=4#1212ed8bfcb381dca38cd76616a1c70f
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests