From 181461df8be913e00a6d18add793caec4b6c92ee Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 18 Oct 2024 12:34:17 +0200 Subject: [PATCH 1/7] :card_file_box: [#4764] Add database field to store reference to price variable A third pricing mode for forms is being added, and for this one, we need to configure on the form which variable may contain the price. If a variable key is set, this implies that price-form-variable is in effect. Otherwise, logic rules will be checked, and finally, the configured product price will be used. The new property needs to be exposed only in the admin-permissions flavour of the form detail endpoint. --- src/openapi.yaml | 25 ++++++++++++--- src/openforms/conf/base.py | 2 +- src/openforms/forms/api/serializers/form.py | 1 + .../0101_form_price_variable_key.py | 31 +++++++++++++++++++ src/openforms/forms/models/form.py | 11 +++++++ 5 files changed, 64 insertions(+), 6 deletions(-) create mode 100644 src/openforms/forms/migrations/0101_form_price_variable_key.py diff --git a/src/openapi.yaml b/src/openapi.yaml index 6559baa1c7..e57f300e8e 100644 --- a/src/openapi.yaml +++ b/src/openapi.yaml @@ -1,7 +1,7 @@ openapi: 3.0.3 info: title: Open Forms API - version: 2.8.0 + version: 2.8.1 description: |2 Open Forms provides an API to manage multi-page or multi-step forms. @@ -1330,6 +1330,7 @@ paths: - `authenticationBackendOptions` - `paymentBackend` - `paymentBackendOptions` + - `priceVariableKey` - `product` - `category` - `theme` @@ -1403,6 +1404,7 @@ paths: - `authenticationBackendOptions` - `paymentBackend` - `paymentBackendOptions` + - `priceVariableKey` - `product` - `category` - `theme` @@ -1920,6 +1922,7 @@ paths: - `authenticationBackendOptions` - `paymentBackend` - `paymentBackendOptions` + - `priceVariableKey` - `product` - `category` - `theme` @@ -1999,6 +2002,7 @@ paths: - `authenticationBackendOptions` - `paymentBackend` - `paymentBackendOptions` + - `priceVariableKey` - `product` - `category` - `theme` @@ -2082,6 +2086,7 @@ paths: - `authenticationBackendOptions` - `paymentBackend` - `paymentBackendOptions` + - `priceVariableKey` - `product` - `category` - `theme` @@ -7576,7 +7581,7 @@ components: Note that this schema is used for both non-admin users filling out forms and admin users designing forms. The fields that are only relevant for admin users are: - `internalName`, `registrationBackends`, `registrationBackend`, `registrationBackendOptions`, `authenticationBackendOptions`, `paymentBackend`, `paymentBackendOptions`, `product`, `category`, `theme`, `activateOn`, `deactivateOn`, `isDeleted`, `submissionConfirmationTemplate`, `askPrivacyConsent`, `askStatementOfTruth`, `submissionsRemovalOptions`, `confirmationEmailTemplate`, `sendConfirmationEmail`, `displayMainWebsiteLink`, `includeConfirmationPageContentInPdf`, `translations`, `brpPersonenRequestOptions`. + `internalName`, `registrationBackends`, `registrationBackend`, `registrationBackendOptions`, `authenticationBackendOptions`, `paymentBackend`, `paymentBackendOptions`, `priceVariableKey`, `product`, `category`, `theme`, `activateOn`, `deactivateOn`, `isDeleted`, `submissionConfirmationTemplate`, `askPrivacyConsent`, `askStatementOfTruth`, `submissionsRemovalOptions`, `confirmationEmailTemplate`, `sendConfirmationEmail`, `displayMainWebsiteLink`, `includeConfirmationPageContentInPdf`, `translations`, `brpPersonenRequestOptions`. properties: uuid: type: string @@ -7639,6 +7644,11 @@ components: items: $ref: '#/components/schemas/PaymentOption' readOnly: true + priceVariableKey: + type: string + description: Key of the variable that contains the calculated submission + price. + pattern: ^(\w|\w[\w.\-]*\w)$ appointmentOptions: allOf: - $ref: '#/components/schemas/AppointmentOptions' @@ -8077,7 +8087,7 @@ components: Note that this schema is used for both non-admin users filling out forms and admin users designing forms. The fields that are only relevant for admin users are: - `internalName`, `registrationBackends`, `registrationBackend`, `registrationBackendOptions`, `authenticationBackendOptions`, `paymentBackend`, `paymentBackendOptions`, `product`, `category`, `theme`, `activateOn`, `deactivateOn`, `isDeleted`, `submissionConfirmationTemplate`, `askPrivacyConsent`, `askStatementOfTruth`, `submissionsRemovalOptions`, `confirmationEmailTemplate`, `sendConfirmationEmail`, `displayMainWebsiteLink`, `includeConfirmationPageContentInPdf`, `translations`, `brpPersonenRequestOptions`. + `internalName`, `registrationBackends`, `registrationBackend`, `registrationBackendOptions`, `authenticationBackendOptions`, `paymentBackend`, `paymentBackendOptions`, `priceVariableKey`, `product`, `category`, `theme`, `activateOn`, `deactivateOn`, `isDeleted`, `submissionConfirmationTemplate`, `askPrivacyConsent`, `askStatementOfTruth`, `submissionsRemovalOptions`, `confirmationEmailTemplate`, `sendConfirmationEmail`, `displayMainWebsiteLink`, `includeConfirmationPageContentInPdf`, `translations`, `brpPersonenRequestOptions`. properties: name: type: string @@ -8248,7 +8258,7 @@ components: Note that this schema is used for both non-admin users filling out forms and admin users designing forms. The fields that are only relevant for admin users are: - `internalName`, `registrationBackends`, `registrationBackend`, `registrationBackendOptions`, `authenticationBackendOptions`, `paymentBackend`, `paymentBackendOptions`, `product`, `category`, `theme`, `activateOn`, `deactivateOn`, `isDeleted`, `submissionConfirmationTemplate`, `askPrivacyConsent`, `askStatementOfTruth`, `submissionsRemovalOptions`, `confirmationEmailTemplate`, `sendConfirmationEmail`, `displayMainWebsiteLink`, `includeConfirmationPageContentInPdf`, `translations`, `brpPersonenRequestOptions`. + `internalName`, `registrationBackends`, `registrationBackend`, `registrationBackendOptions`, `authenticationBackendOptions`, `paymentBackend`, `paymentBackendOptions`, `priceVariableKey`, `product`, `category`, `theme`, `activateOn`, `deactivateOn`, `isDeleted`, `submissionConfirmationTemplate`, `askPrivacyConsent`, `askStatementOfTruth`, `submissionsRemovalOptions`, `confirmationEmailTemplate`, `sendConfirmationEmail`, `displayMainWebsiteLink`, `includeConfirmationPageContentInPdf`, `translations`, `brpPersonenRequestOptions`. properties: name: type: string @@ -9236,7 +9246,7 @@ components: Note that this schema is used for both non-admin users filling out forms and admin users designing forms. The fields that are only relevant for admin users are: - `internalName`, `registrationBackends`, `registrationBackend`, `registrationBackendOptions`, `authenticationBackendOptions`, `paymentBackend`, `paymentBackendOptions`, `product`, `category`, `theme`, `activateOn`, `deactivateOn`, `isDeleted`, `submissionConfirmationTemplate`, `askPrivacyConsent`, `askStatementOfTruth`, `submissionsRemovalOptions`, `confirmationEmailTemplate`, `sendConfirmationEmail`, `displayMainWebsiteLink`, `includeConfirmationPageContentInPdf`, `translations`, `brpPersonenRequestOptions`. + `internalName`, `registrationBackends`, `registrationBackend`, `registrationBackendOptions`, `authenticationBackendOptions`, `paymentBackend`, `paymentBackendOptions`, `priceVariableKey`, `product`, `category`, `theme`, `activateOn`, `deactivateOn`, `isDeleted`, `submissionConfirmationTemplate`, `askPrivacyConsent`, `askStatementOfTruth`, `submissionsRemovalOptions`, `confirmationEmailTemplate`, `sendConfirmationEmail`, `displayMainWebsiteLink`, `includeConfirmationPageContentInPdf`, `translations`, `brpPersonenRequestOptions`. properties: uuid: type: string @@ -9299,6 +9309,11 @@ components: items: $ref: '#/components/schemas/PaymentOption' readOnly: true + priceVariableKey: + type: string + description: Key of the variable that contains the calculated submission + price. + pattern: ^(\w|\w[\w.\-]*\w)$ appointmentOptions: allOf: - $ref: '#/components/schemas/AppointmentOptions' diff --git a/src/openforms/conf/base.py b/src/openforms/conf/base.py index 3cf7a38b32..5ff4d4cf36 100644 --- a/src/openforms/conf/base.py +++ b/src/openforms/conf/base.py @@ -955,7 +955,7 @@ and it plays nice with other available components. """ -API_VERSION = "2.8.0" +API_VERSION = "2.8.1" SPECTACULAR_SETTINGS = { "SCHEMA_PATH_PREFIX": "/api/v2", diff --git a/src/openforms/forms/api/serializers/form.py b/src/openforms/forms/api/serializers/form.py index b91fa097fe..2857e913da 100644 --- a/src/openforms/forms/api/serializers/form.py +++ b/src/openforms/forms/api/serializers/form.py @@ -257,6 +257,7 @@ class Meta: "payment_backend", "payment_backend_options", "payment_options", + "price_variable_key", "appointment_options", "literals", "product", diff --git a/src/openforms/forms/migrations/0101_form_price_variable_key.py b/src/openforms/forms/migrations/0101_form_price_variable_key.py new file mode 100644 index 0000000000..ffc1b746fe --- /dev/null +++ b/src/openforms/forms/migrations/0101_form_price_variable_key.py @@ -0,0 +1,31 @@ +# Generated by Django 4.2.16 on 2024-10-18 10:28 + +import re + +import django.core.validators +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("forms", "0100_merge_20240920_1816"), + ] + + operations = [ + migrations.AddField( + model_name="form", + name="price_variable_key", + field=models.TextField( + blank=True, + help_text="Key of the variable that contains the calculated submission price.", + validators=[ + django.core.validators.RegexValidator( + message="Invalid variable key. It must only contain alphanumeric characters, underscores, dots and dashes and should not be ended by dash or dot.", + regex=re.compile("^(\\w|\\w[\\w.\\-]*\\w)$"), + ) + ], + verbose_name="price variable key", + ), + ), + ] diff --git a/src/openforms/forms/models/form.py b/src/openforms/forms/models/form.py index cf707cc81c..45774b905f 100644 --- a/src/openforms/forms/models/form.py +++ b/src/openforms/forms/models/form.py @@ -23,6 +23,7 @@ from openforms.config.models import GlobalConfiguration from openforms.data_removal.constants import RemovalMethods from openforms.formio.typing import Component +from openforms.formio.validators import variable_key_validator from openforms.payments.fields import PaymentBackendChoiceField from openforms.payments.registry import register as payment_register from openforms.plugins.constants import UNIQUE_ID_MAX_LENGTH @@ -90,6 +91,16 @@ class Form(models.Model): payment_backend_options = models.JSONField( _("payment backend options"), default=dict, blank=True, null=True ) + # XXX a Foreign Key to FormVariable would be nicer, but we can't do this yet since + # the frontend saves the variables *after* the form record itself is saved. + price_variable_key = models.TextField( + _("price variable key"), + blank=True, + help_text=_( + "Key of the variable that contains the calculated submission price." + ), + validators=[variable_key_validator], + ) # authentication authentication_backends = AuthenticationBackendMultiSelectField( From 9c93c26065927a9d47718cd404619ca1224dd8fb Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 18 Oct 2024 13:15:21 +0200 Subject: [PATCH 2/7] :sparkles: [#4764] Calculate the submission price from a variable, if set The fallback behaviour is weird - in principle the UI will make you choose between either variable, logic rules or product price, but the existing code already has some fallback behaviour. I'm not sure if we should crash hard if the reference variable doesn't produce something that can be used as price, or fall back to other behaviours. I'd rather crash hard at runtime. Something to discuss! --- .../submissions/models/submission.py | 2 + src/openforms/submissions/pricing.py | 58 +++++++- .../tests/test_price_calculation.py | 139 ++++++++++++++++++ .../submissions/tests/test_read_submission.py | 36 +++++ 4 files changed, 231 insertions(+), 4 deletions(-) create mode 100644 src/openforms/submissions/tests/test_price_calculation.py diff --git a/src/openforms/submissions/models/submission.py b/src/openforms/submissions/models/submission.py index c3092db628..16370f7ac5 100644 --- a/src/openforms/submissions/models/submission.py +++ b/src/openforms/submissions/models/submission.py @@ -370,6 +370,8 @@ def refresh_from_db(self, *args, **kwargs): super().refresh_from_db(*args, **kwargs) if hasattr(self, "_execution_state"): del self._execution_state + if hasattr(self, "_variables_state"): + del self._variables_state def save_registration_status( self, status: RegistrationStatuses, result: dict, record_attempt: bool = False diff --git a/src/openforms/submissions/pricing.py b/src/openforms/submissions/pricing.py index 3e0c551d4d..73563bf50e 100644 --- a/src/openforms/submissions/pricing.py +++ b/src/openforms/submissions/pricing.py @@ -1,3 +1,6 @@ +from __future__ import annotations + +import decimal import logging from decimal import Decimal from typing import TYPE_CHECKING @@ -11,7 +14,13 @@ logger = logging.getLogger(__name__) -def get_submission_price(submission: "Submission") -> Decimal: +class InvalidPrice(Exception): + def __init__(self, message: str, *args, **kwargs): + self.message = message + super().__init__(message, *args, **kwargs) + + +def get_submission_price(submission: Submission) -> Decimal: """ Calculate the price for a given submission. @@ -32,8 +41,46 @@ def get_submission_price(submission: "Submission") -> Decimal: ), "get_submission_price' may only be called for forms that require payment" form = submission.form - data = submission.data + # + # 1. If a variable key is configured, try that. + # + if var_key := form.price_variable_key: + values_state = submission.load_submission_value_variables_state() + # user defined variables don't show up in `values_state.get_data()` :( so we + # need to access them differently. + # XXX these data structures can be cleaned up - Victorien did an attempt already + # and it revealed it's not easy. + if var_key not in values_state.variables: + # Discussed with DH - it's better to crash hard than the possibly make them + # pay the wrong price. + raise InvalidPrice( + f"No variable '{var_key}' present in the submission data, refusing the " + "temptation to guess.", + ) + value = values_state.get_variable(var_key).value + logger.debug( + "Price for submission %s obtained from variable with key '%s'. Value: %r", + submission.uuid, + var_key, + value, + ) + + invalid_type_error = InvalidPrice( + f"Got an incompatible value type for the price variable '{var_key}': " + f"{type(value)}. We require a value that can be cast to a decimal." + ) + if not isinstance(value, (str, float, int)) or isinstance(value, bool): + raise invalid_type_error + try: + return Decimal(value) + except decimal.InvalidOperation as exc: + raise invalid_type_error from exc + + # + # 2. Check if there are any logic rules defined that match. + # + data = submission.data # test the rules one by one, if relevant price_rules = form.formpricelogic_set.all() for rule in price_rules: @@ -51,9 +98,12 @@ def get_submission_price(submission: "Submission") -> Decimal: # a form return rule.price - # no price rules or no match found -> use linked product + # + # 3. More specific modes didn't produce anything, fall back to the linked product + # price. + # logger.debug( - "Falling back to product price for submission %s after trying %d price rules", + "Falling back to product price for submission %s after trying %d price rules.", submission.uuid, len(price_rules), ) diff --git a/src/openforms/submissions/tests/test_price_calculation.py b/src/openforms/submissions/tests/test_price_calculation.py new file mode 100644 index 0000000000..7ab02684d1 --- /dev/null +++ b/src/openforms/submissions/tests/test_price_calculation.py @@ -0,0 +1,139 @@ +from decimal import Decimal + +from django.test import TestCase + +from openforms.forms.tests.factories import FormPriceLogicFactory, FormVariableFactory +from openforms.variables.constants import FormVariableDataTypes + +from ..pricing import InvalidPrice, get_submission_price +from .factories import SubmissionFactory, SubmissionStepFactory + + +class PriceCalculationTests(TestCase): + + def test_price_from_related_product(self): + submission = SubmissionFactory.create( + completed=True, + form__generate_minimal_setup=True, + form__product__price=Decimal("123.45"), + form__payment_backend="demo", + form__price_variable_key="", + ) + with self.subTest(part="check data setup"): + self.assertTrue(submission.payment_required) + + price = get_submission_price(submission=submission) + + self.assertEqual(price, Decimal("123.45")) + + def test_price_from_logic_rules(self): + submission = SubmissionFactory.create( + form__generate_minimal_setup=True, + form__product__price=Decimal("123.45"), + form__payment_backend="demo", + form__price_variable_key="", + ) + FormVariableFactory.create( + key="test-key", + form=submission.form, + form_definition=submission.form.formstep_set.get().form_definition, + ) + SubmissionStepFactory.create( + submission=submission, + form_step=submission.form.formstep_set.get(), + data={"test-key": "test"}, + ) + FormPriceLogicFactory.create( + form=submission.form, + json_logic_trigger={"==": [{"var": "test-key"}, "test"]}, + price=Decimal("51.15"), + ) + with self.subTest(part="check data setup"): + self.assertTrue(submission.payment_required) + + price = get_submission_price(submission) + + self.assertEqual(price, Decimal("51.15")) + + def test_price_from_form_variable(self): + submission = SubmissionFactory.create( + completed=True, + form__generate_minimal_setup=True, + form__product__price=Decimal("123.45"), + form__payment_backend="demo", + form__price_variable_key="", + ) + FormVariableFactory.create( + user_defined=True, + key="calculatedPrice", + form=submission.form, + data_type=FormVariableDataTypes.float, + # XXX this doesn't play nice with float vs. decimal, but it is what it is + initial_value=420.69, + ) + submission.form.price_variable_key = "calculatedPrice" + submission.form.save() + submission.refresh_from_db() + with self.subTest(part="check data setup"): + self.assertTrue(submission.payment_required) + + price = get_submission_price(submission) + + self.assertEqual(price, Decimal(420.69)) + + def test_price_from_form_variable_error_cases(self): + # # Variable references can be broken in various ways, in those situation we + # # expected hard crashes. + # with self.subTest("broken variable reference"): + # submission = SubmissionFactory.create( + # completed=True, + # form__generate_minimal_setup=False, + # form__product__price=Decimal("123.45"), + # form__payment_backend="demo", + # form__price_variable_key="", + # ) + # submission.form.price_variable_key = "badVariable.reference" + # submission.form.save() + # submission.refresh_from_db() + + # with self.assertRaisesMessage( + # InvalidPrice, + # "No variable 'badVariable.reference' present in the submission data, " + # "refusing the temptation to guess." + # ): + # get_submission_price(submission) + + invalid_values = ( + ("foo", FormVariableDataTypes.string), + (["array"], FormVariableDataTypes.array), + ({"object": "not supported"}, FormVariableDataTypes.object), + (False, FormVariableDataTypes.boolean), + ) + + for index, (value, data_type) in enumerate(invalid_values): + key = f"invalidValue{index}" + with self.subTest("invalid variable values", value=value): + submission = SubmissionFactory.create( + completed=True, + form__generate_minimal_setup=False, + form__product__price=Decimal("123.45"), + form__payment_backend="demo", + form__price_variable_key="", + ) + FormVariableFactory.create( + user_defined=True, + key=key, + form=submission.form, + data_type=data_type, + initial_value=value, + ) + submission.form.price_variable_key = key + submission.form.save() + submission.refresh_from_db() + + with self.assertRaisesMessage( + InvalidPrice, + f"Got an incompatible value type for the price variable '{key}': " + f"{type(value)}. We require a value that can be cast to a decimal.", + ): + get_submission_price(submission) diff --git a/src/openforms/submissions/tests/test_read_submission.py b/src/openforms/submissions/tests/test_read_submission.py index 13067c5e02..fe767c4d18 100644 --- a/src/openforms/submissions/tests/test_read_submission.py +++ b/src/openforms/submissions/tests/test_read_submission.py @@ -227,6 +227,42 @@ def test_submission_payment_information_uses_logic_rules(self): }, ) + def test_submission_payment_information_uses_price_variable(self): + submission = SubmissionFactory.create( + completed=True, + form__generate_minimal_setup=True, + form__product__price=Decimal("123.45"), + form__payment_backend="demo", + form__price_variable_key="", + ) + FormVariableFactory.create( + user_defined=True, + key="calculatedPrice", + form=submission.form, + data_type=FormVariableDataTypes.float, + initial_value=420.69, + ) + submission.form.price_variable_key = "calculatedPrice" + submission.form.save() + submission.refresh_from_db() + submission.calculate_price() + with self.subTest(part="check data setup"): + self.assertTrue(submission.payment_required) + self._add_submission_to_session(submission) + endpoint = reverse("api:submission-detail", kwargs={"uuid": submission.uuid}) + + response = self.client.get(endpoint) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual( + response.json()["payment"], + { + "isRequired": True, + "amount": "420.69", + "hasPaid": False, + }, + ) + @tag("gh-2709") def test_submission_payment_with_logic_using_user_defined_variables(self): submission = SubmissionFactory.from_components( From c7da8accd3055d896f43b726328a54e3cc206862 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 18 Oct 2024 15:05:20 +0200 Subject: [PATCH 3/7] :goal_net: [#4764] Ensure that price calculation errors get logged with the submission --- src/openforms/logging/logevent.py | 16 +++- .../price_calculation_variable_error.txt | 4 + src/openforms/submissions/pricing.py | 89 ++++++++++++------- 3 files changed, 77 insertions(+), 32 deletions(-) create mode 100644 src/openforms/logging/templates/logging/events/price_calculation_variable_error.txt diff --git a/src/openforms/logging/logevent.py b/src/openforms/logging/logevent.py index 685e14cf5c..2dbe321049 100644 --- a/src/openforms/logging/logevent.py +++ b/src/openforms/logging/logevent.py @@ -14,7 +14,7 @@ from openforms.logging.constants import TimelineLogTags from openforms.payments.constants import PaymentStatus from openforms.plugins.plugin import AbstractBasePlugin -from openforms.typing import JSONObject +from openforms.typing import JSONObject, JSONValue if TYPE_CHECKING: from log_outgoing_requests.models import OutgoingRequestsLog @@ -308,6 +308,20 @@ def confirmation_email_skip(submission: Submission): # - - - +def price_calculation_variable_error( + submission: Submission, + variable: str, + error: Exception, + value: JSONValue = None, +): + _create_log( + submission, + "price_calculation_variable_error", + extra_data={"variable": variable, "value": value}, + error=error, + ) + + def payment_flow_start(payment: SubmissionPayment, plugin, from_email: bool = False): _create_log( payment.submission, diff --git a/src/openforms/logging/templates/logging/events/price_calculation_variable_error.txt b/src/openforms/logging/templates/logging/events/price_calculation_variable_error.txt new file mode 100644 index 0000000000..5c2cb0dd8f --- /dev/null +++ b/src/openforms/logging/templates/logging/events/price_calculation_variable_error.txt @@ -0,0 +1,4 @@ +{% load i18n %} +{% blocktrans trimmed with variable=log.extra_data.variable lead=log.fmt_lead error=log.extra_data.error %} + {{ lead }}: Could not get the price from variable '{{ variable }}'. Error: {{ error }}. +{% endblocktrans %}{% if log.extra_data.from_email %} {% trans "Flow entered via email link." %}{% endif %} diff --git a/src/openforms/submissions/pricing.py b/src/openforms/submissions/pricing.py index 73563bf50e..c02b9e3908 100644 --- a/src/openforms/submissions/pricing.py +++ b/src/openforms/submissions/pricing.py @@ -7,6 +7,9 @@ from json_logic import jsonLogic +from openforms.logging import logevent +from openforms.typing import JSONValue + if TYPE_CHECKING: from .models import Submission @@ -15,8 +18,12 @@ class InvalidPrice(Exception): - def __init__(self, message: str, *args, **kwargs): + def __init__( + self, message: str, variable: str, value: JSONValue = None, *args, **kwargs + ): self.message = message + self.variable = variable + self.value = value super().__init__(message, *args, **kwargs) @@ -45,37 +52,19 @@ def get_submission_price(submission: Submission) -> Decimal: # # 1. If a variable key is configured, try that. # - if var_key := form.price_variable_key: - values_state = submission.load_submission_value_variables_state() - # user defined variables don't show up in `values_state.get_data()` :( so we - # need to access them differently. - # XXX these data structures can be cleaned up - Victorien did an attempt already - # and it revealed it's not easy. - if var_key not in values_state.variables: - # Discussed with DH - it's better to crash hard than the possibly make them - # pay the wrong price. - raise InvalidPrice( - f"No variable '{var_key}' present in the submission data, refusing the " - "temptation to guess.", - ) - value = values_state.get_variable(var_key).value - logger.debug( - "Price for submission %s obtained from variable with key '%s'. Value: %r", - submission.uuid, - var_key, - value, + try: + price = _price_from_variable(submission) + except InvalidPrice as exc: + logevent.price_calculation_variable_error( + submission=submission, + variable=exc.variable, + error=exc, + value=exc.value, ) - - invalid_type_error = InvalidPrice( - f"Got an incompatible value type for the price variable '{var_key}': " - f"{type(value)}. We require a value that can be cast to a decimal." - ) - if not isinstance(value, (str, float, int)) or isinstance(value, bool): - raise invalid_type_error - try: - return Decimal(value) - except decimal.InvalidOperation as exc: - raise invalid_type_error from exc + raise + else: + if price is not None: + return price # # 2. Check if there are any logic rules defined that match. @@ -108,3 +97,41 @@ def get_submission_price(submission: Submission) -> Decimal: len(price_rules), ) return form.product.price + + +def _price_from_variable(submission: Submission) -> Decimal | None: + if not (var_key := submission.form.price_variable_key): + return None + + values_state = submission.load_submission_value_variables_state() + # user defined variables don't show up in `values_state.get_data()` :( so we + # need to access them differently. + # XXX these data structures can be cleaned up - Victorien did an attempt already + # and it revealed it's not easy. + if var_key not in values_state.variables: + # Discussed with DH - it's better to crash hard than the possibly make them + # pay the wrong price. + raise InvalidPrice( + f"No variable '{var_key}' present in the submission data, refusing the " + "temptation to guess.", + variable=var_key, + ) + value = values_state.get_variable(var_key).value + logger.debug( + "Price for submission %s obtained from variable with key '%s'. Value: %r", + submission.uuid, + var_key, + value, + ) + + invalid_type_error = InvalidPrice( + f"Got an incompatible value type for the price variable '{var_key}': " + f"{type(value)}. We require a value that can be cast to a decimal.", + variable=var_key, + ) + if not isinstance(value, (str, float, int)) or isinstance(value, bool): + raise invalid_type_error + try: + return Decimal(value) + except decimal.InvalidOperation as exc: + raise invalid_type_error from exc From 32a29c3805b3976d9ea8924e1f9a5f146017eaaf Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 18 Oct 2024 17:38:05 +0200 Subject: [PATCH 4/7] :sparkles: [#4764] Enable setting the submission price calculation to variable The form designer is now able to configure the price calculation mode so that a variable is used to get the final calculated price of the submission. --- .../admin/form_design/PriceLogic.js | 121 +++++++++++++----- .../admin/form_design/form-creation-form.js | 2 + .../tests/test_price_calculation.py | 40 +++--- 3 files changed, 114 insertions(+), 49 deletions(-) diff --git a/src/openforms/js/components/admin/form_design/PriceLogic.js b/src/openforms/js/components/admin/form_design/PriceLogic.js index f9303ad396..650c0ffa09 100644 --- a/src/openforms/js/components/admin/form_design/PriceLogic.js +++ b/src/openforms/js/components/admin/form_design/PriceLogic.js @@ -1,5 +1,5 @@ import PropTypes from 'prop-types'; -import React, {useContext} from 'react'; +import React, {useContext, useState} from 'react'; import {FormattedMessage, defineMessage, useIntl} from 'react-intl'; import ButtonContainer from 'components/admin/forms/ButtonContainer'; @@ -9,6 +9,7 @@ import FormRow from 'components/admin/forms/FormRow'; import {NumberInput} from 'components/admin/forms/Inputs'; import Select from 'components/admin/forms/Select'; import {ValidationErrorContext} from 'components/admin/forms/ValidationErrors'; +import VariableSelection from 'components/admin/forms/VariableSelection'; import {DeleteIcon} from 'components/admin/icons'; import {getTranslatedChoices} from 'utils/i18n'; @@ -32,6 +33,13 @@ const PRICING_MODES = [ defaultMessage: 'Use linked product price', }), ], + [ + 'variable', + defineMessage({ + description: 'variable pricing mode label', + defaultMessage: 'Use a variable for the price', + }), + ], [ 'dynamic', defineMessage({ @@ -41,24 +49,22 @@ const PRICING_MODES = [ ], ]; -const PricingMode = ({hasDynamicPricing, onChange}) => { +const PricingMode = ({mode = 'static', onChange}) => { const intl = useIntl(); return ( - ); }; PricingMode.propTypes = { - hasDynamicPricing: PropTypes.bool.isRequired, + mode: PropTypes.oneOf(PRICING_MODES.map(item => item[0])), onChange: PropTypes.func.isRequired, }; -export const PriceLogic = ({rules = [], onChange, onDelete, onAdd}) => { - const hasDynamicPricing = rules.length > 0; +export const PriceLogic = ({variableKey, rules = [], onChange, onDelete, onAdd, onFieldChange}) => { + const initialPricingMode = + variableKey !== '' ? 'variable' : rules.length > 0 ? 'dynamic' : 'static'; + const [pricingMode, setPricingMode] = useState(initialPricingMode); const validationErrors = parseValidationErrors(useContext(ValidationErrorContext), 'priceRules'); @@ -66,23 +72,51 @@ export const PriceLogic = ({rules = [], onChange, onDelete, onAdd}) => { const onPricingModeChange = event => { const {value} = event.target; - // toggle from static to dynamic -> ensure at least one rule exists - if (value === 'dynamic' && !hasDynamicPricing) { - onAdd(); - // toggle from dynamic to static -> delete all the rules - } else if (value === 'static' && hasDynamicPricing) { - // XXX: iterate in reverse so we delete all rules by removing the last one - // every time. - // State updates in event handlers are batched by React, so removing index 0, 1,... - // in success causes issues since the local `rules` does no langer match the - // parent component state - draft.priceRules.length !== rules.length. - // By reversing, we essentially pop the last element every time which - // works around this. - const maxIndex = rules.length - 1; - for (let offset = 0; offset < rules.length; offset++) { - onDelete(maxIndex - offset); + + const resetVariableKey = () => { + if (variableKey) { + onFieldChange({target: {name: 'form.priceVariableKey', value: ''}}); + } + }; + + const resetRules = () => { + if (rules.length > 0) { + // XXX: iterate in reverse so we delete all rules by removing the last one + // every time. + // State updates in event handlers are batched by React, so removing index 0, 1,... + // in success causes issues since the local `rules` does no langer match the + // parent component state - draft.priceRules.length !== rules.length. + // By reversing, we essentially pop the last element every time which + // works around this. + const maxIndex = rules.length - 1; + for (let offset = 0; offset < rules.length; offset++) { + onDelete(maxIndex - offset); + } + } + }; + + switch (value) { + case 'variable': { + resetRules(); + break; + } + case 'dynamic': { + // toggle from static to dynamic -> ensure at least one rule exists + if (rules.length === 0) { + onAdd(); + } + resetVariableKey(); + break; + } + case 'static': { + // toggle from dynamic to static -> delete all the rules + resetRules(); + resetVariableKey(); + break; } } + + setPricingMode(value); }; return ( @@ -100,7 +134,7 @@ export const PriceLogic = ({rules = [], onChange, onDelete, onAdd}) => { name="pricing.mode" label={} > - + @@ -114,18 +148,47 @@ export const PriceLogic = ({rules = [], onChange, onDelete, onAdd}) => { /> ))} - - - + {pricingMode === 'dynamic' && ( + + + + )} + + {pricingMode === 'variable' && ( + + + } + > + { + // See constant `DATATYPES_CHOICES` in + // src/openforms/js/components/admin/form_design/variables/constants.js + // XXX we *could* consider strings here but then they must be string + // representations of numbers. + return ['int', 'float'].includes(variable.dataType) || variable.key === variableKey; + }} + /> + + + )} ); }; PriceLogic.propTypes = { + variableKey: PropTypes.string.isRequired, rules: PropTypes.arrayOf(PropTypes.object), onChange: PropTypes.func.isRequired, onDelete: PropTypes.func.isRequired, onAdd: PropTypes.func.isRequired, + onFieldChange: PropTypes.func.isRequired, }; const Rule = ({jsonLogicTrigger, price = '', onChange, onDelete, errors = {}}) => { diff --git a/src/openforms/js/components/admin/form_design/form-creation-form.js b/src/openforms/js/components/admin/form_design/form-creation-form.js index eaeb7708d9..6a6752140d 100644 --- a/src/openforms/js/components/admin/form_design/form-creation-form.js +++ b/src/openforms/js/components/admin/form_design/form-creation-form.js @@ -1448,10 +1448,12 @@ const FormCreationForm = ({formUuid, formUrl, formHistoryUrl}) => { onChange={onFieldChange} /> dispatch({type: 'DELETED_PRICE_RULE', payload: {index: index}})} onAdd={() => dispatch({type: 'ADD_PRICE_RULE'})} + onFieldChange={onFieldChange} /> )} diff --git a/src/openforms/submissions/tests/test_price_calculation.py b/src/openforms/submissions/tests/test_price_calculation.py index 7ab02684d1..93f39358a9 100644 --- a/src/openforms/submissions/tests/test_price_calculation.py +++ b/src/openforms/submissions/tests/test_price_calculation.py @@ -82,26 +82,26 @@ def test_price_from_form_variable(self): self.assertEqual(price, Decimal(420.69)) def test_price_from_form_variable_error_cases(self): - # # Variable references can be broken in various ways, in those situation we - # # expected hard crashes. - # with self.subTest("broken variable reference"): - # submission = SubmissionFactory.create( - # completed=True, - # form__generate_minimal_setup=False, - # form__product__price=Decimal("123.45"), - # form__payment_backend="demo", - # form__price_variable_key="", - # ) - # submission.form.price_variable_key = "badVariable.reference" - # submission.form.save() - # submission.refresh_from_db() - - # with self.assertRaisesMessage( - # InvalidPrice, - # "No variable 'badVariable.reference' present in the submission data, " - # "refusing the temptation to guess." - # ): - # get_submission_price(submission) + # Variable references can be broken in various ways, in those situation we + # expected hard crashes. + with self.subTest("broken variable reference"): + submission = SubmissionFactory.create( + completed=True, + form__generate_minimal_setup=False, + form__product__price=Decimal("123.45"), + form__payment_backend="demo", + form__price_variable_key="", + ) + submission.form.price_variable_key = "badVariable.reference" + submission.form.save() + submission.refresh_from_db() + + with self.assertRaisesMessage( + InvalidPrice, + "No variable 'badVariable.reference' present in the submission data, " + "refusing the temptation to guess.", + ): + get_submission_price(submission) invalid_values = ( ("foo", FormVariableDataTypes.string), From 3686f849e5ef2284e69651b4278e840b7d688685 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 18 Oct 2024 17:39:22 +0200 Subject: [PATCH 5/7] :globe_with_meridians: [#4764] Update Dutch translations for price variable selection --- src/openforms/js/compiled-lang/en.json | 36 ++++++++++++++++++++++++++ src/openforms/js/compiled-lang/nl.json | 36 ++++++++++++++++++++++++++ src/openforms/js/lang/en.json | 10 +++++++ src/openforms/js/lang/nl.json | 10 +++++++ 4 files changed, 92 insertions(+) diff --git a/src/openforms/js/compiled-lang/en.json b/src/openforms/js/compiled-lang/en.json index 88cf887f26..1129fbe0ed 100644 --- a/src/openforms/js/compiled-lang/en.json +++ b/src/openforms/js/compiled-lang/en.json @@ -923,6 +923,12 @@ "value": "The text that will be displayed in the form step to save the current information. Leave blank to get value from global configuration." } ], + "8M403q": [ + { + "type": 0, + "value": "Soft required fields should be filled out, but empty values don't block the users' progress. Sometimes this is needed for legal reasons. A component cannot be hard and soft required at the same time." + } + ], "8NbOpb": [ { "type": 0, @@ -1455,6 +1461,12 @@ "value": "Select the allowed authentication plugins to log in at the start of the form." } ], + "Ckzpff": [ + { + "type": 0, + "value": "Use a variable for the price" + } + ], "CtjZFq": [ { "type": 0, @@ -2703,6 +2715,12 @@ "value": "Select existing form definition" } ], + "QL4SGQ": [ + { + "type": 0, + "value": "Soft required" + } + ], "QLTh2N": [ { "type": 0, @@ -4783,6 +4801,12 @@ "value": "Session will expire" } ], + "jtWzSW": [ + { + "type": 0, + "value": "option 2: € 15,99" + } + ], "k1+ljn": [ { "type": 0, @@ -5497,6 +5521,12 @@ "value": "Regular expression for city" } ], + "sxdJ/8": [ + { + "type": 0, + "value": "option 1: € 10,99" + } + ], "t5fg/K": [ { "type": 0, @@ -5835,6 +5865,12 @@ "value": "Cancel" } ], + "x6oxfg": [ + { + "type": 0, + "value": "Variable" + } + ], "xI6md8": [ { "type": 0, diff --git a/src/openforms/js/compiled-lang/nl.json b/src/openforms/js/compiled-lang/nl.json index 6c663add05..4a819d6bea 100644 --- a/src/openforms/js/compiled-lang/nl.json +++ b/src/openforms/js/compiled-lang/nl.json @@ -927,6 +927,12 @@ "value": "De tekst op de knop om de gegevens van de huidige stap op te slaan en het formulier te onderbreken. Laat dit veld leeg om de standaardinstelling te gebruiken." } ], + "8M403q": [ + { + "type": 0, + "value": "Aangeraden velden moeten in principe ingevuld worden, maar ontbrekende waarden blokkeren de voortgang niet. Dit is soms nodig voor juridische redenen. Een component kan niet tegelijk verplicht en aangeraden zijn." + } + ], "8NbOpb": [ { "type": 0, @@ -1459,6 +1465,12 @@ "value": "Selecteer de toegestane authenticatie-plugins om in te loggen aan het begin van het formulier." } ], + "Ckzpff": [ + { + "type": 0, + "value": "Gebruik een variabele" + } + ], "CtjZFq": [ { "type": 0, @@ -2720,6 +2732,12 @@ "value": "Selecteer een bestaande formulierdefinitie" } ], + "QL4SGQ": [ + { + "type": 0, + "value": "Aangeraden (niet-blokkerend verplicht)" + } + ], "QLTh2N": [ { "type": 0, @@ -4805,6 +4823,12 @@ "value": "Sessie gaat vervallen" } ], + "jtWzSW": [ + { + "type": 0, + "value": "optie 2: € 15,99" + } + ], "k1+ljn": [ { "type": 0, @@ -5519,6 +5543,12 @@ "value": "Reguliere expressie" } ], + "sxdJ/8": [ + { + "type": 0, + "value": "optie 1: € 10,99" + } + ], "t5fg/K": [ { "type": 0, @@ -5857,6 +5887,12 @@ "value": "Annuleren" } ], + "x6oxfg": [ + { + "type": 0, + "value": "Variabele" + } + ], "xI6md8": [ { "type": 0, diff --git a/src/openforms/js/lang/en.json b/src/openforms/js/lang/en.json index e756bc3c5b..da39e0f709 100644 --- a/src/openforms/js/lang/en.json +++ b/src/openforms/js/lang/en.json @@ -594,6 +594,11 @@ "description": "Auth plugin field help text", "originalDefault": "Select the allowed authentication plugins to log in at the start of the form." }, + "Ckzpff": { + "defaultMessage": "Use a variable for the price", + "description": "variable pricing mode label", + "originalDefault": "Use a variable for the price" + }, "CtjZFq": { "defaultMessage": "Submission allowed", "description": "Form submissionAllowed field label", @@ -2634,6 +2639,11 @@ "description": "JSON editor: cancel variable edit", "originalDefault": "Cancel" }, + "x6oxfg": { + "defaultMessage": "Variable", + "description": "Price variable label", + "originalDefault": "Variable" + }, "xJBMaf": { "defaultMessage": "Submission confirmation template", "description": "Submission confirmation fieldset title", diff --git a/src/openforms/js/lang/nl.json b/src/openforms/js/lang/nl.json index 8a8810a2d5..69b4252c31 100644 --- a/src/openforms/js/lang/nl.json +++ b/src/openforms/js/lang/nl.json @@ -599,6 +599,11 @@ "description": "Auth plugin field help text", "originalDefault": "Select the allowed authentication plugins to log in at the start of the form." }, + "Ckzpff": { + "defaultMessage": "Gebruik een variabele", + "description": "variable pricing mode label", + "originalDefault": "Use a variable for the price" + }, "CtjZFq": { "defaultMessage": "Inzenden toegestaan", "description": "Form submissionAllowed field label", @@ -2653,6 +2658,11 @@ "description": "JSON editor: cancel variable edit", "originalDefault": "Cancel" }, + "x6oxfg": { + "defaultMessage": "Variabele", + "description": "Price variable label", + "originalDefault": "Variable" + }, "xJBMaf": { "defaultMessage": "Sjabloon bevestigingspagina", "description": "Submission confirmation fieldset title", From 9bd27380e418b75a53a664b60e6f794802cce57f Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 18 Oct 2024 17:51:08 +0200 Subject: [PATCH 6/7] :pencil: [#4764] Update documentation about products and payment Updated to mention the new option. --- docs/manual/forms/basics.rst | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/docs/manual/forms/basics.rst b/docs/manual/forms/basics.rst index b4134f4bcb..83a40cf810 100644 --- a/docs/manual/forms/basics.rst +++ b/docs/manual/forms/basics.rst @@ -199,10 +199,37 @@ product bevat een prijs die gebruikt kan worden als betaald moet worden voor het product. Betaling kan ingesteld worden door de juiste **Betaalprovider** te selecteren. -Ten slotte kunt u ervoor kiezen om de prijs van het gekoppeld product te -gebruiken of logica regels op te stellen voor het bepalen van de prijs. Dit -laatste kunt u instellen onder **Prijslogica**. De **Prijslogica** volgt verder -dezelfde regels als reguliere **Logica**. +Er zijn drie manieren om de prijs van een inzending te bepalen: + +**Gebruik de prijs van het gekoppeld product** + +Dit is de meest eenvoudige variant. Er geldt een vaste prijs, die ingesteld wordt op +het product. + +**Gebruik een variabele** + +Dit is de meest flexibele variant - met behulp van normale **Logica** kan je de waarde +van een variabele zetten om de prijs te berekenen. In het formulier wijs je naar deze +variabele en de waarde van de variabele wordt als prijs gebruikt. Je kan enkel +variabelen selecteren die een numeriek gegevenstype hebben (integer, float). + +Let op - er moet altijd een prijs ingesteld staan op het product om de betalingsmodule +te activeren. + +.. warning:: Deze methode is foutgevoelig - indien de waarde van de variabele niet + geschikt is om als prijs te gebruiken of er komt geen geldig getal uit, dan zal + de eindgebruiker fouten in het formulier ervaren. Zorg ervoor dat de formulieren + goed getest zijn! + +.. versionadded:: 2.8.1 Uitzonderlijk is deze functionaliteit in een patch-release + toegevoegd. Versie 2.8.0 en ouder hebben deze functionaliteit niet. + +**Gebruik prijslogica** + +Voor eenvoudige condities kan je prijslogic instellen. Onder een bepaalde conditie geldt +een bepaalde, vaste, prijs. Indien aan geen enkele conditie voldaan is, dan wordt de +prijs van het gekoppeld product gebruikt. De **Prijslogica** volgt verder dezelfde +regels als reguliere **Logica**. Zie ook: :ref:`configuration_payment_index` From 10404b2f3ffb58304e273388ecca7b9fba4cd057 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 18 Oct 2024 18:09:56 +0200 Subject: [PATCH 7/7] :poop: [#4764] Prevent log record spam when viewing a submission in the admin --- src/openforms/submissions/admin.py | 10 ++++++- src/openforms/submissions/pricing.py | 4 +++ src/openforms/submissions/tests/test_admin.py | 28 +++++++++++++++++++ 3 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/openforms/submissions/admin.py b/src/openforms/submissions/admin.py index 28916eacb6..f764a2be57 100644 --- a/src/openforms/submissions/admin.py +++ b/src/openforms/submissions/admin.py @@ -34,6 +34,7 @@ SubmissionValueVariable, TemporaryFileUpload, ) +from .pricing import InvalidPrice from .tasks import on_post_submission_event @@ -381,7 +382,14 @@ def get_price(self, obj): @admin.display(description=_("Payment required"), boolean=True) def get_payment_required(self, obj): - return obj.payment_required + obj._in_admin_display = True + try: + return obj.payment_required + # InvalidPrice means that pricing/payment was set up and configured, but a + # semi-expected crash happened when trying to calculate the price because of + # mis-configuration. + except InvalidPrice: + return True def change_view(self, request, object_id, form_url="", extra_context=None): submission = self.get_object(request, object_id) diff --git a/src/openforms/submissions/pricing.py b/src/openforms/submissions/pricing.py index c02b9e3908..173c6a25ab 100644 --- a/src/openforms/submissions/pricing.py +++ b/src/openforms/submissions/pricing.py @@ -55,6 +55,10 @@ def get_submission_price(submission: Submission) -> Decimal: try: price = _price_from_variable(submission) except InvalidPrice as exc: + # Dirty, dirty hack - don't create new log records when viewing this in the + # admin. + if getattr(submission, "_in_admin_display", False): # pragma: no cover + raise logevent.price_calculation_variable_error( submission=submission, variable=exc.variable, diff --git a/src/openforms/submissions/tests/test_admin.py b/src/openforms/submissions/tests/test_admin.py index 7a2e051578..8d8bf98f53 100644 --- a/src/openforms/submissions/tests/test_admin.py +++ b/src/openforms/submissions/tests/test_admin.py @@ -1,3 +1,4 @@ +from decimal import Decimal from unittest.mock import patch from django.contrib.admin import AdminSite @@ -11,9 +12,11 @@ from maykin_2fa.test import disable_admin_mfa from openforms.accounts.tests.factories import UserFactory +from openforms.forms.tests.factories import FormVariableFactory from openforms.logging.logevent import submission_start from openforms.logging.models import TimelineLogProxy from openforms.submissions.models.submission import Submission +from openforms.variables.constants import FormVariableDataTypes from ...config.models import GlobalConfiguration from ..admin import SubmissionAdmin, SubmissionTimeListFilter @@ -220,6 +223,31 @@ def test_change_view(self): ) self.app.get(change_url, user=self.user, status=404) + def test_change_view_with_broken_price_variable_config(self): + submission = SubmissionFactory.create( + completed=True, + form__generate_minimal_setup=False, + form__product__price=Decimal("123.45"), + form__payment_backend="demo", + form__price_variable_key="", + ) + FormVariableFactory.create( + user_defined=True, + key="calculatedPrice", + form=submission.form, + data_type=FormVariableDataTypes.string, + initial_value="bwoken", + ) + submission.form.price_variable_key = "calculatedPrice" + submission.form.save() + change_url = reverse( + "admin:submissions_submission_change", kwargs={"object_id": submission.pk} + ) + + change_page = self.app.get(change_url, user=self.user) + + self.assertEqual(change_page.status_code, 200) + class TestSubmissionTimeListFilterAdmin(TestCase): def test_time_filtering(self):