feat: Add Receipt URLs to DB #643
Conversation
ruuushhh
had a problem deploying
to
CI Environment
GitHub Actions
Failure
ruuushhh
had a problem deploying
to
CI Environment
GitHub Actions
Failure
WalkthroughThe changes introduce new fields to four model classes in the Changes
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 (
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (5)
apps/netsuite/models.py (4)
381-382: LGTM! Consider adding a docstring for the new fields.The addition of
receipt_urlsandis_attachment_upload_failedfields aligns well with the PR objectives. The field types and options are appropriate for their intended use.Consider adding a brief docstring to explain the structure of the
receipt_urlsJSONField, e.g., what keys and values are expected. This would improve code maintainability.
585-586: LGTM! Consider adding a docstring for consistency.The addition of
receipt_urlsandis_attachment_upload_failedfields is consistent with the changes in the Bill class and aligns with the PR objectives.For consistency with the suggestion for the Bill class, consider adding a brief docstring to explain the structure of the
receipt_urlsJSONField.
803-804: LGTM! Consider adding a docstring for consistency.The addition of
receipt_urlsandis_attachment_upload_failedfields is consistent with the changes in the other classes and aligns with the PR objectives.For consistency with the suggestions for the other classes, consider adding a brief docstring to explain the structure of the
receipt_urlsJSONField.
1034-1035: LGTM! Consider adding a docstring for consistency.The addition of
receipt_urlsandis_attachment_upload_failedfields is consistent with the changes in the other classes and completes the implementation across all relevant classes.For consistency with the suggestions for the other classes, consider adding a brief docstring to explain the structure of the
receipt_urlsJSONField.apps/netsuite/tasks.py (1)
566-567: Check for emptyattachment_linksbefore saving
attachment_linksmight be empty if no attachments were uploaded. Saving an empty JSON may not be necessary.Consider adding a condition:
if attachment_links: credit_card_charge_object.receipt_urls = json.dumps(attachment_links) credit_card_charge_object.save()
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/netsuite/models.py (4 hunks)
- apps/netsuite/tasks.py (6 hunks)
Additional comments not posted (1)
apps/netsuite/models.py (1)
381-382: Verify usage of new fields in related code.The addition of
receipt_urlsandis_attachment_upload_failedfields is consistent across all modified classes (Bill, CreditCardCharge, ExpenseReport, and JournalEntry) and aligns well with the PR objectives.To ensure these new fields are properly utilized, please verify that related parts of the codebase (e.g., views, serializers, and any business logic handling these models) are updated to work with these new fields. Run the following script to check for potential usage:
Also applies to: 585-586, 803-804, 1034-1035
| credit_card_charge_object.is_attachment_upload_failed = True | ||
| credit_card_charge_object.save() |
There was a problem hiding this comment.
Refactor duplicate code in exception handlers
The lines setting credit_card_charge_object.is_attachment_upload_failed = True and saving the object are duplicated in both exception handlers. This violates the DRY (Don't Repeat Yourself) principle.
Consider refactoring the code to reduce duplication. One approach is to handle both exceptions together:
except (InvalidTokenError, Exception) as e:
logger.info(
'Attachment failed for expense group id %s / workspace id %s Error: %s',
expense.expense_id, workspace_id, {'error': str(e)}
)
credit_card_charge_object.is_attachment_upload_failed = True
credit_card_charge_object.save()Also applies to: 163-164
| task_model.is_attachment_upload_failed = True | ||
| task_model.save() |
There was a problem hiding this comment.
Consolidate duplicated exception handling code
The code setting task_model.is_attachment_upload_failed = True and saving it is duplicated in multiple exceptions. This can be refactored to improve maintainability.
You could set the flag once after the exceptions:
try:
# Existing code
except (NetSuiteRateLimitError, NetSuiteRequestError, NetSuiteLoginError, InvalidTokenError) as exception:
logger.info('Error while uploading attachments to NetSuite workspace_id - %s %s', workspace_id, exception.__dict__)
error_occurred = True
except Exception as exception:
logger.error(
'Error while uploading attachments to NetSuite workspace_id - %s %s %s',
workspace_id, exception, traceback.format_exc()
)
error_occurred = True
if error_occurred:
task_model.is_attachment_upload_failed = True
task_model.save()Also applies to: 437-438
apps/netsuite/tasks.py
Outdated
| task_model = TASK_TYPE_MODEL_MAP[task_log.type] | ||
| task_model = task_model.objects.get(expense_group_id=task_log.expense_group_id) | ||
|
|
There was a problem hiding this comment.
Avoid variable shadowing for better readability
At line 381, task_model is assigned a model class, and then at line 382, it's reassigned to an instance of that model. This shadowing can be confusing.
Consider using distinct variable names:
-task_model = TASK_TYPE_MODEL_MAP[task_log.type]
-task_model = task_model.objects.get(expense_group_id=task_log.expense_group_id)
+task_model_class = TASK_TYPE_MODEL_MAP[task_log.type]
+task_model = task_model_class.objects.get(expense_group_id=task_log.expense_group_id)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.
| task_model = TASK_TYPE_MODEL_MAP[task_log.type] | |
| task_model = task_model.objects.get(expense_group_id=task_log.expense_group_id) | |
| task_model_class = TASK_TYPE_MODEL_MAP[task_log.type] | |
| task_model = task_model_class.objects.get(expense_group_id=task_log.expense_group_id) |
| TASK_TYPE_MODEL_MAP = { | ||
| 'CREATING_EXPENSE_REPORT': ExpenseReport, | ||
| 'CREATING_BILL': Bill, | ||
| 'CREATING_JOURNAL_ENTRY': JournalEntry | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential KeyError due to missing 'CREATING_CREDIT_CARD_CHARGE' in TASK_TYPE_MODEL_MAP
The TASK_TYPE_MODEL_MAP dictionary does not include the 'CREATING_CREDIT_CARD_CHARGE' key, which is used elsewhere and could lead to a KeyError when task_log.type is 'CREATING_CREDIT_CARD_CHARGE'.
Consider adding the missing key to TASK_TYPE_MODEL_MAP:
TASK_TYPE_MODEL_MAP = {
'CREATING_EXPENSE_REPORT': ExpenseReport,
'CREATING_BILL': Bill,
'CREATING_JOURNAL_ENTRY': JournalEntry,
+ 'CREATING_CREDIT_CARD_CHARGE': CreditCardCharge
}Committable suggestion was skipped due to low confidence.
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
tests/test_netsuite/test_tasks.py (1)
827-827: Improve test_load_attachments functionThe changes to the
test_load_attachmentsfunction look good overall. However, there are a few minor improvements we can make:
On line 863-864, we're creating a
credit_card_charge_objectbut not using it immediately. Consider moving this closer to where it's first used.The assertion on line 888 uses
==for comparison withTrue. It's generally preferred to useisfor boolean comparisons.Here's a suggested refactor for the boolean comparison:
- assert credit_card_charge_object.is_attachment_upload_failed == True + assert credit_card_charge_object.is_attachment_upload_failed is TrueAlso, consider moving the
credit_card_charge_objectcreation closer to its first usage for better readability.Also applies to: 863-864, 868-868, 882-882, 887-888
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- apps/netsuite/migrations/0027_auto_20240924_0614.py (1 hunks)
- apps/netsuite/tasks.py (6 hunks)
- tests/sql_fixtures/reset_db_fixtures/reset_db.sql (10 hunks)
- tests/test_netsuite/test_tasks.py (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- apps/netsuite/tasks.py
Additional context used
Ruff
tests/test_netsuite/test_tasks.py
883-883: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
888-888: Avoid equality comparisons to
True; useif ...:for truth checksReplace comparison
(E712)
Additional comments not posted (15)
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (11)
228-230: LGTM: New columns added to 'bills' tableThe addition of
is_attachment_upload_failed(boolean) andreceipt_urls(jsonb) columns to the 'bills' table aligns well with the PR objectives. The data types are appropriate, and the NOT NULL constraint onis_attachment_upload_failedensures data integrity.
399-401: LGTM: Consistent changes applied to 'credit_card_charges' tableThe addition of
is_attachment_upload_failed(boolean) andreceipt_urls(jsonb) columns to the 'credit_card_charges' table is consistent with the changes made to the 'bills' table. This consistency in schema changes across related tables is commendable.
1073-1075: LGTM: Consistent schema changes in 'expense_reports' tableThe addition of
is_attachment_upload_failed(boolean) andreceipt_urls(jsonb) columns to the 'expense_reports' table maintains consistency with the changes made to the 'bills' and 'credit_card_charges' tables. This uniform approach across tables is good for maintaining a coherent database schema.
1529-1531: LGTM: Consistent schema changes maintained across all tablesThe addition of
is_attachment_upload_failed(boolean) andreceipt_urls(jsonb) columns to the 'journal_entries' table completes the consistent application of these changes across all relevant tables (bills, credit_card_charges, expense_reports, and journal_entries). This uniformity in schema modifications is excellent for maintaining a coherent and easily understandable database structure.
2675-2675: LGTM: COPY statement updated for 'bills' tableThe COPY statement for the 'bills' table has been correctly updated to include the newly added columns
is_attachment_upload_failedandreceipt_urls. This ensures that these new fields will be properly handled in any data import or export operations.
2715-2715: LGTM: COPY statement consistently updated for 'credit_card_charges' tableThe COPY statement for the 'credit_card_charges' table has been correctly updated to include the newly added columns
is_attachment_upload_failedandreceipt_urls. This change is consistent with the update made to the 'bills' table COPY statement, maintaining uniformity in data handling across tables.
7987-7987: LGTM: Migration entry added for schema changesA new migration entry has been added for 'netsuite' with the identifier '0027_auto_20240924_0614'. This entry corresponds to the schema changes made in this PR, and the timestamp in the identifier (20240924) is consistent with the expected timeframe for these modifications. This ensures proper tracking and versioning of the database schema changes.
11621-11621: LGTM: COPY statement consistently updated for 'expense_reports' tableThe COPY statement for the 'expense_reports' table has been correctly updated to include the newly added columns
is_attachment_upload_failedandreceipt_urls. This change maintains consistency with the updates made to the 'bills' and 'credit_card_charges' COPY statements, ensuring uniform data handling across all affected tables.
11672-11672: LGTM: Consistent COPY statement updates across all affected tablesThe COPY statement for the 'journal_entries' table has been correctly updated to include the newly added columns
is_attachment_upload_failedandreceipt_urls. This change completes the consistent update of COPY statements across all affected tables (bills, credit_card_charges, expense_reports, and journal_entries). The uniformity in these changes is commendable and ensures consistent data handling throughout the database.
11899-11899: LGTM: Migration sequence updated correctlyThe sequence for 'django_migrations_id_seq' has been properly updated to 198. This update corresponds to the new migration entry (0027_auto_20240924_0614) added earlier and ensures that future migrations will have correct, incremental IDs. This is crucial for maintaining the integrity of the migration history and preventing potential conflicts in future database schema updates.
Line range hint
228-11899: Summary: Comprehensive and consistent schema updates for receipt URL storage and upload failure trackingThis PR implements a set of well-structured and consistent changes across the database schema to support the storage of receipt URLs and tracking of attachment upload failures. Key points:
- New columns (
is_attachment_upload_failedandreceipt_urls) added consistently to bills, credit_card_charges, expense_reports, and journal_entries tables.- COPY statements for all affected tables updated to include the new columns, ensuring proper data handling in import/export operations.
- A new migration (0027_auto_20240924_0614) added and the migration sequence updated accordingly.
These changes effectively fulfill the PR objectives while maintaining data integrity and consistency across the schema. The uniform approach to these modifications will facilitate easier maintenance and understanding of the database structure in the future.
apps/netsuite/migrations/0027_auto_20240924_0614.py (2)
1-12: LGTM: Migration structure is correctThe migration file is properly structured with the correct Django version, dependencies, and migration class definition. This follows Django's migration conventions.
1-53: Consider data migration and indexingWhile the schema changes look good, there are two additional points to consider:
Data Migration: This migration adds new fields but doesn't include any data migration. Depending on your application logic, you might need to populate these fields with appropriate default values for existing records.
Indexing: If you expect to frequently query on the
is_attachment_upload_failedfield, consider adding an index to improve query performance.To help assess the need for these changes, you can run the following commands:
Based on the results, consider if a data migration is necessary and if adding an index would be beneficial.
tests/test_netsuite/test_tasks.py (2)
638-638: LGTM: Added assertion for receipt_urls in CreditCardCharge objectThe added assertion checks if the
receipt_urlsattribute of theCreditCardChargeobject is correctly set. This is a good addition to ensure that the attachment URL is properly associated with the credit card charge.
1628-1629: LGTM: Added assertions for receipt_urls in Bill, JournalEntry, and ExpenseReport objectsThese additions are good. They ensure that the
receipt_urlsattribute is correctly set forBill,JournalEntry, andExpenseReportobjects after the attachment upload process. This helps verify that the attachment URLs are properly associated with their respective objects.Also applies to: 1666-1667, 1704-1705
| migrations.AddField( | ||
| model_name='bill', | ||
| name='is_attachment_upload_failed', | ||
| field=models.BooleanField(default=False, help_text='Is Attachment Upload Failed'), | ||
| ), | ||
| migrations.AddField( | ||
| model_name='bill', | ||
| name='receipt_urls', | ||
| field=models.JSONField(help_text='Expense Receipt URL Map', null=True), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refining field definitions
While the new fields align with the PR objectives, consider the following suggestions:
-
The
is_attachment_upload_failedfield could be more specific. Consider renaming it tohas_receipt_upload_failedfor clarity. -
The
receipt_urlsfield is defined as nullable (null=True). Consider if this is intentional, as it might lead to inconsistencies in data representation. If an empty state is needed, an empty JSON object ({}) might be more appropriate thannull.
Here's a suggested improvement for the bill model (apply similar changes to other models):
migrations.AddField(
model_name='bill',
- name='is_attachment_upload_failed',
+ name='has_receipt_upload_failed',
field=models.BooleanField(default=False, help_text='Is Attachment Upload Failed'),
),
migrations.AddField(
model_name='bill',
name='receipt_urls',
- field=models.JSONField(help_text='Expense Receipt URL Map', null=True),
+ field=models.JSONField(help_text='Expense Receipt URL Map', default=dict),
),Also applies to: 23-32, 33-42, 43-52
| migrations.AddField( | ||
| model_name='bill', | ||
| name='is_attachment_upload_failed', | ||
| field=models.BooleanField(default=False, help_text='Is Attachment Upload Failed'), | ||
| ), | ||
| migrations.AddField( | ||
| model_name='bill', | ||
| name='receipt_urls', | ||
| field=models.JSONField(help_text='Expense Receipt URL Map', null=True), | ||
| ), | ||
| migrations.AddField( | ||
| model_name='creditcardcharge', | ||
| name='is_attachment_upload_failed', | ||
| field=models.BooleanField(default=False, help_text='Is Attachment Upload Failed'), | ||
| ), | ||
| migrations.AddField( | ||
| model_name='creditcardcharge', | ||
| name='receipt_urls', | ||
| field=models.JSONField(help_text='Expense Receipt URL Map', null=True), | ||
| ), | ||
| migrations.AddField( | ||
| model_name='expensereport', | ||
| name='is_attachment_upload_failed', | ||
| field=models.BooleanField(default=False, help_text='Is Attachment Upload Failed'), | ||
| ), | ||
| migrations.AddField( | ||
| model_name='expensereport', | ||
| name='receipt_urls', | ||
| field=models.JSONField(help_text='Expense Receipt URL Map', null=True), | ||
| ), | ||
| migrations.AddField( | ||
| model_name='journalentry', | ||
| name='is_attachment_upload_failed', | ||
| field=models.BooleanField(default=False, help_text='Is Attachment Upload Failed'), | ||
| ), | ||
| migrations.AddField( | ||
| model_name='journalentry', | ||
| name='receipt_urls', | ||
| field=models.JSONField(help_text='Expense Receipt URL Map', null=True), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing for code reuse
The changes are consistent across all four models ('bill', 'creditcardcharge', 'expensereport', and 'journalentry'), which is good for maintainability. However, this repetition might indicate an opportunity for a more DRY (Don't Repeat Yourself) approach.
Consider creating a mixin or abstract base class that includes these fields, and then inherit from it in your models. This would centralize the field definitions and make future changes easier. For example:
from django.db import models
class ReceiptTrackingMixin(models.Model):
has_receipt_upload_failed = models.BooleanField(default=False, help_text='Is Attachment Upload Failed')
receipt_urls = models.JSONField(help_text='Expense Receipt URL Map', default=dict)
class Meta:
abstract = True
# In your models.py
class Bill(ReceiptTrackingMixin, models.Model):
# other fields...
class CreditCardCharge(ReceiptTrackingMixin, models.Model):
# other fields...
# Apply to other models similarlyThis approach would require modifying your models and creating a new migration, but it could simplify future changes and maintain consistency.
Description
Store receipt URLs to db
And note if failed
Clickup
Add Receipt URLs to DB
Summary by CodeRabbit
New Features
Bug Fixes
Tests