feat: Netsuite Tax Override #662
Conversation
ruuushhh
had a problem deploying
to
CI Environment
GitHub Actions
Failure
ruuushhh
had a problem deploying
to
CI Environment
GitHub Actions
Failure
WalkthroughThis pull request introduces a new Boolean field, Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@ruuushhh has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
apps/mappings/models.py (1)
71-71: LGTM! Consider documenting the tax balancing behavior.The new boolean field aligns well with the existing tax-related fields. However, the help text could be more descriptive about when and why tax balancing would be enabled.
Consider enhancing the help text:
- is_tax_balancing_enabled = models.BooleanField(default=False, help_text='Is tax balancing enabled') + is_tax_balancing_enabled = models.BooleanField( + default=False, + help_text='Enable this to balance tax amounts between Fyle and NetSuite transactions' + )apps/netsuite/connector.py (2)
1166-1173: Mismatch between docstring and function parametersThe docstring mentions a parameter
is_credit_card_chargewhich is not present in the function signature. Please update the docstring to reflect the correct parameters.
1364-1375: Refactor duplicated code blocks to improve maintainabilityThe code handling tax overrides in both
AccountBasedExpenseLineDetailandItemBasedExpenseLineDetailsections appears similar. Consider refactoring this logic into a separate method to reduce duplication and enhance code readability.Also applies to: 1397-1406
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/mappings/migrations/0015_generalmapping_is_tax_balancing_enabled.py(1 hunks)apps/mappings/models.py(1 hunks)apps/netsuite/connector.py(12 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/mappings/migrations/0015_generalmapping_is_tax_balancing_enabled.py
🔇 Additional comments (3)
apps/mappings/models.py (1)
73-73: LGTM! Good addition of related_name.
The addition of related_name='general_mappings' to the workspace relationship is a good practice. It provides a clear and intuitive way to access GeneralMapping from a Workspace instance.
Let's verify this doesn't conflict with any existing related names:
✅ Verification successful
Let me search for any other relationships to Workspace model to ensure there are no conflicts.
✓ The related_name='general_mappings' is unique and doesn't conflict
The verification shows that there are only two related_name fields referencing the Workspace model:
workspace_schedulesin workspaces appgeneral_mappingsin mappings app (the one we're reviewing)
These names are distinct and won't cause any conflicts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other models with relationships to Workspace
# that might conflict with 'general_mappings'
rg "models\..*Field.*Workspace.*related_name.*general_mappings" --type py
Length of output: 487
Script:
#!/bin/bash
# Search for all relationships to Workspace model
rg "models\..*Field.*Workspace.*related_name" --type py -A 1
Length of output: 1222
apps/netsuite/connector.py (2)
1253-1257: Verify the use of duplicate custom fields
The custom fields custcolfyle_receipt_link_2 and custcolfyle_expense_url_2 are being added alongside custcolfyle_receipt_link and custcolfyle_expense_url. Please verify if both sets of fields are necessary, or if this is a redundancy.
If only one set of custom fields is required, consider removing the duplicates to streamline the data.
Also applies to: 1271-1277
1285-1291: Ensure all calls to construct_bill_lineitems include the new parameter override_tax_details
The method construct_bill_lineitems has been updated with a new parameter override_tax_details. Please verify that all calls to this method have been updated to pass this parameter to prevent any unexpected behavior.
Run the following script to identify calls to construct_bill_lineitems without the required parameter:
✅ Verification successful
Let me verify the actual content of the call to understand why we're seeing 6 arguments when splitting by comma.
The method call has been properly updated with all required parameters
The verification shows that the only call to construct_bill_lineitems in the codebase is correctly passing all five required parameters:
- bill_lineitems
- {} (empty dictionary for attachment_links)
- cluster_domain
- org_id
- bill.override_tax_details
The false positive in the argument count (showing 6) was due to the comma within the tuple unpacking expense_list, item_list = being counted by the awk command.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all calls to `construct_bill_lineitems` include the new parameter `override_tax_details`.
# Search for all function calls to `construct_bill_lineitems`
rg 'self\.construct_bill_lineitems\(' --no-heading --line-number
# Check if the number of arguments matches the updated signature (should be 5 arguments)
# This is a simplified check assuming arguments are not spread over multiple lines
rg 'self\.construct_bill_lineitems\([^)]{0,}\)' --no-heading --line-number | \
awk -F',' '{if (NF != 5) print "Line "$1": Incorrect number of arguments ("NF")"}'
Length of output: 486
Script:
#!/bin/bash
# Get the context of the function call
rg 'self\.construct_bill_lineitems\(' -A 1 -B 1
Length of output: 299
|
|
||
| if general_mapping.is_tax_balancing_enabled and round(line.tax_amount, 2) != expected_tax_amount: | ||
| # Recalculate the net amount based on the modified tax | ||
| recalculated_net_amount = round((line.tax_amount * 100) / tax_item_rate, 2) |
There was a problem hiding this comment.
Potential division by zero when tax_item_rate is zero
In line 1188, the calculation (line.tax_amount * 100) / tax_item_rate could result in a division by zero error if tax_item_rate is zero. Please add a check to ensure that tax_item_rate is not zero before performing the division.
Apply this diff to handle the division by zero:
+ if tax_item_rate == 0:
+ raise ValueError("Tax item rate cannot be zero")
recalculated_net_amount = round((line.tax_amount * 100) / tax_item_rate, 2)📝 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.
| recalculated_net_amount = round((line.tax_amount * 100) / tax_item_rate, 2) | |
| if tax_item_rate == 0: | |
| raise ValueError("Tax item rate cannot be zero") | |
| recalculated_net_amount = round((line.tax_amount * 100) / tax_item_rate, 2) |
ruuushhh
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apps/netsuite/connector.py (3)
1166-1235: Consider extracting module-specific logic into separate methods.The method handles different export modules (BILL, JOURNAL_ENTRY, EXPENSE_REPORT) with module-specific logic scattered throughout. This makes the code harder to maintain and test.
Consider refactoring into separate methods for each module:
+ def _handle_journal_entry_tax(self, line, tax_amount): + line['grossAmt'] = None + line['debit'] = round(tax_amount, 2) + line.pop('amount', None) + return line def handle_taxed_line_items(self, base_line, line, workspace_id, export_module): # ... existing code ... if export_module == 'JOURNAL_ENTRY': - taxable_line['grossAmt'] = None - taxable_line['debit'] = recalculated_net_amount - taxable_line.pop('amount', None) + taxable_line = self._handle_journal_entry_tax(taxable_line, recalculated_net_amount)
Line range hint
1237-1283: Reduce code duplication in URL generation.The method duplicates the expense URL generation logic. Also, consider adding validation for required parameters.
Consider refactoring:
+ def _generate_expense_url(self, expense_id, org_id): + if not expense_id or not org_id: + raise ValueError("expense_id and org_id are required") + return '{}/app/admin/#/enterprise/view_expense/{}?org_id={}'.format( + settings.FYLE_EXPENSE_URL, + expense_id, + org_id + ) def prepare_custom_segments(self, line_netsuite_custom_segments, attachment_links, expense, org_id, is_credit=False): # ... existing code ... if not is_credit: + expense_url = self._generate_expense_url(expense.expense_id, org_id) netsuite_custom_segments.append({ 'scriptId': 'custcolfyle_expense_url', 'type': 'String', - 'value': '{}/app/admin/#/enterprise/view_expense/{}?org_id={}'.format( - settings.FYLE_EXPENSE_URL, - expense.expense_id, - org_id - ) + 'value': expense_url })
Line range hint
1285-1407: Consider adding validation for tax override scenarios.The method handles tax overrides but could benefit from additional validation and error handling for edge cases.
Add validation:
def construct_bill_lineitems(self, bill_lineitems: List[BillLineitem], attachment_links: Dict, cluster_domain: str, org_id: str, override_tax_details: bool) -> List[Dict]: + if override_tax_details and not hasattr(bill_lineitems[0], 'tax_item_id'): + raise ValueError("Tax item ID is required when override_tax_details is True") # ... rest of the method
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/netsuite/connector.py(12 hunks)
🔇 Additional comments (1)
apps/netsuite/connector.py (1)
1188-1188:
Add protection against division by zero.
The calculation (line.tax_amount * 100) / tax_item_rate could result in a division by zero error if tax_item_rate is zero.
Add a check before the division:
+ if tax_item_rate == 0:
+ raise ValueError("Tax item rate cannot be zero")
recalculated_net_amount = round((line.tax_amount * 100) / tax_item_rate, 2)Likely invalid or redundant comment.
ruuushhh
had a problem deploying
to
CI Environment
GitHub Actions
Failure
ruuushhh
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
|
* feat: netsuite tax override tests * increase cov --------- Co-authored-by: GitHub Actions <[email protected]>
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #662 +/- ##
==========================================
- Coverage 88.44% 88.40% -0.05%
==========================================
Files 59 59
Lines 5444 5501 +57
==========================================
+ Hits 4815 4863 +48
- Misses 629 638 +9
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (14)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (1)
11656-11659: Consider documenting the default value choiceThe default value of
falseforis_tax_balancing_enabledimplies this is an opt-in feature. This looks correct, but consider documenting this decision in the code comments or documentation for future reference.tests/test_netsuite/conftest.py (1)
306-322: Consider enhancing tax destination attributes test coverage.While the fixture correctly creates tax items for each workspace, consider these improvements:
- Using sequential IDs (98765+i) might cause conflicts with other test data
- A fixed tax_rate of 5.0% may not cover all test scenarios
- Using the same destination_id ('103578') across workspaces may not reflect real-world scenarios
Consider this enhanced implementation:
@pytest.fixture def add_tax_destination_attributes(db): - for i in(1, 2, 49): + test_data = [ + {'workspace_id': 1, 'tax_rate': 5.0, 'destination_id': '103578'}, + {'workspace_id': 2, 'tax_rate': 10.0, 'destination_id': '103579'}, + {'workspace_id': 49, 'tax_rate': 7.5, 'destination_id': '103580'} + ] + + for data in test_data: DestinationAttribute.objects.create( - id = 98765+i, attribute_type='TAX_ITEM', value='Rushikesh', - destination_id = '103578', + destination_id=data['destination_id'], active = True, detail = { - 'tax_rate': 5.0 + 'tax_rate': data['tax_rate'] }, - workspace_id = i, + workspace_id=data['workspace_id'], created_at = datetime.now(), updated_at = datetime.now(), )This provides:
- Different tax rates to test various scenarios
- Unique destination IDs per workspace
- Removed explicit ID setting to prevent conflicts
apps/netsuite/connector.py (1)
Line range hint
1236-1282: Apply DRY principle to URL construction.The expense URL construction is duplicated for both custom fields. Consider extracting it to a variable.
if not is_credit: + expense_url = '{}/app/admin/#/enterprise/view_expense/{}?org_id={}'.format( + settings.FYLE_EXPENSE_URL, + expense.expense_id, + org_id + ) netsuite_custom_segments.append( { 'scriptId': 'custcolfyle_expense_url', 'type': 'String', - 'value': '{}/app/admin/#/enterprise/view_expense/{}?org_id={}'.format( - settings.FYLE_EXPENSE_URL, - expense.expense_id, - org_id - ) + 'value': expense_url } ) netsuite_custom_segments.append( { 'scriptId': 'custcolfyle_expense_url_2', 'type': 'String', - 'value': '{}/app/admin/#/enterprise/view_expense/{}?org_id={}'.format( - settings.FYLE_EXPENSE_URL, - expense.expense_id, - org_id - ) + 'value': expense_url } )tests/test_netsuite/test_connector.py (5)
22-22: Avoid hardcodingworkspace_idin test functionsThroughout the test functions,
general_mappingand other objects are retrieved using hardcodedworkspace_idvalues (e.g.,workspace_id=1). Consider parameterizing theworkspace_idor using a fixture variable to enhance flexibility and maintainability of the tests.Also applies to: 33-35, 82-82, 96-96, 108-108, 115-118, 235-238, 302-305, 853-853, 871-871, 889-889, 1024-1025
26-26: Accessing private methods using name manglingThe test code is accessing private methods of
NetSuiteConnectorusing name mangling (e.g.,netsuite_connection._NetSuiteConnector__construct_expense_report). While this approach works, it's better to test through the public interface or refactor these methods to be protected (single underscore) if they need to be accessed for testing purposes.Also applies to: 43-43, 54-54, 70-70, 85-85, 99-99, 111-111, 127-127, 137-137, 149-149, 165-165, 174-174, 246-246, 258-258, 274-274, 313-313, 323-323, 336-336
47-47: Replace magic numbers with descriptive constantsHardcoded values like
'103578'fortaxCode['internalId']are used in multiple assertions. Defining these values as descriptive constants (e.g.,TEST_TAX_CODE_INTERNAL_ID) would improve readability and maintainability.Also applies to: 59-59, 73-73, 130-130, 141-141, 152-152, 251-251, 263-263, 278-278, 317-317, 327-327, 339-339
32-75: Parameterize test cases to reduce duplicationThe test functions related to tax balancing (
test_construct_expense_report_with_tax_balancing,test_construct_bill_item_for_tax_balancing,test_construct_journal_entry_with_tax_balancing,test_contruct_credit_card_charge_with_tax_balancing) share similar structures and code. Consider using parameterized tests or fixtures to eliminate duplication and enhance maintainability.Also applies to: 115-156, 234-284, 301-341
899-899: Unused variableconfigurationIn the
test_post_journal_entry_exceptionfunction, the variableconfigurationis assigned but not used afterward. If it's unnecessary, consider removing it to keep the code clean.apps/netsuite/tasks.py (3)
329-332: Inconsistent Variable Naming: 'general_mapping' vs 'general_mappings'The variable
general_mappingsis used here, while in other parts of the code,general_mappingis used. For consistency and to avoid confusion, it's recommended to use a consistent variable name throughout the codebase.Consider standardizing the variable name to
general_mapping:- credit_line = construct_lines(export_line_items, general_mappings, credit='Credit', org_id=workspace.fyle_org_id) + credit_line = construct_lines(export_line_items, general_mapping, credit='Credit', org_id=workspace.fyle_org_id) - debit_line = construct_lines( - export_line_items, general_mappings, debit='Debit', attachment_links=expense_id_receipt_url_map, - cluster_domain=cluster_domain, org_id=workspace.fyle_org_id - ) + debit_line = construct_lines( + export_line_items, general_mapping, debit='Debit', attachment_links=expense_id_receipt_url_map, + cluster_domain=cluster_domain, org_id=workspace.fyle_org_id + )
615-615: Optimize Retrieval ofgeneral_mappingThe
general_mappingobject is retrieved multiple times in different functions. To improve performance, consider retrieving it once and passing it as an argument where necessary.Apply this change to reduce redundant database queries:
# Retrieve `general_mapping` at the beginning + general_mapping = GeneralMapping.objects.get(workspace_id=expense_group.workspace_id) def create_expense_report(expense_group, task_log_id, last_export): - general_mapping = GeneralMapping.objects.get(workspace_id=expense_group.workspace_id)
680-680: Consistent Use ofgeneral_mappingAs previously noted, ensure consistent retrieval and usage of the
general_mappingobject throughout the code to maintain clarity and improve maintainability.Consider passing
general_mappingas an argument to functions that require it, rather than retrieving it multiple times.tests/test_netsuite/test_tasks.py (3)
178-178: Unused parameteradd_tax_destination_attributesintest_post_bill_successThe parameter
add_tax_destination_attributesappears to be unused within thetest_post_bill_successfunction. Removing it can simplify the test and avoid confusion.
210-212: Variable naming: Useexpenseinstead ofexpensesWithin the loop,
expensesrepresents a single expense in each iteration. Renaming it toexpenseimproves clarity and adheres to standard naming conventions.Apply this change:
-for expenses in expense_group.expenses.all(): - expenses.workspace_id = 2 - expenses.save() +for expense in expense_group.expenses.all(): + expense.workspace_id = 2 + expense.save()
210-212: Avoid hard-codedworkspace_idvalues in testsAssigning
expenses.workspace_id = 2directly may lead to fragile tests if the workspace ID changes. Consider using a variable or fixture to obtain the workspace ID dynamically.Example:
- expenses.workspace_id = 2 + expenses.workspace_id = workspace_idEnsure
workspace_idis defined or passed appropriately in your test setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
apps/netsuite/connector.py(28 hunks)apps/netsuite/tasks.py(8 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(4 hunks)tests/test_netsuite/conftest.py(5 hunks)tests/test_netsuite/fixtures.py(3 hunks)tests/test_netsuite/test_connector.py(11 hunks)tests/test_netsuite/test_tasks.py(2 hunks)
🔇 Additional comments (19)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (3)
7990-7990: LGTM!
The migration record is properly sequenced and timestamped.
11902-11902: LGTM!
The sequence value is correctly updated to match the new migration number.
1442-1443: Verify migration handling for NOT NULL constraint
The new is_tax_balancing_enabled column is marked as NOT NULL without a default value. While this is reflected correctly in the test fixtures, ensure that the corresponding migration handles existing rows appropriately.
✅ Verification successful
Migration safely handles NOT NULL constraint with default value
The migration file shows that is_tax_balancing_enabled column is added with default=False, which safely handles existing rows in the database. This aligns with the test fixture changes and ensures data consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check migration file to verify handling of existing rows
# Look for the migration file that adds this column
fd --type f "0015_generalmapping_is_tax_balancing_enabled.py" | xargs cat
Length of output: 526
tests/test_netsuite/conftest.py (1)
79-81: LGTM! Verify workspace ID consistency across fixtures.
The workspace ID updates are correctly implemented across fixtures. Note that most fixtures use workspace_id=1 except create_credit_card_charge which uses workspace_id=49.
Let's verify the workspace ID usage pattern:
Also applies to: 93-95, 250-252, 264-266
✅ Verification successful
Based on the search results, I can now verify the workspace ID usage pattern across the test fixtures. The results show:
-
Most test fixtures use workspace_id=1 consistently, including:
- Configuration objects
- General mappings
- Task logs
- Expense groups
- Various destination attributes
-
workspace_id=49 is used specifically for:
- Credit card charge related tests
- Custom segments/lists tests
- Currency tests
- Department tests
- Location tests
-
workspace_id=2 is used occasionally for:
- Some task logs
- Specific mapping tests
- Payment trigger tests
Workspace IDs are consistently used across test fixtures for their specific test scenarios
The different workspace IDs (1, 2, and 49) are intentionally used to isolate different test scenarios and maintain test data separation. This is a good testing practice to prevent test interference.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify workspace ID usage consistency across test files
# Check for workspace ID references in test files
rg -A 2 "workspace_id\s*=\s*(?:1|2|49)" tests/test_netsuite/
Length of output: 72680
apps/netsuite/connector.py (3)
Line range hint 1284-1406: LGTM! Well-structured tax handling implementation.
The changes properly integrate tax handling with clear separation between expense and item lists. The code:
- Correctly checks for presence of tax information
- Handles tax overrides appropriately
- Uses the new handle_taxed_line_items method effectively
Line range hint 1584-1622: LGTM! Consistent implementation across different transaction types.
The tax handling changes are consistently implemented across credit card charges, expense reports, and journal entries. The code maintains a uniform approach to:
- Tax validation
- Line item generation
- Integration with the new handle_taxed_line_items method
Also applies to: 1757-1846, 1988-2080
1166-1234: 🛠️ Refactor suggestion
Add safeguards against division by zero and improve documentation.
The tax calculation logic needs additional safeguards and documentation:
- The division by tax_item_rate on line 1187 could cause a division by zero error
- The complex tax calculation logic would benefit from detailed documentation explaining the business rules and edge cases
Add a check for zero tax rate:
tax_item_rate = tax_item.detail['tax_rate']
+ if tax_item_rate == 0:
+ raise ValueError("Tax item rate cannot be zero")
recalculated_net_amount = round((line.tax_amount * 100) / tax_item_rate, 2)Add detailed documentation:
def handle_taxed_line_items(self, base_line, line, workspace_id, export_module, general_mapping: GeneralMapping):
"""
Handle line items where tax is applied or modified by the user.
+
+ Business Rules:
+ 1. When tax balancing is enabled and actual tax differs from expected:
+ - Split into taxable and untaxed line items
+ - Recalculate net amount based on actual tax
+ 2. When tax matches expected or balancing is disabled:
+ - Create single line item with tax
+
:param base_line: The base line item template that will be modified.
:param line: The original line with tax and amount information.
- :param is_credit_card_charge: Boolean flag to differentiate between credit card charges and other transactions.
+ :param workspace_id: The workspace ID for retrieving tax items
+ :param export_module: The type of export (BILL, EXPENSE_REPORT, JOURNAL_ENTRY)
+ :param general_mapping: General mapping configuration containing tax settings
:return: List of lines (taxed and/or untaxed).
"""Likely invalid or redundant comment.
tests/test_netsuite/fixtures.py (3)
5-31: LGTM! Well-structured tax details test data
The new tax_list_detail structure provides comprehensive test coverage for tax scenarios with all required fields.
1205-1208: LGTM! Proper custom field type additions
The addition of the 'type' field to customFieldList entries aligns with NetSuite API requirements.
994-999: LGTM! Consistent taxCode structure updates
The taxCode structure updates are consistent across both occurrences and match NetSuite API requirements.
Also applies to: 1215-1217
tests/test_netsuite/test_connector.py (3)
1028-1031: Ensure mock objects are fully configured
When setting up the mock Mapping object, verify that all required attributes and methods are correctly mocked. For instance, confirm that mock_mapping.destination.detail.get.return_value and mock_mapping.destination.detail.all.return_value are providing the expected values for the test.
242-245: Validate tax calculations with given inputs
In the tests involving tax amounts and items, ensure that the amount, tax_amount, and tax_item_id values are consistent and lead to accurate tax calculations. This will help verify that tax balancing is functioning as intended.
Also applies to: 271-273, 308-311, 331-334
1022-1070: Confirm correctness of construct_tax_details_list method
The test_constructs_tax_details_list_for_multiple_items function sets up mocks to test construct_tax_details_list. Ensure that the method handles multiple items correctly and the test accurately reflects realistic scenarios.
apps/netsuite/tasks.py (6)
570-570: Confirm post_credit_card_charge Method Parameters
Ensure that the post_credit_card_charge method accepts general_mappings as a parameter and handles it appropriately.
Run the following script to check the method definition:
#!/bin/bash
# Description: Verify `post_credit_card_charge` method parameters
rg -A 2 "def post_credit_card_charge\(self, credit_card_charge_object, credit_card_charge_lineitems_object, general_mappings, attachment_links, refund\):"637-637: Verify post_expense_report Method Accepts general_mapping
Check that the post_expense_report method includes general_mapping as a parameter and uses it correctly.
Run the following script to confirm:
#!/bin/bash
# Description: Verify `post_expense_report` method parameters
rg -A 2 "def post_expense_report\(self, expense_report_object, expense_report_lineitems_objects, general_mapping\):"344-344: Confirm Function Signature Matches in construct_lines Call
Verify that the function construct_lines is defined to accept general_mappings as a parameter in this context.
Run the following script to confirm the function definition:
#!/bin/bash
# Description: Check definition of `construct_lines` function for matching parameters
# Locate the definition of `construct_lines` related to this call
rg -A 2 "def construct_credit_card_charge_lineitems\("
rg -A 2 "def construct_expense_report_lineitems\("703-703: Ensure post_journal_entry Method Receives Correct Parameters
Confirm that the post_journal_entry method in NetSuiteConnector accepts configuration and general_mapping as parameters and that they are utilized properly within the method.
Run the following script to verify the method signature:
#!/bin/bash
# Description: Verify `post_journal_entry` method parameters
rg -A 2 "def post_journal_entry\(self, journal_entry_object, journal_entry_lineitems_objects, configuration, general_mapping\):"490-490: Ensure post_bill Method Accepts general_mappings Parameter
Confirm that the post_bill method in NetSuiteConnector accepts general_mappings as a parameter and utilizes it correctly within the method.
Run the following script to verify the method signature:
#!/bin/bash
# Description: Verify `post_bill` method accepts `general_mappings`
rg -A 2 "def post_bill\(self, bill_object, bill_lineitems_objects, general_mappings\):"340-340: Verify Parameters in Function Call to construct_lines
Ensure that the parameters passed to construct_lines match its definition. Specifically, confirm that general_mappings.override_tax_details is a valid attribute and that general_mappings is an accepted parameter.
Run the following script to check if construct_lines accepts general_mappings and if override_tax_details exists:
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
ruuushhh
added
deploy
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
* fix: Get Tax Mapping * fix tests --------- Co-authored-by: GitHub Actions <[email protected]>
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
ruuushhh
added
deploy
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/netsuite/models.py (1)
118-129: LGTM! Consider adding type hints and docstring.The changes improve the tax code determination logic by first checking for tax settings existence. This is a good optimization that prevents unnecessary database queries.
Consider these improvements:
- Add type hints and docstring:
-def get_tax_item_id_or_none(expense_group: ExpenseGroup, general_mapping: GeneralMapping, lineitem: Expense = None): +def get_tax_item_id_or_none(expense_group: ExpenseGroup, general_mapping: GeneralMapping, lineitem: Expense = None) -> Optional[str]: + """ + Get tax item ID based on tax group mapping or default tax code. + + Args: + expense_group: The expense group containing the expense + general_mapping: General mapping settings + lineitem: Optional expense line item + + Returns: + str: Tax item ID if found, None otherwise + """
- Remove extra blank line after line 121:
).first() - if tax_setting:
* feat: Netsuite Tax Override * fix comments * Fix tests * feat: netsuite tax override tests (#667) * feat: netsuite tax override tests * increase cov --------- Co-authored-by: GitHub Actions <[email protected]> * fix: get tax mapping (#669) * fix: Get Tax Mapping * fix tests --------- Co-authored-by: GitHub Actions <[email protected]> --------- Co-authored-by: GitHub Actions <[email protected]>
Description
Netsuite Tax Override
Clickup
https://app.clickup.com/1864988/v/l/6-901604646819-1
Summary by CodeRabbit
New Features
Bug Fixes