feat: add masked corporate card number and employee name #673
Conversation
WalkthroughThe changes in this pull request involve the addition of a new field, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
anishfyle
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/fyle/models.py(1 hunks)apps/netsuite/models.py(1 hunks)
🔇 Additional comments (3)
apps/fyle/models.py (1)
122-122: LGTM! Field addition looks good.
The new masked_corporate_card_number field is well-defined with appropriate characteristics:
- Nullable for backward compatibility
- Reasonable max length
- Clear help text
apps/netsuite/models.py (2)
270-271: LGTM! Verify memo structure configuration.
The addition of employee name and masked card number fields is implemented securely. The card number is properly masked and both fields are handled safely with appropriate null checks.
Please verify that the memo structure configuration includes these new fields if they are intended to be used in the expense purpose. Run this script to check the configuration:
Line range hint 269-278: Verify data availability across all expense types.
The get_expense_purpose method is used by multiple models (Bill, CreditCardCharge, ExpenseReport, JournalEntry). Ensure that employee_name and masked_corporate_card_number are consistently available across all expense types.
Run this script to verify the data availability:
✅ Verification successful
Let me gather more information about the expense fields to verify their availability.
Let me try a different approach to check the field mappings and assignments.
Let me check the Expense model and the field mappings in the actual creation logic.
Based on the extensive code analysis, I can now provide a final response:
The fields are consistently available across all expense types.
The code analysis shows that:
- All fields used in
get_expense_purpose()come from the baseExpensemodel (apps/fyle/models.py) which is referenced by all lineitem types through a foreign key relationship. - The fields are used consistently across all lineitem creation methods (Bill, CreditCardCharge, ExpenseReport, JournalEntry) through the expense group's expenses.
- The memo structure is configurable and handles missing values gracefully with empty string fallbacks for all fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new fields are consistently available across expense types
# Expected: All expense types should have access to these fields
# Search for expense creation/mapping code to verify field availability
rg -A 10 "def create_.*lineitem" apps/
Length of output: 3932
Script:
#!/bin/bash
# Check the expense model fields and their usage in lineitem creation
ast-grep --pattern 'class $_(Expense|BillLineitem|CreditCardChargeLineitem|ExpenseReportLineitem|JournalEntryLineitem)($$$)'
# Also check the actual field assignments in lineitem creation
rg -A 5 'lineitem_object = \w+Lineitem\.objects\.create' apps/
Length of output: 583
Script:
#!/bin/bash
# Search for field assignments in lineitem creation methods
rg -B 2 -A 10 "BillLineitem\.objects\.create|CreditCardChargeLineitem\.objects\.create|ExpenseReportLineitem\.objects\.create|JournalEntryLineitem\.objects\.create" apps/
# Also check the model definitions
rg -A 20 "class (BillLineitem|CreditCardChargeLineitem|ExpenseReportLineitem|JournalEntryLineitem)\(" apps/
Length of output: 2356
Script:
#!/bin/bash
# Check the Expense model definition
rg -A 20 "class Expense\(" apps/
# Check where these fields are being mapped from expense to lineitem
rg -B 2 -A 5 "employee_name|employee_email|masked_corporate_card_number|vendor|category|purpose|claim_number|spent_at" apps/netsuite/
Length of output: 98772
anishfyle
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 (2)
apps/fyle/migrations/0035_expense_masked_corporate_card_number.py(1 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/fyle/migrations/0035_expense_masked_corporate_card_number.py
🔇 Additional comments (3)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (3)
7996-7996: LGTM! Migration record properly added.
The migration record is correctly sequenced and follows naming conventions.
11908-11908: LGTM! Sequence value correctly updated.
The django_migrations_id_seq is properly set to 205, matching the latest migration ID.
1152-1153: LGTM! Column definition looks appropriate.
The masked_corporate_card_number column is correctly defined as a nullable VARCHAR(255), which is suitable for storing masked card numbers.
Let's verify the column definition matches the Django model:
✅ Verification successful
Column definition in SQL matches the Django model perfectly
The masked_corporate_card_number column is defined consistently:
- SQL:
VARCHAR(255)nullable - Django:
CharField(max_length=255, null=True)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify column definition matches Django model
# Expected: CharField(max_length=255) definition in the model
rg -A 2 "masked_corporate_card_number" "apps/fyle/models.py"
Length of output: 321
| COPY public.expenses (id, employee_email, category, sub_category, project, expense_id, expense_number, claim_number, amount, currency, foreign_amount, foreign_currency, settlement_id, reimbursable, state, vendor, cost_center, purpose, report_id, spent_at, approved_at, expense_created_at, expense_updated_at, created_at, updated_at, fund_source, custom_properties, verified_at, paid_on_netsuite, billable, org_id, tax_amount, tax_group_id, project_id, file_ids, corporate_card_id, is_skipped, report_title, employee_name, posted_at, accounting_export_summary, previous_export_state, workspace_id, paid_on_fyle, is_posted_at_null, masked_corporate_card_number) FROM stdin; | ||
| 1 [email protected] Accounts Payable Accounts Payable \N txjvDntD9ZXR E/2021/11/T/11 C/2021/11/R/5 50 USD \N \N set6GUp6tcEEp t PAYMENT_PROCESSING \N Treasury \N rpuN3bgphxbK 2021-11-15 00:00:00+00 2021-11-15 00:00:00+00 2021-11-15 10:27:53.649+00 2021-11-15 10:28:46.775+00 2021-11-15 10:29:07.597095+00 2021-11-15 10:29:07.597111+00 PERSONAL {"Team": "", "Class": "", "Klass": "", "Team 2": "", "Location": "", "Team Copy": "", "Tax Groups": "", "Departments": "", "User Dimension": "", "Location Entity": "", "Operating System": "", "System Operating": "", "User Dimension Copy": ""} \N f \N or79Cob97KSh \N \N \N \N \N f \N \N \N {} \N \N f f \N | ||
| 2 [email protected] Accounts Payable Accounts Payable \N txy6folbrG2j E/2021/11/T/12 C/2021/11/R/6 100 USD \N \N setNVTcPkZ6on f PAYMENT_PROCESSING Ashwin Vendor \N \N rpHLA9Dfp9hN 2021-11-15 00:00:00+00 2021-11-15 00:00:00+00 2021-11-15 13:11:22.304+00 2021-11-15 13:11:58.032+00 2021-11-15 13:12:12.250613+00 2021-11-15 13:12:12.250638+00 CCC {"Team": "", "Class": "", "Klass": "Klass", "Team 2": "", "Location": "", "Team Copy": "", "Tax Groups": "", "Departments": "", "User Dimension": "", "Location Entity": "", "Operating System": "", "System Operating": "", "User Dimension Copy": ""} \N f \N or79Cob97KSh \N \N \N \N \N f \N \N \N {} \N \N f f \N | ||
| 3 [email protected] Accounts Payable Accounts Payable \N txeLau9Rdu4X E/2021/11/T/1 C/2021/11/R/2 80 USD \N \N setqgvGQnsAya t PAYMENT_PROCESSING \N \N \N rpu5W0LYrk6e 2021-11-16 00:00:00+00 2021-11-16 00:00:00+00 2021-11-16 04:24:18.688+00 2021-11-16 04:25:21.996+00 2021-11-16 04:25:49.174565+00 2021-11-16 04:25:49.174584+00 PERSONAL {"Klass": "Klass", "Device Type": "", "Fyle Category": ""} \N f \N oraWFQlEpjbb 4.53 tg31j9m4PoEO \N \N \N f \N \N \N {} \N \N f f \N | ||
| 4 [email protected] Accounts Payable Accounts Payable \N txMLGb6Xy8m8 E/2021/11/T/2 C/2021/11/R/1 100 USD \N \N setqgvGQnsAya f PAYMENT_PROCESSING \N \N \N rprqDvARHUnv 2021-11-16 00:00:00+00 2021-11-16 00:00:00+00 2021-11-16 04:24:38.141+00 2021-11-16 04:25:21.996+00 2021-11-16 04:25:49.192351+00 2021-11-16 04:25:49.192367+00 CCC {"Device Type": "", "Fyle Category": ""} \N f \N oraWFQlEpjbb 16.67 tgSYjXsBCviv \N \N \N f \N \N \N {} \N \N f f \N | ||
| 173 [email protected] Food Food Project 2 tx7A5QpesrV5 E/2021/12/T/1 C/2021/12/R/1 120 USD \N \N set15sMvtRIiS t PAYMENT_PROCESSING \N Sales and Cross \N rpXqCutQj85N 2021-12-03 00:00:00+00 2021-12-03 00:00:00+00 2021-12-03 10:58:30.076+00 2021-12-03 11:00:22.64+00 2021-12-03 11:26:58.685597+00 2021-12-03 11:26:58.685616+00 PERSONAL {} \N f \N orHe8CpW2hyN \N \N \N \N \N f \N \N \N {} \N \N f f \N | ||
| 174 [email protected] Food Food Project 2 txcKVVELn1Vl E/2021/12/T/2 C/2021/12/R/1 130 USD \N \N set15sMvtRIiS f PAYMENT_PROCESSING \N Sales and Cross \N rpXqCutQj85N 2021-12-03 00:00:00+00 2021-12-03 00:00:00+00 2021-12-03 10:58:49.51+00 2021-12-03 11:00:22.64+00 2021-12-03 11:26:58.702183+00 2021-12-03 11:26:58.702209+00 CCC {} \N f \N orHe8CpW2hyN \N \N \N \N \N f \N \N \N {} \N \N f f \N | ||
| 600 [email protected] Food Food Project 2 txcKVVELn1Vlkill E/2021/12/T/298 C/2021/12/R/198 130 USD \N \N set15sMvtRIiSkill f PAYMENT_PROCESSING \N Sales and Cross \N rpXqCutQj85Nkill 2021-12-03 00:00:00+00 2021-12-03 00:00:00+00 2021-12-03 10:58:49.51+00 2021-12-03 11:00:22.64+00 2021-12-03 11:26:58.702183+00 2021-12-03 11:26:58.702209+00 CCC {} \N f \N or79Cob97KSh \N \N \N \N \N t \N \N \N {} \N \N f f \N | ||
| 601 [email protected] Food Food Project 2 txcKVVELn1Vlgon E/2021/12/T/299 C/2021/12/R/199 130 USD \N \N set15sMvtRIiSgon f PAYMENT_PROCESSING \N Sales and Cross \N rpXqCutQj85Ngon 2021-12-03 00:00:00+00 2021-12-03 00:00:00+00 2021-12-03 10:58:49.51+00 2021-12-03 11:00:22.64+00 2021-12-03 11:26:58.702183+00 2021-12-03 11:26:58.702209+00 CCC {} \N f \N or79Cob97KSh \N \N \N \N \N t \N \N \N {} \N \N f f \N |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test data quality and coverage.
Two suggestions for improvement:
- Fix inconsistent spacing in claim numbers (e.g.,
C/2021/12/R/198vsC/2021/12/R/198). - Add test cases with non-NULL
masked_corporate_card_numbervalues, especially for records withfund_source = 'CCC'(Corporate Credit Card).
Example test data improvement:
600 [email protected] Food Food Project 2 txcKVVELn1Vlkill E/2021/12/T/298 C/2021/12/R/198 130 USD \N \N set15sMvtRIiSkill f PAYMENT_PROCESSING \N Sales and Cross \N rpXqCutQj85Nkill 2021-12-03 00:00:00+00 2021-12-03 00:00:00+00 2021-12-03 10:58:49.51+00 2021-12-03 11:00:22.64+00 2021-12-03 11:26:58.702183+00 2021-12-03 11:26:58.702209+00 CCC {} \N f \N or79Cob97KSh \N \N \N \N \N t \N \N \N {} \N \N f f \N
-601 [email protected] Food Food Project 2 txcKVVELn1Vlgon E/2021/12/T/299 C/2021/12/R/199 130 USD \N \N set15sMvtRIiSgon f PAYMENT_PROCESSING \N Sales and Cross \N rpXqCutQj85Ngon 2021-12-03 00:00:00+00 2021-12-03 00:00:00+00 2021-12-03 10:58:49.51+00 2021-12-03 11:00:22.64+00 2021-12-03 11:26:58.702183+00 2021-12-03 11:26:58.702209+00 CCC {} \N f \N or79Cob97KSh \N \N \N \N \N t \N \N \N {} \N \N f f \N
+601 [email protected] Food Food Project 2 txcKVVELn1Vlgon E/2021/12/T/299 C/2021/12/R/199 130 USD \N \N set15sMvtRIiSgon f PAYMENT_PROCESSING \N Sales and Cross \N rpXqCutQj85Ngon 2021-12-03 00:00:00+00 2021-12-03 00:00:00+00 2021-12-03 10:58:49.51+00 2021-12-03 11:00:22.64+00 2021-12-03 11:26:58.702183+00 2021-12-03 11:26:58.702209+00 CCC {} \N f \N or79Cob97KSh \N \N \N \N \N t \N \N \N {} \N \N f f XXXX-XXXX-XXXX-1234Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/netsuite/models.py (1)
270-271: Consider adding format validation for the masked card number.The changes look good and align with the PR objectives. However, consider adding validation to ensure the masked card number follows the expected format (e.g., "XXXX-XXXX-XXXX-1234").
Add validation before using the masked card number:
- 'card_number': '{0}'.format(lineitem.masked_corporate_card_number) if lineitem.masked_corporate_card_number else '', + 'card_number': '{0}'.format(lineitem.masked_corporate_card_number) if lineitem.masked_corporate_card_number and + re.match(r'^[\dX]{4}-[\dX]{4}-[\dX]{4}-\d{4}$', lineitem.masked_corporate_card_number) else '',Don't forget to add the import at the top of the file:
import re
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/fyle/models.py(1 hunks)apps/netsuite/models.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/fyle/models.py
🔇 Additional comments (1)
apps/netsuite/models.py (1)
270-271: Verify memo_structure configuration for new fields.
The new fields will only appear in the purpose string if they are included in the memo_structure configuration. Ensure that the configuration is updated if these fields need to be included.
Run the following script to check the current memo_structure configuration:
✅ Verification successful
Based on the search results, I can now generate the final response as we have found the necessary information about memo structure configuration and its default fields.
No configuration update needed for new fields
The new fields (employee_name and card_number) are being added to the available fields dictionary in the get_expense_purpose function, but they don't require any configuration changes because:
- The default memo structure (defined in
apps/workspaces/models.py) already includes essential fields:['employee_email', 'category', 'merchant', 'spent_on', 'report_number', 'purpose'] - The memo structure is configurable through the workspace settings, and users can modify it as needed through the advanced settings API
- The changes are backward compatible as the new fields are optional and only appear in the purpose string if explicitly added to the
memo_structureconfiguration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new fields are included in any memo_structure configurations
# Search for memo_structure configurations
rg -A 5 'memo_structure.*=.*\[.*\]'
# Search for any existing usage of employee_name or card_number in memo structures
rg -i '(employee_name|card_number).*memo_structure'
Length of output: 636
Script:
#!/bin/bash
# Let's search for memo_structure configurations in different ways and contexts
# Search for memo_structure in Python files
rg -t py 'memo_structure' -B 2 -A 2
# Search for any configuration related to memo fields
rg -t py '(memo_fields|memo_format|memo_template)' -B 2 -A 2
# Search specifically in models.py and settings related files
fd -e py "(model|setting)" --exec rg -l 'memo' {}
Length of output: 6974
anishfyle
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
anishfyle
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
anishfyle
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
|
Description
Clickup
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Database Updates