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

feat: allow selection of memo structure in advance settings #413

Merged
merged 5 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions apps/workspaces/apis/advanced_settings/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class Meta:
model = WorkspaceGeneralSettings
fields = [
"change_accounting_period",
"memo_structure",
"sync_fyle_to_xero_payments",
"sync_xero_to_fyle_payments",
"auto_create_destination_entity",
Expand Down Expand Up @@ -94,6 +95,9 @@ def update(self, instance, validated):
"change_accounting_period": workspace_general_settings.get(
"change_accounting_period"
),
"memo_structure": workspace_general_settings.get(
"memo_structure"
),
"sync_fyle_to_xero_payments": workspace_general_settings.get(
"sync_fyle_to_xero_payments"
),
Expand Down
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),
),
]
9 changes: 9 additions & 0 deletions apps/workspaces/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@
EXPORT_MODE_CHOICES = (("MANUAL", "MANUAL"), ("AUTO", "AUTO"))


def get_default_memo_fields():
return ['employee_email', 'category', 'merchant', 'spent_on', 'report_number', 'purpose']


def get_default_chart_of_accounts():
return ["EXPENSE"]

Expand Down Expand Up @@ -178,6 +182,11 @@ class WorkspaceGeneralSettings(models.Model):
null=True,
choices=AUTO_MAP_EMPLOYEE,
)
memo_structure = ArrayField(
base_field=models.CharField(max_length=100), default=get_default_memo_fields,
help_text='list of system fields for creating custom memo'
)

auto_create_destination_entity = models.BooleanField(
default=False, help_text="Auto create contact"
)
Expand Down
2 changes: 1 addition & 1 deletion apps/workspaces/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ def post(self, request, **kwargs):
)


class GeneralSettingsView(generics.RetrieveAPIView):
class GeneralSettingsView(generics.RetrieveUpdateAPIView):
"""
General Settings
"""
Expand Down
51 changes: 25 additions & 26 deletions apps/xero/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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
Copy link

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.

     memo_structure = workspace_general_settings.memo_structure
+    if not memo_structure:
+        return ''
 
-    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)
+    # Use list comprehension and join for better performance
+    memo_parts = [details[field] for field in memo_structure if field in details]
+    return ' - '.join(filter(None, memo_parts))

Also applies to: 167-173


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
Copy link

Choose a reason for hiding this comment

The 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)
 }

Committable suggestion skipped: line range outside the PR's diff.


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):
Expand Down Expand Up @@ -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)

Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions apps/xero/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ def create_bill(
with transaction.atomic():
bill_object = Bill.create_bill(expense_group)

bill_lineitems_objects = BillLineItem.create_bill_lineitems(expense_group)
bill_lineitems_objects = BillLineItem.create_bill_lineitems(expense_group, general_settings)

created_bill = xero_connection.post_bill(
bill_object, bill_lineitems_objects, general_settings
Expand Down Expand Up @@ -456,7 +456,7 @@ def create_bank_transaction(
)

bank_transaction_lineitems_objects = (
BankTransactionLineItem.create_bank_transaction_lineitems(expense_group)
BankTransactionLineItem.create_bank_transaction_lineitems(expense_group, general_settings)
)

created_bank_transaction = xero_connection.post_bank_transaction(
Expand Down
14 changes: 8 additions & 6 deletions tests/sql_fixtures/reset_db_fixtures/reset_db.sql
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
-- PostgreSQL database dump
--

-- Dumped from database version 15.9 (Debian 15.9-1.pgdg120+1)
-- Dumped by pg_dump version 15.8 (Debian 15.8-1.pgdg120+1)
-- Dumped from database version 15.10 (Debian 15.10-1.pgdg120+1)
-- Dumped by pg_dump version 15.10 (Debian 15.10-1.pgdg120+1)

SET statement_timeout = 0;
SET lock_timeout = 0;
Expand Down Expand Up @@ -1535,7 +1535,8 @@ CREATE TABLE public.workspace_general_settings (
change_accounting_period boolean NOT NULL,
auto_create_merchant_destination_entity boolean NOT NULL,
is_simplify_report_closure_enabled boolean NOT NULL,
import_suppliers_as_merchants boolean NOT NULL
import_suppliers_as_merchants boolean NOT NULL,
memo_structure character varying(100)[] NOT NULL
);


Expand Down Expand Up @@ -2643,6 +2644,7 @@ COPY public.django_migrations (id, app, name, applied) FROM stdin;
157 tasks 0010_alter_tasklog_expense_group 2024-11-17 21:11:41.133814+00
158 workspaces 0039_alter_workspacegeneralsettings_change_accounting_period 2024-11-18 04:43:45.472917+00
159 fyle 0022_support_split_expense_grouping 2024-11-18 10:49:49.550689+00
160 workspaces 0040_workspacegeneralsettings_memo_structure 2024-12-03 21:13:46.617079+00
\.


Expand Down Expand Up @@ -5066,8 +5068,8 @@ COPY public.users (password, last_login, id, email, user_id, full_name, active,
-- Data for Name: workspace_general_settings; Type: TABLE DATA; Schema: public; Owner: postgres
--

COPY public.workspace_general_settings (id, reimbursable_expenses_object, corporate_credit_card_expenses_object, created_at, updated_at, workspace_id, sync_fyle_to_xero_payments, sync_xero_to_fyle_payments, import_categories, auto_map_employees, auto_create_destination_entity, map_merchant_to_contact, skip_cards_mapping, import_tax_codes, charts_of_accounts, import_customers, change_accounting_period, auto_create_merchant_destination_entity, is_simplify_report_closure_enabled, import_suppliers_as_merchants) FROM stdin;
1 PURCHASE BILL BANK TRANSACTION 2022-08-02 20:25:24.644164+00 2022-08-02 20:25:24.644209+00 1 f t t \N t t f t {EXPENSE} t t f f f
COPY public.workspace_general_settings (id, reimbursable_expenses_object, corporate_credit_card_expenses_object, created_at, updated_at, workspace_id, sync_fyle_to_xero_payments, sync_xero_to_fyle_payments, import_categories, auto_map_employees, auto_create_destination_entity, map_merchant_to_contact, skip_cards_mapping, import_tax_codes, charts_of_accounts, import_customers, change_accounting_period, auto_create_merchant_destination_entity, is_simplify_report_closure_enabled, import_suppliers_as_merchants, memo_structure) FROM stdin;
1 PURCHASE BILL BANK TRANSACTION 2022-08-02 20:25:24.644164+00 2022-08-02 20:25:24.644209+00 1 f t t \N t t f t {EXPENSE} t t f f f {employee_email,category,merchant,spent_on,report_number,purpose}
\.


Expand Down Expand Up @@ -5180,7 +5182,7 @@ SELECT pg_catalog.setval('public.django_content_type_id_seq', 40, true);
-- Name: django_migrations_id_seq; Type: SEQUENCE SET; Schema: public; Owner: postgres
--

SELECT pg_catalog.setval('public.django_migrations_id_seq', 159, true);
SELECT pg_catalog.setval('public.django_migrations_id_seq', 160, true);


--
Expand Down
1 change: 1 addition & 0 deletions tests/test_workspaces/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
"import_suppliers_as_merchants": True,
"auto_map_employees": "EMAIL",
"auto_create_destination_entity": True,
"memo_structure": ["report_number", "spent_on"],
"skip_cards_mapping": False,
"import_tax_codes": True,
"import_customers": False,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"sync_fyle_to_xero_payments": False,
"sync_xero_to_fyle_payments": True,
"auto_create_destination_entity": True,
"memo_structure": ["report_number", "expense_link", "spent_on"],
"auto_create_merchant_destination_entity": True,
},
"general_mappings": {
Expand All @@ -24,6 +25,7 @@
"sync_xero_to_fyle_payments": True,
"auto_create_destination_entity": True,
"auto_create_merchant_destination_entity": True,
"memo_structure": ["report_number", "expense_link", "spent_on"]
},
"general_mappings": {
"payment_account": {"id": "2", "name": "Business Savings Account"}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
"sync_xero_to_fyle_payments": True,
"auto_create_destination_entity": True,
"auto_create_merchant_destination_entity": True,
"memo_structure": ["report_number", "expense_link", "spent_on"]
},
"general_mappings": {
"payment_account": {"id": "2", "name": "Business Savings Account"}
Expand Down Expand Up @@ -122,6 +123,7 @@
"sync_xero_to_fyle_payments": False,
"auto_create_destination_entity": False,
"change_accounting_period": False,
"memo_structure": ["report_number", "expense_link", "spent_on"],
"auto_create_merchant_destination_entity": False,
},
"general_mappings": {"payment_account": {"name": None, "id": None}},
Expand Down
14 changes: 9 additions & 5 deletions tests/test_xero/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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)
Copy link

Choose a reason for hiding this comment

The 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 workspace_general_settings is obtained using WorkspaceGeneralSettings.objects.filter() without .first(), while the get_expense_purpose() function in apps/xero/models.py expects a single object. This matches with other usages in the test file where .first() is correctly applied.

  • Line 171 in tests/test_xero/test_models.py needs to be updated to use .first() on the workspace_general_settings QuerySet.
🔗 Analysis chain

Update 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 executed

The 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 - "
)


Expand Down
Loading