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.

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

PR1: add support for accounting export and expense creations #72

Merged
merged 9 commits into from
Nov 10, 2023

Conversation

NileshPant1999
Copy link
Contributor

No description provided.

Copy link

github-actions bot commented Nov 6, 2023

Tests Skipped Failures Errors Time
20 0 💤 0 ❌ 0 🔥 2.336s ⏱️

@NileshPant1999 NileshPant1999 changed the title add support for accounting export and expense creations PR1: add support for accounting export and expense creations Nov 7, 2023
Copy link

github-actions bot commented Nov 7, 2023

Tests Skipped Failures Errors Time
20 0 💤 0 ❌ 0 🔥 2.702s ⏱️

)

# Doing this to avoid circular import
AccountingExport = import_string('apps.accounting_exports.models.AccountingExport')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is to avoid circular import

AccountingExport = import_string('apps.accounting_exports.models.AccountingExport')

# Check if an AccountingExport related to the expense object already exists
if not AccountingExport.objects.filter(expenses__id=expense_object.id).first():
Copy link
Contributor

Choose a reason for hiding this comment

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

expenses_with_accounting_export = Expense.objects.filter(accountingexport__isnull=False).distinct()

Try something like this

@@ -50,6 +100,56 @@ class AccountingExport(BaseForeignWorkspaceModel):
class Meta:
db_table = 'accounting_exports'

@staticmethod
def create_accounting_export_report_id(expense_objects: List[Expense], fund_source: str, workspace_id):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def create_accounting_export_report_id(expense_objects: List[Expense], fund_source: str, workspace_id):
def create_accounting_export(expense_objects: List[Expense], fund_source: str, workspace_id):

reimbursable_expense_date = export_setting.reimbursable_expense_date

if fund_source == 'CCC':
group_fields = ['report_id', 'claim_number'] if credit_card_expense_grouped_by == 'REPORT' else ['expense_id', 'expense_number']
Copy link
Contributor

Choose a reason for hiding this comment

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

{
    'CCC': {
        'REPORT': [..],
        'EXPENSE': [..]
    }
}

if credit_card_expense_date != 'LAST_SPENT_AT':
group_fields.append(credit_card_expense_date.lower())

if fund_source == 'PERSONAL':
Copy link
Contributor

Choose a reason for hiding this comment

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

the group fields are same, you can merge the two condition


# Extract expense IDs from the provided expenses
expense_ids = [expense.id for expense in expenses]

Copy link
Contributor

Choose a reason for hiding this comment

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

add something like

    group_fields.append(fund_source)


# Format date fields according to the specified format
for key in expense_group:
if key in ALLOWED_FORM_INPUT['export_date_type']:
Copy link
Contributor

Choose a reason for hiding this comment

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

ALLOWED_FORM_INPUT and all is not required

expense_group_fields = ['employee_email', 'fund_source']

# Group expenses based on specified fields and fund_source
expense_groups = _group_expenses(expense_objects, expense_group_fields, export_setting, fund_source)
Copy link
Contributor

Choose a reason for hiding this comment

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

user term accounting_exports everywhere
remove expense_groups completely


for expense_group in expense_groups:
# Determine the date field based on fund_source
if fund_source == 'PERSONAL':
Copy link
Contributor

Choose a reason for hiding this comment

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

the conditions can be cleaned up using dictionaties

Copy link
Contributor

Choose a reason for hiding this comment

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

data = EXPORT_SETTINGS_CONFIG[fund_source]['date_field']

from apps.fyle.models import Expense


ALLOWED_FIELDS = [
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

Copy link

Tests Skipped Failures Errors Time
20 0 💤 0 ❌ 0 🔥 2.302s ⏱️

@NileshPant1999 NileshPant1999 merged commit 6ce3a18 into master Nov 10, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants