feat: allow selection of memo structure in advance settings #413
Conversation
WalkthroughThe changes in this pull request introduce a new field, Changes
Poem
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 (
|
NileshPant1999
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
apps/workspaces/apis/advanced_settings/serializers.py (1)
43-43: Consider adding field-level validation for memo_structure.While the field is added correctly, consider adding validation to ensure the memo structure fields are valid and follow expected format.
Example validation:
memo_structure = serializers.ListField( child=serializers.CharField(max_length=100), allow_empty=False, help_text='List of system fields for creating custom memo' )apps/workspaces/views.py (1)
161-161: Consider adding explicit permission checks for update operations.While the change to RetrieveUpdateAPIView is appropriate for supporting memo structure updates, consider adding explicit permission checks for update operations to ensure only authorized users can modify the settings.
Example:
def get_permissions(self): if self.request.method in ['PUT', 'PATCH']: return [IsAuthenticated(), IsWorkspaceAdmin()] # Create IsWorkspaceAdmin permission class return [IsAuthenticated()]apps/workspaces/models.py (1)
33-34: Add docstring to the functionThe function looks good, but adding a docstring would improve code documentation.
def get_default_memo_fields(): + """ + Returns a list of default system fields used for creating custom memos. + + Returns: + list: Default memo fields including employee email, category, merchant, etc. + """ return ['employee_email', 'category', 'merchant', 'spent_on', 'report_number', 'purpose']apps/xero/models.py (1)
147-173: Add type hints to improve code clarityThe function implementation looks good with configurable memo structure, but could benefit from type hints.
-def get_expense_purpose(workspace_id, lineitem, category, workspace_general_settings) -> str: +def get_expense_purpose( + workspace_id: int, + lineitem: Expense, + category: str, + workspace_general_settings: WorkspaceGeneralSettings +) -> str:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
apps/workspaces/apis/advanced_settings/serializers.py(2 hunks)apps/workspaces/migrations/0040_workspacegeneralsettings_memo_structure.py(1 hunks)apps/workspaces/models.py(2 hunks)apps/workspaces/views.py(1 hunks)apps/xero/models.py(6 hunks)apps/xero/tasks.py(1 hunks)
🔇 Additional comments (7)
apps/workspaces/migrations/0040_workspacegeneralsettings_memo_structure.py (1)
15-19: LGTM! Verify the default function implementation.
The migration is well-structured and uses appropriate field types. The ArrayField with CharField is a good choice for storing memo structure fields.
Let's verify the implementation of the default function:
✅ Verification successful
Default function implementation verified and looks good
The get_default_memo_fields() function returns a sensible list of default fields that are commonly used in memo structures: employee_email, category, merchant, spent_on, report_number, and purpose. The implementation is straightforward and appropriate for the migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of get_default_memo_fields function
ast-grep --pattern 'def get_default_memo_fields():
$$$
'
Length of output: 243
apps/workspaces/apis/advanced_settings/serializers.py (1)
98-100: LGTM! Update logic handles the new field correctly.
The update method properly integrates the memo_structure field using the safe get() method.
apps/workspaces/models.py (1)
185-188: LGTM! Field definition is well structured.
The memo_structure field is properly defined with appropriate constraints and default values.
apps/xero/models.py (3)
12-12: LGTM! Import statement is correctly placed.
265-265: LGTM! Method signature update is consistent.
The changes properly integrate the new workspace_general_settings parameter.
Also applies to: 292-292
452-452: LGTM! Method signature update is consistent.
The changes properly integrate the new workspace_general_settings parameter.
Also applies to: 484-484
apps/xero/tasks.py (1)
459-459: LGTM! Parameter passing is correct.
The change properly integrates the workspace general settings parameter.
NileshPant1999
had a problem deploying
to
CI Environment
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/xero/tasks.py(2 hunks)tests/test_xero/test_models.py(3 hunks)
🔇 Additional comments (4)
tests/test_xero/test_models.py (2)
29-32: LGTM: Proper integration of workspace_general_settings
The test correctly retrieves workspace settings and passes it to create_bill_lineitems.
46-50: LGTM: Consistent implementation of workspace_general_settings
The test maintains consistency with the bill creation pattern, properly integrating workspace settings.
apps/xero/tasks.py (2)
284-284: LGTM: Proper usage of general_settings
The general_settings parameter is correctly passed to create_bill_lineitems, maintaining consistency with the updated method signature.
459-459: LGTM: Consistent implementation with create_bill
The general_settings parameter is correctly passed to create_bank_transaction_lineitems, maintaining consistency across both transaction types.
tests/test_xero/test_models.py
Outdated
| workspace_general_settings = WorkspaceGeneralSettings.objects.filter(workspace_id=workspace_id) | ||
|
|
There was a problem hiding this comment.
Add .first() to workspace settings retrieval
The current implementation returns a QuerySet. Use .first() to ensure a single object is passed:
- workspace_general_settings = WorkspaceGeneralSettings.objects.filter(workspace_id=workspace_id)
+ workspace_general_settings = WorkspaceGeneralSettings.objects.filter(workspace_id=workspace_id).first()📝 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.
| workspace_general_settings = WorkspaceGeneralSettings.objects.filter(workspace_id=workspace_id) | |
| 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.
💡 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.pyneeds to be updated to use.first()on theworkspace_general_settingsQuerySet.
🔗 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
|
NileshPant1999
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)
tests/test_workspaces/test_apis/test_clone_settings/fixtures.py (1)
72-72: Consider adding test cases for edge scenarios.The current fixtures only cover the happy path with a valid memo structure. Consider adding test cases for:
- Empty memo structure array
- Invalid field names in the array
- Missing memo structure field
Here's a suggested addition to the fixtures:
"clone_settings_edge_cases": { "advanced_settings": { "workspace_general_settings": { # Empty array "memo_structure": [], # Invalid field names "memo_structure": ["invalid_field", "unknown_field"], # Missing field (remove memo_structure key entirely) } } }Also applies to: 126-126
apps/xero/models.py (1)
147-172: Consider security and formatting improvementsWhile the new implementation is more flexible, consider these improvements:
- URL Construction: The expense link URL construction could benefit from using
urllib.parse.urljoin()for safer URL handling.- String Formatting: Consider using f-strings for better readability and performance.
Here's a suggested improvement:
- memo = '{0} - '.format(memo) + memo = f"{memo} - " - 'expense_link': '{0}/app/admin/#/enterprise/view_expense/{1}?org_id={2}'.format(fyle_url, lineitem.expense_id, org_id) + 'expense_link': f"{fyle_url}/app/admin/#/enterprise/view_expense/{lineitem.expense_id}?org_id={org_id}"tests/test_workspaces/fixtures.py (1)
15-15: Consider adding test cases for all possible memo fieldsThe expense object in the fixture contains various fields that could be used in memo structures (e.g., expense_number, claim_number, spent_at). Consider adding test cases with different memo structure combinations to ensure all possible fields work correctly.
Example additional test combinations:
# Additional test cases to consider { "memo_structure": ["claim_number", "expense_number"], "memo_structure": ["spent_at", "vendor", "category"], "memo_structure": ["purpose", "project", "cost_center"] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
apps/xero/models.py(6 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(5 hunks)tests/test_workspaces/fixtures.py(1 hunks)tests/test_workspaces/test_apis/test_advanced_settings/fixtures.py(2 hunks)tests/test_workspaces/test_apis/test_clone_settings/fixtures.py(2 hunks)tests/test_xero/test_models.py(2 hunks)
🔥 Files not summarized due to errors (1)
- tests/sql_fixtures/reset_db_fixtures/reset_db.sql: Error: Server error: no LLM provider could handle the message
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_xero/test_models.py
🔇 Additional comments (10)
tests/test_workspaces/test_apis/test_clone_settings/fixtures.py (1)
72-72: LGTM! The memo structure implementation is consistent.
The memo_structure field is consistently implemented in both the response and clone settings fixtures with the same structure and field names.
Also applies to: 126-126
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (4)
5071-5072: Review the default memo structure elements
The default memo structure includes: {employee_email,category,merchant,spent_on,report_number,purpose}. This appears comprehensive but please verify:
- If these are all the required fields for memo generation
- If the order of these fields matches the expected memo format
Consider documenting these fields and their purpose in a configuration file or documentation for better maintainability.
2647-2647: Migration sequence looks correct
The migration 0040_workspacegeneralsettings_memo_structure is properly sequenced after the previous migrations and has a recent timestamp.
5185-5185: Verify migration sequence value
The sequence value of 160 matches the latest migration ID. This ensures proper tracking of migrations.
1538-1539: Verify the NOT NULL constraint requirement
The memo_structure column is defined with a NOT NULL constraint. Ensure this aligns with the business requirements, as it means every workspace must have a memo structure defined.
✅ Verification successful
NOT NULL constraint for memo_structure is appropriate
The NOT NULL constraint for the memo_structure column is correctly implemented and justified based on the following evidence:
- The migration (0040_workspacegeneralsettings_memo_structure) has been applied successfully
- The sample data shows that existing records are populated with a default structure:
{employee_email,category,merchant,spent_on,report_number,purpose} - The workspace_general_settings table has a one-to-one relationship with workspaces (unique constraint on workspace_id)
- The memo structure is essential for formatting expense memos consistently within a workspace
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing rows that might be affected by NOT NULL constraint
rg -l "WorkspaceGeneralSettings" | xargs rg "memo_structure.*=.*None|memo_structure.*=.*\[\]"
Length of output: 93
Script:
#!/bin/bash
# Let's check for any references to memo_structure in the codebase
rg -A 3 "memo_structure" --type sql
# Also check for any default values or constraints in the SQL file
rg -A 3 "workspace_general_settings" --type sql
Length of output: 8671
tests/test_workspaces/test_apis/test_advanced_settings/fixtures.py (1)
8-8: LGTM! Consistent memo structure implementation
The memo structure is consistently implemented in both the request and response fixtures with a logical ordering of fields.
Also applies to: 28-28
apps/xero/models.py (3)
12-12: LGTM! Clean import addition
The WorkspaceGeneralSettings import is appropriately placed with other workspace-related imports.
264-264: LGTM! Consistent parameter updates
The workspace_general_settings parameter has been consistently added to both methods with proper type hints maintained.
Also applies to: 451-451
264-264: Verify all callers are updated
Since the method signatures have changed, we should verify that all calling code has been updated accordingly.
Also applies to: 451-451
✅ Verification successful
All callers have been properly updated
All calls to both create_bill_lineitems and create_bank_transaction_lineitems are passing the required expense_group and workspace_general_settings parameters correctly:
tests/test_xero/test_models.pytests/test_xero/conftest.pyapps/xero/tasks.py
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to these methods to ensure they've been updated with the new parameter
echo "Searching for calls to create_bill_lineitems..."
rg "create_bill_lineitems\(" --type python -A 2
echo "Searching for calls to create_bank_transaction_lineitems..."
rg "create_bank_transaction_lineitems\(" --type python -A 2
Length of output: 407
Script:
#!/bin/bash
# Search for direct calls to these methods to ensure they've been updated with the new parameter
# Using ripgrep without file type filter since python extension wasn't recognized
echo "Searching for calls to create_bill_lineitems..."
rg "create_bill_lineitems\(" -A 2
echo "Searching for calls to create_bank_transaction_lineitems..."
rg "create_bank_transaction_lineitems\(" -A 2
Length of output: 2093
tests/test_workspaces/fixtures.py (1)
15-15: Verify memo structure test values align with implementation
The test fixture includes default memo structure values of ["report_number", "spent_on"]. Let's verify these are valid options and consider adding test cases for other possible combinations.
✅ Verification successful
Test values for memo structure are valid and consistent with implementation
The test values ["report_number", "spent_on"] are valid memo structure fields as they are part of the default memo fields defined in get_default_memo_fields(): ['employee_email', 'category', 'merchant', 'spent_on', 'report_number', 'purpose'].
The test fixtures demonstrate consistent usage across different test scenarios:
- Basic fixture with
["report_number", "spent_on"] - Advanced settings tests with
["report_number", "expense_link", "spent_on"] - Clone settings tests with same valid fields
The implementation in xero/models.py properly handles these fields by iteratively building the memo string from the provided structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for memo structure related code to verify valid options
# Check for memo structure field definition and validation
rg -A 5 "memo_structure" --type python
# Look for any constants or enums defining valid memo fields
ast-grep --pattern 'memo_structure = $_'
Length of output: 78588
Script:
#!/bin/bash
# Search for memo structure related code and constants
rg -g "*.py" -A 5 "memo_structure"
# Look for any memo field related constants or enums
rg -g "*.py" -A 5 "MEMO_FIELDS|memo_fields"
# Look for default memo field function
rg -g "*.py" -A 5 "get_default_memo_fields"
Length of output: 8815
|
NileshPant1999
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
NileshPant1999
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (1)
949-950: Note: Additional columns added to expenses tableWhile reviewing the memo structure changes, noticed that two new columns were added to the expenses table:
- bank_transaction_id (varchar 255)
- masked_corporate_card_number (varchar 255)
These appear to be related to corporate card transaction tracking rather than the memo structure feature.
tests/test_fyle/fixtures.py (1)
182-182: Consider adding more variety to test data.Currently, all test cases use the same masked card number
'**** 4567'. Consider adding variations to better represent real-world scenarios and edge cases:
- Different last 4 digits
- Different masking patterns (e.g., 'XXXX XXXX XXXX 4567')
- Empty or null values
- Invalid formats
This would help ensure the system handles various card number formats correctly.
Example variations:
masked_card_variations = [ '**** 4567', '**** 1234', 'XXXX XXXX XXXX 9876', None, '' ]Also applies to: 239-239, 259-259, 329-329, 375-375, 421-421, 467-467, 492-492, 728-728, 798-798
apps/xero/models.py (4)
147-147: Add type hints and docstring for better code maintainability.The function signature should include type hints and a docstring explaining its purpose, parameters, and return value.
-def get_expense_purpose(workspace_id, lineitem, category, workspace_general_settings) -> str: +def get_expense_purpose( + workspace_id: int, + lineitem: Expense, + category: str, + workspace_general_settings: WorkspaceGeneralSettings +) -> str: + """ + Generate a formatted memo string based on the configured memo structure. + + Args: + workspace_id: The ID of the workspace + lineitem: The expense line item + category: The expense category + workspace_general_settings: Workspace settings containing memo structure + + Returns: + A formatted memo string + """
164-164: Consider making the expense URL format configurable.The hardcoded URL format might be brittle if the URL structure changes.
Consider moving the URL format to settings or configuration:
- 'expense_link': '{0}/app/admin/#/enterprise/view_expense/{1}?org_id={2}'.format(fyle_url, lineitem.expense_id, org_id) + 'expense_link': settings.EXPENSE_URL_FORMAT.format( + base_url=fyle_url, + expense_id=lineitem.expense_id, + org_id=org_id + )
Line range hint
266-315: Consider breaking down the function for better maintainability.The function is handling multiple responsibilities and could benefit from being split into smaller, focused functions.
Consider extracting the following into separate functions:
- Category formatting logic
- Account mapping lookup
- Line item creation logic
Example refactor:
+def _format_category(lineitem: Expense) -> str: + return ( + lineitem.category + if (lineitem.category == lineitem.sub_category or lineitem.sub_category is None) + else f"{lineitem.category} / {lineitem.sub_category}" + ) + +def _get_account_mapping(category: str, workspace_id: int) -> Mapping: + return Mapping.objects.filter( + source_type=FyleAttributeEnum.CATEGORY, + source__value=category, + destination_type="ACCOUNT", + workspace_id=workspace_id, + ).first()
Line range hint
453-515: Reduce code duplication with bill line items creation.There's significant code duplication between
create_bank_transaction_lineitemsandcreate_bill_lineitems. Consider extracting common logic into a shared utility function.+def _create_common_line_item_data( + expense_group: ExpenseGroup, + lineitem: Expense, + workspace_general_settings: WorkspaceGeneralSettings +) -> dict: + """ + Create common line item data for both bills and bank transactions. + """ + category = _format_category(lineitem) + account = _get_account_mapping(category, expense_group.workspace_id) + + return { + 'item_code': get_item_code_or_none(expense_group, lineitem), + 'customer_id': get_customer_id_or_none(expense_group, lineitem), + 'description': get_expense_purpose( + expense_group.workspace_id, + lineitem, + category, + workspace_general_settings + ), + 'tracking_categories': get_tracking_category(expense_group, lineitem), + 'amount': lineitem.amount, + 'tax_code': get_tax_code_id_or_none(expense_group, lineitem), + 'tax_amount': lineitem.tax_amount, + }Then use this in both line item creation methods:
common_data = _create_common_line_item_data(expense_group, lineitem, workspace_general_settings) line_item_object, _ = BankTransactionLineItem.objects.update_or_create( bank_transaction=bank_transaction, expense_id=lineitem.id, defaults=common_data )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
apps/fyle/migrations/0023_expense_masked_corporate_card_number.py(1 hunks)apps/fyle/models.py(2 hunks)apps/xero/models.py(6 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(7 hunks)tests/test_fyle/fixtures.py(10 hunks)tests/test_workspaces/fixtures.py(2 hunks)
🔥 Files not summarized due to errors (1)
- tests/sql_fixtures/reset_db_fixtures/reset_db.sql: Error: Server error: no LLM provider could handle the message
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_workspaces/fixtures.py
🔇 Additional comments (8)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (3)
1539-1540: LGTM: Memo structure column definition is appropriate
The memo_structure column is correctly defined as a NOT NULL array of varchar(100), which aligns with the feature requirement of allowing selection of memo structure in advanced settings.
2648-2649: LGTM: Migration sequence is properly ordered
The migrations are correctly ordered:
- workspaces.0040_workspacegeneralsettings_memo_structure
- fyle.0023_expense_masked_corporate_card_number
The sequence number (161) in django_migrations_id_seq matches the latest migration.
5073-5074: Verify the default memo structure configuration
The default memo structure configuration includes: employee_email, category, merchant, spent_on, report_number, and purpose. Please ensure this default configuration aligns with the product requirements.
✅ Verification successful
Let me check the actual memo structure configuration in the codebase to verify the default values.
Let me check the default memo fields function to verify the configuration.
Default memo structure configuration is correctly implemented
The default memo structure configuration in the SQL fixture matches exactly with the get_default_memo_fields() function implementation in apps/workspaces/models.py. The fields are:
- employee_email
- category
- merchant
- spent_on
- report_number
- purpose
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any configuration documentation or constants defining the allowed memo structure fields
rg -l "memo.*structure" | xargs rg -l "employee_email|category|merchant|spent_on|report_number|purpose"
Length of output: 420
Script:
#!/bin/bash
# Check the memo structure configuration in models and serializers
ast-grep --pattern 'memo_structure' apps/workspaces/models.py
rg -A 5 -B 5 'memo_structure' apps/workspaces/models.py
Length of output: 572
Script:
#!/bin/bash
# Find the get_default_memo_fields function implementation
ast-grep --pattern 'def get_default_memo_fields' apps/workspaces/models.py
rg -A 5 -B 5 'get_default_memo_fields' apps/workspaces/models.py
Length of output: 872
tests/test_fyle/fixtures.py (1)
182-182: LGTM! Consistent implementation of masked card numbers.
The addition of masked_corporate_card_number field follows security best practices by masking sensitive data in test fixtures.
Also applies to: 239-239, 259-259, 329-329, 375-375, 421-421, 467-467, 492-492, 728-728, 798-798
apps/fyle/migrations/0023_expense_masked_corporate_card_number.py (1)
1-18: LGTM! Migration looks good.
The migration is well-structured and correctly adds the masked_corporate_card_number field to the expense model with appropriate constraints.
apps/fyle/models.py (2)
154-154: LGTM! Field definition is appropriate.
The masked_corporate_card_number field is well-defined with appropriate constraints and clear help text.
208-208: 🛠️ Refactor suggestion
Consider adding validation for masked card number format.
While the implementation works, consider adding validation to ensure:
- The masked card number follows a consistent format (e.g., "**** 1234").
- Only the last 4 digits are exposed, with the rest properly masked.
- The data is handled according to PCI compliance requirements.
Consider adding a validator method:
def validate_masked_card_number(value):
if value and not re.match(r'^\*+\s?\d{4}$', value):
raise ValidationError('Invalid masked card number format')apps/xero/models.py (1)
Line range hint 147-485: Verify all callers are updated with the new parameter.
The functions get_expense_purpose, create_bill_lineitems, and create_bank_transaction_lineitems have been modified to accept a new workspace_general_settings parameter. Let's verify all callers have been updated.
✅ Verification successful
Let me check the content of the test files and tasks to verify if the new parameter is properly handled.
Based on the code analysis, I can now generate the final response:
All callers have been properly updated with the new parameter
All callers of the modified functions are correctly passing the workspace_general_settings parameter:
-
get_expense_purpose():- Called in
test_get_expense_purpose()with correct parameter - Called in
create_bill_lineitems()andcreate_bank_transaction_lineitems()with correct parameter
- Called in
-
create_bill_lineitems():- Called in tests with
workspace_general_settings - Called in
create_bill()task withgeneral_settings
- Called in tests with
-
create_bank_transaction_lineitems():- Called in tests with
workspace_general_settings - Called in
create_bank_transaction()task withgeneral_settings
- Called in tests with
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to the modified functions
echo "Searching for calls to get_expense_purpose:"
rg "get_expense_purpose\(" --type py
echo -e "\nSearching for calls to create_bill_lineitems:"
rg "create_bill_lineitems\(" --type py
echo -e "\nSearching for calls to create_bank_transaction_lineitems:"
rg "create_bank_transaction_lineitems\(" --type py
# Search for related test files
echo -e "\nSearching for related test files:"
fd "test.*models\.py$"
Length of output: 2093
Script:
#!/bin/bash
# Check test_models.py content
echo "Content of tests/test_xero/test_models.py:"
cat tests/test_xero/test_models.py
echo -e "\nContent of apps/xero/tasks.py:"
cat apps/xero/tasks.py
Length of output: 47728
| org_id = Workspace.objects.get(id=workspace_id).fyle_org_id | ||
| memo_structure = workspace_general_settings.memo_structure |
There was a problem hiding this comment.
🛠️ 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
apps/xero/models.py
Outdated
| ) | ||
| details = { | ||
| 'employee_email': lineitem.employee_email, | ||
| 'merchant': '{} - '.format(lineitem.vendor) if lineitem.vendor else '', |
NileshPant1999
force-pushed
the
xero-advance-memo
branch
from
cfd7f9e to
5659af3
Compare
NileshPant1999
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
NileshPant1999
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/xero/models.py(6 hunks)
🔇 Additional comments (5)
apps/xero/models.py (5)
Line range hint 264-291: LGTM! Function signature updated correctly.
The workspace_general_settings parameter is properly integrated and passed to the get_expense_purpose function.
Line range hint 451-483: LGTM! Function signature updated correctly.
The workspace_general_settings parameter is properly integrated and passed to the get_expense_purpose function.
165-171: 🛠️ Refactor suggestion
Optimize string concatenation using join operation.
The current implementation uses inefficient string concatenation in a loop. Using list comprehension with join would be more performant.
- 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)
+ memo_parts = [details[field] for field in memo_structure if field in details]
+ return ' - '.join(filter(None, memo_parts))Line range hint 147-483: Verify all callers are updated with the new parameter.
Let's ensure all callers of these functions have been updated to include the workspace_general_settings parameter.
✅ Verification successful
All callers have been properly updated with the new workspace_general_settings parameter
The verification shows that all callers of the modified functions have been updated correctly:
-
get_expense_purpose(): Called with workspace_general_settings in:- BillLineItem.create_bill_lineitems()
- BankTransactionLineItem.create_bank_transaction_lineitems()
- test_get_expense_purpose()
-
create_bill_lineitems(): Called with workspace_general_settings in:- xero/tasks.py (using general_settings)
- test_models.py
- conftest.py
-
create_bank_transaction_lineitems(): Called with workspace_general_settings in:- xero/tasks.py (using general_settings)
- test_models.py
- conftest.py
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for function calls that might need updates
echo "Searching for calls to get_expense_purpose..."
rg "get_expense_purpose\(" --type py -A 2
echo "Searching for calls to create_bill_lineitems..."
rg "create_bill_lineitems\(" --type py -A 2
echo "Searching for calls to create_bank_transaction_lineitems..."
rg "create_bank_transaction_lineitems\(" --type py -A 2
Length of output: 3294
147-151:
Add validation for memo_structure parameter.
The function should handle cases where memo_structure is None or empty to prevent potential runtime errors.
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
+ if not workspace_general_settings or not workspace_general_settings.memo_structure:
+ return ''
memo_structure = workspace_general_settings.memo_structureLikely invalid or redundant comment.
| 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) | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
|
https://app.clickup.com/t/