From 8d20ca24a53d2bb8240756ea260118384a5881ae Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 07:35:03 +0200 Subject: [PATCH 01/16] Add upgrade task to refresh invoices zeroed by backfill --- src/onegov/org/upgrade.py | 74 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/src/onegov/org/upgrade.py b/src/onegov/org/upgrade.py index 482aed25f2..bdf59854ce 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,76 @@ 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') +def refresh_zeroed_reservation_invoices(context: UpgradeContext) -> None: + """ The `Backfill reservation prices from invoice lines` upgrade repairs + the reservation *data*, but invoice lines that were already zeroed by a + refresh (before the data was corrected) still show 0. Recompute those + invoices now that both the data and the price fallback are in place. + + Scoped to reservation tickets that actually need it (a reservation line at + 0 while the reservation now has a non-zero price) and only refreshed when + it is safe to do so (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 an org with a + # rounding base); other apps sharing these tables have nothing to do here + 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 ii.unit = 0 + JOIN reservations r + ON replace(r.token::text, '-', '') = t.handler_id + WHERE t.handler_code = 'RSV' + AND ( + COALESCE((r.data->>'price_per_item')::numeric, 0) <> 0 + OR COALESCE((r.data->>'price_per_hour')::numeric, 0) <> 0 + ) + """)).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 zeroed reservation invoice(s): %s', + len(refreshed), ', '.join(refreshed) + ) + if skipped: + log.warning( + 'Skipped %d zeroed reservation invoice(s) that could not be ' + 'safely refreshed (non-manual or non-open payment): %s', + len(skipped), ', '.join(skipped) + ) From c75daf6ecfdae95d92ff5e04fa23cd745f249b32 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 07:36:16 +0200 Subject: [PATCH 02/16] Recover corrupted cases --- .../reservation/models/custom_reservation.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/onegov/reservation/models/custom_reservation.py b/src/onegov/reservation/models/custom_reservation.py index 0e93ca2c44..c55a65a574 100644 --- a/src/onegov/reservation/models/custom_reservation.py +++ b/src/onegov/reservation/models/custom_reservation.py @@ -90,6 +90,24 @@ def invoice_item( resource.pricing_scheme ) cost_object = data.get('cost_object', resource.cost_object) + + # a stored price of 0 may be the result of a botched migration + # (see upgrade `Backfill reservation prices from invoice lines`); + # don't let it silently zero the invoice on refresh, fall back to + # the allocation and then the resource. Only load the allocation in + # this rare corrupted case, to keep the common path cheap. + if pricing_method == 'per_item' and not price_per_item: + allocation = allocation or self.allocation_obj + price_per_item = ( + (allocation.data or {}).get('price_per_item') + if allocation is not None else None + ) or resource.price_per_item + elif pricing_method == 'per_hour' and not price_per_hour: + allocation = allocation or self.allocation_obj + price_per_hour = ( + (allocation.data or {}).get('price_per_hour') + if allocation is not None else None + ) or resource.price_per_hour else: resource = resource or self.resource_obj allocation = allocation or self.allocation_obj From 0c4f756980cb1a802ef74f18c336f260813e2407 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 07:37:19 +0200 Subject: [PATCH 03/16] Fix backfill db upgrade task --- src/onegov/reservation/upgrade.py | 139 ++++++++++++++++++------------ 1 file changed, 82 insertions(+), 57 deletions(-) diff --git a/src/onegov/reservation/upgrade.py b/src/onegov/reservation/upgrade.py index e2ed8b5774..d24f6f737a 100644 --- a/src/onegov/reservation/upgrade.py +++ b/src/onegov/reservation/upgrade.py @@ -506,67 +506,92 @@ 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: - if not context.has_table('resources'): +@upgrade_task('Backfill reservation prices from invoice lines') +def backfill_reservation_prices_from_invoice(context: UpgradeContext) -> None: + """ A previous backfill (`Store pricing settings on reservations`) stored + ``price_per_item``/``price_per_hour`` = 0.0 on reservations whose price + lived on the allocation rather than on the resource content: it matched + allocations on the wrong ``pricing_method`` constants and fell back to the + (empty) resource content. Historical allocations no longer carry the price + either, so it cannot be recovered from them. + + We recover the price from two sources, in order of trust: + + 1. the reservation's invoice line, which reflects what was actually + charged (best for old reservations whose allocation is long gone), and + 2. the allocation the reservation targets, for reservations whose invoice + line was itself already zeroed by a refresh (nothing to read there) but + whose allocation still carries the price (typically future bookings). + + We only touch reservations whose stored price is still 0, keyed by the + invoice item's ``reservation_id`` / the allocation ``group`` so + multi-reservation tickets map correctly. Genuinely free reservations (no + price anywhere) are left untouched. + """ + if not context.has_table('reservations'): + return + if not context.has_table('invoice_items'): + return + if not context.has_table('allocations'): return + # per_item: the reservation invoice line's unit is the price per item context.session.execute(text(""" - WITH adata AS ( - SELECT "group", - jsonb_build_object( - 'pricing_method', - data->'pricing_method', - 'price_per_hour', - COALESCE(data->'price_per_hour', '0.0'::jsonb), - 'price_per_item', - COALESCE(data->'price_per_item', '0.0'::jsonb), - 'currency', - COALESCE(data->'currency', '"CHF"'::jsonb) - ) 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' - ) - UPDATE reservations - SET data = COALESCE(data, '{}'::jsonb) || - CASE - WHEN EXISTS (SELECT 1 FROM adata WHERE adata."group" = target) - THEN - ( - SELECT pricing - FROM adata - WHERE adata."group" = target - LIMIT 1 - ) || jsonb_build_object( - 'cost_object', - resources.content->'cost_object' - ) - ELSE - jsonb_build_object( - 'pricing_method', - resources.content->'pricing_method', - 'price_per_hour', - COALESCE( - resources.content->'price_per_hour', - '0.0'::jsonb - ), - 'price_per_item', - COALESCE( - resources.content->'price_per_item', - '0.0'::jsonb - ), - 'currency', - resources.content->'currency', - 'cost_object', - resources.content->'cost_object' - ) - END - FROM resources - WHERE resources.id = resource + UPDATE reservations r + SET data = COALESCE(r.data, '{}'::jsonb) + || jsonb_build_object('price_per_item', ii.unit) + FROM invoice_items ii + WHERE ii.reservation_id = r.id + AND ii.group = 'reservation' + AND ii.unit IS NOT NULL + AND ii.unit <> 0 + AND r.data->>'pricing_method' = 'per_item' + AND COALESCE((r.data->>'price_per_item')::numeric, 0) = 0 + """)) + # per_hour: the reservation invoice line's unit is the price per hour + context.session.execute(text(""" + UPDATE reservations r + SET data = COALESCE(r.data, '{}'::jsonb) + || jsonb_build_object('price_per_hour', ii.unit) + FROM invoice_items ii + WHERE ii.reservation_id = r.id + AND ii.group = 'reservation' + AND ii.unit IS NOT NULL + AND ii.unit <> 0 + AND r.data->>'pricing_method' = 'per_hour' + AND COALESCE((r.data->>'price_per_hour')::numeric, 0) = 0 + """)) + + # fallback for reservations whose invoice line was itself already zeroed: + # recover from the master allocation the reservation targets, if it still + # carries a non-zero price + context.session.execute(text(""" + UPDATE reservations r + SET data = COALESCE(r.data, '{}'::jsonb) + || jsonb_build_object( + 'price_per_item', a.data->'price_per_item') + FROM allocations a + WHERE a."group" = r.target + AND a.resource = a.mirror_of + AND a.data->>'pricing_method' = 'per_item' + AND COALESCE((a.data->>'price_per_item')::numeric, 0) <> 0 + AND r.data->>'pricing_method' = 'per_item' + AND COALESCE((r.data->>'price_per_item')::numeric, 0) = 0 + """)) + + context.session.execute(text(""" + UPDATE reservations r + SET data = COALESCE(r.data, '{}'::jsonb) + || jsonb_build_object( + 'price_per_hour', a.data->'price_per_hour') + FROM allocations a + WHERE a."group" = r.target + AND a.resource = a.mirror_of + AND a.data->>'pricing_method' = 'per_hour' + AND COALESCE((a.data->>'price_per_hour')::numeric, 0) <> 0 + AND r.data->>'pricing_method' = 'per_hour' + AND COALESCE((r.data->>'price_per_hour')::numeric, 0) = 0 """)) From e553f6c8bfae738a6357473c1dc540fb52cdcd24 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 07:40:42 +0200 Subject: [PATCH 04/16] Adding test --- tests/onegov/org/test_pricing_schemes.py | 102 ++++++++++++++++++++++- 1 file changed, 99 insertions(+), 3 deletions(-) diff --git a/tests/onegov/org/test_pricing_schemes.py b/tests/onegov/org/test_pricing_schemes.py index 558c5e2ec1..48c48c20db 100644 --- a/tests/onegov/org/test_pricing_schemes.py +++ b/tests/onegov/org/test_pricing_schemes.py @@ -4,7 +4,8 @@ from datetime import datetime from freezegun import freeze_time -from onegov.reservation import ResourceCollection +from onegov.reservation import Reservation, ResourceCollection +from sqlalchemy.orm.attributes import flag_modified from typing import TYPE_CHECKING if TYPE_CHECKING: @@ -77,9 +78,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( @@ -129,3 +130,98 @@ def test_stadtschulen_zug(client: Client) -> None: ) ticket = edit_page.form.submit().follow() assert '105.00' in ticket + + + +@freeze_time('2017-07-09', tick=True) +def test_parktower_panorama_24_surcharge_zeroes_positions( + client: Client, +) -> None: + """ OGC-3406: adding a Zuschlag/Abzug zeroes all reservation positions. + + Real-world cause on 'Parktower Panorama 24' (Stadt Zug): the resource uses + ``pricing_method='per_item'`` with the price living on the allocations. + Reservations that were *imported* carry ``price_per_item = 0.0`` in their + ``reservation.data`` (and lack the ``pricing_scheme`` key that today's code + writes). Their invoice was created at the correct price by the import, but + ``custom_reservation.invoice_item`` recomputes the price from that stored + ``0.0``. + + Any ``refresh_invoice_items`` therefore wipes the reservation lines to 0 -- + and adding a Zuschlag/Abzug is exactly what triggers that refresh. The + invoice total collapses and the payment is dropped ("Rechnung + abgeschlossen"). + + To reproduce manually: + 1. Have a per_item reservation whose ``data['price_per_item']`` is 0 but + whose invoice line shows the real price (as produced by the import). + 2. Open the invoice and add a Zuschlag or Abzug. + -> every reservation position drops to 0.00. + """ + 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(2017, 7, 9), datetime(2017, 7, 9)), + 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.form.submit().follow().form.submit().follow() + assert 'RSV-' in ticket.text + + client.login_editor() + page = client.get('/tickets/ALL/open').click('Annehmen').follow() + + invoice = page.click('Rechnung anzeigen') + assert '200.00' in invoice # reservation priced correctly + + # simulate the imported/legacy reservation: its stored data has + # ``price_per_item = 0.0`` and lacks the ``pricing_scheme`` key that + # today's code writes (matches the real Stadt Zug data, e.g. RSV-4872-7335) + transaction.begin() + session = client.app.session() + for reservation in session.query(Reservation): + reservation.data = { + 'currency': 'CHF', + 'cost_object': None, + 'price_per_hour': 0.0, + 'price_per_item': 0.0, + 'pricing_method': 'per_item', + } + flag_modified(reservation, 'data') + transaction.commit() + + # adding a Zuschlag triggers refresh_invoice_items, which recomputes the + # reservation line -- with the stored price at 0.0 it would fall back to + # the allocation and then the resource price + invoice = client.get(invoice.request.url) + item = invoice.click('Abzug / Zuschlag') + item.form['booking_text'] = 'Zuschlag' + item.select_radio('kind', 'Zuschlag') + item.form['surcharge'] = '50.00' + invoice = item.form.submit().follow() + + # the fallback keeps the real price: the reservation position must not be + # silently wiped to 0 + reservation_row = invoice.pyquery( + 'tr:contains("Parktower Panorama 24")' + ).text() + assert '200.00' in reservation_row From a806ce0597c63c9f27e1d611054da1fa63f17725 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 08:04:30 +0200 Subject: [PATCH 05/16] Reservation: Harden pricing fallback and order the invoice refresh Keeps the price fallback in `CustomReservation.invoice_item` from turning a stored 0 into None (which would trip the `price_per_item`/`price_per_hour` asserts and 500 on refresh) by only overriding when a positive price is recovered. Makes the org `Refresh reservation invoices zeroed by the pricing backfill` task explicitly require the reservation `Backfill reservation prices from invoice lines` task, so the refresh always runs after the data is repaired regardless of module ordering. Adds tests covering the refresh task and the full backfill-from-allocation to refresh recovery chain. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01D4Jcf1cSmRS3pc9suqpYMc --- src/onegov/org/upgrade.py | 6 +- .../reservation/models/custom_reservation.py | 12 +- tests/onegov/org/test_upgrade.py | 213 ++++++++++++++++++ 3 files changed, 227 insertions(+), 4 deletions(-) create mode 100644 tests/onegov/org/test_upgrade.py diff --git a/src/onegov/org/upgrade.py b/src/onegov/org/upgrade.py index bdf59854ce..03e537a59e 100644 --- a/src/onegov/org/upgrade.py +++ b/src/onegov/org/upgrade.py @@ -1000,7 +1000,11 @@ def switch_to_parsed_event_filters(context: UpgradeContext) -> None: org.event_filter_parsed_definition = ParsedForm.from_formcode(definition) -@upgrade_task('Refresh reservation invoices zeroed by the pricing backfill') +@upgrade_task( + 'Refresh reservation invoices zeroed by the pricing backfill', + requires='onegov.reservation:' + 'Backfill reservation prices from invoice lines' +) def refresh_zeroed_reservation_invoices(context: UpgradeContext) -> None: """ The `Backfill reservation prices from invoice lines` upgrade repairs the reservation *data*, but invoice lines that were already zeroed by a diff --git a/src/onegov/reservation/models/custom_reservation.py b/src/onegov/reservation/models/custom_reservation.py index c55a65a574..28d47ae294 100644 --- a/src/onegov/reservation/models/custom_reservation.py +++ b/src/onegov/reservation/models/custom_reservation.py @@ -95,19 +95,25 @@ def invoice_item( # (see upgrade `Backfill reservation prices from invoice lines`); # don't let it silently zero the invoice on refresh, fall back to # the allocation and then the resource. Only load the allocation in - # this rare corrupted case, to keep the common path cheap. + # this rare corrupted case, to keep the common path cheap. We keep + # the original value if nothing better is found, so we never turn + # a 0 into a None (which would trip the asserts below). if pricing_method == 'per_item' and not price_per_item: allocation = allocation or self.allocation_obj - price_per_item = ( + recovered = ( (allocation.data or {}).get('price_per_item') if allocation is not None else None ) or resource.price_per_item + if recovered: + price_per_item = recovered elif pricing_method == 'per_hour' and not price_per_hour: allocation = allocation or self.allocation_obj - price_per_hour = ( + recovered = ( (allocation.data or {}).get('price_per_hour') if allocation is not None else None ) or resource.price_per_hour + if recovered: + price_per_hour = recovered else: resource = resource or self.resource_obj allocation = allocation or self.allocation_obj diff --git a/tests/onegov/org/test_upgrade.py b/tests/onegov/org/test_upgrade.py new file mode 100644 index 0000000000..f5b968772e --- /dev/null +++ b/tests/onegov/org/test_upgrade.py @@ -0,0 +1,213 @@ +from __future__ import annotations + +import transaction + +from datetime import datetime +from decimal import Decimal +from freezegun import freeze_time +from onegov.core.utils import Bunch +from onegov.org.upgrade import refresh_zeroed_reservation_invoices +from onegov.reservation import ResourceCollection +from onegov.reservation.upgrade import ( + backfill_reservation_prices_from_invoice) +from onegov.ticket import TicketCollection +from sqlalchemy.orm.attributes import flag_modified + +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('2017-07-09', 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(2017, 7, 9), datetime(2017, 7, 9)), + 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 + + # simulate the already-zeroed state: reservation line at 0 and the payment + # dropped, while the reservation price data is still correct (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('2017-07-09', tick=True) +def test_backfill_and_refresh_double_zeroed_reservation( + client: Client, +) -> None: + """ The invoice line AND the stored price are both 0 (the invoice was + zeroed before the data was corrected). The price only survives on the + allocation. The backfill must recover it from the allocation, after which + the refresh restores the invoice line and 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(2017, 7, 9), datetime(2017, 7, 9)), + whole_day=True, + quota=4, + ) + # the price lives on the allocation (as it does in production) + allocations[0].data = { + 'pricing_method': 'per_item', + 'price_per_item': 200.0, + 'price_per_hour': 0.0, + } + flag_modified(allocations[0], 'data') + 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' + page.form.submit().follow().form.submit().follow() + + client.login_editor() + client.get('/tickets/ALL/open').click('Annehmen').follow() + + # simulate the fully-collapsed state: invoice line 0, stored price 0, no + # payment -- the price only survives on the allocation. Everything below + # runs in a single session/transaction: the backfill uses raw SQL, which + # zope.sqlalchemy would roll back on a plain transaction.commit(). + 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 = [] + for reservation in handler.reservations: + assert reservation.data is not None + reservation.data['price_per_item'] = 0.0 + flag_modified(reservation, 'data') + reservation.payment = None + if payment is not None: + ticket.payment = None + ticket.payment_id = None + session.delete(payment) + session.flush() + ticket_id = ticket.id + + context = Bunch( + has_table=lambda table: True, + session=session, + app=Bunch(org=Bunch(price_rounding=None)), + request=Bunch(session=session, translate=lambda text: text), + ) + + # backfill recovers the price from the allocation ... + backfill_reservation_prices_from_invoice(cast('UpgradeContext', context)) + session.expire_all() + ticket = TicketCollection(session).query().filter_by(id=ticket_id).one() + handler = cast('ReservationHandler', ticket.handler) + for reservation in handler.reservations: + assert reservation.data is not None + assert reservation.data['price_per_item'] == 200.0 + + # ... and the refresh then restores the invoice line and payment + refresh_zeroed_reservation_invoices(cast('UpgradeContext', context)) + session.flush() + + 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.handler.payment is not None + assert ticket.handler.payment.amount == Decimal('200') + transaction.abort() From 2aa4fb53fc863a350accfe0002bd6b27fafacb38 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 08:24:59 +0200 Subject: [PATCH 06/16] Add fixme comment --- src/onegov/reservation/models/custom_reservation.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/onegov/reservation/models/custom_reservation.py b/src/onegov/reservation/models/custom_reservation.py index 28d47ae294..782f488257 100644 --- a/src/onegov/reservation/models/custom_reservation.py +++ b/src/onegov/reservation/models/custom_reservation.py @@ -91,6 +91,8 @@ def invoice_item( ) cost_object = data.get('cost_object', resource.cost_object) + # FIXME: Remove once we've fixed all the reservations with a + # stored price of 0.0 OGC-3406. # a stored price of 0 may be the result of a botched migration # (see upgrade `Backfill reservation prices from invoice lines`); # don't let it silently zero the invoice on refresh, fall back to @@ -114,6 +116,7 @@ def invoice_item( ) or resource.price_per_hour if recovered: price_per_hour = recovered + # end of FIXME else: resource = resource or self.resource_obj allocation = allocation or self.allocation_obj From 692426cab0f95bd5fb57141ba030131cf18da4ee Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 08:36:27 +0200 Subject: [PATCH 07/16] Compact comments --- src/onegov/org/upgrade.py | 17 ++++----- .../reservation/models/custom_reservation.py | 17 ++++----- src/onegov/reservation/upgrade.py | 30 +++++++--------- tests/onegov/org/test_pricing_schemes.py | 35 +++++-------------- tests/onegov/org/test_upgrade.py | 10 +++--- 5 files changed, 39 insertions(+), 70 deletions(-) diff --git a/src/onegov/org/upgrade.py b/src/onegov/org/upgrade.py index 03e537a59e..6c71c47926 100644 --- a/src/onegov/org/upgrade.py +++ b/src/onegov/org/upgrade.py @@ -1006,14 +1006,12 @@ def switch_to_parsed_event_filters(context: UpgradeContext) -> None: 'Backfill reservation prices from invoice lines' ) def refresh_zeroed_reservation_invoices(context: UpgradeContext) -> None: - """ The `Backfill reservation prices from invoice lines` upgrade repairs - the reservation *data*, but invoice lines that were already zeroed by a - refresh (before the data was corrected) still show 0. Recompute those - invoices now that both the data and the price fallback are in place. - - Scoped to reservation tickets that actually need it (a reservation line at - 0 while the reservation now has a non-zero price) and only refreshed when - it is safe to do so (manual, still-open payment). + """ `Backfill reservation prices from invoice lines` repairs the + reservation data, but invoice lines already zeroed by an earlier refresh + still show 0. Recompute them now that data and fallback are in place. + + Scoped to tickets that need it (reservation line at 0 but reservation price + now non-zero) and only refreshed when safe (manual, still-open payment). """ from onegov.ticket import Ticket @@ -1024,8 +1022,7 @@ def refresh_zeroed_reservation_invoices(context: UpgradeContext) -> None: if not context.has_table('reservations'): return - # only org-based apps have reservation tickets (and an org with a - # rounding base); other apps sharing these tables have nothing to do here + # only org-based apps have reservation tickets and a rounding base org = getattr(context.app, 'org', None) if org is None: return diff --git a/src/onegov/reservation/models/custom_reservation.py b/src/onegov/reservation/models/custom_reservation.py index 782f488257..ce0d230f52 100644 --- a/src/onegov/reservation/models/custom_reservation.py +++ b/src/onegov/reservation/models/custom_reservation.py @@ -91,15 +91,12 @@ def invoice_item( ) cost_object = data.get('cost_object', resource.cost_object) - # FIXME: Remove once we've fixed all the reservations with a - # stored price of 0.0 OGC-3406. - # a stored price of 0 may be the result of a botched migration - # (see upgrade `Backfill reservation prices from invoice lines`); - # don't let it silently zero the invoice on refresh, fall back to - # the allocation and then the resource. Only load the allocation in - # this rare corrupted case, to keep the common path cheap. We keep - # the original value if nothing better is found, so we never turn - # a 0 into a None (which would trip the asserts below). + # FIXME: Remove once all reservations with a stored price of 0.0 + # are fixed (OGC-3406). A stored 0 may be from a botched migration + # (`Backfill reservation prices from invoice lines`); fall back to + # allocation then resource instead of zeroing the invoice on + # refresh. Allocation loaded only in this rare case. Never turn a + # 0 into None (would trip the asserts below). if pricing_method == 'per_item' and not price_per_item: allocation = allocation or self.allocation_obj recovered = ( @@ -116,7 +113,7 @@ def invoice_item( ) or resource.price_per_hour if recovered: price_per_hour = recovered - # end of FIXME + # end FIXME OGC-3406 else: resource = resource or self.resource_obj allocation = allocation or self.allocation_obj diff --git a/src/onegov/reservation/upgrade.py b/src/onegov/reservation/upgrade.py index d24f6f737a..3ae9f7e807 100644 --- a/src/onegov/reservation/upgrade.py +++ b/src/onegov/reservation/upgrade.py @@ -509,24 +509,19 @@ def add_source_id_to_reserved_slots(context: UpgradeContext) -> None: @upgrade_task('Backfill reservation prices from invoice lines') def backfill_reservation_prices_from_invoice(context: UpgradeContext) -> None: """ A previous backfill (`Store pricing settings on reservations`) stored - ``price_per_item``/``price_per_hour`` = 0.0 on reservations whose price - lived on the allocation rather than on the resource content: it matched - allocations on the wrong ``pricing_method`` constants and fell back to the - (empty) resource content. Historical allocations no longer carry the price - either, so it cannot be recovered from them. + price 0.0 on reservations whose price lived on the allocation, not the + resource content (it matched allocations on the wrong ``pricing_method`` + constants). Recover the price from two sources, in order of trust: - We recover the price from two sources, in order of trust: - - 1. the reservation's invoice line, which reflects what was actually - charged (best for old reservations whose allocation is long gone), and + 1. the reservation's invoice line (what was actually charged; best for old + reservations whose allocation is gone), then 2. the allocation the reservation targets, for reservations whose invoice - line was itself already zeroed by a refresh (nothing to read there) but - whose allocation still carries the price (typically future bookings). + line was itself already zeroed but whose allocation still carries a + price (typically future bookings). - We only touch reservations whose stored price is still 0, keyed by the - invoice item's ``reservation_id`` / the allocation ``group`` so - multi-reservation tickets map correctly. Genuinely free reservations (no - price anywhere) are left untouched. + Only reservations with a stored price of 0 are touched, keyed by + ``reservation_id`` / allocation ``group`` so multi-reservation tickets map + correctly. Genuinely free reservations are left untouched. """ if not context.has_table('reservations'): return @@ -563,9 +558,8 @@ def backfill_reservation_prices_from_invoice(context: UpgradeContext) -> None: AND COALESCE((r.data->>'price_per_hour')::numeric, 0) = 0 """)) - # fallback for reservations whose invoice line was itself already zeroed: - # recover from the master allocation the reservation targets, if it still - # carries a non-zero price + # fallback: invoice line already zeroed, recover from the master allocation + # if it still carries a non-zero price context.session.execute(text(""" UPDATE reservations r SET data = COALESCE(r.data, '{}'::jsonb) diff --git a/tests/onegov/org/test_pricing_schemes.py b/tests/onegov/org/test_pricing_schemes.py index 48c48c20db..21a55f206d 100644 --- a/tests/onegov/org/test_pricing_schemes.py +++ b/tests/onegov/org/test_pricing_schemes.py @@ -139,24 +139,11 @@ def test_parktower_panorama_24_surcharge_zeroes_positions( ) -> None: """ OGC-3406: adding a Zuschlag/Abzug zeroes all reservation positions. - Real-world cause on 'Parktower Panorama 24' (Stadt Zug): the resource uses - ``pricing_method='per_item'`` with the price living on the allocations. - Reservations that were *imported* carry ``price_per_item = 0.0`` in their - ``reservation.data`` (and lack the ``pricing_scheme`` key that today's code - writes). Their invoice was created at the correct price by the import, but - ``custom_reservation.invoice_item`` recomputes the price from that stored - ``0.0``. - - Any ``refresh_invoice_items`` therefore wipes the reservation lines to 0 -- - and adding a Zuschlag/Abzug is exactly what triggers that refresh. The - invoice total collapses and the payment is dropped ("Rechnung - abgeschlossen"). - - To reproduce manually: - 1. Have a per_item reservation whose ``data['price_per_item']`` is 0 but - whose invoice line shows the real price (as produced by the import). - 2. Open the invoice and add a Zuschlag or Abzug. - -> every reservation position drops to 0.00. + Cause: a per_item resource with the price on the allocations. Imported + reservations carry ``price_per_item = 0.0`` in their data, so + ``custom_reservation.invoice_item`` recomputes their line from that 0.0. + Any ``refresh_invoice_items`` (adding a Zuschlag/Abzug triggers one) then + wipes the reservation lines to 0 and drops the payment. """ resources = ResourceCollection(client.app.libres_context) @@ -193,9 +180,7 @@ def test_parktower_panorama_24_surcharge_zeroes_positions( invoice = page.click('Rechnung anzeigen') assert '200.00' in invoice # reservation priced correctly - # simulate the imported/legacy reservation: its stored data has - # ``price_per_item = 0.0`` and lacks the ``pricing_scheme`` key that - # today's code writes (matches the real Stadt Zug data, e.g. RSV-4872-7335) + # simulate the imported/legacy reservation: stored price_per_item = 0.0 transaction.begin() session = client.app.session() for reservation in session.query(Reservation): @@ -209,9 +194,8 @@ def test_parktower_panorama_24_surcharge_zeroes_positions( flag_modified(reservation, 'data') transaction.commit() - # adding a Zuschlag triggers refresh_invoice_items, which recomputes the - # reservation line -- with the stored price at 0.0 it would fall back to - # the allocation and then the resource price + # adding a Zuschlag triggers refresh_invoice_items; the 0.0 stored price + # falls back to the allocation then resource invoice = client.get(invoice.request.url) item = invoice.click('Abzug / Zuschlag') item.form['booking_text'] = 'Zuschlag' @@ -219,8 +203,7 @@ def test_parktower_panorama_24_surcharge_zeroes_positions( item.form['surcharge'] = '50.00' invoice = item.form.submit().follow() - # the fallback keeps the real price: the reservation position must not be - # silently wiped to 0 + # the fallback keeps the real price; the position must not be wiped to 0 reservation_row = invoice.pyquery( 'tr:contains("Parktower Panorama 24")' ).text() diff --git a/tests/onegov/org/test_upgrade.py b/tests/onegov/org/test_upgrade.py index f5b968772e..f9e9693eb8 100644 --- a/tests/onegov/org/test_upgrade.py +++ b/tests/onegov/org/test_upgrade.py @@ -58,8 +58,7 @@ def test_refresh_zeroed_reservation_invoices(client: Client) -> None: ) assert '200.00' in invoice - # simulate the already-zeroed state: reservation line at 0 and the payment - # dropped, while the reservation price data is still correct (200) + # already-zeroed state: line at 0, payment dropped, price data still 200 transaction.begin() session = client.app.session() ticket = TicketCollection(session).query().filter_by( @@ -154,10 +153,9 @@ def test_backfill_and_refresh_double_zeroed_reservation( client.login_editor() client.get('/tickets/ALL/open').click('Annehmen').follow() - # simulate the fully-collapsed state: invoice line 0, stored price 0, no - # payment -- the price only survives on the allocation. Everything below - # runs in a single session/transaction: the backfill uses raw SQL, which - # zope.sqlalchemy would roll back on a plain transaction.commit(). + # fully-collapsed state: invoice line 0, stored price 0, no payment; price + # only survives on the allocation. Single session/transaction throughout: + # the backfill's raw SQL would be rolled back by zope.sqlalchemy on commit. transaction.begin() session = client.app.session() ticket = TicketCollection(session).query().filter_by( From 05c4a9f431b3b632a06ae79a6a1e45e69b3892c1 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 10:33:51 +0200 Subject: [PATCH 08/16] Remove unused runtime fix --- .../reservation/models/custom_reservation.py | 24 ------------------- 1 file changed, 24 deletions(-) diff --git a/src/onegov/reservation/models/custom_reservation.py b/src/onegov/reservation/models/custom_reservation.py index ce0d230f52..0e93ca2c44 100644 --- a/src/onegov/reservation/models/custom_reservation.py +++ b/src/onegov/reservation/models/custom_reservation.py @@ -90,30 +90,6 @@ def invoice_item( resource.pricing_scheme ) cost_object = data.get('cost_object', resource.cost_object) - - # FIXME: Remove once all reservations with a stored price of 0.0 - # are fixed (OGC-3406). A stored 0 may be from a botched migration - # (`Backfill reservation prices from invoice lines`); fall back to - # allocation then resource instead of zeroing the invoice on - # refresh. Allocation loaded only in this rare case. Never turn a - # 0 into None (would trip the asserts below). - if pricing_method == 'per_item' and not price_per_item: - allocation = allocation or self.allocation_obj - recovered = ( - (allocation.data or {}).get('price_per_item') - if allocation is not None else None - ) or resource.price_per_item - if recovered: - price_per_item = recovered - elif pricing_method == 'per_hour' and not price_per_hour: - allocation = allocation or self.allocation_obj - recovered = ( - (allocation.data or {}).get('price_per_hour') - if allocation is not None else None - ) or resource.price_per_hour - if recovered: - price_per_hour = recovered - # end FIXME OGC-3406 else: resource = resource or self.resource_obj allocation = allocation or self.allocation_obj From da4d7b7d38f0bb92b46576edf71c56faee323057 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 13:32:10 +0200 Subject: [PATCH 09/16] Rework back to original upgrade and fix --- src/onegov/org/upgrade.py | 6 +- src/onegov/reservation/upgrade.py | 185 ++++++++++++++++++------------ 2 files changed, 112 insertions(+), 79 deletions(-) diff --git a/src/onegov/org/upgrade.py b/src/onegov/org/upgrade.py index 6c71c47926..ec3dc104d3 100644 --- a/src/onegov/org/upgrade.py +++ b/src/onegov/org/upgrade.py @@ -1003,12 +1003,12 @@ def switch_to_parsed_event_filters(context: UpgradeContext) -> None: @upgrade_task( 'Refresh reservation invoices zeroed by the pricing backfill', requires='onegov.reservation:' - 'Backfill reservation prices from invoice lines' + 'Store pricing settings on reservations (fixed)' ) def refresh_zeroed_reservation_invoices(context: UpgradeContext) -> None: - """ `Backfill reservation prices from invoice lines` repairs the + """ `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 data and fallback are in place. + still show 0. Recompute them now that the reservation prices are restored. Scoped to tickets that need it (reservation line at 0 but reservation price now non-zero) and only refreshed when safe (manual, still-open payment). diff --git a/src/onegov/reservation/upgrade.py b/src/onegov/reservation/upgrade.py index 3ae9f7e807..668b6ed94e 100644 --- a/src/onegov/reservation/upgrade.py +++ b/src/onegov/reservation/upgrade.py @@ -5,6 +5,8 @@ # pragma: exclude file from __future__ import annotations +import logging + from libres.db.models import Allocation, Reservation from libres.db.models.types.json_type import JSON from onegov.core.upgrade import upgrade_task @@ -16,6 +18,9 @@ bindparam, text, Column, Enum, ForeignKey, Integer, Text, UUID) +log = logging.getLogger('onegov.reservation') + + from typing import Any, TYPE_CHECKING if TYPE_CHECKING: from onegov.core.upgrade import UpgradeContext @@ -506,87 +511,115 @@ def add_source_id_to_reserved_slots(context: UpgradeContext) -> None: ) -@upgrade_task('Backfill reservation prices from invoice lines') -def backfill_reservation_prices_from_invoice(context: UpgradeContext) -> None: - """ A previous backfill (`Store pricing settings on reservations`) stored - price 0.0 on reservations whose price lived on the allocation, not the - resource content (it matched allocations on the wrong ``pricing_method`` - constants). Recover the price from two sources, in order of trust: - - 1. the reservation's invoice line (what was actually charged; best for old - reservations whose allocation is gone), then - 2. the allocation the reservation targets, for reservations whose invoice - line was itself already zeroed but whose allocation still carries a - price (typically future bookings). - - Only reservations with a stored price of 0 are touched, keyed by - ``reservation_id`` / allocation ``group`` so multi-reservation tickets map - correctly. Genuinely free reservations are left untouched. +@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('reservations'): - return - if not context.has_table('invoice_items'): - return - if not context.has_table('allocations'): + if not context.has_table('resources'): return - # per_item: the reservation invoice line's unit is the price per item - context.session.execute(text(""" - UPDATE reservations r - SET data = COALESCE(r.data, '{}'::jsonb) - || jsonb_build_object('price_per_item', ii.unit) - FROM invoice_items ii - WHERE ii.reservation_id = r.id - AND ii.group = 'reservation' - AND ii.unit IS NOT NULL - AND ii.unit <> 0 - AND r.data->>'pricing_method' = 'per_item' - AND COALESCE((r.data->>'price_per_item')::numeric, 0) = 0 - """)) - - # per_hour: the reservation invoice line's unit is the price per hour - context.session.execute(text(""" - UPDATE reservations r - SET data = COALESCE(r.data, '{}'::jsonb) - || jsonb_build_object('price_per_hour', ii.unit) - FROM invoice_items ii - WHERE ii.reservation_id = r.id - AND ii.group = 'reservation' - AND ii.unit IS NOT NULL - AND ii.unit <> 0 - AND r.data->>'pricing_method' = 'per_hour' - AND COALESCE((r.data->>'price_per_hour')::numeric, 0) = 0 - """)) - - # fallback: invoice line already zeroed, recover from the master allocation - # if it still carries a non-zero price - context.session.execute(text(""" - UPDATE reservations r - SET data = COALESCE(r.data, '{}'::jsonb) - || jsonb_build_object( - 'price_per_item', a.data->'price_per_item') - FROM allocations a - WHERE a."group" = r.target - AND a.resource = a.mirror_of - AND a.data->>'pricing_method' = 'per_item' - AND COALESCE((a.data->>'price_per_item')::numeric, 0) <> 0 - AND r.data->>'pricing_method' = 'per_item' - AND COALESCE((r.data->>'price_per_item')::numeric, 0) = 0 + result = context.session.execute(text(""" + WITH adata AS ( + SELECT "group", + jsonb_build_object( + 'pricing_method', + data->'pricing_method', + 'price_per_hour', + COALESCE(data->'price_per_hour', '0.0'::jsonb), + 'price_per_item', + COALESCE(data->'price_per_item', '0.0'::jsonb), + 'currency', + COALESCE(data->'currency', '"CHF"'::jsonb) + ) AS pricing + FROM allocations + WHERE resource = mirror_of + AND ( + data->>'pricing_method' = 'per_item' + OR data->>'pricing_method' = 'per_hour' + OR data->>'pricing_method' = 'free' + ) + ), + computed AS ( + SELECT r.id AS id, + r.data AS old_data, + COALESCE(r.data, '{}'::jsonb) || + CASE + WHEN EXISTS ( + SELECT 1 FROM adata WHERE adata."group" = r.target + ) + THEN + ( + SELECT pricing + FROM adata + WHERE adata."group" = r.target + LIMIT 1 + ) || jsonb_build_object( + 'cost_object', + res.content->'cost_object' + ) + ELSE + jsonb_build_object( + 'pricing_method', + res.content->'pricing_method', + 'price_per_hour', + COALESCE( + res.content->'price_per_hour', + '0.0'::jsonb + ), + 'price_per_item', + COALESCE( + res.content->'price_per_reservation', + '0.0'::jsonb + ), + 'currency', + res.content->'currency', + 'cost_object', + res.content->'cost_object' + ) + END AS new_data + FROM reservations r + JOIN resources res ON res.id = r.resource + ) + UPDATE reservations + SET data = computed.new_data + FROM computed + WHERE reservations.id = computed.id + AND computed.new_data IS DISTINCT FROM computed.old_data + -- leave free reservations untouched: the price is never applied + -- (invoice_item returns None for 'free'), so don't churn them + AND computed.new_data->>'pricing_method' IS DISTINCT FROM 'free' + RETURNING reservations.id, + reservations.data->>'pricing_method' AS pricing_method, + reservations.data->>'price_per_item' AS price_per_item, + reservations.data->>'price_per_hour' AS price_per_hour """)) - context.session.execute(text(""" - UPDATE reservations r - SET data = COALESCE(r.data, '{}'::jsonb) - || jsonb_build_object( - 'price_per_hour', a.data->'price_per_hour') - FROM allocations a - WHERE a."group" = r.target - AND a.resource = a.mirror_of - AND a.data->>'pricing_method' = 'per_hour' - AND COALESCE((a.data->>'price_per_hour')::numeric, 0) <> 0 - AND r.data->>'pricing_method' = 'per_hour' - AND COALESCE((r.data->>'price_per_hour')::numeric, 0) = 0 - """)) + count = 0 + for row in result: + count += 1 + log.info( + 'Stored pricing on reservation %s: method=%s, per_item=%s, ' + 'per_hour=%s', row.id, row.pricing_method, + row.price_per_item, row.price_per_hour + ) + if count: + log.info('Stored pricing settings on %d reservation(s)', count) @upgrade_task('Migrate resources.parent_id to association table') From 69605abc4e640b455ab0b6a367f989e3c84f0c06 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 13:33:10 +0200 Subject: [PATCH 10/16] Adjust tests --- tests/onegov/org/test_pricing_schemes.py | 81 +-------------- tests/onegov/org/test_upgrade.py | 103 ------------------- tests/onegov/reservation/test_upgrade.py | 121 ++++++++++++++++++++++- 3 files changed, 121 insertions(+), 184 deletions(-) diff --git a/tests/onegov/org/test_pricing_schemes.py b/tests/onegov/org/test_pricing_schemes.py index 21a55f206d..e1e4707435 100644 --- a/tests/onegov/org/test_pricing_schemes.py +++ b/tests/onegov/org/test_pricing_schemes.py @@ -4,8 +4,7 @@ from datetime import datetime from freezegun import freeze_time -from onegov.reservation import Reservation, ResourceCollection -from sqlalchemy.orm.attributes import flag_modified +from onegov.reservation import ResourceCollection from typing import TYPE_CHECKING if TYPE_CHECKING: @@ -130,81 +129,3 @@ def test_stadtschulen_zug(client: Client) -> None: ) ticket = edit_page.form.submit().follow() assert '105.00' in ticket - - - -@freeze_time('2017-07-09', tick=True) -def test_parktower_panorama_24_surcharge_zeroes_positions( - client: Client, -) -> None: - """ OGC-3406: adding a Zuschlag/Abzug zeroes all reservation positions. - - Cause: a per_item resource with the price on the allocations. Imported - reservations carry ``price_per_item = 0.0`` in their data, so - ``custom_reservation.invoice_item`` recomputes their line from that 0.0. - Any ``refresh_invoice_items`` (adding a Zuschlag/Abzug triggers one) then - wipes the reservation lines to 0 and drops 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(2017, 7, 9), datetime(2017, 7, 9)), - 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.form.submit().follow().form.submit().follow() - assert 'RSV-' in ticket.text - - client.login_editor() - page = client.get('/tickets/ALL/open').click('Annehmen').follow() - - invoice = page.click('Rechnung anzeigen') - assert '200.00' in invoice # reservation priced correctly - - # simulate the imported/legacy reservation: stored price_per_item = 0.0 - transaction.begin() - session = client.app.session() - for reservation in session.query(Reservation): - reservation.data = { - 'currency': 'CHF', - 'cost_object': None, - 'price_per_hour': 0.0, - 'price_per_item': 0.0, - 'pricing_method': 'per_item', - } - flag_modified(reservation, 'data') - transaction.commit() - - # adding a Zuschlag triggers refresh_invoice_items; the 0.0 stored price - # falls back to the allocation then resource - invoice = client.get(invoice.request.url) - item = invoice.click('Abzug / Zuschlag') - item.form['booking_text'] = 'Zuschlag' - item.select_radio('kind', 'Zuschlag') - item.form['surcharge'] = '50.00' - invoice = item.form.submit().follow() - - # the fallback keeps the real price; the position must not be wiped to 0 - reservation_row = invoice.pyquery( - 'tr:contains("Parktower Panorama 24")' - ).text() - assert '200.00' in reservation_row diff --git a/tests/onegov/org/test_upgrade.py b/tests/onegov/org/test_upgrade.py index f9e9693eb8..d6f73ba979 100644 --- a/tests/onegov/org/test_upgrade.py +++ b/tests/onegov/org/test_upgrade.py @@ -8,10 +8,7 @@ from onegov.core.utils import Bunch from onegov.org.upgrade import refresh_zeroed_reservation_invoices from onegov.reservation import ResourceCollection -from onegov.reservation.upgrade import ( - backfill_reservation_prices_from_invoice) from onegov.ticket import TicketCollection -from sqlalchemy.orm.attributes import flag_modified from typing import cast, TYPE_CHECKING if TYPE_CHECKING: @@ -109,103 +106,3 @@ def test_refresh_zeroed_reservation_invoices(client: Client) -> None: assert ticket.invoice.total_amount == Decimal('200') assert ticket.handler.payment is not None assert ticket.handler.payment.amount == Decimal('200') - - -@freeze_time('2017-07-09', tick=True) -def test_backfill_and_refresh_double_zeroed_reservation( - client: Client, -) -> None: - """ The invoice line AND the stored price are both 0 (the invoice was - zeroed before the data was corrected). The price only survives on the - allocation. The backfill must recover it from the allocation, after which - the refresh restores the invoice line and 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(2017, 7, 9), datetime(2017, 7, 9)), - whole_day=True, - quota=4, - ) - # the price lives on the allocation (as it does in production) - allocations[0].data = { - 'pricing_method': 'per_item', - 'price_per_item': 200.0, - 'price_per_hour': 0.0, - } - flag_modified(allocations[0], 'data') - 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' - page.form.submit().follow().form.submit().follow() - - client.login_editor() - client.get('/tickets/ALL/open').click('Annehmen').follow() - - # fully-collapsed state: invoice line 0, stored price 0, no payment; price - # only survives on the allocation. Single session/transaction throughout: - # the backfill's raw SQL would be rolled back by zope.sqlalchemy on commit. - 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 = [] - for reservation in handler.reservations: - assert reservation.data is not None - reservation.data['price_per_item'] = 0.0 - flag_modified(reservation, 'data') - reservation.payment = None - if payment is not None: - ticket.payment = None - ticket.payment_id = None - session.delete(payment) - session.flush() - ticket_id = ticket.id - - context = Bunch( - has_table=lambda table: True, - session=session, - app=Bunch(org=Bunch(price_rounding=None)), - request=Bunch(session=session, translate=lambda text: text), - ) - - # backfill recovers the price from the allocation ... - backfill_reservation_prices_from_invoice(cast('UpgradeContext', context)) - session.expire_all() - ticket = TicketCollection(session).query().filter_by(id=ticket_id).one() - handler = cast('ReservationHandler', ticket.handler) - for reservation in handler.reservations: - assert reservation.data is not None - assert reservation.data['price_per_item'] == 200.0 - - # ... and the refresh then restores the invoice line and payment - refresh_zeroed_reservation_invoices(cast('UpgradeContext', context)) - session.flush() - - 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.handler.payment is not None - assert ticket.handler.payment.amount == Decimal('200') - transaction.abort() diff --git a/tests/onegov/reservation/test_upgrade.py b/tests/onegov/reservation/test_upgrade.py index 8b6ae828d5..7936455edc 100644 --- a/tests/onegov/reservation/test_upgrade.py +++ b/tests/onegov/reservation/test_upgrade.py @@ -1,16 +1,23 @@ from __future__ import annotations +import pytest + from datetime import datetime from libres.db.models import 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 sqlalchemy.orm.attributes import flag_modified from uuid import uuid4 -from typing import TYPE_CHECKING +from typing import 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 +101,115 @@ 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)] + + +@pytest.mark.parametrize('resource_method,price_item,price_hour,alloc_data,' + 'expected_method,expected_item,expected_hour', [ + # price on the allocation + ('per_item', 0.0, 0.0, + {'pricing_method': 'per_item', 'price_per_item': 50.0, + 'price_per_hour': 0.0}, + 'per_item', 50.0, 0.0), + ('per_hour', 0.0, 0.0, + {'pricing_method': 'per_hour', 'price_per_hour': 30.0, + 'price_per_item': 0.0}, + 'per_hour', 0.0, 30.0), + # price inherited from the resource content (allocation defines nothing) + ('per_item', 200.0, 0.0, {}, 'per_item', 200.0, 0.0), + ('per_hour', 0.0, 80.0, {}, 'per_hour', 0.0, 80.0), +]) +def test_store_pricing_settings_permutations( + libres_context: Context, + resource_method: str, + price_item: float, + price_hour: float, + alloc_data: dict[str, object], + expected_method: str, + expected_item: float, + expected_hour: float, +) -> None: + """ The (fixed) migration snapshots the correct pricing onto each + reservation, whether the price lives on the allocation or the resource, + for every pricing_method permutation (OGC-3406). + """ + collection = ResourceCollection(libres_context) + resource = collection.add('Room', 'Europe/Zurich') + resource.pricing_method = resource_method + resource.price_per_item = price_item + resource.price_per_hour = price_hour + 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, + )[0] + allocation.data = alloc_data + flag_modified(allocation, 'data') + + token = scheduler.reserve( + 'info@example.org', (datetime(2015, 8, 5, 8), datetime(2015, 8, 5, 10)) + ) + scheduler.approve_reservations(token) + session.flush() + + # legacy state: no pricing stored on the reservation yet + reservation = session.query(Reservation).filter_by(token=token).one() + 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() + + reservation = session.query(Reservation).filter_by(token=token).one() + assert reservation.data is not None + assert reservation.data['pricing_method'] == expected_method + assert reservation.data['price_per_item'] == expected_item + assert reservation.data['price_per_hour'] == expected_hour + + +def test_store_pricing_settings_leaves_free_untouched( + libres_context: Context, +) -> None: + """ Free reservations are never touched: their price is never applied + (invoice_item returns None for 'free'), so the migration must not churn + them, even when a stale non-zero price sits in the data (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() + stale = {'pricing_method': 'free', 'price_per_item': 45.0, + 'price_per_hour': 0.0} + reservation.data = dict(stale) + 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() + + # left exactly as-is, not zeroed or rewritten + reservation = session.query(Reservation).filter_by(token=token).one() + assert reservation.data == stale From 0ef73cab183fb453bfc5c2aaf813cc505f1c1c78 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 14:07:46 +0200 Subject: [PATCH 11/16] Add all permutation test --- tests/onegov/reservation/test_upgrade.py | 135 +++++++++++++---------- 1 file changed, 79 insertions(+), 56 deletions(-) diff --git a/tests/onegov/reservation/test_upgrade.py b/tests/onegov/reservation/test_upgrade.py index 7936455edc..bcc8f9945d 100644 --- a/tests/onegov/reservation/test_upgrade.py +++ b/tests/onegov/reservation/test_upgrade.py @@ -1,7 +1,5 @@ from __future__ import annotations -import pytest - from datetime import datetime from libres.db.models import Reservation, ReservedSlot from onegov.core.utils import Bunch @@ -11,10 +9,10 @@ store_pricing_settings_on_reservations_fixed) from sqlalchemy import text from sqlalchemy.orm.attributes import flag_modified -from uuid import uuid4 +from uuid import uuid4, UUID -from typing import cast, TYPE_CHECKING +from typing import Any, cast, TYPE_CHECKING if TYPE_CHECKING: from libres.context.core import Context from onegov.core.upgrade import UpgradeContext @@ -103,62 +101,80 @@ def test_backfill_reserved_slot_source_ids(libres_context: Context) -> None: assert s.source_id == expected[(s.resource, s.start)] -@pytest.mark.parametrize('resource_method,price_item,price_hour,alloc_data,' - 'expected_method,expected_item,expected_hour', [ - # price on the allocation - ('per_item', 0.0, 0.0, - {'pricing_method': 'per_item', 'price_per_item': 50.0, - 'price_per_hour': 0.0}, - 'per_item', 50.0, 0.0), - ('per_hour', 0.0, 0.0, - {'pricing_method': 'per_hour', 'price_per_hour': 30.0, - 'price_per_item': 0.0}, - 'per_hour', 0.0, 30.0), - # price inherited from the resource content (allocation defines nothing) - ('per_item', 200.0, 0.0, {}, 'per_item', 200.0, 0.0), - ('per_hour', 0.0, 80.0, {}, 'per_hour', 0.0, 80.0), -]) def test_store_pricing_settings_permutations( libres_context: Context, - resource_method: str, - price_item: float, - price_hour: float, - alloc_data: dict[str, object], - expected_method: str, - expected_item: float, - expected_hour: float, ) -> None: - """ The (fixed) migration snapshots the correct pricing onto each - reservation, whether the price lives on the allocation or the resource, - for every pricing_method permutation (OGC-3406). + """ 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 = collection.add('Room', 'Europe/Zurich') - resource.pricing_method = resource_method - resource.price_per_item = price_item - resource.price_per_hour = price_hour - 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, - )[0] - allocation.data = alloc_data - flag_modified(allocation, 'data') + # (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': {}, + } - token = scheduler.reserve( - 'info@example.org', (datetime(2015, 8, 5, 8), datetime(2015, 8, 5, 10)) - ) - scheduler.approve_reservations(token) + 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 None # free -> left untouched + # 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 None # free resource -> left untouched + + 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 the reservation yet - reservation = session.query(Reservation).filter_by(token=token).one() - reservation.data = None - flag_modified(reservation, 'data') + # 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) @@ -166,11 +182,18 @@ def test_store_pricing_settings_permutations( 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'] == expected_method - assert reservation.data['price_per_item'] == expected_item - assert reservation.data['price_per_hour'] == expected_hour + 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 + if exp is None: + # free -> untouched: no pricing written (legacy None reads as {}) + assert not data, (res_method, alloc_name, data) + else: + 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_leaves_free_untouched( From 0ba30f63dee5f8512c4f075e73f1fb40dd1b19e7 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 14:34:43 +0200 Subject: [PATCH 12/16] Simplify, remove logging, fix wrong free and adjust tests --- src/onegov/org/upgrade.py | 2 +- src/onegov/reservation/upgrade.py | 28 +------------------ tests/onegov/reservation/test_upgrade.py | 34 +++++++++++------------- 3 files changed, 17 insertions(+), 47 deletions(-) diff --git a/src/onegov/org/upgrade.py b/src/onegov/org/upgrade.py index ec3dc104d3..10c616ce43 100644 --- a/src/onegov/org/upgrade.py +++ b/src/onegov/org/upgrade.py @@ -1036,7 +1036,7 @@ def refresh_zeroed_reservation_invoices(context: UpgradeContext) -> None: AND ii.group = 'reservation' AND ii.unit = 0 JOIN reservations r - ON replace(r.token::text, '-', '') = t.handler_id + ON r.token = t.handler_id::uuid WHERE t.handler_code = 'RSV' AND ( COALESCE((r.data->>'price_per_item')::numeric, 0) <> 0 diff --git a/src/onegov/reservation/upgrade.py b/src/onegov/reservation/upgrade.py index 668b6ed94e..31b114879a 100644 --- a/src/onegov/reservation/upgrade.py +++ b/src/onegov/reservation/upgrade.py @@ -5,8 +5,6 @@ # pragma: exclude file from __future__ import annotations -import logging - from libres.db.models import Allocation, Reservation from libres.db.models.types.json_type import JSON from onegov.core.upgrade import upgrade_task @@ -17,10 +15,6 @@ from sqlalchemy import ( bindparam, text, Column, Enum, ForeignKey, Integer, Text, UUID) - -log = logging.getLogger('onegov.reservation') - - from typing import Any, TYPE_CHECKING if TYPE_CHECKING: from onegov.core.upgrade import UpgradeContext @@ -534,7 +528,7 @@ def store_pricing_settings_on_reservations_fixed( if not context.has_table('resources'): return - result = context.session.execute(text(""" + context.session.execute(text(""" WITH adata AS ( SELECT "group", jsonb_build_object( @@ -557,7 +551,6 @@ def store_pricing_settings_on_reservations_fixed( ), computed AS ( SELECT r.id AS id, - r.data AS old_data, COALESCE(r.data, '{}'::jsonb) || CASE WHEN EXISTS ( @@ -600,27 +593,8 @@ def store_pricing_settings_on_reservations_fixed( SET data = computed.new_data FROM computed WHERE reservations.id = computed.id - AND computed.new_data IS DISTINCT FROM computed.old_data - -- leave free reservations untouched: the price is never applied - -- (invoice_item returns None for 'free'), so don't churn them - AND computed.new_data->>'pricing_method' IS DISTINCT FROM 'free' - RETURNING reservations.id, - reservations.data->>'pricing_method' AS pricing_method, - reservations.data->>'price_per_item' AS price_per_item, - reservations.data->>'price_per_hour' AS price_per_hour """)) - count = 0 - for row in result: - count += 1 - log.info( - 'Stored pricing on reservation %s: method=%s, per_item=%s, ' - 'per_hour=%s', row.id, row.pricing_method, - row.price_per_item, row.price_per_hour - ) - if count: - log.info('Stored pricing settings on %d reservation(s)', count) - @upgrade_task('Migrate resources.parent_id to association table') def migrate_resources_parent_id_to_association_table( diff --git a/tests/onegov/reservation/test_upgrade.py b/tests/onegov/reservation/test_upgrade.py index bcc8f9945d..70815d1bff 100644 --- a/tests/onegov/reservation/test_upgrade.py +++ b/tests/onegov/reservation/test_upgrade.py @@ -136,13 +136,13 @@ def expected(res_method: str, res_ppi: float, res_pph: float, if alloc == 'per_hour': return {'method': 'per_hour', 'ppi': 0.0, 'pph': 30.0} if alloc == 'free': - return None # free -> left untouched + 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 None # free resource -> left untouched + return {'method': 'free', 'ppi': 0.0, 'pph': 0.0} # free resource session = None tokens: dict[tuple[str, str], UUID] = {} @@ -186,22 +186,19 @@ def expected(res_method: str, res_ppi: float, res_pph: float, ppi, pph = resources[res_method] exp = expected(res_method, ppi, pph, alloc_name) data = session.query(Reservation).filter_by(token=token).one().data - if exp is None: - # free -> untouched: no pricing written (legacy None reads as {}) - assert not data, (res_method, alloc_name, data) - else: - 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'] + 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_leaves_free_untouched( +def test_store_pricing_settings_stores_free( libres_context: Context, ) -> None: - """ Free reservations are never touched: their price is never applied - (invoice_item returns None for 'free'), so the migration must not churn - them, even when a stale non-zero price sits in the data (OGC-3406). + """ 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') @@ -222,9 +219,8 @@ def test_store_pricing_settings_leaves_free_untouched( # a free reservation carrying a stale non-zero price reservation = session.query(Reservation).filter_by(token=token).one() - stale = {'pricing_method': 'free', 'price_per_item': 45.0, - 'price_per_hour': 0.0} - reservation.data = dict(stale) + reservation.data = {'pricing_method': 'free', 'price_per_item': 45.0, + 'price_per_hour': 0.0} flag_modified(reservation, 'data') session.flush() @@ -233,6 +229,6 @@ def test_store_pricing_settings_leaves_free_untouched( cast('UpgradeContext', context)) session.expire_all() - # left exactly as-is, not zeroed or rewritten reservation = session.query(Reservation).filter_by(token=token).one() - assert reservation.data == stale + assert reservation.data is not None + assert reservation.data['pricing_method'] == 'free' From e666140e20972554ad5d4fb71adae9e9ef92de8c Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 15:01:45 +0200 Subject: [PATCH 13/16] Limit to updated invoices after rollout date --- src/onegov/org/upgrade.py | 25 +++++++++++++++++-------- tests/onegov/org/test_upgrade.py | 4 ++-- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/onegov/org/upgrade.py b/src/onegov/org/upgrade.py index 10c616ce43..bcb23928a2 100644 --- a/src/onegov/org/upgrade.py +++ b/src/onegov/org/upgrade.py @@ -1010,8 +1010,10 @@ def refresh_zeroed_reservation_invoices(context: UpgradeContext) -> None: 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 tickets that need it (reservation line at 0 but reservation price - now non-zero) and only refreshed when safe (manual, still-open payment). + Scoped to reservation invoices touched since the backfill rollout that are + tied to a paying reservation: an allocation priced `per_item`/`per_hour`, + or a `per_item` resource (whose content fallback was the broken one). Only + refreshed when safe (manual, still-open payment). """ from onegov.ticket import Ticket @@ -1034,13 +1036,20 @@ def refresh_zeroed_reservation_invoices(context: UpgradeContext) -> None: JOIN invoice_items ii ON ii.invoice_id = t.invoice_id AND ii.group = 'reservation' - AND ii.unit = 0 + 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 ( - COALESCE((r.data->>'price_per_item')::numeric, 0) <> 0 - OR COALESCE((r.data->>'price_per_hour')::numeric, 0) <> 0 + 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') + ) ) """)).scalars().all() @@ -1063,12 +1072,12 @@ def refresh_zeroed_reservation_invoices(context: UpgradeContext) -> None: if refreshed: log.info( - 'Refreshed %d zeroed reservation invoice(s): %s', + 'Refreshed %d reservation invoice(s): %s', len(refreshed), ', '.join(refreshed) ) if skipped: log.warning( - 'Skipped %d zeroed reservation invoice(s) that could not be ' - 'safely refreshed (non-manual or non-open payment): %s', + '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/tests/onegov/org/test_upgrade.py b/tests/onegov/org/test_upgrade.py index d6f73ba979..c40e79a3ad 100644 --- a/tests/onegov/org/test_upgrade.py +++ b/tests/onegov/org/test_upgrade.py @@ -17,7 +17,7 @@ from tests.onegov.org.conftest import Client -@freeze_time('2017-07-09', tick=True) +@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 @@ -34,7 +34,7 @@ def test_refresh_zeroed_reservation_invoices(client: Client) -> None: resource.currency = 'CHF' scheduler = resource.get_scheduler(client.app.libres_context) allocations = scheduler.allocate( - dates=(datetime(2017, 7, 9), datetime(2017, 7, 9)), + dates=(datetime(2026, 8, 20), datetime(2026, 8, 20)), whole_day=True, quota=4, ) From 55e030d4b481fdd44b21fda3a1bd8414b980318f Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 15:18:55 +0200 Subject: [PATCH 14/16] Simplify Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013jTHeEgh3qDGixrqK36HkV --- src/onegov/reservation/upgrade.py | 84 +++++++++++++------------------ 1 file changed, 36 insertions(+), 48 deletions(-) diff --git a/src/onegov/reservation/upgrade.py b/src/onegov/reservation/upgrade.py index 31b114879a..68622a69ac 100644 --- a/src/onegov/reservation/upgrade.py +++ b/src/onegov/reservation/upgrade.py @@ -543,56 +543,44 @@ def store_pricing_settings_on_reservations_fixed( ) AS pricing FROM allocations WHERE resource = mirror_of - AND ( - data->>'pricing_method' = 'per_item' - OR data->>'pricing_method' = 'per_hour' - OR data->>'pricing_method' = 'free' - ) - ), - computed AS ( - SELECT r.id AS id, - COALESCE(r.data, '{}'::jsonb) || - CASE - WHEN EXISTS ( - SELECT 1 FROM adata WHERE adata."group" = r.target - ) - THEN - ( - SELECT pricing - FROM adata - WHERE adata."group" = r.target - LIMIT 1 - ) || jsonb_build_object( - 'cost_object', - res.content->'cost_object' - ) - ELSE - jsonb_build_object( - 'pricing_method', - res.content->'pricing_method', - 'price_per_hour', - COALESCE( - res.content->'price_per_hour', - '0.0'::jsonb - ), - 'price_per_item', - COALESCE( - res.content->'price_per_reservation', - '0.0'::jsonb - ), - 'currency', - res.content->'currency', - 'cost_object', - res.content->'cost_object' - ) - END AS new_data - FROM reservations r - JOIN resources res ON res.id = r.resource + AND data->>'pricing_method' IN ('per_item', 'per_hour', 'free') ) UPDATE reservations - SET data = computed.new_data - FROM computed - WHERE reservations.id = computed.id + SET data = COALESCE(data, '{}'::jsonb) || + CASE + WHEN EXISTS (SELECT 1 FROM adata WHERE adata."group" = target) + THEN + ( + SELECT pricing + FROM adata + WHERE adata."group" = target + LIMIT 1 + ) || jsonb_build_object( + 'cost_object', + resources.content->'cost_object' + ) + ELSE + jsonb_build_object( + 'pricing_method', + resources.content->'pricing_method', + 'price_per_hour', + COALESCE( + resources.content->'price_per_hour', + '0.0'::jsonb + ), + 'price_per_item', + COALESCE( + resources.content->'price_per_reservation', + '0.0'::jsonb + ), + 'currency', + resources.content->'currency', + 'cost_object', + resources.content->'cost_object' + ) + END + FROM resources + WHERE resources.id = resource """)) From 0d726b46d0c7bccb2ba35170d5a338502a269a90 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 15:36:13 +0200 Subject: [PATCH 15/16] Add 'free' to the allocation filter --- src/onegov/org/upgrade.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/onegov/org/upgrade.py b/src/onegov/org/upgrade.py index bcb23928a2..a3c4c02fd2 100644 --- a/src/onegov/org/upgrade.py +++ b/src/onegov/org/upgrade.py @@ -1011,8 +1011,9 @@ def refresh_zeroed_reservation_invoices(context: UpgradeContext) -> None: 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 a paying reservation: an allocation priced `per_item`/`per_hour`, - or a `per_item` resource (whose content fallback was the broken one). Only + 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 @@ -1048,7 +1049,7 @@ def refresh_zeroed_reservation_invoices(context: UpgradeContext) -> None: SELECT 1 FROM allocations a WHERE a."group" = r.target AND a.data->>'pricing_method' - IN ('per_item', 'per_hour') + IN ('per_item', 'per_hour', 'free') ) ) """)).scalars().all() From 039f966f9e6775f92fdb68866955457d7234d8c8 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Mon, 24 Aug 2026 15:45:34 +0200 Subject: [PATCH 16/16] quota mirror tset and more --- tests/onegov/org/test_upgrade.py | 91 ++++++++++++++++++++++++ tests/onegov/reservation/test_upgrade.py | 60 +++++++++++++++- 2 files changed, 150 insertions(+), 1 deletion(-) diff --git a/tests/onegov/org/test_upgrade.py b/tests/onegov/org/test_upgrade.py index c40e79a3ad..329b063f83 100644 --- a/tests/onegov/org/test_upgrade.py +++ b/tests/onegov/org/test_upgrade.py @@ -5,6 +5,7 @@ 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 @@ -106,3 +107,93 @@ def test_refresh_zeroed_reservation_invoices(client: Client) -> None: 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 70815d1bff..d49fe04c05 100644 --- a/tests/onegov/reservation/test_upgrade.py +++ b/tests/onegov/reservation/test_upgrade.py @@ -1,7 +1,7 @@ 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 @@ -193,6 +193,64 @@ def expected(res_method: str, res_ppi: float, res_pph: float, 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: