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: Single credit line for Journal Entry #655

Merged
merged 4 commits into from
Oct 16, 2024
Merged

feat: Single credit line for Journal Entry #655

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f6cddc4
feat: Single credit line for Journal Entry
ashwin1111 Oct 11, 2024
885678a
merge master
ashwin1111 Oct 11, 2024
1e38d5f
test: add tests for single credit line <> JE (#656)
ashwin1111 Oct 16, 2024
b220cf2
dummy
ashwin1111 Oct 16, 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
1 change: 1 addition & 0 deletions apps/fyle/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ def create_expense_objects(expenses: List[Dict], workspace_id, skip_update: bool
if expense_data_to_append:
defaults.update(expense_data_to_append)


Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Usage of update_or_create in a loop may affect performance.

The use of Expense.objects.update_or_create() inside a loop can lead to multiple database hits, which may affect performance when processing a large number of expenses. Consider optimizing this by batching operations.

You might refactor the code to use bulk operations. For instance, collect the expenses to be created and use bulk_create for new expenses, and handle updates separately with bulk_update. This can significantly reduce the number of database queries.

Copy link
Contributor

Choose a reason for hiding this comment

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

please remove extra line

expense_object, _ = Expense.objects.update_or_create(
expense_id=expense['id'],
defaults=defaults
Expand Down
2 changes: 1 addition & 1 deletion apps/fyle/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def get_task_log_and_fund_source(workspace_id: int):

configuration = Configuration.objects.get(workspace_id=workspace_id)
fund_source = []

if configuration.reimbursable_expenses_object:
fund_source.append('PERSONAL')
if configuration.corporate_credit_card_expenses_object:
Expand Down
95 changes: 90 additions & 5 deletions apps/netsuite/connector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2085,8 +2085,89 @@ def construct_journal_entry_lineitems(self, journal_entry_lineitems: List[Journa

return lines

def construct_single_itemized_credit_line(self, journal_entry_lineitems: List[JournalEntryLineItem]):
"""
Create journal entry line items for single credit line
:return: constructed line items
"""
lines = []
distinct_line_ids = {}

for line in journal_entry_lineitems:
account_ref = line.debit_account_id
entity_id = line.entity_id
line_id = '{account_ref}::::{entity_id}'.format(account_ref=account_ref, entity_id=entity_id)

if line_id in distinct_line_ids:
distinct_line_ids[line_id] += line.amount
Comment on lines +2100 to +2103
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use Tuples Instead of String Concatenation for Dictionary Keys

Using string concatenation with separators for dictionary keys can lead to potential issues if the account_ref or entity_id contain the separator string '::::'. Consider using tuples as dictionary keys to avoid this problem.

Apply this diff to fix the issue:

-                line_id = '{account_ref}::::{entity_id}'.format(account_ref=account_ref, entity_id=entity_id)

-                if line_id in distinct_line_ids:
-                    distinct_line_ids[line_id] += line.amount
-                else:
-                    distinct_line_ids[line_id] = line.amount
+                key = (account_ref, entity_id)
+
+                if key in distinct_line_ids:
+                    distinct_line_ids[key] += line.amount
+                else:
+                    distinct_line_ids[key] = line.amount

Committable suggestion was skipped due to low confidence.

else:
distinct_line_ids[line_id] = line.amount

for line_id, amount in distinct_line_ids.items():
account_ref, entity_id = line_id.split('::::')
lineitem = {
'account': {
'name': None,
Comment on lines +2107 to +2111
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Unpack Tuple Keys Without String Splitting

Since tuples are used as keys, updating the unpacking logic improves clarity and efficiency by avoiding string manipulation.

Apply this diff to refactor the code:

-            for line_id, amount in distinct_line_ids.items():
-                account_ref, entity_id = line_id.split('::::')
+            for (account_ref, entity_id), amount in distinct_line_ids.items():
📝 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
for line_id, amount in distinct_line_ids.items():
account_ref, entity_id = line_id.split('::::')
lineitem = {
'account': {
'name': None,
for (account_ref, entity_id), amount in distinct_line_ids.items():
lineitem = {
'account': {
'name': None,

'internalId': account_ref,
'externalId': None,
'type': 'account'
},
'department': {
'name': None,
'internalId': None,
'externalId': None,
'type': 'department'
},
'location': {
'name': None,
'internalId': None,
'externalId': None,
'type': 'location'
},
'class': {
'name': None,
'internalId': None,
'externalId': None,
'type': 'classification'
},
'entity': {
'name': None,
'internalId': entity_id,
'externalId': None,
'type': 'vendor'
},
'credit': amount,
'creditTax': None,
'customFieldList': [],
'debit': None,
'debitTax': None,
'eliminate': None,
'endDate': None,
'grossAmt': None,
'line': None,
'lineTaxCode': None,
'lineTaxRate': None,
'memo': 'Total Credit Amount',
Copy link
Contributor

Choose a reason for hiding this comment

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

One question should we also add the unique identifier for this

For example we have 2 credit lines
123-ACB
456-XYZ

If both will have same memo then there is no referral to memo
We should add something like Total Credit Amount for ACC/Entity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

account and entity info is present in same line, hence didn't add - will check with Hemanth once

'residual': None,
'revenueRecognitionRule': None,
'schedule': None,
'scheduleNum': None,
'startDate': None,
'tax1Acct': None,
'taxAccount': None,
'taxBasis': None,
'tax1Amt': None,
'taxCode': None,
'taxRate1': None,
'totalAmount': None,
}

lines.append(lineitem)

return lines

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using tuples as dictionary keys instead of string concatenation

In the construct_single_itemized_credit_line method, you're concatenating account_ref and entity_id into a string to use as a dictionary key. This approach can lead to issues if account_ref or entity_id contain the separator ::::. A more robust solution is to use tuples as dictionary keys.

Here's how you can modify the code:

- line_id = '{account_ref}::::{entity_id}'.format(account_ref=account_ref, entity_id=entity_id)
...
- if line_id in distinct_line_ids:
+ key = (account_ref, entity_id)
+ if key in distinct_line_ids:
    distinct_line_ids[key] += line.amount
...
- for line_id, amount in distinct_line_ids.items():
-     account_ref, entity_id = line_id.split('::::')
+ for (account_ref, entity_id), amount in distinct_line_ids.items():
📝 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
def construct_single_itemized_credit_line(self, journal_entry_lineitems: List[JournalEntryLineItem]):
"""
Create journal entry line items for single credit line
:return: constructed line items
"""
lines = []
distinct_line_ids = {}
for line in journal_entry_lineitems:
account_ref = line.debit_account_id
entity_id = line.entity_id
line_id = '{account_ref}::::{entity_id}'.format(account_ref=account_ref, entity_id=entity_id)
if line_id in distinct_line_ids:
distinct_line_ids[line_id] += line.amount
else:
distinct_line_ids[line_id] = line.amount
for line_id, amount in distinct_line_ids.items():
account_ref, entity_id = line_id.split('::::')
lineitem = {
'account': {
'name': None,
'internalId': account_ref,
'externalId': None,
'type': 'account'
},
'department': {
'name': None,
'internalId': None,
'externalId': None,
'type': 'department'
},
'location': {
'name': None,
'internalId': None,
'externalId': None,
'type': 'location'
},
'class': {
'name': None,
'internalId': None,
'externalId': None,
'type': 'classification'
},
'entity': {
'name': None,
'internalId': entity_id,
'externalId': None,
'type': 'vendor'
},
'credit': amount,
'creditTax': None,
'customFieldList': [],
'debit': None,
'debitTax': None,
'eliminate': None,
'endDate': None,
'grossAmt': None,
'line': None,
'lineTaxCode': None,
'lineTaxRate': None,
'memo': 'Total Credit Amount',
'residual': None,
'revenueRecognitionRule': None,
'schedule': None,
'scheduleNum': None,
'startDate': None,
'tax1Acct': None,
'taxAccount': None,
'taxBasis': None,
'tax1Amt': None,
'taxCode': None,
'taxRate1': None,
'totalAmount': None,
}
lines.append(lineitem)
return lines
def construct_single_itemized_credit_line(self, journal_entry_lineitems: List[JournalEntryLineItem]):
"""
Create journal entry line items for single credit line
:return: constructed line items
"""
lines = []
distinct_line_ids = {}
for line in journal_entry_lineitems:
account_ref = line.debit_account_id
entity_id = line.entity_id
key = (account_ref, entity_id)
if key in distinct_line_ids:
distinct_line_ids[key] += line.amount
else:
distinct_line_ids[key] = line.amount
for (account_ref, entity_id), amount in distinct_line_ids.items():
lineitem = {
'account': {
'name': None,
'internalId': account_ref,
'externalId': None,
'type': 'account'
},
'department': {
'name': None,
'internalId': None,
'externalId': None,
'type': 'department'
},
'location': {
'name': None,
'internalId': None,
'externalId': None,
'type': 'location'
},
'class': {
'name': None,
'internalId': None,
'externalId': None,
'type': 'classification'
},
'entity': {
'name': None,
'internalId': entity_id,
'externalId': None,
'type': 'vendor'
},
'credit': amount,
'creditTax': None,
'customFieldList': [],
'debit': None,
'debitTax': None,
'eliminate': None,
'endDate': None,
'grossAmt': None,
'line': None,
'lineTaxCode': None,
'lineTaxRate': None,
'memo': 'Total Credit Amount',
'residual': None,
'revenueRecognitionRule': None,
'schedule': None,
'scheduleNum': None,
'startDate': None,
'tax1Acct': None,
'taxAccount': None,
'taxBasis': None,
'tax1Amt': None,
'taxCode': None,
'taxRate1': None,
'totalAmount': None,
}
lines.append(lineitem)
return lines

def __construct_journal_entry(self, journal_entry: JournalEntry,
journal_entry_lineitems: List[JournalEntryLineItem]) -> Dict:
journal_entry_lineitems: List[JournalEntryLineItem], configuration: Configuration) -> Dict:
ruuushhh marked this conversation as resolved.
Show resolved Hide resolved
"""
Create a journal entry report
:return: constructed journal entry
Expand All @@ -2096,7 +2177,11 @@ def __construct_journal_entry(self, journal_entry: JournalEntry,
cluster_domain = fyle_credentials.cluster_domain
org_id = Workspace.objects.get(id=journal_entry.expense_group.workspace_id).fyle_org_id

credit_line = self.construct_journal_entry_lineitems(journal_entry_lineitems, credit='Credit', org_id=org_id)
if configuration.je_single_credit_line:
credit_line = self.construct_single_itemized_credit_line(journal_entry_lineitems)
else:
credit_line = self.construct_journal_entry_lineitems(journal_entry_lineitems, credit='Credit', org_id=org_id)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Control Flow Based on Configuration Should Handle Edge Cases

When using configuration.je_single_credit_line, ensure that both the True and False branches are thoroughly tested, and consider what happens if the configuration is missing or None.

Consider adding a check to handle cases where configuration might not have the je_single_credit_line attribute.


debit_line = self.construct_journal_entry_lineitems(
journal_entry_lineitems,
debit='Debit', attachment_links={},
Expand Down Expand Up @@ -2166,13 +2251,13 @@ def __construct_journal_entry(self, journal_entry: JournalEntry,
return journal_entry_payload

def post_journal_entry(self, journal_entry: JournalEntry,
journal_entry_lineitems: List[JournalEntryLineItem]):
journal_entry_lineitems: List[JournalEntryLineItem], configuration: Configuration):
"""
Post journal entries to NetSuite
"""
configuration = Configuration.objects.get(workspace_id=self.workspace_id)
try:
journal_entry_payload = self.__construct_journal_entry(journal_entry, journal_entry_lineitems)
journal_entry_payload = self.__construct_journal_entry(journal_entry, journal_entry_lineitems, configuration)

logger.info("| Payload for Journal Entry creation | Content: {{WORKSPACE_ID: {} EXPENSE_GROUP_ID: {} JOURNAL_ENTRY_PAYLOAD: {}}}".format(self.workspace_id, journal_entry.expense_group.id, journal_entry_payload))

Expand All @@ -2186,7 +2271,7 @@ def post_journal_entry(self, journal_entry: JournalEntry,

if configuration.change_accounting_period and detail['message'] == message:
first_day_of_month = datetime.today().date().replace(day=1)
journal_entry_payload = self.__construct_journal_entry(journal_entry, journal_entry_lineitems)
journal_entry_payload = self.__construct_journal_entry(journal_entry, journal_entry_lineitems, configuration)
journal_entry_payload['tranDate'] = first_day_of_month
created_journal_entry = self.connection.journal_entries.post(journal_entry_payload)

Expand Down
2 changes: 0 additions & 2 deletions apps/netsuite/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1134,8 +1134,6 @@ def create_journal_entry_lineitems(expense_group: ExpenseGroup, configuration: C
configuration = Configuration.objects.get(workspace_id=expense_group.workspace_id)
employee_field_mapping = configuration.employee_field_mapping

debit_account_id = None

journal_entry_lineitem_objects = []

for lineitem in expenses:
Expand Down
2 changes: 1 addition & 1 deletion apps/netsuite/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,7 @@ def create_journal_entry(expense_group, task_log_id, last_export):
)

created_journal_entry = netsuite_connection.post_journal_entry(
journal_entry_object, journal_entry_lineitems_objects
journal_entry_object, journal_entry_lineitems_objects, configuration
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistent Use of configuration Parameter in create_journal_entry Calls

The configuration parameter has been added to the post_journal_entry method; however, not all calls to create_journal_entry are passing this new parameter. This inconsistency may lead to potential runtime errors or unexpected behaviors.

  • Affected Files:
    • apps/netsuite/tasks.py: Multiple calls to create_journal_entry need to include the configuration parameter.
    • tests/test_netsuite/test_tasks.py: Tests invoking create_journal_entry should be updated to pass the configuration parameter.
🔗 Analysis chain

Verify the impact of the new configuration parameter

The post_journal_entry method now includes a configuration parameter. This change allows for more flexible journal entry creation based on configuration settings.

To ensure this change doesn't break existing functionality and is properly implemented throughout the codebase, please run the following checks:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the configuration parameter is properly passed in all calls to create_journal_entry
rg "create_journal_entry\(" --type py

# Verify if the post_journal_entry method in the NetSuiteConnector class accepts the new configuration parameter
rg "def post_journal_entry" --type py

# Check for any tests that need to be updated due to this change
rg "test.*create_journal_entry" --type py

Length of output: 1868

)
worker_logger.info('Created Journal Entry with Expense Group %s successfully', expense_group.id)

Expand Down
3 changes: 2 additions & 1 deletion apps/workspaces/apis/advanced_settings/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ class Meta:
'sync_netsuite_to_fyle_payments',
'auto_create_destination_entity',
'auto_create_merchants',
'memo_structure'
'memo_structure',
'je_single_credit_line'
]


Expand Down
18 changes: 18 additions & 0 deletions apps/workspaces/migrations/0039_configuration_je_single_credit_line.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 3.2.14 on 2024-10-09 12:10

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('workspaces', '0038_configuration_allow_intercompany_vendors'),
]

operations = [
migrations.AddField(
model_name='configuration',
name='je_single_credit_line',
field=models.BooleanField(default=False, help_text='Journal Entry Single Credit Line'),
),
]
Comment on lines +12 to +18
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding an explicit reverse migration.

While Django automatically handles reverse migrations for field additions, it's a good practice to explicitly define them for clarity and to ensure proper rollback functionality.

Consider updating the operations list to include a reverse migration:

operations = [
    migrations.AddField(
        model_name='configuration',
        name='je_single_credit_line',
        field=models.BooleanField(default=False, help_text='Enable single credit line for journal entries instead of multiple lines'),
    ),
]

# Add this method to the Migration class
def reverse_migration(apps, schema_editor):
    Configuration = apps.get_model('workspaces', 'Configuration')
    Configuration.objects.all().update(je_single_credit_line=False)

operations.append(migrations.RunPython(migrations.noop, reverse_migration))

This ensures that if the migration needs to be reversed, the field will be properly removed and any data cleaned up.

1 change: 1 addition & 0 deletions apps/workspaces/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ class Configuration(models.Model):
updated_at = models.DateTimeField(auto_now=True, help_text='Updated at')
name_in_journal_entry = models.CharField(max_length=100, help_text='Name in jounral entry for ccc expense only', default='MERCHANT',choices=NAME_IN_JOURNAL_ENTRY)
allow_intercompany_vendors = models.BooleanField(default=False, help_text='Allow intercompany vendors')
je_single_credit_line = models.BooleanField(default=False, help_text='Journal Entry Single Credit Line')
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Update all affected files to handle the new je_single_credit_line field appropriately.

The addition of the je_single_credit_line field in the Configuration model impacts multiple areas of the application. Please ensure that the following files are reviewed and updated to handle this new configuration option:

  • tests/test_workspaces/fixtures.py
  • tests/test_workspaces/test_apis/test_export_settings/test_views.py
  • tests/test_netsuite/test_views.py
  • tests/test_netsuite/test_tasks.py
  • tests/test_netsuite/conftest.py
  • scripts/python/parse-error.py
  • tests/conftest.py
  • apps/workspaces/models.py
  • apps/netsuite/models.py
  • apps/netsuite/connector.py
  • apps/netsuite/queue.py
  • apps/netsuite/tasks.py
  • apps/netsuite/migrations/0002_expensereports_journalentries.py
  • apps/tasks/models.py
  • apps/workspaces/actions.py
  • apps/workspaces/serializers.py

Ensure that each of these files properly references and handles the new je_single_credit_line field to maintain consistency and functionality across the application.

🔗 Analysis chain

Verify related code for consistency with the new field.

While the addition of the je_single_credit_line field is straightforward, it's important to ensure that other parts of the application are updated to handle this new configuration option.

Please run the following script to identify areas of the codebase that might need updates:

Review the output and update any relevant code to handle the new je_single_credit_line configuration option.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find potential areas that might need updates due to the new je_single_credit_line field.

# Search for uses of the Configuration model
echo "Files referencing the Configuration model:"
rg "from .+models import .+Configuration" --type python

# Search for journal entry creation or processing
echo "\nFiles potentially involved in journal entry processing:"
rg "journal.+entr(y|ies)" --type python

# Search for existing uses of configuration fields related to journal entries
echo "\nExisting uses of configuration fields for journal entries:"
rg "configuration\..+journal" --type python

Length of output: 589

Script:

#!/bin/bash
# Description: Find potential areas that might need updates due to the new je_single_credit_line field.

# Search for uses of the Configuration model
echo "Files referencing the Configuration model:"
rg "from .+models import .+Configuration" --type py

# Search for journal entry creation or processing
echo "\nFiles potentially involved in journal entry processing:"
rg "journal.+entr(y|ies)" --type py

# Search for existing uses of configuration fields related to journal entries
echo "\nExisting uses of configuration fields for journal entries:"
rg "configuration\..+journal" --type py

Length of output: 18413


class Meta:
db_table = 'configurations'
Expand Down
7 changes: 5 additions & 2 deletions tests/test_netsuite/test_connector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,10 @@ def test_construct_bill_item_and_account_based(create_bill_item_and_account_base
def test_construct_journal_entry(create_journal_entry):
netsuite_credentials = NetSuiteCredentials.objects.get(workspace_id=1)
netsuite_connection = NetSuiteConnector(netsuite_credentials=netsuite_credentials, workspace_id=1)
configuration = Configuration.objects.get(workspace_id=1)

journal_entry, journal_entry_lineitem = create_journal_entry
journal_entry_object = netsuite_connection._NetSuiteConnector__construct_journal_entry(journal_entry, journal_entry_lineitem)
journal_entry_object = netsuite_connection._NetSuiteConnector__construct_journal_entry(journal_entry, journal_entry_lineitem, configuration)

journal_entry_object['tranDate'] = data['journal_entry_without_single_line'][0]['tranDate']

Expand Down Expand Up @@ -635,13 +636,15 @@ def test_post_journal_entry_exception(db, mocker, create_journal_entry):

journal_entry_transaction, journal_entry_transaction_lineitems = create_journal_entry

configuration = Configuration.objects.get(workspace_id=workspace_id)

workspace_general_setting = Configuration.objects.get(workspace_id=workspace_id)
workspace_general_setting.change_accounting_period = True
workspace_general_setting.save()

with mock.patch('netsuitesdk.api.journal_entries.JournalEntries.post') as mock_call:
mock_call.side_effect = [NetSuiteRequestError('An error occured in a upsert request: The transaction date you specified is not within the date range of your accounting period.'), None]
netsuite_connection.post_journal_entry(journal_entry_transaction, journal_entry_transaction_lineitems)
netsuite_connection.post_journal_entry(journal_entry_transaction, journal_entry_transaction_lineitems, configuration)

def test_update_destination_attributes(db, mocker):
mocker.patch(
Expand Down
Loading