fix: resolve review issues #181-#187

app/my_practice/tests/test_billing_helpers.py

@@ -0,0 +1,250 @@
+"""Tests for billing_helpers: session-to-InvoiceItem creation utilities."""
+
+from datetime import date
+from decimal import Decimal
+
+from django.test import TestCase
+
+from ..models import Client, Invoice, InvoiceItem, Practice, ServiceType, Session
+from ..utils.billing_helpers import (
+    build_service_type_map,
+    create_invoice_item_for_session,
+    is_session_already_billed,
+    resolve_session_rate,
+)
+
+
+def _make_practice():
+    return Practice.objects.create(
+        name="Billing Test Practice",
+        slug="billing-helpers-test",
+        email="billing@example.com",
+    )
+
+
+def _make_service_types(practice):
+    st60 = ServiceType.objects.create(
+        code="therapy_60",
+        name="60-Min Therapy",
+        default_duration=60,
+        practice=practice,
+    )
+    st90 = ServiceType.objects.create(
+        code="therapy_90",
+        name="90-Min Therapy",
+        default_duration=90,
+        practice=practice,
+    )
+    st_free = ServiceType.objects.create(
+        code="therapy_free",
+        name="Free Consultation",
+        default_duration=20,
+        practice=practice,
+    )
+    return st60, st90, st_free
+
+
+def _make_client(practice):
+    return Client.objects.create(
+        client_code="BH",
+        full_name="Max Mustermann",
+        practice=practice,
+        hourly_rate_60=Decimal("100.00"),
+        hourly_rate_90=Decimal("150.00"),
+    )
+
+
+def _make_invoice(client, status=Invoice.Status.DRAFT):
+    return Invoice.objects.create(
+        practice=client.practice,
+        client=client,
+        invoice_number="BH-1",
+        invoice_date=date.today(),
+        status=status,
+    )
+
+
+def _make_session(client, duration=60):
+    return Session.objects.create(
+        client=client,
+        session_date=date.today(),
+        duration=duration,
+    )
+
+
+class BuildServiceTypeMapTests(TestCase):
+    def setUp(self):
+        self.practice = _make_practice()
+        self.st60, self.st90, self.st_free = _make_service_types(self.practice)
+
+    def test_maps_duration_to_service_type(self):
+        m = build_service_type_map(self.practice)
+        self.assertEqual(m[60], self.st60)
+        self.assertEqual(m[90], self.st90)
+        self.assertEqual(m[20], self.st_free)
+
+    def test_deterministic_on_duplicate_duration(self):
+        # Dict is built in ascending code order; the last entry (higher code) wins.
+        ServiceType.objects.create(
+            code="aaa_therapy_60",
+            name="Earlier code",
+            default_duration=60,
+            practice=self.practice,
+        )
+        m = build_service_type_map(self.practice)
+        # "therapy_60" > "aaa_therapy_60" alphabetically → it wins.
+        self.assertEqual(m[60].code, "therapy_60")
+
+    def test_includes_global_service_types(self):
+        global_st = ServiceType.objects.create(
+            code="zzz_global",
+            name="Global Type",
+            default_duration=45,
+            practice=None,
+        )
+        m = build_service_type_map(self.practice)
+        self.assertEqual(m[45], global_st)
+
+
+class ResolveSessionRateTests(TestCase):
+    def setUp(self):
+        self.practice = _make_practice()
+        self.st60, self.st90, self.st_free = _make_service_types(self.practice)
+        self.client = _make_client(self.practice)
+
+    def test_therapy_free_returns_zero(self):
+        self.assertEqual(resolve_session_rate(self.client, self.st_free), Decimal("0"))
+
+    def test_60min_uses_hourly_rate_60(self):
+        self.assertEqual(resolve_session_rate(self.client, self.st60), Decimal("100.00"))
+
+    def test_90min_uses_hourly_rate_90(self):
+        self.assertEqual(resolve_session_rate(self.client, self.st90), Decimal("150.00"))
+
+    def test_90min_falls_back_to_60_when_90_is_zero(self):
+        # hourly_rate_90 = 0 is falsy → falls back to hourly_rate_60.
+        self.client.hourly_rate_90 = Decimal("0")
+        self.client.save()
+        self.assertEqual(resolve_session_rate(self.client, self.st90), Decimal("100.00"))
+
+
+class IsSessionAlreadyBilledTests(TestCase):
+    def setUp(self):
+        self.practice = _make_practice()
+        self.st60, _, _ = _make_service_types(self.practice)
+        self.client = _make_client(self.practice)
+
+    def test_unbilled_session_returns_false(self):
+        session = _make_session(self.client)
+        self.assertFalse(is_session_already_billed(session))
+
+    def test_session_on_active_invoice_returns_true(self):
+        session = _make_session(self.client)
+        invoice = _make_invoice(self.client)
+        InvoiceItem.objects.create(
+            invoice=invoice,
+            session=session,
+            service_type=self.st60,
+            rate=Decimal("100.00"),
+            quantity=Decimal("1.00"),
+            total=Decimal("100.00"),
+        )
+        self.assertTrue(is_session_already_billed(session))
+
+    def test_session_on_cancelled_invoice_returns_false(self):
+        session = _make_session(self.client)
+        invoice = _make_invoice(self.client, status=Invoice.Status.CANCELLED)
+        InvoiceItem.objects.create(
+            invoice=invoice,
+            session=session,
+            service_type=self.st60,
+            rate=Decimal("100.00"),
+            quantity=Decimal("1.00"),
+            total=Decimal("100.00"),
+        )
+        # Cancelled invoice does not count as billed.
+        self.assertFalse(is_session_already_billed(session))
+
+
+class CreateInvoiceItemForSessionTests(TestCase):
+    def setUp(self):
+        self.practice = _make_practice()
+        self.st60, self.st90, self.st_free = _make_service_types(self.practice)
+        self.client = _make_client(self.practice)
+        self.invoice = _make_invoice(self.client)
+        self.service_type_map = {60: self.st60, 90: self.st90, 20: self.st_free}
+
+    def test_creates_item_for_unbilled_session(self):
+        session = _make_session(self.client, duration=60)
+        item = create_invoice_item_for_session(self.invoice, session, self.service_type_map)
+        self.assertIsNotNone(item)
+        self.assertEqual(item.rate, Decimal("100.00"))
+        self.assertEqual(item.service_type, self.st60)
+
+    def test_returns_none_for_already_billed_session(self):
+        session = _make_session(self.client, duration=60)
+        InvoiceItem.objects.create(
+            invoice=self.invoice,
+            session=session,
+            service_type=self.st60,
+            rate=Decimal("100.00"),
+            quantity=Decimal("1.00"),
+            total=Decimal("100.00"),
+        )
+        item = create_invoice_item_for_session(self.invoice, session, self.service_type_map)
+        self.assertIsNone(item)
+
+    def test_session_on_cancelled_invoice_can_be_rebilled(self):
+        session = _make_session(self.client, duration=60)
+        cancelled_invoice = Invoice.objects.create(
+            practice=self.client.practice,
+            client=self.client,
+            invoice_number="BH-99",
+            invoice_date=date.today(),
+            status=Invoice.Status.CANCELLED,
+        )
+        InvoiceItem.objects.create(
+            invoice=cancelled_invoice,
+            session=session,
+            service_type=self.st60,
+            rate=Decimal("100.00"),
+            quantity=Decimal("1.00"),
+            total=Decimal("100.00"),
+        )
+        item = create_invoice_item_for_session(self.invoice, session, self.service_type_map)
+        self.assertIsNotNone(item)
+
+    def test_returns_none_when_no_service_type(self):
+        session = _make_session(self.client, duration=75)
+        item = create_invoice_item_for_session(self.invoice, session, self.service_type_map)
+        self.assertIsNone(item)
+
+    def test_uses_fallback_service_type(self):
+        session = _make_session(self.client, duration=75)
+        item = create_invoice_item_for_session(
+            self.invoice, session, self.service_type_map, fallback_service_type=self.st60
+        )
+        self.assertIsNotNone(item)
+        self.assertEqual(item.service_type, self.st60)
+
+    def test_therapy_free_creates_zero_rate_item(self):
+        session = _make_session(self.client, duration=20)
+        item = create_invoice_item_for_session(self.invoice, session, self.service_type_map)
+        self.assertIsNotNone(item)
+        self.assertEqual(item.rate, Decimal("0"))
+
+    def test_90min_session_uses_90min_rate(self):
+        session = _make_session(self.client, duration=90)
+        item = create_invoice_item_for_session(self.invoice, session, self.service_type_map)
+        self.assertIsNotNone(item)
+        self.assertEqual(item.rate, Decimal("150.00"))
+
+    def test_item_quantity_is_one(self):
+        session = _make_session(self.client, duration=60)
+        item = create_invoice_item_for_session(self.invoice, session, self.service_type_map)
+        self.assertEqual(item.quantity, Decimal("1.00"))
+
+    def test_item_total_equals_rate(self):
+        session = _make_session(self.client, duration=60)
+        item = create_invoice_item_for_session(self.invoice, session, self.service_type_map)
+        self.assertEqual(item.total, item.rate)

app/my_practice/utils/__init__.py

@@ -7,7 +7,7 @@
 from .action_queue_builder import ActionQueueBuilder
 from .client_detail_builder import ClientDetailContextBuilder
 from .dashboard_context_builder import DashboardContextAssembler
-from .tax_context_builder import TaxYearContextBuilder
+from .tax_context_builder import TaxYearContextBuilder, available_data_years
 from .bank_import import BankStatementImporter
 from .calculations import (
     apply_remainder_distribution,
@@ -43,6 +43,12 @@
 from .financial_list_context_builder import FinancialListContextBuilder
 from .import_helpers import build_client_map
 from .invoice_filter_helper import InvoiceFilterHelper
+from .billing_helpers import (
+    build_service_type_map,
+    create_invoice_item_for_session,
+    is_session_already_billed,
+    resolve_session_rate,
+)
 from .invoice_helpers import get_next_invoice_number
 from .practice_days import FahrtkostenResult, PracticeDayCalculator
 from .practice_helpers import (
@@ -67,6 +73,10 @@
     "count_sessions_rounded",
     "apply_remainder_distribution",
     "get_next_invoice_number",
+    "build_service_type_map",
+    "create_invoice_item_for_session",
+    "is_session_already_billed",
+    "resolve_session_rate",
     "RevenueCalculator",
     "DateRangeHelper",
     "format_month_key",
@@ -90,6 +100,7 @@
     "ClientDetailContextBuilder",
     "DashboardContextAssembler",
     "TaxYearContextBuilder",
+    "available_data_years",
     "FinancialListContextBuilder",
     "InvoiceFilterHelper",
     "AgendaWidgetBuilder",

app/my_practice/utils/billing_helpers.py

@@ -0,0 +1,97 @@
+"""
+Shared helpers for session → InvoiceItem creation.
+
+Three call sites need consistent service-type resolution, rate computation, and
+already-billed detection:
+  1. invoice_views.add_sessions_to_invoice
+  2. invoice_views.create_invoice_with_sessions
+  3. calendar_views (approval flow)
+
+Keeping this logic in one place means bugs fixed here fix all three.
+"""
+
+from decimal import Decimal
+
+from django.db.models import Q
+
+from ..models import Invoice, InvoiceItem, ServiceType
+
+
+def build_service_type_map(practice) -> dict[int, ServiceType]:
+    """
+    Return a {default_duration: ServiceType} dict for a practice.
+
+    Includes global service types (practice=None). When two types share the
+    same default_duration the one with the higher code (alphabetically) wins —
+    the dict is built in ascending code order so later entries overwrite earlier
+    ones, and iteration order is stable.
+    """
+    return {
+        st.default_duration: st
+        for st in ServiceType.objects.filter(
+            Q(practice=practice) | Q(practice__isnull=True)
+        ).order_by("code")
+    }
+
+
+def resolve_session_rate(client, service_type) -> Decimal:
+    """
+    Return the billing rate for a session of this service type for the client.
+
+    Returns Decimal("0") for free consultations (service_type.code == "therapy_free").
+    Otherwise uses the client's 90-min rate for types with default_duration >= 90,
+    and the 60-min rate for shorter types.
+    """
+    if service_type.code == "therapy_free":
+        return Decimal("0")
+    if service_type.default_duration >= 90:
+        return Decimal(str(client.hourly_rate_90 or client.hourly_rate_60 or 0))
+    return Decimal(str(client.hourly_rate_60 or 0))
+
+
+def is_session_already_billed(session) -> bool:
+    """
+    Return True if the session is attached to any non-cancelled InvoiceItem.
+
+    Cancelled invoices are excluded so that a re-billed session (after
+    cancellation) is not refused.
+    """
+    return (
+        InvoiceItem.objects.filter(session=session)
+        .exclude(invoice__status=Invoice.Status.CANCELLED)
+        .exists()
+    )
+
+
+def create_invoice_item_for_session(
+    invoice,
+    session,
+    service_type_map: dict[int, ServiceType],
+    fallback_service_type: ServiceType | None = None,
+) -> InvoiceItem | None:
+    """
+    Create an InvoiceItem for a session on a draft invoice.
+
+    Returns the created InvoiceItem, or None when:
+    - the session is already billed on a non-cancelled invoice, or
+    - no service type could be resolved.
+
+    Does NOT call recalculate_invoice_total — the caller is responsible for
+    that after all items have been created.
+    """
+    if is_session_already_billed(session):
+        return None
+
+    service_type = service_type_map.get(session.duration, fallback_service_type)
+    if service_type is None:
+        return None
+
+    rate = resolve_session_rate(invoice.client, service_type)
+    return InvoiceItem.objects.create(
+        invoice=invoice,
+        session=session,
+        service_type=service_type,
+        rate=rate,
+        quantity=Decimal("1.00"),
+        total=rate,
+    )