diff --git a/src/onegov/org/upgrade.py b/src/onegov/org/upgrade.py index 482aed25f2..a3c4c02fd2 100644 --- a/src/onegov/org/upgrade.py +++ b/src/onegov/org/upgrade.py @@ -22,6 +22,7 @@ from onegov.form import FormDefinition from onegov.form.parser import ParsedForm from onegov.newsletter import Newsletter +from onegov.org import log from onegov.org.models import ( Organisation, Topic, News, ExtendedDirectory, PushNotification) from onegov.org.models.political_business import ( @@ -997,3 +998,87 @@ def switch_to_parsed_event_filters(context: UpgradeContext) -> None: return org.event_filter_parsed_definition = ParsedForm.from_formcode(definition) + + +@upgrade_task( + 'Refresh reservation invoices zeroed by the pricing backfill', + requires='onegov.reservation:' + 'Store pricing settings on reservations (fixed)' +) +def refresh_zeroed_reservation_invoices(context: UpgradeContext) -> None: + """ `Store pricing settings on reservations (fixed)` repairs the + reservation data, but invoice lines already zeroed by an earlier refresh + still show 0. Recompute them now that the reservation prices are restored. + + Scoped to reservation invoices touched since the backfill rollout that are + tied to an allocation the bug could have disturbed (`per_item`/`per_hour`/ + `free` — the original broken OR only ever matched `free`), or to a + `per_item` resource (whose content fallback was the broken one). Only + refreshed when safe (manual, still-open payment). + """ + from onegov.ticket import Ticket + + if not context.has_table('tickets'): + return + if not context.has_table('invoice_items'): + return + if not context.has_table('reservations'): + return + + # only org-based apps have reservation tickets and a rounding base + org = getattr(context.app, 'org', None) + if org is None: + return + rounding_base = org.price_rounding + + ticket_ids = context.session.execute(text(""" + SELECT DISTINCT t.id + FROM tickets t + JOIN invoice_items ii + ON ii.invoice_id = t.invoice_id + AND ii.group = 'reservation' + AND COALESCE(ii.modified, ii.created) >= '2026-08-18' + JOIN reservations r + ON r.token = t.handler_id::uuid + JOIN resources res + ON res.id = r.resource + WHERE t.handler_code = 'RSV' + AND ( + res.content->>'pricing_method' = 'per_item' + OR EXISTS ( + SELECT 1 FROM allocations a + WHERE a."group" = r.target + AND a.data->>'pricing_method' + IN ('per_item', 'per_hour', 'free') + ) + ) + """)).scalars().all() + + refreshed: list[str] = [] + skipped: list[str] = [] + for ticket_id in ticket_ids: + ticket = context.session.get(Ticket, ticket_id) + if ticket is None: + continue + + handler = ticket.handler + if not handler.refreshing_invoice_is_safe( + context.request, rounding_base + ): + skipped.append(ticket.number) + continue + + handler.refresh_invoice_items(context.request, rounding_base) + refreshed.append(ticket.number) + + if refreshed: + log.info( + 'Refreshed %d reservation invoice(s): %s', + len(refreshed), ', '.join(refreshed) + ) + if skipped: + log.warning( + 'Skipped %d reservation invoice(s) that could not be safely ' + 'refreshed (non-manual or non-open payment): %s', + len(skipped), ', '.join(skipped) + ) diff --git a/src/onegov/reservation/upgrade.py b/src/onegov/reservation/upgrade.py index e2ed8b5774..68622a69ac 100644 --- a/src/onegov/reservation/upgrade.py +++ b/src/onegov/reservation/upgrade.py @@ -15,7 +15,6 @@ from sqlalchemy import ( bindparam, text, Column, Enum, ForeignKey, Integer, Text, UUID) - from typing import Any, TYPE_CHECKING if TYPE_CHECKING: from onegov.core.upgrade import UpgradeContext @@ -506,8 +505,26 @@ def add_source_id_to_reserved_slots(context: UpgradeContext) -> None: ) -@upgrade_task('Store pricing settings on reservations') -def store_pricing_settings_on_reservations(context: UpgradeContext) -> None: +@upgrade_task('Store pricing settings on reservations (fixed)') +def store_pricing_settings_on_reservations_fixed( + context: UpgradeContext +) -> None: + """ Snapshots each reservation's pricing onto its own ``data``, from the + master allocation when it defines a price, otherwise from the resource + content. + + Re-run of `Store pricing settings on reservations` under a new name so it + executes again on already-upgraded databases. Three bugs in the original + zeroed prices: + + - the allocation lookup matched on the wrong ``pricing_method`` constants + (``price_per_item``/``price_per_hour`` instead of ``per_item``/ + ``per_hour``), so allocation-priced reservations never matched, + - the ``resource = mirror_of`` guard was mis-parenthesised (``AND`` bound + tighter than the following ``OR``s), and + - the resource-content fallback read ``content->'price_per_item'``, but the + resource stores that value under ``price_per_reservation``. + """ if not context.has_table('resources'): return @@ -526,9 +543,7 @@ def store_pricing_settings_on_reservations(context: UpgradeContext) -> None: ) AS pricing FROM allocations WHERE resource = mirror_of - AND data->>'pricing_method' = 'price_per_item' - OR data->>'pricing_method' = 'price_per_hour' - OR data->>'pricing_method' = 'free' + AND data->>'pricing_method' IN ('per_item', 'per_hour', 'free') ) UPDATE reservations SET data = COALESCE(data, '{}'::jsonb) || @@ -555,7 +570,7 @@ def store_pricing_settings_on_reservations(context: UpgradeContext) -> None: ), 'price_per_item', COALESCE( - resources.content->'price_per_item', + resources.content->'price_per_reservation', '0.0'::jsonb ), 'currency', @@ -566,7 +581,6 @@ def store_pricing_settings_on_reservations(context: UpgradeContext) -> None: END FROM resources WHERE resources.id = resource - """)) diff --git a/tests/onegov/org/test_pricing_schemes.py b/tests/onegov/org/test_pricing_schemes.py index 558c5e2ec1..e1e4707435 100644 --- a/tests/onegov/org/test_pricing_schemes.py +++ b/tests/onegov/org/test_pricing_schemes.py @@ -77,9 +77,9 @@ def test_stadtschulen_zug(client: Client) -> None: transaction.begin() scheduler = ( - ResourceCollection(client.app.libres_context) # type: ignore[union-attr] + ResourceCollection(client.app.libres_context) .by_name('tageskarte') - .get_scheduler(client.app.libres_context) + .get_scheduler(client.app.libres_context) # type: ignore[union-attr] ) allocations = scheduler.allocate( diff --git a/tests/onegov/org/test_upgrade.py b/tests/onegov/org/test_upgrade.py new file mode 100644 index 0000000000..329b063f83 --- /dev/null +++ b/tests/onegov/org/test_upgrade.py @@ -0,0 +1,199 @@ +from __future__ import annotations + +import transaction + +from datetime import datetime +from decimal import Decimal +from freezegun import freeze_time +from sqlalchemy.orm.attributes import flag_modified +from onegov.core.utils import Bunch +from onegov.org.upgrade import refresh_zeroed_reservation_invoices +from onegov.reservation import ResourceCollection +from onegov.ticket import TicketCollection + +from typing import cast, TYPE_CHECKING +if TYPE_CHECKING: + from onegov.core.upgrade import UpgradeContext + from onegov.org.models.ticket import ReservationHandler + from tests.onegov.org.conftest import Client + + +@freeze_time('2026-08-20', tick=True) +def test_refresh_zeroed_reservation_invoices(client: Client) -> None: + """ A reservation invoice whose line was zeroed (before the price data was + corrected) is recomputed by the upgrade, restoring the price and the + payment. + """ + resources = ResourceCollection(client.app.libres_context) + + transaction.begin() + resource = resources.add( + 'Parktower Panorama 24', 'Europe/Zurich', type='room') + resource.pricing_method = 'per_item' + resource.price_per_item = 200.00 + resource.payment_method = 'manual' + resource.currency = 'CHF' + scheduler = resource.get_scheduler(client.app.libres_context) + allocations = scheduler.allocate( + dates=(datetime(2026, 8, 20), datetime(2026, 8, 20)), + whole_day=True, + quota=4, + ) + reserve = client.bound_reserve(allocations[0]) + transaction.commit() + + reserve(quota=1, whole_day=True) + page = client.get('/resource/parktower-panorama-24/form') + page.form['email'] = 'info@example.org' + ticket_page = page.form.submit().follow().form.submit().follow() + assert 'RSV-' in ticket_page.text + + client.login_editor() + invoice = ( + client.get('/tickets/ALL/open') + .click('Annehmen').follow() + .click('Rechnung anzeigen') + ) + assert '200.00' in invoice + + # already-zeroed state: line at 0, payment dropped, price data still 200 + transaction.begin() + session = client.app.session() + ticket = TicketCollection(session).query().filter_by( + handler_code='RSV').one() + handler = cast('ReservationHandler', ticket.handler) + payment = handler.payment + assert ticket.invoice is not None + for item in ticket.invoice.items: + if item.group == 'reservation': + item.unit = Decimal('0') + item.payments = [] + if payment is not None: + for reservation in handler.reservations: + reservation.payment = None + ticket.payment = None + ticket.payment_id = None + session.delete(payment) + session.flush() + ticket_id = ticket.id + transaction.commit() + + # sanity: the invoice is collapsed and has no payment + session = client.app.session() + ticket = TicketCollection(session).query().filter_by(id=ticket_id).one() + assert ticket.invoice is not None + assert ticket.invoice.total_amount == Decimal('0') + assert ticket.handler.payment is None + + # run the upgrade task + context = Bunch( + has_table=lambda table: True, + session=session, + app=Bunch(org=Bunch(price_rounding=None)), + request=Bunch(session=session, translate=lambda text: text), + ) + refresh_zeroed_reservation_invoices(cast('UpgradeContext', context)) + transaction.commit() + + # the reservation line and the payment are restored + session = client.app.session() + ticket = TicketCollection(session).query().filter_by(id=ticket_id).one() + assert ticket.invoice is not None + reservation_items = [ + item for item in ticket.invoice.items if item.group == 'reservation' + ] + assert reservation_items + assert all(item.unit == Decimal('200') for item in reservation_items) + assert ticket.invoice.total_amount == Decimal('200') + assert ticket.handler.payment is not None + assert ticket.handler.payment.amount == Decimal('200') + + +@freeze_time('2026-08-20', tick=True) +def test_refresh_zeroed_reservation_invoices_allocation_priced( + client: Client, +) -> None: + """ Same recovery, but the price comes from a per_item allocation override + on an otherwise free resource. Exercises the allocation branch of the + refresh scoping (not the per_item resource branch) (OGC-3406). + """ + resources = ResourceCollection(client.app.libres_context) + + transaction.begin() + resource = resources.add( + 'Free Room', 'Europe/Zurich', type='room') + resource.pricing_method = 'free' # resource branch must not match + resource.payment_method = 'manual' + resource.currency = 'CHF' + scheduler = resource.get_scheduler(client.app.libres_context) + allocations = scheduler.allocate( + dates=(datetime(2026, 8, 20), datetime(2026, 8, 20)), + whole_day=True, + quota=4, + ) + allocations[0].data = { + 'pricing_method': 'per_item', 'price_per_item': 200.0, + 'price_per_hour': 0.0, 'currency': 'CHF', + } + flag_modified(allocations[0], 'data') + reserve = client.bound_reserve(allocations[0]) + transaction.commit() + + reserve(quota=1, whole_day=True) + page = client.get('/resource/free-room/form') + page.form['email'] = 'info@example.org' + ticket_page = page.form.submit().follow().form.submit().follow() + assert 'RSV-' in ticket_page.text + + client.login_editor() + invoice = ( + client.get('/tickets/ALL/open') + .click('Annehmen').follow() + .click('Rechnung anzeigen') + ) + assert '200.00' in invoice + + # already-zeroed state: line at 0, payment dropped + transaction.begin() + session = client.app.session() + ticket = TicketCollection(session).query().filter_by( + handler_code='RSV').one() + handler = cast('ReservationHandler', ticket.handler) + payment = handler.payment + assert ticket.invoice is not None + for item in ticket.invoice.items: + if item.group == 'reservation': + item.unit = Decimal('0') + item.payments = [] + if payment is not None: + for reservation in handler.reservations: + reservation.payment = None + ticket.payment = None + ticket.payment_id = None + session.delete(payment) + session.flush() + ticket_id = ticket.id + transaction.commit() + + session = client.app.session() + context = Bunch( + has_table=lambda table: True, + session=session, + app=Bunch(org=Bunch(price_rounding=None)), + request=Bunch(session=session, translate=lambda text: text), + ) + refresh_zeroed_reservation_invoices(cast('UpgradeContext', context)) + transaction.commit() + + # restored via the allocation branch + session = client.app.session() + ticket = TicketCollection(session).query().filter_by(id=ticket_id).one() + assert ticket.invoice is not None + reservation_items = [ + item for item in ticket.invoice.items if item.group == 'reservation' + ] + assert reservation_items + assert all(item.unit == Decimal('200') for item in reservation_items) + assert ticket.invoice.total_amount == Decimal('200') + assert ticket.handler.payment is not None + assert ticket.handler.payment.amount == Decimal('200') diff --git a/tests/onegov/reservation/test_upgrade.py b/tests/onegov/reservation/test_upgrade.py index 8b6ae828d5..d49fe04c05 100644 --- a/tests/onegov/reservation/test_upgrade.py +++ b/tests/onegov/reservation/test_upgrade.py @@ -1,16 +1,21 @@ from __future__ import annotations from datetime import datetime -from libres.db.models import Reservation, ReservedSlot +from libres.db.models import Allocation, Reservation, ReservedSlot +from onegov.core.utils import Bunch from onegov.reservation import ResourceCollection from onegov.reservation.upgrade import backfill_reserved_slot_source_ids +from onegov.reservation.upgrade import ( + store_pricing_settings_on_reservations_fixed) from sqlalchemy import text -from uuid import uuid4 +from sqlalchemy.orm.attributes import flag_modified +from uuid import uuid4, UUID -from typing import TYPE_CHECKING +from typing import Any, cast, TYPE_CHECKING if TYPE_CHECKING: from libres.context.core import Context + from onegov.core.upgrade import UpgradeContext def test_backfill_reserved_slot_source_ids(libres_context: Context) -> None: @@ -94,3 +99,194 @@ def test_backfill_reserved_slot_source_ids(libres_context: Context) -> None: for s in remaining: assert s.source_id is not None assert s.source_id == expected[(s.resource, s.start)] + + +def test_store_pricing_settings_permutations( + libres_context: Context, +) -> None: + """ Full matrix (OGC-3406): 3 resources (per_item, per_hour, free) x 4 + allocation settings (per_item, per_hour, free, inherit). The allocation + wins when it defines pricing; otherwise the resource content does. A + reservation resolving to `free` is left untouched (``None`` here). A single + migration run must land every one of the 12 reservations correctly. + """ + collection = ResourceCollection(libres_context) + + # (resource_method, price_item, price_hour) + resources = { + 'per_item': (200.0, 0.0), + 'per_hour': (0.0, 80.0), + 'free': (0.0, 0.0), + } + # allocation settings keyed by name; {} means inherit from the resource + allocs: dict[str, dict[str, Any]] = { + 'per_item': {'pricing_method': 'per_item', + 'price_per_item': 50.0, 'price_per_hour': 0.0}, + 'per_hour': {'pricing_method': 'per_hour', + 'price_per_hour': 30.0, 'price_per_item': 0.0}, + 'free': {'pricing_method': 'free'}, + 'inherit': {}, + } + + def expected(res_method: str, res_ppi: float, res_pph: float, + alloc: str) -> dict[str, object] | None: + # the allocation wins when it defines pricing + if alloc == 'per_item': + return {'method': 'per_item', 'ppi': 50.0, 'pph': 0.0} + if alloc == 'per_hour': + return {'method': 'per_hour', 'ppi': 0.0, 'pph': 30.0} + if alloc == 'free': + return {'method': 'free', 'ppi': 0.0, 'pph': 0.0} + # inherit: fall back to the resource content + if res_method == 'per_item': + return {'method': 'per_item', 'ppi': res_ppi, 'pph': res_pph} + if res_method == 'per_hour': + return {'method': 'per_hour', 'ppi': res_ppi, 'pph': res_pph} + return {'method': 'free', 'ppi': 0.0, 'pph': 0.0} # free resource + + session = None + tokens: dict[tuple[str, str], UUID] = {} + hour = 6 + for res_method, (ppi, pph) in resources.items(): + resource = collection.add(f'Room {res_method}', 'Europe/Zurich') + resource.pricing_method = res_method + resource.price_per_item = ppi + resource.price_per_hour = pph + resource.currency = 'CHF' + scheduler = resource.get_scheduler(libres_context) + session = scheduler.session + + for alloc_name, alloc_data in allocs.items(): + start = datetime(2015, 8, 5, hour) + end = datetime(2015, 8, 5, hour + 1) + hour += 1 + allocation = scheduler.allocate( + (start, end), partly_available=False)[0] + allocation.data = alloc_data + flag_modified(allocation, 'data') + token = scheduler.reserve('info@example.org', (start, end)) + scheduler.approve_reservations(token) + tokens[(res_method, alloc_name)] = token + + assert session is not None + session.flush() + + # legacy state: no pricing stored on any reservation yet + for reservation in session.query(Reservation): + reservation.data = None + flag_modified(reservation, 'data') + session.flush() + + context = Bunch(has_table=lambda table: True, session=session) + store_pricing_settings_on_reservations_fixed( + cast('UpgradeContext', context)) + session.expire_all() + + for (res_method, alloc_name), token in tokens.items(): + ppi, pph = resources[res_method] + exp = expected(res_method, ppi, pph, alloc_name) + data = session.query(Reservation).filter_by(token=token).one().data + assert exp is not None + assert data is not None, (res_method, alloc_name) + assert data['pricing_method'] == exp['method'] + assert data['price_per_item'] == exp['ppi'] + assert data['price_per_hour'] == exp['pph'] + + +def test_store_pricing_settings_quota_mirrors( + libres_context: Context, +) -> None: + """ With quota > 1, reserving beyond the master persists mirror + allocations (resource != mirror_of) that copy the master's pricing data. + The migration's `resource = mirror_of` guard must read only the master, so + every reservation in the group still lands the allocation price (OGC-3406). + """ + collection = ResourceCollection(libres_context) + resource = collection.add('Room', 'Europe/Zurich') + resource.pricing_method = 'free' # allocation overrides to per_item + resource.currency = 'CHF' + + scheduler = resource.get_scheduler(libres_context) + session = scheduler.session + allocation = scheduler.allocate( + (datetime(2015, 8, 5, 8), datetime(2015, 8, 5, 10)), + partly_available=False, + quota=2, + )[0] + allocation.data = {'pricing_method': 'per_item', 'price_per_item': 50.0, + 'price_per_hour': 0.0, 'currency': 'CHF'} + flag_modified(allocation, 'data') + + # two reservations: the second consumes a mirror slot + tokens = [] + for _ in range(2): + token = scheduler.reserve( + 'info@example.org', + (datetime(2015, 8, 5, 8), datetime(2015, 8, 5, 10)), + ) + scheduler.approve_reservations(token) + tokens.append(token) + session.flush() + + # a mirror allocation (resource != mirror_of) now exists for the group + mirrors = session.query(Allocation).filter( + Allocation.resource != Allocation.mirror_of).count() + assert mirrors >= 1 + + # legacy state: no pricing stored yet + for reservation in session.query(Reservation): + reservation.data = None + flag_modified(reservation, 'data') + session.flush() + + context = Bunch(has_table=lambda table: True, session=session) + store_pricing_settings_on_reservations_fixed( + cast('UpgradeContext', context)) + session.expire_all() + + for token in tokens: + data = session.query(Reservation).filter_by(token=token).one().data + assert data is not None + assert data['pricing_method'] == 'per_item' + assert data['price_per_item'] == 50.0 + + +def test_store_pricing_settings_stores_free( + libres_context: Context, +) -> None: + """ Free reservations must record that they are free, so a later change to + the resource/allocation can't make them carry a cost via the fallback: the + stored `free` method is the guard, regardless of any price (OGC-3406). + """ + collection = ResourceCollection(libres_context) + resource = collection.add('Room', 'Europe/Zurich') + resource.pricing_method = 'free' + resource.price_per_item = 45.0 # stale price on a free resource + resource.currency = 'CHF' + + scheduler = resource.get_scheduler(libres_context) + session = scheduler.session + scheduler.allocate( + (datetime(2015, 8, 5, 8), datetime(2015, 8, 5, 10)), + partly_available=False, + ) + token = scheduler.reserve( + 'info@example.org', (datetime(2015, 8, 5, 8), datetime(2015, 8, 5, 10)) + ) + scheduler.approve_reservations(token) + + # a free reservation carrying a stale non-zero price + reservation = session.query(Reservation).filter_by(token=token).one() + reservation.data = {'pricing_method': 'free', 'price_per_item': 45.0, + 'price_per_hour': 0.0} + flag_modified(reservation, 'data') + session.flush() + + context = Bunch(has_table=lambda table: True, session=session) + store_pricing_settings_on_reservations_fixed( + cast('UpgradeContext', context)) + session.expire_all() + + reservation = session.query(Reservation).filter_by(token=token).one() + assert reservation.data is not None + assert reservation.data['pricing_method'] == 'free'