Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: fix posted at null #412

Merged
merged 3 commits into from
Nov 20, 2024
Merged

fix: fix posted at null #412

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f0bae4
fix: fix posted at null
anishfyle Nov 20, 2024
4ddea93
fixtures
anishfyle Nov 20, 2024
5c13060
fixtures
anishfyle Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/fyle/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ def create_expense_objects(expenses: List[Dict], workspace_id: int, skip_update:
"file_ids": expense["file_ids"],
"spent_at": expense["spent_at"],
"posted_at": expense["posted_at"],
"is_posted_at_null": expense["is_posted_at_null"],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider using Django's built-in null checking instead of a separate flag field

The is_posted_at_null field seems redundant as Django already provides ways to check if posted_at is null. This additional field could lead to data inconsistency if not properly synchronized with posted_at.

Consider these alternatives:

  1. Use Django's built-in null checking:
# In your queries
expenses = Expense.objects.filter(posted_at__isnull=True)
  1. If you need this for performance reasons, consider adding a database index:
posted_at = models.DateTimeField(
    null=True, 
    help_text="Date when the money is taken from the bank",
    db_index=True  # Add this if you frequently query on null values
)
  1. If you absolutely need this field, add database constraints to ensure consistency:
class Meta:
    constraints = [
        models.CheckConstraint(
            check=models.Q(
                is_posted_at_null=True, 
                posted_at__isnull=True
            ) | models.Q(
                is_posted_at_null=False, 
                posted_at__isnull=False
            ),
            name='posted_at_null_flag_consistency'
        )
    ]

⚠️ Potential issue

Add validation to ensure consistency between posted_at and is_posted_at_null

The is_posted_at_null value is taken directly from the input dictionary without validation against the actual posted_at value. This could lead to data inconsistency.

Add validation before setting the defaults:

 defaults = {
     ...
-    "is_posted_at_null": expense["is_posted_at_null"],
+    "is_posted_at_null": expense["posted_at"] is None,
     "fund_source": SOURCE_ACCOUNT_MAP[
         expense["source_account_type"]
     ],
     ...
 }

Committable suggestion skipped: line range outside the PR's diff.

"fund_source": SOURCE_ACCOUNT_MAP[
expense["source_account_type"]
],
Expand Down
Loading