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.

Sign up for GitHub

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

feat: Add Receipt URLs to DB #643

Closed
wants to merge 2 commits into from
Closed

feat: Add Receipt URLs to DB #643

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
952a06a
feat: Add Receipt URLs to DB
Sep 24, 2024
e94720e
Fix tests and comments
Sep 24, 2024
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
8 changes: 8 additions & 0 deletions apps/netsuite/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,8 @@ class Bill(models.Model):
is_retired = models.BooleanField(help_text='Is Payment sync retried', default=False)
created_at = models.DateTimeField(auto_now_add=True, help_text='Created at')
updated_at = models.DateTimeField(auto_now=True, help_text='Updated at')
receipt_urls = JSONField(help_text='Expense Receipt URL Map', null=True)
is_attachment_upload_failed = models.BooleanField(help_text='Is Attachment Upload Failed', default=False)

class Meta:
db_table = 'bills'
Expand Down Expand Up @@ -580,6 +582,8 @@ class CreditCardCharge(models.Model):
transaction_date = models.DateTimeField(help_text='CC Charge transaction date')
created_at = models.DateTimeField(auto_now_add=True, help_text='Created at')
updated_at = models.DateTimeField(auto_now=True, help_text='Updated at')
receipt_urls = JSONField(help_text='Expense Receipt URL Map', null=True)
is_attachment_upload_failed = models.BooleanField(help_text='Is Attachment Upload Failed', default=False)

class Meta:
db_table = 'credit_card_charges'
Expand Down Expand Up @@ -796,6 +800,8 @@ class ExpenseReport(models.Model):
is_retired = models.BooleanField(help_text='Is Payment sync retried', default=False)
created_at = models.DateTimeField(auto_now_add=True, help_text='Created at')
updated_at = models.DateTimeField(auto_now=True, help_text='Updated at')
receipt_urls = JSONField(help_text='Expense Receipt URL Map', null=True)
is_attachment_upload_failed = models.BooleanField(help_text='Is Attachment Upload Failed', default=False)

class Meta:
db_table = 'expense_reports'
Expand Down Expand Up @@ -1025,6 +1031,8 @@ class JournalEntry(models.Model):
paid_on_netsuite = models.BooleanField(help_text='Payment Status in NetSuite', default=False)
created_at = models.DateTimeField(auto_now_add=True, help_text='Created at')
updated_at = models.DateTimeField(auto_now=True, help_text='Updated at')
receipt_urls = JSONField(help_text='Expense Receipt URL Map', null=True)
is_attachment_upload_failed = models.BooleanField(help_text='Is Attachment Upload Failed', default=False)

class Meta:
db_table = 'journal_entries'
Expand Down
26 changes: 24 additions & 2 deletions apps/netsuite/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@
'CREATING_CREDIT_CARD_CHARGE': 'construct_credit_card_charge_lineitems'
}

TASK_TYPE_MODEL_MAP = {
'CREATING_EXPENSE_REPORT': ExpenseReport,
'CREATING_BILL': Bill,
'CREATING_JOURNAL_ENTRY': JournalEntry
}

Comment on lines +53 to +58
Copy link

Choose a reason for hiding this comment

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

Potential KeyError due to missing 'CREATING_CREDIT_CARD_CHARGE' in TASK_TYPE_MODEL_MAP

The TASK_TYPE_MODEL_MAP dictionary does not include the 'CREATING_CREDIT_CARD_CHARGE' key, which is used elsewhere and could lead to a KeyError when task_log.type is 'CREATING_CREDIT_CARD_CHARGE'.

Consider adding the missing key to TASK_TYPE_MODEL_MAP:

 TASK_TYPE_MODEL_MAP = {
     'CREATING_EXPENSE_REPORT': ExpenseReport,
     'CREATING_BILL': Bill,
     'CREATING_JOURNAL_ENTRY': JournalEntry,
+    'CREATING_CREDIT_CARD_CHARGE': CreditCardCharge
 }

Committable suggestion was skipped due to low confidence.

TASK_TYPE_EXPORT_COL_MAP = {
'CREATING_EXPENSE_REPORT': 'expense_report',
'CREATING_BILL': 'bill',
Expand Down Expand Up @@ -90,7 +96,7 @@ def update_expense_and_post_summary(in_progress_expenses: List[Expense], workspa
post_accounting_export_summary(fyle_org_id, workspace_id, fund_source)


def load_attachments(netsuite_connection: NetSuiteConnector, expense: Expense, expense_group: ExpenseGroup):
def load_attachments(netsuite_connection: NetSuiteConnector, expense: Expense, expense_group: ExpenseGroup, credit_card_charge_object: CreditCardCharge):
"""
Get attachments from Fyle
:param netsuite_connection: NetSuite Connection
Expand Down Expand Up @@ -145,13 +151,17 @@ def load_attachments(netsuite_connection: NetSuiteConnector, expense: Expense, e

except InvalidTokenError:
logger.info('Invalid Fyle refresh token for workspace %s', workspace_id)
credit_card_charge_object.is_attachment_upload_failed = True
credit_card_charge_object.save()
Comment on lines +154 to +155
Copy link

Choose a reason for hiding this comment

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

Refactor duplicate code in exception handlers

The lines setting credit_card_charge_object.is_attachment_upload_failed = True and saving the object are duplicated in both exception handlers. This violates the DRY (Don't Repeat Yourself) principle.

Consider refactoring the code to reduce duplication. One approach is to handle both exceptions together:

except (InvalidTokenError, Exception) as e:
    logger.info(
        'Attachment failed for expense group id %s / workspace id %s Error: %s',
        expense.expense_id, workspace_id, {'error': str(e)}
    )
    credit_card_charge_object.is_attachment_upload_failed = True
    credit_card_charge_object.save()

Also applies to: 163-164


except Exception:
error = traceback.format_exc()
logger.info(
'Attachment failed for expense group id %s / workspace id %s Error: %s',
expense.expense_id, workspace_id, {'error': error}
)
credit_card_charge_object.is_attachment_upload_failed = True
credit_card_charge_object.save()


def get_or_create_credit_card_vendor(expense_group: ExpenseGroup, merchant: str, auto_create_merchants: bool):
Expand Down Expand Up @@ -368,6 +378,9 @@ def upload_attachments_and_update_export(expenses: List[Expense], task_log: Task
netsuite_connection = NetSuiteConnector(netsuite_credentials, workspace_id)
workspace = netsuite_credentials.workspace

task_model = TASK_TYPE_MODEL_MAP[task_log.type]
task_model = task_model.objects.get(expense_group_id=task_log.expense_group_id)
ruuushhh marked this conversation as resolved.
Show resolved Hide resolved

Copy link

Choose a reason for hiding this comment

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

Avoid variable shadowing for better readability

At line 381, task_model is assigned a model class, and then at line 382, it's reassigned to an instance of that model. This shadowing can be confusing.

Consider using distinct variable names:

-task_model = TASK_TYPE_MODEL_MAP[task_log.type]
-task_model = task_model.objects.get(expense_group_id=task_log.expense_group_id)
+task_model_class = TASK_TYPE_MODEL_MAP[task_log.type]
+task_model = task_model_class.objects.get(expense_group_id=task_log.expense_group_id)
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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
task_model = TASK_TYPE_MODEL_MAP[task_log.type]
task_model = task_model.objects.get(expense_group_id=task_log.expense_group_id)
task_model_class = TASK_TYPE_MODEL_MAP[task_log.type]
task_model = task_model_class.objects.get(expense_group_id=task_log.expense_group_id)

platform = PlatformConnector(fyle_credentials=fyle_credentials)

expense_id_receipt_url_map = {}
Expand Down Expand Up @@ -408,15 +421,21 @@ def upload_attachments_and_update_export(expenses: List[Expense], task_log: Task
expense_id_receipt_url_map[expense.expense_id] = receipt_url

construct_payload_and_update_export(expense_id_receipt_url_map, task_log, workspace, fyle_credentials.cluster_domain, netsuite_connection)
task_model.receipt_urls = json.dumps(expense_id_receipt_url_map)
ruuushhh marked this conversation as resolved.
Show resolved Hide resolved
task_model.save()

except (NetSuiteRateLimitError, NetSuiteRequestError, NetSuiteLoginError, InvalidTokenError) as exception:
logger.info('Error while uploading attachments to netsuite workspace_id - %s %s', workspace_id, exception.__dict__)
task_model.is_attachment_upload_failed = True
task_model.save()
Comment on lines +429 to +430
Copy link

Choose a reason for hiding this comment

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

Consolidate duplicated exception handling code

The code setting task_model.is_attachment_upload_failed = True and saving it is duplicated in multiple exceptions. This can be refactored to improve maintainability.

You could set the flag once after the exceptions:

try:
    # Existing code
except (NetSuiteRateLimitError, NetSuiteRequestError, NetSuiteLoginError, InvalidTokenError) as exception:
    logger.info('Error while uploading attachments to NetSuite workspace_id - %s %s', workspace_id, exception.__dict__)
    error_occurred = True
except Exception as exception:
    logger.error(
        'Error while uploading attachments to NetSuite workspace_id - %s %s %s',
        workspace_id, exception, traceback.format_exc()
    )
    error_occurred = True

if error_occurred:
    task_model.is_attachment_upload_failed = True
    task_model.save()

Also applies to: 437-438


except Exception as exception:
logger.error(
'Error while uploading attachments to netsuite workspace_id - %s %s %s',
workspace_id, exception, traceback.format_exc()
)
task_model.is_attachment_upload_failed = True
task_model.save()


def resolve_errors_for_exported_expense_group(expense_group, workspace_id=None):
Expand Down Expand Up @@ -539,11 +558,14 @@ def create_credit_card_charge(expense_group, task_log_id, last_export):
if expense.amount < 0:
refund = True

attachment_link = load_attachments(netsuite_connection, expense, expense_group)
attachment_link = load_attachments(netsuite_connection, expense, expense_group, credit_card_charge_object)

if attachment_link:
attachment_links[expense.expense_id] = attachment_link

credit_card_charge_object.receipt_urls = json.dumps(attachment_links)
ruuushhh marked this conversation as resolved.
Show resolved Hide resolved
credit_card_charge_object.save()

created_credit_card_charge = netsuite_connection.post_credit_card_charge(
credit_card_charge_object, credit_card_charge_lineitems_object, attachment_links, refund
)
Expand Down
Loading