Feat: Add toggle for enabling uploading attachments #687
Conversation
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe pull request introduces a new configuration option Changes
Sequence DiagramsequenceDiagram
participant Admin as Workspace Admin
participant Config as Configuration
participant Task as Attachment Upload Task
Admin->>Config: Set is_attachment_upload_enabled
Config-->>Task: Provide upload configuration
alt Upload Enabled
Task->>Task: Execute Attachment Upload
else Upload Disabled
Task->>Task: Skip Attachment Upload
end
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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: 1
🧹 Nitpick comments (2)
apps/workspaces/models.py (1)
184-184: Enhance help text descriptionThe help text could be more descriptive about the feature's purpose and impact.
Consider updating the help text to be more specific:
- is_attachment_upload_enabled = models.BooleanField(default=True, help_text='Is Attachment upload enabled') + is_attachment_upload_enabled = models.BooleanField( + default=True, + help_text='Controls whether expense attachments are uploaded to NetSuite during export' + )apps/netsuite/tasks.py (1)
513-517: Fix indentation to maintain consistency.The indentation of this block is inconsistent with the rest of the file. Python is sensitive to indentation, and while this will work, it's important to maintain consistent style.
- if configuration.is_attachment_upload_enabled: - async_task( - 'apps.netsuite.tasks.upload_attachments_and_update_export', - expense_group.expenses.all(), task_log, fyle_credentials, expense_group.workspace_id - ) + if configuration.is_attachment_upload_enabled: + async_task( + 'apps.netsuite.tasks.upload_attachments_and_update_export', + expense_group.expenses.all(), task_log, fyle_credentials, expense_group.workspace_id + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/netsuite/tasks.py(4 hunks)apps/workspaces/migrations/0041_configuration_is_attachment_upload_enabled.py(1 hunks)apps/workspaces/models.py(1 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(6 hunks)tests/test_workspaces/data.json(1 hunks)
🔇 Additional comments (10)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (5)
6-6: LGTM: pg_dump version update
The update to pg_dump version 15.10 is a minor version change that includes bug fixes and improvements.
2701-2704: LGTM: Consistent data population
The data population maintains consistency with is_attachment_upload_enabled=true for all existing configurations, ensuring backward compatibility.
8001-8002: LGTM: Migration entries are properly sequenced
The migration entries are properly tracked and sequenced, with the sequence updated accordingly.
Also applies to: 11914-11914
326-327: Verify data migration strategy for the new column
The addition of is_attachment_upload_enabled with NOT NULL constraint is good for data integrity. However, let's verify the migration strategy.
✅ Verification successful
Let me gather more information about the migration file to ensure we have a complete picture.
Migration strategy for is_attachment_upload_enabled is properly implemented
The migration strategy is well-designed:
- Column is added with
default=True, ensuring no data inconsistency for existing records - The model definition matches the migration with the same default value
- The feature is properly integrated in
netsuite/tasks.pywith appropriate conditional checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check migration file for default value and backfill strategy
# Check migration file content
rg -A 10 "is_attachment_upload_enabled" apps/workspaces/migrations/
# Verify if there are any existing configurations that might be affected
ast-grep --pattern 'class Configuration:
$$$
is_attachment_upload_enabled = $_
$$$'
Length of output: 632
Script:
#!/bin/bash
# Get the full content of the migration file to verify the migration strategy
cat apps/workspaces/migrations/0041_configuration_is_attachment_upload_enabled.py
# Check if there are any existing usages of this configuration in the codebase
rg "is_attachment_upload_enabled" --type py
Length of output: 1178
12402-12408: Verify impact of new unique constraint on employee_mappings
The addition of a unique constraint on source_employee_id appears unrelated to the attachment upload feature. While it improves data integrity, we should verify:
- No existing duplicate source_employee_ids
- Impact on existing functionality
apps/workspaces/migrations/0041_configuration_is_attachment_upload_enabled.py (1)
1-18: LGTM!
The migration is correctly structured and maintains proper dependency chain.
tests/test_workspaces/data.json (1)
Line range hint 184-184: Verify implementation in tasks.py
The configuration toggle needs to be checked in the attachment upload logic.
Let's verify the implementation:
✅ Verification successful
Configuration toggle is properly implemented in tasks.py
The verification shows that the is_attachment_upload_enabled configuration toggle is correctly implemented:
- The main attachment upload logic is in
upload_attachments_and_update_exportfunction - The toggle is properly checked before calling the upload function in all three expense creation flows:
- Bill creation
- Credit Card Charge creation
- Expense Report creation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new configuration field is properly used in tasks
# Look for attachment upload related functions and their usage of the new toggle
rg -A 5 "def create_(bill|credit_card_charge|expense_report)" apps/netsuite/tasks.py
# Check for attachment upload logic
rg -B 2 -A 5 "upload.*attachment" apps/netsuite/tasks.py
Length of output: 3477
apps/netsuite/tasks.py (3)
565-570: LGTM! Well-structured attachment handling.
The implementation correctly:
- Checks the configuration flag
- Processes attachments for each expense
- Stores attachment links for later use
664-668: LGTM! Clean implementation of conditional attachment upload.
The code correctly checks the configuration flag before initiating the async task for attachment upload.
513-517: Verify attachment upload operations across the codebase.
Let's ensure all attachment-related operations are properly gated by the new configuration flag.
Also applies to: 565-570, 664-668
✅ Verification successful
All attachment upload operations are properly gated by the configuration flag
Based on the search results, all attachment-related operations are correctly controlled by the is_attachment_upload_enabled flag:
- The flag is properly defined in the Configuration model (
apps/workspaces/models.py) - All attachment upload operations in
apps/netsuite/tasks.pyare gated by this flag:- Line 513: Credit card charge attachments
- Line 565: Bill attachments
- Line 664: Expense report attachments
The implementation is consistent and complete, with no missed attachment operations found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all attachment-related operations to verify they're properly controlled
# Search for attachment upload related function calls
echo "Searching for attachment upload related functions..."
rg -l "upload_attachments|load_attachments"
# Search for attachment-related configuration checks
echo "Searching for attachment configuration checks..."
rg "is_attachment_upload_enabled"
# Search for potential missed attachment operations
echo "Searching for potential missed attachment operations..."
rg -i "attachment" -g "!*.md"
Length of output: 16726
| "je_single_credit_line": false, | ||
| "is_attachment_upload_enabled": true |
There was a problem hiding this comment.
Fix JSON formatting
Ensure proper comma separation between properties.
Apply this fix:
"allow_intercompany_vendors": false,
- "je_single_credit_line": false,
+ "je_single_credit_line": false,
"is_attachment_upload_enabled": trueCommittable suggestion skipped: line range outside the PR's diff.
|
Description
Please add PR description here, add screenshots if needed
Clickup
Please add link here
https://app.clickup.com/1864988/v/l/6-901604887942-1
Summary by CodeRabbit
New Features
Bug Fixes
Database Changes
Tests