feat: masked card num employee name description #578
Conversation
anishfyle
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThis pull request introduces changes to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Expense
participant SageIntacct
User->>Expense: Create Expense with masked_corporate_card_number
Expense->>SageIntacct: Send expense details
SageIntacct->>SageIntacct: Process expense purpose
SageIntacct-->>User: Confirm expense created
Possibly related PRs
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 (
|
|
PR title must start with "fix:", "feat:", "chore:", "refactor", or "test:" (case-insensitive) |
|
|
PR title must start with "fix:", "feat:", "chore:", "refactor", or "test:" (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: 3
🧹 Outside diff range and nitpick comments (3)
apps/fyle/migrations/0035_expense_masked_corporate_card_number.py (1)
13-17: Refine the field definition for masked card numbersWhile the basic structure is correct, consider these essential refinements:
- Reduce
max_lengthto a more appropriate value (e.g., 20 characters should be sufficient for a masked 16-digit card number)- Add a
db_index=Trueif you plan to query by this field- Consider adding a validator for the masked format
Here's the suggested implementation:
migrations.AddField( model_name='expense', name='masked_corporate_card_number', - field=models.CharField(help_text='Masked Corporate Card Number', max_length=255, null=True), + field=models.CharField( + help_text='Masked Corporate Card Number (e.g., ****-****-****-1234)', + max_length=20, + null=True, + db_index=True, + validators=[ + RegexValidator( + regex=r'^\*+[-\s]*\*+[-\s]*\*+[-\s]*\d{4}$', + message='Invalid masked card number format' + ) + ] + ), ),apps/sage_intacct/models.py (1)
453-454: Consider adding data privacy documentation.Since the code handles sensitive data (masked card numbers), consider adding documentation about data privacy requirements and masking rules in the module docstring.
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (1)
1168-1169: Consider adding length constraints for masked card numbersWhile the schema changes look good, consider adding CHECK constraints to ensure the masked_corporate_card_number follows a consistent format (e.g., length and pattern).
Example constraint:
ALTER TABLE expenses ADD CONSTRAINT check_masked_card_number CHECK (length(masked_corporate_card_number) >= 4 AND length(masked_corporate_card_number) <= 16);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/fyle/migrations/0035_expense_masked_corporate_card_number.py(1 hunks)apps/fyle/models.py(1 hunks)apps/sage_intacct/models.py(1 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(4 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
🔇 Additional comments (5)
apps/fyle/models.py (1)
147-147: LGTM! Good security practice using masked card numbers.
The new field is properly defined with appropriate constraints and follows security best practices by storing masked rather than full card numbers.
apps/sage_intacct/models.py (2)
453-454: LGTM! The changes enhance expense purpose details.
The addition of employee_name and masked card number to the expense purpose details improves the context available in the expense description.
Line range hint 1-1200: LGTM! Model changes are well-structured.
The addition of the expense_payment_type field to the relevant models follows Django best practices and is properly documented.
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (2)
4139-4139: LGTM: Migration record properly added
The migration for adding the masked_corporate_card_number is correctly recorded in the migrations table.
8147-8147: LGTM: Sequence value correctly updated
The migration sequence is properly updated to match the latest migration ID.
anishfyle
changed the title
anishfyle
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
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 (1)
tests/test_workspaces/fixtures.py (1)
65-66: LGTM! Consider adding more test cases.The addition of the masked corporate card number field with proper masking format (showing only last 4 digits) is good. However, consider enriching the test fixtures with additional test cases to cover various scenarios.
Consider adding more expense entries to cover scenarios such as:
'expenses':[ { // ... existing test case ... 'masked_corporate_card_number': '**** 4567' }, + { + // ... similar expense but with different card ... + 'masked_corporate_card_number': '**** 9876' + }, + { + // ... expense without card number ... + 'masked_corporate_card_number': None + } ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
tests/test_fyle/fixtures.py(14 hunks)tests/test_workspaces/fixtures.py(1 hunks)
🔇 Additional comments (2)
tests/test_fyle/fixtures.py (2)
293-294: Implementation of masked corporate card numbers looks good!
The masked corporate card number field has been consistently added across all test fixtures with proper masking pattern showing only the last 4 digits. This follows security best practices for handling sensitive card data.
Also applies to: 340-341, 387-388, 432-433, 479-480, 524-525, 576-577, 629-630, 681-682, 734-735, 787-788, 841-842, 892-893, 946-947
293-294: Verify consistent masking pattern across the application
While the test fixtures use a consistent masking pattern '**** 4567', let's verify this matches the implementation.
Also applies to: 340-341, 387-388, 432-433, 479-480, 524-525, 576-577, 629-630, 681-682, 734-735, 787-788, 841-842, 892-893, 946-947
✅ Verification successful
Let me gather more information about the card number masking implementation.
Let me try one final search to check for any card number formatting in the API or utility layers.
Card number masking appears to be handled externally
Based on the codebase analysis, the masked_corporate_card_number field is only used to store and pass through already masked card numbers. There's no masking logic implemented within this codebase, suggesting the masking is handled by the Fyle API or external service before the data reaches this application. The test fixtures are using a consistent pattern that likely matches what's received from the external service.
- The field is defined in
apps/fyle/models.pyas a CharField that stores pre-masked numbers - The value is passed through without modification in
apps/sage_intacct/models.py - No masking/formatting logic was found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for masked card number patterns in the codebase
# to verify consistent implementation
# Look for masking pattern definitions
rg -g '!tests/*' "masked.*card.*number|card.*mask" -A 2
# Look for actual masking implementation
ast-grep --pattern 'mask$_($_, $_) {
$$$
}'
Length of output: 1447
Script:
#!/bin/bash
# Search for any card number masking or formatting patterns in the codebase
rg -g '!tests/*' -i "card.*number.*format|format.*card|mask.*card" -A 3
# Look for any utility functions that might handle card number formatting
ast-grep --pattern 'def $_($$$) {
$$$
card
$$$
}'
# Check if there are any constants or configurations related to card masking
rg -g '!tests/*' "MASK|FORMAT.*CARD|CARD.*FORMAT" -A 2
Length of output: 2515
Script:
#!/bin/bash
# Search for any API or utility code that might handle card numbers
rg -g '!tests/*' "def.*expense.*card|class.*expense.*card" -A 5
# Look for any card-related serializers or formatters
ast-grep --pattern 'class $_Serializer($_) {
$$$
card
$$$
}'
# Check for any card-related API endpoints
rg -g '!tests/*' "@api_view|@route" -A 3 | rg -i "card"
Length of output: 190
|
Description
Clickup
Summary by CodeRabbit
New Features
Bug Fixes
Documentation