feat: add masked corporate card number and employee name #694
Conversation
anishfyle
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces a new field, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 3
🧹 Outside diff range and nitpick comments (2)
apps/fyle/models.py (1)
Line range hint
155-182: Update create_expense_objects to handle masked_corporate_card_numberThe
defaultsdictionary increate_expense_objectsmethod needs to be updated to include the new field.Add the new field to the defaults dictionary:
defaults = { 'employee_email': expense['employee_email'], 'employee_name': expense['employee_name'], 'category': expense['category'], 'sub_category': expense['sub_category'], 'project': expense['project'], 'expense_number': expense['expense_number'], 'org_id': expense['org_id'], + 'masked_corporate_card_number': expense.get('masked_corporate_card_number'), 'amount': _round_to_currency_fraction(expense['amount'], expense['currency']),apps/quickbooks_online/models.py (1)
37-38: Add null check for employee_name fieldWhile the card number has proper null handling, consider adding similar null handling for employee_name to maintain consistency and prevent potential issues with None values.
- 'employee_name': lineitem.employee_name, + 'employee_name': '{0}'.format(lineitem.employee_name) if lineitem.employee_name else '',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/fyle/models.py(1 hunks)apps/quickbooks_online/models.py(1 hunks)
🔇 Additional comments (1)
apps/quickbooks_online/models.py (1)
Line range hint 1-1000: LGTM! Type annotations improve code clarity
The addition of return type annotations to methods and parameter type hints enhances code readability and type safety. The annotations are accurate and consistently applied across all methods.
anishfyle
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 (2)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (2)
999-1000: LGTM! Consider adding a column comment.The column definition is appropriate with
character varying(255)type and NULL constraint.Consider adding a column comment to document the masking format:
is_posted_at_null boolean NOT NULL, - masked_corporate_card_number character varying(255) + masked_corporate_card_number character varying(255), + COMMENT ON COLUMN public.expenses.masked_corporate_card_number IS 'Stores the masked version of corporate card number (e.g., XXXX-XXXX-XXXX-1234)'
999-1000: Important: Ensure proper card number masking implementation.When implementing the masking logic, ensure:
- Only the last 4 digits of the card number are visible
- Masking is applied before storing in the database
- Original card numbers are never logged or stored
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/fyle/migrations/0040_expense_masked_corporate_card_number.py(1 hunks)apps/fyle/models.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/0040_expense_masked_corporate_card_number.py
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/fyle/models.py
🔇 Additional comments (2)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (2)
4075-4075: Migration sequence properly updated.
The migration sequence is correctly incremented to 193 and the migration history is updated with the new migration for adding the masked corporate card number.
Also applies to: 34004-34004
33387-33447: Verify proper NULL handling in sample data.
The sample data appropriately handles the new column by setting NULL values for existing records, maintaining data integrity during the migration.
✅ Verification successful
Let me analyze the results and generate additional verification scripts to ensure proper NULL handling.
Based on the results, I can now provide the final response:
Proper NULL handling confirmed for masked_corporate_card_number column
The verification shows:
- Column is defined as
character varying(255)without NOT NULL constraint - Sample data shows NULL values are properly handled in existing records
- No default values are enforced, allowing NULL values as expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the masked card numbers follow proper format when present
rg -A 1 'masked_corporate_card_number.*?[^N]' tests/sql_fixtures/reset_db_fixtures/reset_db.sql
Length of output: 1518
Script:
#!/bin/bash
# Check for any non-NULL values in masked_corporate_card_number column
rg -B 1 'masked_corporate_card_number.*?[^\\N]' tests/sql_fixtures/reset_db_fixtures/reset_db.sql
# Check the column definition and constraints
rg -A 3 'ADD COLUMN.*masked_corporate_card_number' tests/sql_fixtures/reset_db_fixtures/reset_db.sql
# Look for any default values or NOT NULL constraints
rg 'masked_corporate_card_number.*NOT NULL' tests/sql_fixtures/reset_db_fixtures/reset_db.sql
Length of output: 1254
anishfyle
had a problem deploying
to
CI Environment
GitHub Actions
Failure
anishfyle
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/fixtures.py (2)
133-134: Consider using consistent test data patterns for sensitive information.While the masked card number follows a common pattern (
**** 4567), consider:
- Using a consistent format that matches your production masking pattern
- Adding a comment to indicate this is test data
- Using constants for sensitive test data to maintain consistency across test cases
- 'masked_corporate_card_number': '**** 4567', + # Test data: masked card number following production pattern XXXX XXXX XXXX 4567 + 'masked_corporate_card_number': 'XXXX XXXX XXXX 4567',
Line range hint
77-134: Consider adding more test cases for edge scenarios.The current test data covers basic scenarios, but consider adding test cases for:
- Different card number formats (e.g., Amex vs. Visa)
- Missing or null card numbers
- Special characters in employee names
- International employee names with non-ASCII characters
Consider adding additional test cases to the expenses array:
{ # Test case for Amex format 'masked_corporate_card_number': 'XXXX XXXXXX XXXXX', 'employee_name': 'João Silva', # International name }, { # Test case for null card 'masked_corporate_card_number': None, 'employee_name': "O'Connor-Smith", # Special characters }tests/test_fyle/fixtures.py (1)
512-512: Fix indentation inconsistenciesThe indentation of the 'masked_corporate_card_number' field is inconsistent in these locations. Some lines have extra spaces.
Apply consistent indentation:
- 'masked_corporate_card_number': '**** 4567', + 'masked_corporate_card_number': '**** 4567',Also applies to: 557-557, 603-603, 1593-1593, 1688-1688
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/fyle/models.py(2 hunks)tests/test_fyle/fixtures.py(28 hunks)tests/test_workspaces/fixtures.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/fyle/models.py
🔇 Additional comments (4)
tests/test_workspaces/fixtures.py (1)
Line range hint 69-77: Verify configuration changes align with feature requirements.
The general settings show significant changes that affect corporate card and employee handling:
corporate_credit_card_expenses_objectis set to 'BILL'employee_field_mappingis set to 'EMPLOYEE'auto_create_destination_entityandsync_fyle_to_qbo_paymentsare disabled
Please verify these configuration changes support the new masked card number and employee name features as intended.
✅ Verification successful
Configuration changes are properly aligned with test cases
The configuration changes are well-supported by existing test cases across the codebase:
corporate_credit_card_expenses_object: 'BILL'is verified in multiple test files includingtest_tasks.py,test_utils.py, andtest_models.pyemployee_field_mapping: 'EMPLOYEE'is validated in employee mapping tests and QBO utils tests- The disabled settings (
auto_create_destination_entity: Falseandsync_fyle_to_qbo_payments: False) are consistently used in advanced configuration and clone settings tests
The test coverage indicates these configurations are intentional and properly tested for the feature implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any related configuration tests that might need updating
# Look for test files that verify these configuration settings
rg -l "corporate_credit_card_expenses_object|employee_field_mapping" tests/
Length of output: 810
Script:
#!/bin/bash
# Check the actual test cases and assertions related to these configurations
rg -A 5 "corporate_credit_card_expenses_object.*BILL|employee_field_mapping.*EMPLOYEE" tests/
# Also check for any test cases verifying auto_create and sync settings
rg -A 5 "auto_create_destination_entity.*False|sync_fyle_to_qbo_payments.*False" tests/
Length of output: 13370
tests/test_fyle/fixtures.py (3)
11-11: Consistent implementation of masked corporate card number
The masked card number format follows security best practices by only showing the last 4 digits. The field has been consistently added across various test scenarios.
Also applies to: 247-247, 292-292, 336-336, 380-380, 512-512, 557-557, 603-603, 640-640, 694-694, 739-739, 970-970, 1023-1023, 1075-1075, 1128-1128, 1181-1181, 1235-1235, 1286-1286, 1340-1340, 1593-1593, 1688-1688, 2112-2112, 2394-2394, 2786-2786, 10063-10063, 10120-10120, 10177-10177, 10237-10237
Line range hint 10062-10237: Verify complete test coverage
Let's ensure the new fields are added to all relevant test cases in the fixtures.
✅ Verification successful
Let me run another script to specifically check for the presence of the new field in all expense fixtures.
The new field masked_corporate_card_number has been consistently added to all expense fixtures
Based on the verification results, the field masked_corporate_card_number has been properly added to all relevant test fixtures including:
- Regular expenses
- CCC expenses
- Split expenses
- Expense groups
- Refund expenses
- Skipped expenses
The value '**** 4567' is consistently used across all test cases, maintaining proper test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all expense-related dictionaries in test fixtures
rg -A 5 "'id':\s*['\"]" tests/test_fyle/fixtures.py | grep -v "masked_corporate_card_number"
Length of output: 8812
Script:
#!/bin/bash
# Find all expense dictionaries and check for masked_corporate_card_number field
rg -B 5 -A 5 "'masked_corporate_card_number':" tests/test_fyle/fixtures.py
Length of output: 11581
246-246: Clarify the addition of 'is_posted_at_null' field
The PR objectives mention adding masked corporate card number and employee name, but there's no context for adding the 'is_posted_at_null' field. Please clarify if this is an intentional change.
Also applies to: 291-291, 335-335, 379-379, 511-511, 556-556, 602-602, 639-639, 693-693, 738-738, 969-969, 1022-1022, 1074-1074, 1127-1127, 1180-1180, 1234-1234, 1285-1285, 1339-1339, 1592-1592, 1687-1687, 2111-2111, 2393-2393, 2785-2785, 10062-10062, 10119-10119
✅ Verification successful
Let me check the migration history to understand when and why this field was added.
Let me check the PR description or commit messages to understand the context of this change.
Let me check if there are any references to this field in the actual code logic to understand its usage.
The is_posted_at_null field addition appears to be a valid and intentional change
Based on the codebase analysis, this field is a legitimate addition that works in conjunction with the existing posted_at field in the Expense model. It serves as a flag to explicitly track whether the posted_at field is null, as indicated by its help text. The field is properly:
- Defined in the model with appropriate default value and documentation
- Used in the expense data mapping logic
- Added through a proper migration (0039)
- Referenced consistently in test fixtures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this field is used in the codebase
rg "is_posted_at_null" --type py
Length of output: 2464
Script:
#!/bin/bash
# Check the migration file content
cat apps/fyle/migrations/0039_expense_is_posted_at_null.py
Length of output: 514
Script:
#!/bin/bash
# Check recent commits mentioning posted_at or the migration
git log --grep="posted_at\|0039_expense" -n 5
Length of output: 0
Script:
#!/bin/bash
# Look for usage of is_posted_at_null in actual code logic
rg "is_posted_at_null" apps/fyle/ -g "!migrations/*" -A 3 -B 3
Length of output: 2613
anishfyle
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Description
Clickup
Summary by CodeRabbit
New Features
Improvements
These changes improve the accuracy and detail of expense tracking within the application.