Base Export Structure added #50
Conversation
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
|
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe recent updates include the introduction of a new function to track export details, improved error handling in the Business Central integration, expansion of export mode options, and the creation of a new model to store export details. These changes enhance the application's ability to manage and log export activities, as well as handle exceptions more effectively. Changes
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: 4
Configuration used: CodeRabbit UI
Files selected for processing (5)
- apps/business_central/actions.py (1 hunks)
- apps/business_central/exceptions.py (1 hunks)
- apps/workspaces/models.py (2 hunks)
- apps/workspaces/serializers.py (2 hunks)
- ms_business_central_api/exceptions.py (1 hunks)
Files skipped from review due to trivial changes (1)
- ms_business_central_api/exceptions.py
Additional comments: 11
apps/business_central/actions.py (2)
17-19: The calculation of
total_accounting_exports_countby summingfailed_accounting_exports_countandsuccessful_accounting_exports_countassumes that there are no other export statuses to consider. If there are other statuses that should be included in the total count, this calculation may need to be adjusted.22-22: Returning the updated
last_export_detailobject is a good practice as it allows the caller to use the updated details immediately if needed.apps/business_central/exceptions.py (3)
16-27:
Thehandle_business_central_errorfunction correctly logs the exception response, updates theErrormodel with the error details, and sets theaccounting_exportstatus to 'FAILED'. This aligns with the PR objectives to improve exception handling.30-76:
Thehandle_business_central_exceptionsdecorator function provides comprehensive error handling for different types of exceptions, ensuring that theaccounting_exportstatus and details are updated accordingly. The inclusion ofupdate_last_export_detailsat the end of the try-except block ensures that export details are updated in all cases.64-70:
The catch-all for genericExceptionis appropriate here as it's used to set theaccounting_exportstatus to 'FATAL' and log the error, which is suitable for unexpected errors that are not explicitly handled by the other except blocks.apps/workspaces/models.py (3)
133-136: The
EXPORT_MODE_CHOICEStuple is correctly defined following Django's convention for choices. This will be used in theLastExportDetailmodel'sexport_modefield.139-140: The
BaseModelis correctly used as an abstract base class for theLastExportDetailmodel, ensuring that thecreated_atandupdated_atfields are consistently managed across models.249-250: The
db_tablename forLastExportDetailfollows the established naming convention and clearly describes the table's purpose.apps/workspaces/serializers.py (3)
19-19: The import of
LastExportDetailis correctly placed and follows the alphabetical order of imports.67-67: Verify that the
LastExportDetailmodel'sworkspace_idfield is sufficient for creating a new instance and that no other mandatory fields are required. If additional fields are necessary, they should be included in the creation call.64-71: Ensure that the
userfield is correctly handled when adding a user to the workspace. It might be necessary to verify that theUser.objects.get(user_id=user)call returns the expected user object and that theuser_idfield matches theuservariable obtained from the request context.
| def update_last_export_details(workspace_id): | ||
| last_export_detail = LastExportDetail.objects.get(workspace_id=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', | ||
| ).count() | ||
|
|
||
| last_export_detail.failed_accounting_exports_count = failed_exports | ||
| last_export_detail.successful_accounting_exports_count = successful_exports | ||
| last_export_detail.total_accounting_exports_count = failed_exports + successful_exports | ||
| last_export_detail.save() | ||
|
|
||
| return last_export_detail |
There was a problem hiding this comment.
The function update_last_export_details uses LastExportDetail.objects.get(workspace_id=workspace_id) without handling the potential DoesNotExist exception that is raised if no LastExportDetail instance is found for the given workspace_id. It's important to handle this case to avoid unhandled exceptions which could disrupt the flow of the application.
def update_last_export_details(workspace_id):
+ try:
last_export_detail = LastExportDetail.objects.get(workspace_id=workspace_id)
+ except LastExportDetail.DoesNotExist:
+ # Handle the exception, e.g., create a new LastExportDetail instance or log an error
+ passCommittable 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.
| def update_last_export_details(workspace_id): | |
| last_export_detail = LastExportDetail.objects.get(workspace_id=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', | |
| ).count() | |
| last_export_detail.failed_accounting_exports_count = failed_exports | |
| last_export_detail.successful_accounting_exports_count = successful_exports | |
| last_export_detail.total_accounting_exports_count = failed_exports + successful_exports | |
| last_export_detail.save() | |
| return last_export_detail | |
| def update_last_export_details(workspace_id): | |
| try: | |
| last_export_detail = LastExportDetail.objects.get(workspace_id=workspace_id) | |
| except LastExportDetail.DoesNotExist: | |
| # Handle the exception, e.g., create a new LastExportDetail instance or log an error | |
| pass | |
| 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', | |
| ).count() | |
| last_export_detail.failed_accounting_exports_count = failed_exports | |
| last_export_detail.successful_accounting_exports_count = successful_exports | |
| last_export_detail.total_accounting_exports_count = failed_exports + successful_exports | |
| last_export_detail.save() | |
| return last_export_detail |
| def decorator(func): | ||
| def new_fn(*args): | ||
|
|
||
| accounting_export = args[0] |
There was a problem hiding this comment.
Consider using keyword arguments or explicit argument names to access accounting_export within the decorator to avoid potential errors if the function signature changes in the future.
| try: | ||
| return func(*args) | ||
| except (FyleCredential.DoesNotExist): | ||
| logger.info('Fyle credentials not found %s', accounting_export.workspace_id) | ||
| accounting_export.detail = {'message': 'Fyle credentials do not exist in workspace'} | ||
| accounting_export.status = 'FAILED' | ||
|
|
||
| accounting_export.save() | ||
|
|
||
| except BusinessCentralCredentials.DoesNotExist: | ||
| logger.info('Business Central Account not connected / token expired for workspace_id %s / accounting export %s', accounting_export.workspace_id, accounting_export.id) | ||
| detail = {'accounting_export_id': accounting_export.id, 'message': 'Business Central Account not connected / token expired'} | ||
| accounting_export.status = 'FAILED' | ||
| accounting_export.detail = detail | ||
|
|
||
| accounting_export.save() | ||
|
|
||
| except WrongParamsError as exception: | ||
| handle_business_central_error(exception, accounting_export, 'Purchase Invoice') | ||
|
|
||
| except BulkError as exception: | ||
| logger.info(exception.response) | ||
| detail = exception.response | ||
| accounting_export.status = 'FAILED' | ||
| accounting_export.detail = detail | ||
|
|
||
| accounting_export.save() | ||
|
|
||
| except Exception as error: | ||
| error = traceback.format_exc() | ||
| accounting_export.detail = {'error': error} | ||
| accounting_export.status = 'FATAL' | ||
|
|
||
| accounting_export.save() | ||
| logger.error('Something unexpected happened workspace_id: %s %s', accounting_export.workspace_id, accounting_export.detail) | ||
|
|
||
| update_last_export_details(accounting_export.workspace_id) |
There was a problem hiding this comment.
There is repeated code for updating accounting_export status and saving it within each except block. Consider refactoring to a common function or using a finally block to reduce duplication and improve maintainability.
| class LastExportDetail(BaseModel): | ||
| """ | ||
| Table to store Last Export Details | ||
| """ | ||
|
|
||
| id = models.AutoField(primary_key=True) | ||
| last_exported_at = models.DateTimeField(help_text='Last exported at datetime', null=True) | ||
| export_mode = models.CharField(max_length=50, help_text='Mode of the export Auto / Manual', choices=EXPORT_MODE_CHOICES, null=True) | ||
| total_accounting_exports_count = models.IntegerField(help_text='Total count of accounting exports exported', null=True) | ||
| successful_accounting_exports_count = models.IntegerField(help_text='count of successful accounting_exports ', null=True) | ||
| failed_accounting_exports_count = models.IntegerField(help_text='count of failed accounting_exports ', null=True) | ||
|
|
||
| class Meta: | ||
| db_table = 'last_export_details' |
There was a problem hiding this comment.
The LastExportDetail model is well-defined with appropriate field types, help texts, and nullability. However, consider adding indexes to frequently queried fields like last_exported_at and export_mode to improve database performance.
class LastExportDetail(BaseModel):
...
last_exported_at = models.DateTimeField(help_text='Last exported at datetime', null=True, db_index=True)
export_mode = models.CharField(max_length=50, help_text='Mode of the export Auto / Manual', choices=EXPORT_MODE_CHOICES, null=True, db_index=True)
...
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor, Style, Tests, Chores, Revert