Invoice create/review form fixes

commcare_connect/opportunity/forms.py

@@ -14,7 +14,7 @@
 from django.template.loader import render_to_string
 from django.urls import reverse
 from django.utils.html import format_html
-from django.utils.timezone import now
+from django.utils.timezone import localdate, now
 from django.utils.translation import gettext
 from django.utils.translation import gettext_lazy as _
 from waffle import switch_is_active
@@ -59,7 +59,10 @@
     get_start_date_for_invoice,
 )
 from commcare_connect.opportunity.utils.invoice_export import get_exportable_invoices
-from commcare_connect.opportunity.utils.invoice_line_items import bill_invoice
+from commcare_connect.opportunity.utils.invoice_line_items import (
+    bill_invoice,
+    get_billable_line_items,
+)
 from commcare_connect.organization.models import Organization
 from commcare_connect.program.helpers import eligible_supervising_organizations
 from commcare_connect.program.models import ProgramApplicationStatus
@@ -1756,6 +1759,7 @@ def __init__(self, *args, **kwargs):
         self.is_opportunity_pm = kwargs.pop("is_opportunity_pm")
 
         super().__init__(*args, **kwargs)
+        self.fields["end_date"].widget.attrs["max"] = localdate() - datetime.timedelta(days=1)
 
         self.prepare_fields()
 
@@ -1849,7 +1853,13 @@ def get_form_layout(self):
         if not self.read_only:
             invoice_form_fields.append(
                 Div(
-                    Submit("submit", gettext("Submit"), css_class="button button-md primary-dark"),
+                    Submit(
+                        "submit",
+                        gettext("Submit"),
+                        css_class="button button-md primary-dark",
+                        # Disable when there is an error while fetching the line items
+                        **{":disabled": "isServiceDelivery && lineItemsError"},
+                    ),
                     css_class="flex justify-end mt-4",
                 )
             )
@@ -2022,8 +2032,32 @@ def clean(self):
             if end_date < start_date:
                 raise ValidationError({"end_date": "End date cannot be earlier than start date."})
 
+            if end_date >= localdate():
+                raise ValidationError({"end_date": _("End date must be before today.")})
+
+            self._reject_stale_total(amount, start_date, end_date)
+
         return cleaned_data
 
+    def _reject_stale_total(self, amount, start_date, end_date):
+        """Reject a submit if the posted total no longer matches the current billable total.
+
+        This provides a user-facing error when the preview has gone stale. `save()` still
+        recomputes the total under a lock and never trusts the posted amount.
+        """
+
+        # Use the same calculation as the preview, so a mismatch reflects a real state change.
+        billable_total = sum(
+            item.total_pay.local for item in get_billable_line_items(self.opportunity, start_date, end_date)
+        )
+        if amount != billable_total:
+            raise ValidationError(
+                _(
+                    "The billable total for this period has changed since this page was loaded. "
+                    "Please refresh the page to see the latest line items and total, then submit again."
+                )
+            )
+
     def save(self, commit=True):
         instance = super().save(commit=False)
         instance.opportunity = self.opportunity
@@ -2058,39 +2092,17 @@ def is_service_delivery(self):
 
     @property
     def line_items(self):
-        if self.line_items_table:
-            table = HTML(
-                """
-                {% load django_tables2 %}
-                <div class="overflow-x-auto mb-4">
-                    {% render_table form.line_items_table %}
-                </div>
-                """
-            )
-        else:
-            table = HTML(
-                """
-                <div id="invoice-line-items-wrapper" class="space-y-1 text-sm text-gray-500 mb-4"></div>
-            """
-            )
-
         return Fieldset(
             "Line Items",
-            table,
-            HTML(
-                """
-                <div id="download-line-items-wrapper" x-cloak x-show="showDownloadButton" class="my-4">
-                    <a type="button"
-                    class="button button-md outline-style"
-                    :href="downloadLineItemsUrl()"
-                    target="_blank"
-                    >
-                        <i class="fa-solid fa-download mr-2"></i>
-                        {% load i18n %}{% translate "Download All Items" %}
-                    </a>
-                </div>
-                """
-            ),
+            HTML('{% include "opportunity/partials/invoice_line_items_fieldset.html" %}'),
+        )
+
+    @property
+    def line_items_released(self):
+        """True when line items were released due to cancellation or rejection."""
+        return self.instance.pk is not None and self.instance.status in (
+            InvoiceStatus.CANCELLED_BY_NM,
+            InvoiceStatus.REJECTED_BY_PM,
         )
 
 

commcare_connect/opportunity/tasks.py

@@ -700,14 +700,20 @@ def generate_automated_service_delivery_invoice():
     for opportunity in Opportunity.objects.filter(
         active=True, is_test=False, start_date__gte=OPPORTUNITY_AUTO_INVOICE_START_DATE
     ).iterator(chunk_size=CHUNK_SIZE):
-        window_start = get_start_date_for_invoice(opportunity)
-        # Below indicates there are no unbilled completed works to invoice in previous month or earlier
-        if window_start > end_date_prev_month:
-            continue
+        try:
+            window_start = get_start_date_for_invoice(opportunity)
+            # Below indicates there are no unbilled completed works to invoice in previous month or earlier
+            if window_start > end_date_prev_month:
+                continue
 
-        invoice_id = _bill_opportunity(opportunity, window_start, end_date_prev_month)
-        if invoice_id:
-            created_invoices_ids.append(invoice_id)
+            invoice_id = _bill_opportunity(opportunity, window_start, end_date_prev_month)
+            if invoice_id:
+                created_invoices_ids.append(invoice_id)
+        except Exception:
+            logger.exception(
+                "Automated invoicing failed for Opportunity %s (%s)", opportunity.name, opportunity.opportunity_id
+            )
+            continue
 
     _send_auto_invoice_created_notification(created_invoices_ids)
 

commcare_connect/opportunity/tests/test_forms.py

@@ -37,6 +37,7 @@
     CredentialConfiguration,
     OpportunityActiveEvent,
     OpportunitySupervisingOrganizationEvent,
+    PaymentInvoice,
     PaymentUnit,
     TaskTypeModeChoices,
 )
@@ -790,48 +791,119 @@ def test_non_service_delivery_form(self, mock_bill_invoice, valid_opportunity):
         assert invoice.title is None
         mock_bill_invoice.assert_not_called()
 
-    @patch("commcare_connect.opportunity.forms.bill_invoice")
-    def test_service_delivery_form(self, mock_bill_invoice, valid_opportunity):
+    def test_service_delivery_invoice_snapshots_its_line_items(self, valid_opportunity):
         ExchangeRateFactory(rate_date="2020-01-01")
+        payment_unit = PaymentUnitFactory(opportunity=valid_opportunity, amount=100, org_amount=20)
+        cw = CompletedWorkFactory(
+            opportunity_access__opportunity=valid_opportunity,
+            payment_unit=payment_unit,
+            status=CompletedWorkStatus.approved,
+            saved_approved_count=1,
+            invoiced_approved_count=0,
+        )
+        cw.status_modified_date = datetime.date(2025, 10, 5)
+        cw.save()
 
         form = AutomatedPaymentInvoiceForm(
             opportunity=valid_opportunity,
             invoice_type="service_delivery",
             data={
                 "invoice_number": "INV-001",
-                "amount": 100.0,
+                "amount": 120.0,
                 "date": "2025-11-06",
                 "title": "Consulting Services Invoice",
-                "start_date": "2025-10-01",
+                # Deliberately wider than the single month that has billable work, so the
+                # assertions below prove the posted window survives rather than collapsing onto
+                # the month that happened to be billed.
+                "start_date": "2025-09-01",
                 "end_date": "2025-10-31",
                 "description": "Monthly consulting services rendered.",
             },
             is_opportunity_pm=False,
         )
+
         assert form.is_valid()
         invoice = form.save()
-        assert invoice.service_delivery
-        assert str(invoice.start_date) == "2025-10-01"
-        assert str(invoice.end_date) == "2025-10-31"
-        assert invoice.description == "Monthly consulting services rendered."
 
-        mock_bill_invoice.assert_called_once()
+        row = CompletedWorkInvoice.objects.get(invoice=invoice)
+        assert row.completed_work_id == cw.id
+        assert row.billed_count == 1
+        assert row.month == datetime.date(2025, 10, 1)
+        assert row.flw_amount_local == Decimal("100")
+        assert row.org_amount_local == Decimal("20")
 
-    @patch("commcare_connect.opportunity.forms.bill_invoice", return_value=[])
-    def test_service_delivery_amount_is_never_the_posted_one(self, mock_bill_invoice, valid_opportunity):
-        """The posted total is a preview artefact and must never survive onto the invoice.
+        invoice.refresh_from_db()
+        assert invoice.amount == Decimal("120")  # server-computed from the frozen rows
+        # The window is the NM's input and is never narrowed to the months that were billable.
+        assert invoice.start_date == datetime.date(2025, 9, 1)
+        assert invoice.end_date == datetime.date(2025, 10, 31)
 
-        bill_invoice is patched to return [] to stand in for the delta being billed
-        elsewhere between clean() and save() -- the race no validation can close. The invoice must
-        then be 0 rather than the 100 that was posted, so it never claims money it has no rows for.
-        """
+        cw.refresh_from_db()
+        assert cw.invoiced_approved_count == 1
+
+    @pytest.mark.parametrize(
+        "start_date, end_date, expected_error",
+        [
+            ("2025-10-01", "2025-10-31", None),
+            ("2025-10-31", "2025-10-01", "End date cannot be earlier than start date."),
+            ("2025-10-01", "2025-11-06", "End date must be before today."),
+            ("2025-10-01", "2025-11-30", "End date must be before today."),
+        ],
+        ids=["completed-period", "end-before-start", "ends-today", "future-end"],
+    )
+    @patch("commcare_connect.opportunity.forms.localdate", return_value=datetime.date(2025, 11, 6))
+    def test_service_delivery_window_must_be_a_completed_period(
+        self, mock_localdate, valid_opportunity, start_date, end_date, expected_error
+    ):
         ExchangeRateFactory(rate_date=datetime.date(2020, 1, 1))
+
         form = AutomatedPaymentInvoiceForm(
             opportunity=valid_opportunity,
             invoice_type="service_delivery",
             data={
-                "invoice_number": "INV-STALE",
-                "amount": 100.0,
+                "invoice_number": "INV-WINDOW",
+                "amount": 0,
+                "date": "2025-11-06",
+                "start_date": start_date,
+                "end_date": end_date,
+                "description": "Monthly consulting services rendered.",
+            },
+            is_opportunity_pm=False,
+        )
+
+        assert form.is_valid() is (expected_error is None)
+        if expected_error:
+            assert expected_error in str(form.errors)
+
+    @pytest.mark.parametrize(
+        "posted_amount, expect_valid",
+        [(120.0, True), (100.0, False), (0, False)],
+        ids=["matches", "stale-lower", "stale-zero"],
+    )
+    def test_service_delivery_rejects_a_total_that_no_longer_matches_the_window(
+        self, valid_opportunity, posted_amount, expect_valid
+    ):
+        """The amount is server-filled from the preview, so a mismatch means the deliveries moved.
+
+        Saving anyway would hand the NM an invoice for a figure they never reviewed.
+        """
+        ExchangeRateFactory(rate_date=datetime.date(2020, 1, 1), currency_code="USD", rate=1.0)
+        payment_unit = PaymentUnitFactory(opportunity=valid_opportunity, amount=100, org_amount=20)
+        cw = CompletedWorkFactory(
+            opportunity_access__opportunity=valid_opportunity,
+            payment_unit=payment_unit,
+            status=CompletedWorkStatus.approved,
+            saved_approved_count=1,
+        )
+        cw.status_modified_date = datetime.date(2025, 10, 5)
+        cw.save()
+
+        form = AutomatedPaymentInvoiceForm(
+            opportunity=valid_opportunity,
+            invoice_type="service_delivery",
+            data={
+                "invoice_number": "INV-001",
+                "amount": posted_amount,
                 "date": "2025-11-06",
                 "start_date": "2025-10-01",
                 "end_date": "2025-10-31",
@@ -840,25 +912,26 @@ def test_service_delivery_amount_is_never_the_posted_one(self, mock_bill_invoice
             is_opportunity_pm=False,
         )
 
-        assert form.is_valid()
-        invoice = form.save()
+        assert form.is_valid() is expect_valid
+        if not expect_valid:
+            assert "The billable total for this period has changed since this page was loaded." in str(form.errors)
+            assert not PaymentInvoice.objects.filter(invoice_number="INV-001").exists()
 
-        invoice.refresh_from_db()
-        assert invoice.amount == Decimal("0")
-        assert invoice.amount_usd == Decimal("0")
-        assert not invoice.work_items.exists()
+    @patch("commcare_connect.opportunity.forms.bill_invoice", return_value=[])
+    def test_service_delivery_amount_is_never_the_posted_one(self, mock_bill_invoice, valid_opportunity):
+        """The posted total is a preview artefact and must never survive onto the invoice.
 
-    def test_service_delivery_invoice_snapshots_its_line_items(self, valid_opportunity):
-        # Explicit date: the factory default is Faker("date_time"), which can land after the
-        # billed month and miss `latest_exchange_rate`'s rate_date filter intermittently.
+        bill_invoice is patched to return [] to stand in for the delta being billed
+        elsewhere between clean() and save() -- the race no validation can close. The invoice must
+        then be 0 rather than the 120 that was posted, so it never claims money it has no rows for.
+        """
         ExchangeRateFactory(rate_date=datetime.date(2020, 1, 1))
         payment_unit = PaymentUnitFactory(opportunity=valid_opportunity, amount=100, org_amount=20)
         cw = CompletedWorkFactory(
             opportunity_access__opportunity=valid_opportunity,
             payment_unit=payment_unit,
             status=CompletedWorkStatus.approved,
             saved_approved_count=1,
-            invoiced_approved_count=0,
         )
         cw.status_modified_date = datetime.date(2025, 10, 5)
         cw.save()
@@ -867,14 +940,12 @@ def test_service_delivery_invoice_snapshots_its_line_items(self, valid_opportuni
             opportunity=valid_opportunity,
             invoice_type="service_delivery",
             data={
-                "invoice_number": "INV-001",
-                "amount": 100.0,
+                "invoice_number": "INV-STALE",
+                # Matches the window at validation time, so clean() passes; the patched writer then
+                # reports no delta, as if it had been billed elsewhere in between.
+                "amount": 120.0,
                 "date": "2025-11-06",
-                "title": "Consulting Services Invoice",
-                # Deliberately wider than the single month that has billable work, so the
-                # assertions below prove the posted window survives rather than collapsing onto
-                # the month that happened to be billed.
-                "start_date": "2025-09-01",
+                "start_date": "2025-10-01",
                 "end_date": "2025-10-31",
                 "description": "Monthly consulting services rendered.",
             },
@@ -884,21 +955,10 @@ def test_service_delivery_invoice_snapshots_its_line_items(self, valid_opportuni
         assert form.is_valid()
         invoice = form.save()
 
-        row = CompletedWorkInvoice.objects.get(invoice=invoice)
-        assert row.completed_work_id == cw.id
-        assert row.billed_count == 1
-        assert row.month == datetime.date(2025, 10, 1)
-        assert row.flw_amount_local == Decimal("100")
-        assert row.org_amount_local == Decimal("20")
-
         invoice.refresh_from_db()
-        assert invoice.amount == Decimal("120")  # server-computed from the rows, not the posted 100
-        # The window is the NM's input and is never narrowed to the months that were billable.
-        assert invoice.start_date == datetime.date(2025, 9, 1)
-        assert invoice.end_date == datetime.date(2025, 10, 31)
-
-        cw.refresh_from_db()
-        assert cw.invoiced_approved_count == 1
+        assert invoice.amount == Decimal("0")
+        assert invoice.amount_usd == Decimal("0")
+        assert not invoice.work_items.exists()
 
     def test_readonly_form_initialization(self, valid_opportunity):
         invoice = PaymentInvoiceFactory(