feat: masked card num employee name description #411
Conversation
anishfyle
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThis pull request introduces changes to the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@anishfyle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 12 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🔥 Files not summarized due to errors (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (5)tests/sql_fixtures/reset_db_fixtures/reset_db.sql (5)
The new
Migration
The
The sample data shows:
However, let's verify if any existing records should have masked card numbers populated. ✅ Verification successfulData integrity for new column is verified The verification shows:
Since there's no existing card number data in the database to migrate, having NULL values for 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check if there are any corporate card related fields or tables that might contain card numbers
rg -l "corporate.*card|card.*number" --type sql
Length of output: 144 Script: #!/bin/bash
# Let's check the content of the corporate card related SQL files
echo "=== Checking corporate card schema ==="
rg -A 5 -B 5 "corporate.*card|card.*number" --type sql
# Also check if there are any existing corporate_card_id values
echo "=== Checking corporate_card_id values ==="
cat tests/sql_fixtures/reset_db_fixtures/reset_db.sql | grep -A 1 "COPY public.expenses" | grep -v "COPY" | tr '|' '\n' | awk -F'\t' '{print $27}' | sort | uniq
Length of output: 15031
The dump was created using PostgreSQL 15.10 but dumped using version 15.8. While minor version differences are generally compatible, ensure all deployment environments support PostgreSQL 15.x. ✅ Verification successfulPostgreSQL version compatibility is properly configured The project explicitly uses PostgreSQL 15 across all environments:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check if README or deployment docs specify PostgreSQL version requirements
fd -t f "README|deploy" -X grep -l "postgres"
Length of output: 57 Script: #!/bin/bash
# Check PostgreSQL version requirements in README.md
cat README.md | grep -i "postgres\|postgresql" -A 2 -B 2
Length of output: 414 Script: #!/bin/bash
# Check for any Docker or deployment configuration files that might specify PostgreSQL version
fd -t f "docker-compose|Dockerfile" -X grep -l "postgres"
Length of output: 130 Script: #!/bin/bash
# Check PostgreSQL version specifications in Docker files
echo "=== docker-compose.yml.template ===="
cat docker-compose.yml.template | grep -i "postgres"
echo -e "\n=== docker-compose-pipeline.yml ===="
cat docker-compose-pipeline.yml | grep -i "postgres"
echo -e "\n=== Dockerfile ===="
cat Dockerfile | grep -i "postgres"
Length of output: 1194 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: 0
🧹 Outside diff range and nitpick comments (4)
tests/test_xero/test_models.py (1)
Line range hint
1-1: Consider adding edge case test coverageTo ensure robust testing of the new description format, consider adding test cases for these scenarios:
- Very long masked card numbers
- Special characters in employee names
- Different formats of masked numbers (e.g., "XXXX-1234" vs "****1234")
- Missing employee name but present card number and vice versa
Here's a suggested test case structure:
def test_get_expense_purpose_edge_cases(db): workspace_id = 1 expense_group = ExpenseGroup.objects.get(id=10) expense = expense_group.expenses.first() test_cases = [ { 'employee_name': 'O\'Connor-Smith Jr.', # Special characters 'masked_card': 'XXXX-XXXX-XXXX-XXXX-XXXX-1234' # Long number }, { 'employee_name': '李华', # Unicode characters 'masked_card': '****1234' # Different format }, { 'employee_name': None, # Missing name 'masked_card': 'XXXX-1234' }, { 'employee_name': 'John Doe', 'masked_card': None # Missing card } ] for case in test_cases: expense.employee_name = case['employee_name'] expense.masked_corporate_card_number = case['masked_card'] expense.save() description = get_expense_purpose(workspace_id, expense, expense.category) if case['employee_name']: assert case['employee_name'] in description if case['masked_card']: assert case['masked_card'] in descriptionapps/xero/models.py (1)
167-171: Consider using f-strings for better readability.The current format string is becoming long and could benefit from better readability. Consider using f-strings for a more maintainable solution.
Here's a suggested improvement:
- return "{0}{1} ({2}){3}, category - {4} {5}, report number - {6} {7} - {8}".format( - vendor, - lineitem.employee_email, - lineitem.employee_name, - card_number, - category, - spent_at, - lineitem.claim_number, - expense_purpose, - expense_link, - ) + return f"{vendor}{lineitem.employee_email} ({lineitem.employee_name}){card_number}, category - {category} {spent_at}, report number - {lineitem.claim_number} {expense_purpose} - {expense_link}"apps/fyle/models.py (1)
157-159: LGTM! Consider adding docstring comments.The new fields are well-designed:
masked_corporate_card_numberfollows PCI compliance by storing only masked numbersprevious_export_statehelps track export state changesworkspaceFK enables proper data isolationConsider adding docstring comments to explain:
- The masking format for card numbers
- Valid values for export states
- The relationship with workspace
- masked_corporate_card_number = models.CharField(max_length=255, help_text='Masked Corporate Card Number', null=True) - previous_export_state = models.CharField(max_length=255, help_text='Previous export state', null=True) - workspace = models.ForeignKey(Workspace, on_delete=models.PROTECT, help_text='To which workspace this expense belongs to', null=True) + masked_corporate_card_number = models.CharField( + max_length=255, + help_text='Masked Corporate Card Number (e.g., "XXXX-XXXX-XXXX-1234")', + null=True + ) + previous_export_state = models.CharField( + max_length=255, + help_text='Previous state before export (e.g., "APPROVED", "PAID")', + null=True + ) + workspace = models.ForeignKey( + Workspace, + on_delete=models.PROTECT, + help_text='Workspace that owns this expense', + null=True + )tests/sql_fixtures/reset_db_fixtures/reset_db.sql (1)
949-950: Consider adding a length constraint for the masked card number.The column definition looks good, but consider adding a CHECK constraint to ensure the masked card number follows a consistent format (e.g., typically showing only last 4 digits like 'XXXX-XXXX-XXXX-1234').
ALTER TABLE expenses ADD CONSTRAINT check_masked_card_number CHECK (masked_corporate_card_number IS NULL OR LENGTH(masked_corporate_card_number) BETWEEN 16 AND 19);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
apps/fyle/migrations/0023_expense_masked_corporate_card_number.py(1 hunks)apps/fyle/models.py(1 hunks)apps/xero/models.py(1 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(5 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 due to trivial changes (1)
- apps/fyle/migrations/0023_expense_masked_corporate_card_number.py
🔇 Additional comments (5)
tests/test_xero/test_models.py (2)
37-37: 🛠️ Refactor suggestion
Add test cases for masked card number scenarios
The current test only covers the case where the masked card number is None. To ensure comprehensive test coverage for the new feature, we should add test cases that verify the description format when a masked card number is present.
Let's verify if there are any other test cases for this scenario:
Consider adding these test cases:
def test_create_bill_with_masked_card(db):
expense_group = ExpenseGroup.objects.get(id=4)
expense = expense_group.expenses.first()
expense.masked_corporate_card_number = "XXXX-XXXX-XXXX-1234"
expense.save()
bill = Bill.create_bill(expense_group)
bill_lineitems = BillLineItem.create_bill_lineitems(expense_group)
for bill_lineitem in bill_lineitems:
assert "XXXX-XXXX-XXXX-1234" in bill_lineitem.description171-171: 🛠️ Refactor suggestion
Add test coverage for employee name in description
The PR objectives mention adding both masked card number and employee name to the description, but the test only verifies the email format. We should add assertions to verify that the employee name is correctly included in the description when available.
Let's verify the current implementation:
Consider adding these test cases:
def test_get_expense_purpose_with_employee_details(db):
workspace_id = 1
expense_group = ExpenseGroup.objects.get(id=10)
expense = expense_group.expenses.first()
# Test with both name and card number
expense.employee_name = "John Doe"
expense.masked_corporate_card_number = "XXXX-XXXX-XXXX-1234"
expense.save()
category = expense.category if expense.category == expense.sub_category \
else "{0} / {1}".format(expense.category, expense.sub_category)
expense_purpose = get_expense_purpose(workspace_id, expense, category)
assert "John Doe" in expense_purpose
assert "XXXX-XXXX-XXXX-1234" in expense_purposeapps/xero/models.py (1)
165-166: LGTM! Secure handling of masked card number.
The implementation properly handles the conditional display of the masked corporate card number with appropriate null checks. The card number is pre-masked, ensuring secure handling of sensitive data.
apps/fyle/models.py (1)
157-159: Verify migration file exists for the model changes.
The changes require a database migration.
✅ Verification successful
Migration files exist for all model changes ✅
The necessary migration files are present:
masked_corporate_card_numberfield is added in migration0023_expense_masked_corporate_card_number.pyprevious_export_stateandworkspacefields are added in migration0018_auto_20240213_0450.py
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if migration file exists for the new fields
# Expected: Find migration file that adds masked_corporate_card_number, previous_export_state, and workspace fields
fd -e py -x grep -l "masked_corporate_card_number\|previous_export_state\|workspace" {} \; migrations/
Length of output: 107
Script:
#!/bin/bash
# Let's first check if the migrations directory exists at the repository root
fd -t d migrations
# Then let's check all possible locations for migrations
fd -t f -e py . | grep -i "migr"
# Finally, let's check the content of any Python files that might contain these field definitions
rg -l "masked_corporate_card_number|previous_export_state|workspace" -g "*.py"
Length of output: 12720
Script:
#!/bin/bash
# Let's check the content of the migration file that adds masked_corporate_card_number
cat apps/fyle/migrations/0023_expense_masked_corporate_card_number.py
# Let's also check migrations that might contain previous_export_state and workspace fields
rg "previous_export_state|workspace = models" apps/fyle/migrations/
Length of output: 702
Script:
#!/bin/bash
# Let's check the content of the migration that adds previous_export_state
cat apps/fyle/migrations/0018_auto_20240213_0450.py
# Let's check for workspace field in initial migration
cat apps/fyle/migrations/0001_initial.py
Length of output: 7602
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (1)
4888-4898: Ensure PCI DSS compliance for card number masking.
When populating this new column, ensure that:
- Only the last 4 digits of the card number are visible
- The masking follows PCI DSS requirements
- Proper data encryption is in place during transmission and storage
✅ Verification successful
Based on the search results, I can now provide a final response:
Card number masking implementation appears compliant with PCI DSS requirements
The codebase shows:
- All card numbers in test fixtures use proper masking format ("Sample-Bank-xxxxxx-NNNNN")
- The
masked_corporate_card_numbercolumn is properly defined as a nullable CharField - No unmasked 16-digit card numbers were found in the codebase
- Card numbers are only used in a masked format when displaying in descriptions (e.g. "card {}" format in xero/models.py)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential unmasked card numbers in the codebase
rg -i "(\b\d{4}[- ]?\d{4}[- ]?\d{4}[- ]?\d{4}\b)" --type-add 'sql:*.sql' --type sql
Length of output: 85
Script:
#!/bin/bash
# Check for any references to card numbers or masking in the codebase
rg -i "(card.*number|mask.*card|credit.*card.*number)" -g "!*.sql" -A 2 -B 2
# Check SQL schema for card number related columns and their types
rg -i "create.*table" -A 20 -g "*.sql" | rg -i -A 5 -B 5 "(card|mask)"
Length of output: 13786
anishfyle
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 (1)
apps/fyle/models.py(2 hunks)
🔇 Additional comments (1)
apps/fyle/models.py (1)
158-158: Verify the usage of previous_export_state field
The previous_export_state field is added but not mentioned in the PR objectives. Let's verify its usage in the codebase.
✅ Verification successful
The previous_export_state field is used appropriately for export state tracking
The field is used to:
- Store the previous state of expense exports in bulk updates
- Control auto-export behavior by checking for ERROR states
- Track export state transitions as part of the accounting export summary feature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of previous_export_state in the codebase
echo "Searching for previous_export_state usage..."
rg -l "previous_export_state"
# Search for any state tracking patterns
echo "Searching for state transition patterns..."
ast-grep --pattern 'expense.previous_export_state = $$$'
Length of output: 468
Script:
#!/bin/bash
# Check the implementation details in actions.py and queue.py
echo "Checking actions.py implementation:"
rg "previous_export_state" apps/fyle/actions.py -B 2 -A 2
echo "\nChecking queue.py implementation:"
rg "previous_export_state" apps/xero/queue.py -B 2 -A 2
# Check migration file to understand the purpose
echo "\nChecking migration file:"
cat apps/fyle/migrations/0018_auto_20240213_0450.py
Length of output: 2657
| masked_corporate_card_number = models.CharField(max_length=255, help_text='Masked Corporate Card Number', null=True) | ||
| previous_export_state = models.CharField(max_length=255, help_text='Previous export state', null=True) | ||
| workspace = models.ForeignKey(Workspace, on_delete=models.PROTECT, help_text='To which workspace this expense belongs to', null=True) |
There was a problem hiding this comment.
Consider removing null=True from workspace ForeignKey
The workspace field is a foreign key relationship that should always be present as every expense must belong to a workspace. Having null=True might lead to data integrity issues.
Apply this change:
- workspace = models.ForeignKey(Workspace, on_delete=models.PROTECT, help_text='To which workspace this expense belongs to', null=True)
+ workspace = models.ForeignKey(Workspace, on_delete=models.PROTECT, help_text='To which workspace this expense belongs to')Don't forget to create a data migration to set the workspace for any existing expenses before making this field non-nullable.
📝 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.
| masked_corporate_card_number = models.CharField(max_length=255, help_text='Masked Corporate Card Number', null=True) | |
| previous_export_state = models.CharField(max_length=255, help_text='Previous export state', null=True) | |
| workspace = models.ForeignKey(Workspace, on_delete=models.PROTECT, help_text='To which workspace this expense belongs to', null=True) | |
| masked_corporate_card_number = models.CharField(max_length=255, help_text='Masked Corporate Card Number', null=True) | |
| previous_export_state = models.CharField(max_length=255, help_text='Previous export state', null=True) | |
| workspace = models.ForeignKey(Workspace, on_delete=models.PROTECT, help_text='To which workspace this expense belongs to') |
| @@ -201,6 +202,7 @@ def create_expense_objects(expenses: List[Dict], workspace_id: int, skip_update: | |||
| else None, | |||
| "cost_center": expense["cost_center"], | |||
| "corporate_card_id": expense["corporate_card_id"], | |||
| 'masked_corporate_card_number': expense['masked_corporate_card_number'], | |||
There was a problem hiding this comment.
Add validation for masked corporate card number format
To prevent potential exposure of sensitive card information, consider adding validation to ensure the corporate card number is properly masked before storing it.
Add a validation function and use it in the defaults:
+def validate_masked_card_number(number: str) -> str:
+ """
+ Validates that the card number is properly masked
+ Expected format: XXXX XXXX XXXX 1234
+ """
+ if not number:
+ return None
+
+ # Remove spaces for validation
+ cleaned = number.replace(' ', '')
+
+ # Check length (16 digits is standard)
+ if len(cleaned) != 16:
+ return None
+
+ # Ensure first 12 digits are masked
+ if not cleaned[:12].replace('X', '') == '':
+ return None
+
+ # Ensure last 4 digits are numbers
+ if not cleaned[12:].isdigit():
+ return None
+
+ return number
# In defaults dictionary:
- 'masked_corporate_card_number': expense['masked_corporate_card_number'],
+ 'masked_corporate_card_number': validate_masked_card_number(expense['masked_corporate_card_number']),Committable suggestion skipped: line range outside the PR's diff.
|
anishfyle
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
Description
Clickup