fix(data): raise clear error instead of crashing on missing invoice dates

[Deez-Automations]

Summary

Fixes #290, fixes #291.

Invoice.to_dict() calls .isoformat() directly on invoice_date and due_date with no null guard. Both columns are NOT NULL in the schema, so this is only reachable via corrupted or anomalous data (e.g. a raw SQL write bypassing the ORM constraint) — but when it happens, it crashed with an opaque AttributeError: 'NoneType' object has no attribute 'isoformat' instead of a diagnosable error. Since get_invoice_for_payment (finbot/tools/data/payment.py) calls to_dict() directly, the crash propagated straight into payment processing with no useful message.

Fix

Raises a clear ValueError naming the invoice ID and the specific missing field, instead of crashing with an unhandled AttributeError. Checked that no code anywhere in the codebase catches AttributeError around a to_dict() call, so nothing depends on the old crash type/behavior.

Comparison against another open fix for the same issue

Another contributor has an independent, already-open PR (#293) for the same two issues. Worth comparing openly since both are unmerged:

Both issues' own Expected behavior and Acceptance criteria explicitly say: "ValueError raised with a meaningful message" / "test_pay_field_001 raises ValueError, not AttributeError." This PR does exactly that. PR #293 takes a different approach — it returns None for the missing field instead of raising anything, so the invoice silently serializes with a missing date and flows onward into payment processing rather than surfacing as an error.

That's not necessarily wrong as a general design choice, but it doesn't satisfy what the linked issues themselves ask for, and for a payment pipeline specifically, a loud, diagnosable failure on corrupted data seems safer than a quiet one — silently treating "date field missing" as normal risks masking a real data-integrity problem rather than surfacing it for review. Flagging this for whoever merges, not to relitigate #293, just so the choice between the two approaches is visible.

Test plan

• New test file tests/unit/data/test_invoice_model.py — reproduces both crashes first (confirmed failing against the unfixed code, exact AttributeError matching both issue reports), then confirms the fix
• Regression test confirms invoices with valid dates are completely unaffected
• pytest tests/unit/data/test_invoice_model.py — 3/3 passing

[Copilot]

Pull request overview

This PR hardens invoice serialization in the data layer by turning a previously opaque AttributeError (from calling .isoformat() on missing dates) into a clear, diagnosable ValueError, preventing confusing crashes in payment processing paths that rely on Invoice.to_dict().

Changes:

• Add explicit None guards for invoice_date and due_date in Invoice.to_dict() that raise ValueError with invoice ID + missing field name.
• Add unit tests covering both missing-date cases and a regression case where both dates are present.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File	Description
finbot/core/data/models.py	Adds explicit validation for missing invoice date fields before serializing to dict.
tests/unit/data/test_invoice_model.py	Introduces targeted unit coverage for the new ValueError behavior and a regression test for valid dates.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.