-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: allow selection of memo structure in advance settings #413
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
# Generated by Django 3.2.14 on 2024-12-03 10:44 | ||
|
||
import apps.workspaces.models | ||
import django.contrib.postgres.fields | ||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('workspaces', '0039_alter_workspacegeneralsettings_change_accounting_period'), | ||
] | ||
|
||
operations = [ | ||
migrations.AddField( | ||
model_name='workspacegeneralsettings', | ||
name='memo_structure', | ||
field=django.contrib.postgres.fields.ArrayField(base_field=models.CharField(max_length=100), default=apps.workspaces.models.get_default_memo_fields, help_text='list of system fields for creating custom memo', size=None), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ | |
from apps.fyle.enums import FyleAttributeEnum | ||
from apps.fyle.models import Expense, ExpenseGroup | ||
from apps.mappings.models import GeneralMapping | ||
from apps.workspaces.models import FyleCredential, Workspace | ||
from apps.workspaces.models import FyleCredential, Workspace, WorkspaceGeneralSettings | ||
|
||
|
||
def get_tracking_category(expense_group: ExpenseGroup, lineitem: Expense): | ||
|
@@ -144,33 +144,32 @@ def get_customer_id_or_none(expense_group: ExpenseGroup, lineitem: Expense): | |
return customer_id | ||
|
||
|
||
def get_expense_purpose(workspace_id, lineitem, category) -> str: | ||
fyle_credentials = FyleCredential.objects.get(workspace_id=workspace_id) | ||
def get_expense_purpose(workspace_id, lineitem, category, workspace_general_settings) -> str: | ||
|
||
fyle_credentials = FyleCredential.objects.get(workspace_id=workspace_id) | ||
org_id = Workspace.objects.get(id=workspace_id).fyle_org_id | ||
memo_structure = workspace_general_settings.memo_structure | ||
|
||
fyle_url = fyle_credentials.cluster_domain if settings.BRAND_ID == 'fyle' else settings.FYLE_APP_URL | ||
|
||
expense_link = "{0}/app/admin/#/enterprise/view_expense/{1}?org_id={2}".format( | ||
fyle_url, lineitem.expense_id, org_id | ||
) | ||
details = { | ||
'employee_email': lineitem.employee_email, | ||
'merchant': '{}'.format(lineitem.vendor) if lineitem.vendor else '', | ||
'category': '{0}'.format(category) if lineitem.category else '', | ||
'purpose': '{0}'.format(lineitem.purpose) if lineitem.purpose else '', | ||
'report_number': '{0}'.format(lineitem.claim_number), | ||
'spent_on': '{0}'.format(lineitem.spent_at.date()) if lineitem.spent_at else '', | ||
'expense_link': '{0}/app/admin/#/enterprise/view_expense/{1}?org_id={2}'.format(fyle_url, lineitem.expense_id, org_id) | ||
} | ||
Comment on lines
+155
to
+163
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Extract URL construction to a separate utility function. The URL construction logic should be separated from the business logic for better maintainability and reusability. +def get_expense_url(fyle_credentials, expense_id, org_id):
+ fyle_url = fyle_credentials.cluster_domain if settings.BRAND_ID == 'fyle' else settings.FYLE_APP_URL
+ return '{0}/app/admin/#/enterprise/view_expense/{1}?org_id={2}'.format(
+ fyle_url, expense_id, org_id
+ )
+
details = {
'employee_email': lineitem.employee_email,
'merchant': '{}'.format(lineitem.vendor) if lineitem.vendor else '',
'category': '{0}'.format(category) if lineitem.category else '',
'purpose': '{0}'.format(lineitem.purpose) if lineitem.purpose else '',
'report_number': '{0}'.format(lineitem.claim_number),
'spent_on': '{0}'.format(lineitem.spent_at.date()) if lineitem.spent_at else '',
- 'expense_link': '{0}/app/admin/#/enterprise/view_expense/{1}?org_id={2}'.format(fyle_url, lineitem.expense_id, org_id)
+ 'expense_link': get_expense_url(fyle_credentials, lineitem.expense_id, org_id)
}
|
||
|
||
expense_purpose = ( | ||
"purpose - {0}".format(lineitem.purpose) if lineitem.purpose else "" | ||
) | ||
spent_at = ( | ||
"spent on {0}".format(lineitem.spent_at.date()) if lineitem.spent_at else "" | ||
) | ||
vendor = "{} - ".format(lineitem.vendor) if lineitem.vendor else "" | ||
return "{0}{1}, category - {2} {3}, report number - {4} {5} - {6}".format( | ||
vendor, | ||
lineitem.employee_email, | ||
category, | ||
spent_at, | ||
lineitem.claim_number, | ||
expense_purpose, | ||
expense_link, | ||
) | ||
memo = '' | ||
for id, field in enumerate(memo_structure): | ||
if field in details: | ||
memo += details[field] | ||
if id + 1 != len(memo_structure): | ||
memo = '{0} - '.format(memo) | ||
|
||
return memo | ||
|
||
|
||
def get_tax_code_id_or_none(expense_group: ExpenseGroup, lineitem: Expense): | ||
|
@@ -262,7 +261,7 @@ class Meta: | |
db_table = "bill_lineitems" | ||
|
||
@staticmethod | ||
def create_bill_lineitems(expense_group: ExpenseGroup): | ||
def create_bill_lineitems(expense_group: ExpenseGroup, workspace_general_settings: WorkspaceGeneralSettings): | ||
expenses = expense_group.expenses.all() | ||
bill = Bill.objects.get(expense_group=expense_group) | ||
|
||
|
@@ -289,7 +288,7 @@ def create_bill_lineitems(expense_group: ExpenseGroup): | |
customer_id = get_customer_id_or_none(expense_group, lineitem) | ||
|
||
description = get_expense_purpose( | ||
expense_group.workspace_id, lineitem, category | ||
expense_group.workspace_id, lineitem, category, workspace_general_settings | ||
) | ||
|
||
tracking_categories = get_tracking_category(expense_group, lineitem) | ||
|
@@ -449,7 +448,7 @@ class Meta: | |
db_table = "bank_transaction_lineitems" | ||
|
||
@staticmethod | ||
def create_bank_transaction_lineitems(expense_group: ExpenseGroup): | ||
def create_bank_transaction_lineitems(expense_group: ExpenseGroup, workspace_general_settings: WorkspaceGeneralSettings): | ||
""" | ||
Create bank transaction lineitems | ||
:param expense_group: expense group | ||
|
@@ -481,7 +480,7 @@ def create_bank_transaction_lineitems(expense_group: ExpenseGroup): | |
customer_id = get_customer_id_or_none(expense_group, lineitem) | ||
|
||
description = get_expense_purpose( | ||
expense_group.workspace_id, lineitem, category | ||
expense_group.workspace_id, lineitem, category, workspace_general_settings | ||
) | ||
|
||
tracking_categories = get_tracking_category(expense_group, lineitem) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,26 +26,28 @@ | |
|
||
def test_create_bill(db): | ||
expense_group = ExpenseGroup.objects.get(id=4) | ||
workspace_general_settings = WorkspaceGeneralSettings.objects.filter(workspace_id=expense_group.workspace_id).first() | ||
|
||
bill = Bill.create_bill(expense_group) | ||
bill_lineitems = BillLineItem.create_bill_lineitems(expense_group) | ||
bill_lineitems = BillLineItem.create_bill_lineitems(expense_group, workspace_general_settings) | ||
|
||
for bill_lineitem in bill_lineitems: | ||
assert bill_lineitem.amount == 10.0 | ||
assert ( | ||
bill_lineitem.description | ||
== "[email protected], category - Food spent on 2020-01-01, report number - C/2022/06/R/1 - https://staging.fyle.tech/app/admin/#/enterprise/view_expense/txGilVGolf60?org_id=orPJvXuoLqvJ" | ||
== "[email protected] - Food - - 2020-01-01 - C/2022/06/R/1 - " | ||
) | ||
|
||
assert bill.currency == "USD" | ||
|
||
|
||
def test_bank_transaction(db): | ||
expense_group = ExpenseGroup.objects.get(id=5) | ||
workspace_general_settings = WorkspaceGeneralSettings.objects.filter(workspace_id=expense_group.workspace_id).first() | ||
|
||
bank_transaction = BankTransaction.create_bank_transaction(expense_group, True) | ||
bank_transaction_lineitems = ( | ||
BankTransactionLineItem.create_bank_transaction_lineitems(expense_group) | ||
BankTransactionLineItem.create_bank_transaction_lineitems(expense_group, workspace_general_settings) | ||
) | ||
|
||
for bank_transaction_lineitem in bank_transaction_lineitems: | ||
|
@@ -157,18 +159,20 @@ def test_get_expense_purpose(db): | |
expense_group = ExpenseGroup.objects.get(id=10) | ||
expenses = expense_group.expenses.all() | ||
|
||
workspace_general_settings = WorkspaceGeneralSettings.objects.filter(workspace_id=workspace_id).first() | ||
|
||
for lineitem in expenses: | ||
category = ( | ||
lineitem.category | ||
if lineitem.category == lineitem.sub_category | ||
else "{0} / {1}".format(lineitem.category, lineitem.sub_category) | ||
) | ||
|
||
expense_purpose = get_expense_purpose(workspace_id, lineitem, category) | ||
expense_purpose = get_expense_purpose(workspace_id, lineitem, category, workspace_general_settings) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification The review comment is correct - workspace_general_settings needs .first() Looking at the code context, we can see that
🔗 Analysis chainUpdate needed after applying the .first() fix This line will work correctly after applying the suggested fix to retrieve a single workspace_general_settings object. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Verify the get_expense_purpose function signature
ast-grep --pattern 'def get_expense_purpose($_, $_, $_, $_) {
$$$
}'
Length of output: 72 Script: #!/bin/bash
# Let's try to find the get_expense_purpose function definition with ripgrep
rg "def get_expense_purpose" -A 5
Length of output: 497 Script: #!/bin/bash
# Let's check the test file to understand the context of workspace_general_settings
rg "workspace_general_settings" tests/test_xero/test_models.py -B 2 -A 2
Length of output: 1218 |
||
|
||
assert ( | ||
expense_purpose | ||
== "[email protected], category - WIP / None spent on 2022-05-25, report number - C/2022/05/R/13 - https://staging.fyle.tech/app/admin/#/enterprise/view_expense/txBMQRkBQciI?org_id=orPJvXuoLqvJ" | ||
== "[email protected] - WIP / None - - 2022-05-25 - C/2022/05/R/13 - " | ||
) | ||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add validation for memo structure and optimize string concatenation.
The current implementation lacks validation for memo_structure and uses inefficient string concatenation.
Also applies to: 167-173