Custom invoice form updates

[Charl1996]

Product Description

The custom invoice now have additional input fields:

1. Date of expense incurred: the date the expense was made.
2. Justification: a mandatory description.

Technical Summary

Ticket: https://dimagi.atlassian.net/browse/CCCT-1965

Couple things:

1. The AutomatedPaymentInvoiceForm was refactored to more clearly split up the composition of the custom vs service delivery form fields' layout.
2. The notes field on the PaymentInvoice model was renamed to description since both the service delivery invoice and custom invoice now have a field representing essentially a description. (service delivery has "Notes" and custom has "Justification").
3. No migration is added to migrate the notes to description, as all new invoice changes sits behind a switch on production, so there's no data there to migrate.

How the "Date of expense" relates to the currency

The idea is that you're able to specify the date of the expense, but since the amount specified can be tied to a currency other than USD, we simply use "today's rate" for the conversion, lest users game the system and submit expenses for a date in the past when the conversion works in their favour.

Since we're using today's rate, this works conveniently in our case since the rate is fetched using the date input, which, with this PR, we're autofilling to today as a readonly input, so no additional work required there.

Safety Assurance

Safety story

Tested locally

Automated test coverage

No new tests as I think the current tests suffice.

QA Plan

QA planned

Labels & Review

• The set of people pinged as reviewers is appropriate for the level of risk of the change

[coderabbitai]

Walkthrough

This PR introduces a data model restructuring for payment invoices and corresponding form refactoring. The PaymentInvoice model's notes field is renamed to description, and a new date_of_expense field is added. The AutomatedPaymentInvoiceForm is refactored to incorporate these fields, with field preparation logic centralized in a prepare_fields method and form layout construction delegated to a new get_form_layout method. The form conditionally requires date_of_expense for service-delivery invoices and separates field grouping logic for service-delivery versus custom invoices. Test cases are updated to align with the field changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Form refactoring complexity: The AutomatedPaymentInvoiceForm undergoes significant structural changes with multiple new methods (prepare_fields, get_form_layout, service_delivery_invoice_fields, custom_invoice_fields) that require verification for correct field grouping and conditional logic.
• Migration and model changes: Verify that field renaming (notes → description) and addition (date_of_expense) align across model, migration, and form logic.
• Validation logic: The conditional requirement of date_of_expense for service-delivery invoices requires careful review to ensure the logic in prepare_fields correctly identifies the invoice type and applies the constraint.
• Test coverage comprehensiveness: Multiple test updates spanning service-delivery and custom invoice scenarios need verification to ensure all paths are correctly exercised with the new fields.

Possibly related PRs

• Make service delivery invoice generation date non-editable #895: Modifies AutomatedPaymentInvoiceForm's date handling by making the generation date widget readonly on creation for service-delivery invoices.
• Invoice Review #882: Updates AutomatedPaymentInvoiceForm field handling and layout initialization behavior, overlapping with this PR's form refactoring.
• Cs/ccct 1879 update invoice form #836: Modifies AutomatedPaymentInvoiceForm and PaymentInvoice fields, layout, and service-delivery-specific behavior.

Suggested reviewers

• calellowitz
• pxwxnvermx
• zandre-eng
• ajeety4

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Custom invoice form updates' accurately describes the main change—enhancing custom invoice forms with new fields and refactoring the form structure.
Description check	✅ Passed	The description is clearly related to the changeset, providing product context, technical rationale, and safety assurance aligned with the code modifications.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch cs/CCCT-1965-update-custom-invoice-form

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

commcare_connect/opportunity/forms.py (1)

1703-1724: Eliminate duplicate exchange rate fetching.

The exchange rate is fetched twice for non-service-delivery invoices (lines 1704 and 1715-1717). The first fetch assigns to cleaned_data["exchange_rate"] and the second fetch is redundant.

🔎 Apply this diff to remove the duplication:

         if not self.is_service_delivery:
             exchange_rate = ExchangeRate.latest_exchange_rate(self.opportunity.currency, date)
             if not exchange_rate:
                 raise ValidationError("Exchange rate not available for selected date.")
 
             cleaned_data["exchange_rate"] = exchange_rate
-            cleaned_data["amount_usd"] = round(amount / exchange_rate.rate, 2)
 
             cleaned_data["title"] = None
             cleaned_data["start_date"] = None
             cleaned_data["end_date"] = None
 
-            exchange_rate = ExchangeRate.latest_exchange_rate(self.opportunity.currency, date)
-            if not exchange_rate:
-                raise ValidationError("Exchange rate not available for selected date.")
-
             if cleaned_data.get("usd_currency"):
                 cleaned_data["amount_usd"] = amount
                 cleaned_data["amount"] = round(amount * exchange_rate.rate, 2)
             else:
                 cleaned_data["amount"] = amount
                 cleaned_data["amount_usd"] = round(amount / exchange_rate.rate, 2)

🧹 Nitpick comments (1)

commcare_connect/opportunity/forms.py (1)

1500-1504: Good refactoring separating field preparation from layout construction.

The introduction of prepare_fields() and get_form_layout() methods improves code organization and makes it easier to maintain different invoice type layouts.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c2f342f and 1aa8add.

📒 Files selected for processing (4)

• commcare_connect/opportunity/forms.py (5 hunks)
• commcare_connect/opportunity/migrations/0089_rename_notes_paymentinvoice_description_and_more.py (1 hunks)
• commcare_connect/opportunity/models.py (1 hunks)
• commcare_connect/opportunity/tests/test_forms.py (5 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

commcare_connect/opportunity/forms.py (1)

commcare_connect/opportunity/models.py (12)

• Meta (247-248)
• Meta (265-270)
• Meta (371-376)
• Meta (451-454)
• Meta (486-487)
• Meta (551-552)
• Meta (750-758)
• Meta (775-778)
• Meta (817-821)
• Meta (861-862)
• Meta (875-876)
• PaymentInvoice (468-487)

commcare_connect/opportunity/tests/test_forms.py (1)

commcare_connect/opportunity/forms.py (7)

• save (272-290)
• save (487-530)
• save (621-657)
• save (944-959)
• save (1063-1065)
• save (1423-1432)
• save (1739-1753)

🪛 Ruff (0.14.8)

commcare_connect/opportunity/migrations/0089_rename_notes_paymentinvoice_description_and_more.py

7-9: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

---

11-22: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: pytest

🔇 Additional comments (11)

commcare_connect/opportunity/models.py (1)

483-484: LGTM!

The field rename from notes to description and the addition of date_of_expense are correctly implemented with appropriate null/blank settings.

commcare_connect/opportunity/migrations/0089_rename_notes_paymentinvoice_description_and_more.py (1)

1-22: Migration operations are correct.

The field rename and addition operations are properly structured. The static analysis hints about ClassVar annotations are false positives—Django migration operations are not class attributes that need ClassVar typing.

Note on PR description: The PR objectives state "No DB migration provided to move notes → description because changes are behind a production switch and there is no data to migrate," but this migration file performs exactly that rename. Please clarify whether this migration is intended or if the PR description needs updating.

commcare_connect/opportunity/tests/test_forms.py (4)

596-601: LGTM!

Test data correctly updated to use description instead of the old notes field.

---

615-624: LGTM!

Test properly validates the new custom invoice flow with both description (mandatory) and date_of_expense fields.

---

640-650: LGTM!

Test correctly verifies that custom invoices do not save start_date, end_date, or title fields, confirming the field grouping logic works as expected.

---

666-676: LGTM!

Service delivery invoice test properly validates that description is saved and start_date/end_date are preserved for this invoice type.

commcare_connect/opportunity/forms.py (5)

1457-1466: LGTM!

The new date_of_expense and description fields are properly defined with appropriate widgets and labels.

---

1470-1480: LGTM!

Meta.fields correctly updated to include all relevant fields including the new description and date_of_expense.

---

1506-1550: LGTM!

Field preparation logic correctly:

• Makes date readonly for non-readonly forms (line 1511)
• Sets appropriate labels and placeholders for service delivery vs custom invoices
• Makes description and date_of_expense required for custom invoices (lines 1543, 1550)

---

1578-1677: LGTM on layout refactoring!

The separation of service_delivery_invoice_fields and custom_invoice_fields properties provides clear, maintainable layout definitions for each invoice type.

---

1739-1753: LGTM!

The save() method correctly persists the new date_of_expense field and maintains the existing logic for linking service delivery invoices to completed works.

[zandre-eng]

Generally looks good, just a few minor comments.

No migration is added to migrate the notes to description, as all new invoice changes sits behind a switch on production, so there's no data there to migrate.

Curious about this statement in the description. My understanding is that we are simply renaming the notes column to description which wouldn't require any sort of data migration.

[zandre-eng]

If we are using a currency other than USD, would we want to enforce using today's date for this field? This could give better feedback to the user that we're enforcing the exchange rate date.

[zandre-eng]

If we're making the description a required field we should do a check in clean() to make sure that users aren't just adding empty spaces to fill this requirement.