Bank transfer: Properly deal with fees of aborted payment methods

src/pretix/base/services/orders.py

@@ -9,7 +9,7 @@ import pytz
 from celery.exceptions import MaxRetriesExceededError
 from django.conf import settings
 from django.db import transaction
-from django.db.models import F, Max, Q, Sum
+from django.db.models import Exists, F, Max, OuterRef, Q, Sum
 from django.db.models.functions import Greatest
 from django.dispatch import receiver
 from django.utils.formats import date_format
@@ -1432,3 +1432,49 @@ def cancel_order(self, order: int, user: int=None, send_mail: bool=True, api_tok
             self.retry()
     except (MaxRetriesExceededError, LockTimeoutException):
         raise OrderError(error_messages['busy'])
+
+
+def change_payment_provider(order: Order, payment_provider, amount=None):
+    e = OrderPayment.objects.filter(fee=OuterRef('pk'), state__in=(OrderPayment.PAYMENT_STATE_CONFIRMED,
+                                                                   OrderPayment.PAYMENT_STATE_REFUNDED))
+    open_fees = list(
+        order.fees.annotate(has_p=Exists(e)).filter(
+            Q(fee_type=OrderFee.FEE_TYPE_PAYMENT) & ~Q(has_p=True)
+        )
+    )
+    if open_fees:
+        fee = open_fees[0]
+        if len(open_fees) > 1:
+            for f in open_fees[1:]:
+                f.delete()
+    else:
+        fee = OrderFee(fee_type=OrderFee.FEE_TYPE_PAYMENT, value=Decimal('0.00'), order=order)
+    old_fee = fee.value
+
+    new_fee = payment_provider.calculate_fee(
+        order.pending_sum - old_fee if amount is None else amount
+    )
+    with transaction.atomic():
+        if new_fee:
+            fee.value = new_fee
+            fee.internal_type = payment_provider.identifier
+            fee._calculate_tax()
+            fee.save()
+        else:
+            if fee.pk:
+                fee.delete()
+            fee = None
+
+    open_payment = None
+    lp = order.payments.exclude(provider=payment_provider.identifier).last()
+    if lp and lp.state not in (OrderPayment.PAYMENT_STATE_CONFIRMED, OrderPayment.PAYMENT_STATE_REFUNDED):
+        open_payment = lp
+
+    if open_payment and open_payment.state in (OrderPayment.PAYMENT_STATE_PENDING,
+                                               OrderPayment.PAYMENT_STATE_CREATED):
+        open_payment.state = OrderPayment.PAYMENT_STATE_CANCELED
+        open_payment.save(update_fields=['state'])
+
+    order.total = (order.positions.aggregate(sum=Sum('price'))['sum'] or 0) + (order.fees.aggregate(sum=Sum('value'))['sum'] or 0)
+    order.save(update_fields=['total'])
+    return old_fee, new_fee, fee

src/pretix/plugins/banktransfer/tasks.py

@@ -16,6 +16,7 @@ from pretix.base.models import (
 )
 from pretix.base.services.locking import LockTimeoutException
 from pretix.base.services.mail import SendMailException
+from pretix.base.services.orders import change_payment_provider
 from pretix.base.services.tasks import TransactionAwareTask
 from pretix.celery_app import app
 from pretix.multidomain.urlreverse import build_absolute_uri
@@ -25,6 +26,38 @@ from .models import BankImportJob, BankTransaction
 logger = logging.getLogger(__name__)
 
 
+def notify_incomplete_payment(o: Order):
+    with language(o.locale):
+        tz = pytz.timezone(o.event.settings.get('timezone', settings.TIME_ZONE))
+        try:
+            invoice_name = o.invoice_address.name
+            invoice_company = o.invoice_address.company
+        except InvoiceAddress.DoesNotExist:
+            invoice_name = ""
+            invoice_company = ""
+        email_template = o.event.settings.mail_text_order_expire_warning
+        email_context = {
+            'event': o.event.name,
+            'url': build_absolute_uri(o.event, 'presale:event.order', kwargs={
+                'order': o.code,
+                'secret': o.secret
+            }),
+            'expire_date': date_format(o.expires.astimezone(tz), 'SHORT_DATE_FORMAT'),
+            'invoice_name': invoice_name,
+            'invoice_company': invoice_company,
+        }
+        email_subject = ugettext('Your order received an incomplete payment: %(code)s') % {'code': o.code}
+
+        try:
+            o.send_mail(
+                email_subject, email_template, email_context,
+                'pretix.event.order.email.expire_warning_sent'
+            )
+        except SendMailException:
+            logger.exception('Reminder email could not be sent')
+
+
+@transaction.atomic
 def _handle_transaction(trans: BankTransaction, code: str, event: Event=None, organizer: Organizer=None,
                         slug: str=None):
     if event:
@@ -59,20 +92,28 @@ def _handle_transaction(trans: BankTransaction, code: str, event: Event=None, or
         trans.state = BankTransaction.STATE_ERROR
         trans.message = ugettext_noop('The order has already been canceled.')
     else:
-        p = trans.order.payments.get_or_create(
+        p, created = trans.order.payments.get_or_create(
             amount=trans.amount,
             provider='banktransfer',
             state__in=(OrderPayment.PAYMENT_STATE_CREATED, OrderPayment.PAYMENT_STATE_PENDING),
             defaults={
                 'state': OrderPayment.PAYMENT_STATE_CREATED,
             }
-        )[0]
+        )
         p.info_data = {
             'reference': trans.reference,
             'date': trans.date,
             'payer': trans.payer,
             'trans_id': trans.pk
         }
+
+        if created:
+            # We're perform a payment method switchign on-demand here
+            old_fee, new_fee, fee = change_payment_provider(trans.order, p.payment_provider, p.amount)  # noqa
+            if fee:
+                p.fee = fee
+                p.save(update_fields=['fee'])
+
         try:
             p.confirm()
         except Quota.QuotaExceededException:
@@ -97,35 +138,7 @@ def _handle_transaction(trans: BankTransaction, code: str, event: Event=None, or
             o = trans.order
             o.refresh_from_db()
             if o.pending_sum > Decimal('0.00') and o.status == Order.STATUS_PENDING:
-                print("send mail")
-                with language(o.locale):
-                    tz = pytz.timezone(o.event.settings.get('timezone', settings.TIME_ZONE))
-                    try:
-                        invoice_name = o.invoice_address.name
-                        invoice_company = o.invoice_address.company
-                    except InvoiceAddress.DoesNotExist:
-                        invoice_name = ""
-                        invoice_company = ""
-                    email_template = o.event.settings.mail_text_order_expire_warning
-                    email_context = {
-                        'event': o.event.name,
-                        'url': build_absolute_uri(o.event, 'presale:event.order', kwargs={
-                            'order': o.code,
-                            'secret': o.secret
-                        }),
-                        'expire_date': date_format(o.expires.astimezone(tz), 'SHORT_DATE_FORMAT'),
-                        'invoice_name': invoice_name,
-                        'invoice_company': invoice_company,
-                    }
-                    email_subject = ugettext('Your order received an incomplete payment: %(code)s') % {'code': o.code}
-
-                    try:
-                        o.send_mail(
-                            email_subject, email_template, email_context,
-                            'pretix.event.order.email.expire_warning_sent'
-                        )
-                    except SendMailException:
-                        logger.exception('Reminder email could not be sent')
+                notify_incomplete_payment(o)
 
     trans.save()
 
@@ -197,13 +210,11 @@ def process_banktransfers(self, job: int, data: list) -> None:
                 if match:
                     if job.event:
                         code = match.group(1)
-                        with transaction.atomic():
-                            _handle_transaction(trans, code, event=job.event)
+                        _handle_transaction(trans, code, event=job.event)
                     else:
                         slug = match.group(1)
                         code = match.group(2)
-                        with transaction.atomic():
-                            _handle_transaction(trans, code, organizer=job.organizer, slug=slug)
+                        _handle_transaction(trans, code, organizer=job.organizer, slug=slug)
                 else:
                     trans.state = BankTransaction.STATE_NOMATCH
                     trans.save()

src/pretix/presale/checkoutflow.py

@@ -440,7 +440,6 @@ class QuestionsStep(QuestionsViewMixin, CartMixin, TemplateFlowStep):
                 )
 
             for q in cp.item.questions_to_ask:
-                print("question", q, "is required", question_is_required(q), "has answer", q.id in answ)
                 if question_is_required(q) and not answ.get(q.id):
                     if warn:
                         messages.warning(request, _('Please fill in answers to all required questions.'))

src/pretix/presale/views/order.py

@@ -25,7 +25,7 @@ from pretix.base.services.invoices import (
     generate_cancellation, generate_invoice, invoice_pdf, invoice_pdf_task,
     invoice_qualified,
 )
-from pretix.base.services.orders import cancel_order
+from pretix.base.services.orders import cancel_order, change_payment_provider
 from pretix.base.services.tickets import generate
 from pretix.base.signals import allow_ticket_download, register_ticket_outputs
 from pretix.base.views.mixins import OrderQuestionsViewMixin
@@ -359,7 +359,7 @@ class OrderPayChangeMethod(EventViewMixin, OrderDetailMixin, TemplateView):
     def open_fees(self):
         e = OrderPayment.objects.filter(
             fee=OuterRef('pk'),
-            state=OrderPayment.PAYMENT_STATE_CONFIRMED
+            state__in=(OrderPayment.PAYMENT_STATE_CONFIRMED, OrderPayment.PAYMENT_STATE_REFUNDED)
         )
         return self.order.fees.annotate(has_p=Exists(e)).filter(
             Q(fee_type=OrderFee.FEE_TYPE_PAYMENT) & ~Q(has_p=True)
@@ -374,7 +374,7 @@ class OrderPayChangeMethod(EventViewMixin, OrderDetailMixin, TemplateView):
 
     @cached_property
     def _position_sum(self):
-        return self.order.positions.aggregate(sum=Sum('price'))['sum']
+        return self.order.positions.aggregate(sum=Sum('price'))['sum'] or Decimal('0.00')
 
     @cached_property
     def provider_forms(self):
@@ -407,35 +407,8 @@ class OrderPayChangeMethod(EventViewMixin, OrderDetailMixin, TemplateView):
                 request.session['payment'] = p['provider'].identifier
                 request.session['payment_change_{}'.format(self.order.pk)] = '1'
 
-                fees = list(self.open_fees)
-                if fees:
-                    fee = fees[0]
-                    if len(fees) > 1:
-                        for f in fees[1:]:
-                            f.delete()
-                else:
-                    fee = OrderFee(fee_type=OrderFee.FEE_TYPE_PAYMENT, value=Decimal('0.00'), order=self.order)
-                old_fee = fee.value
-
-                new_fee = p['provider'].calculate_fee(self.order.pending_sum - old_fee)
                 with transaction.atomic():
-                    if new_fee:
-                        fee.value = new_fee
-                        fee.internal_type = p['provider'].identifier
-                        fee._calculate_tax()
-                        fee.save()
-                    else:
-                        if fee.pk:
-                            fee.delete()
-                        fee = None
-
-                    if self.open_payment and self.open_payment.state in (OrderPayment.PAYMENT_STATE_PENDING,
-                                                                         OrderPayment.PAYMENT_STATE_CREATED):
-                        self.open_payment.state = OrderPayment.PAYMENT_STATE_CANCELED
-                        self.open_payment.save(update_fields=['state'])
-
-                    self.order.total = self._position_sum + (self.order.fees.aggregate(sum=Sum('value'))['sum'] or 0)
-                    self.order.save(update_fields=['total'])
+                    old_fee, new_fee, fee = change_payment_provider(self.order, p['provider'], None)
                     newpayment = self.order.payments.create(
                         state=OrderPayment.PAYMENT_STATE_CREATED,
                         provider=p['provider'].identifier,

src/tests/plugins/banktransfer/test_import.py

@@ -8,8 +8,8 @@ from django.core.files.uploadedfile import SimpleUploadedFile
 from django.utils.timezone import now
 
 from pretix.base.models import (
-    Event, Item, Order, OrderPayment, OrderPosition, Organizer, Quota, Team,
-    User,
+    Event, Item, Order, OrderFee, OrderPayment, OrderPosition, Organizer,
+    Quota, Team, User,
 )
 from pretix.plugins.banktransfer.models import BankImportJob
 from pretix.plugins.banktransfer.tasks import process_banktransfers
@@ -20,7 +20,7 @@ def env():
     o = Organizer.objects.create(name='Dummy', slug='dummy')
     event = Event.objects.create(
         organizer=o, name='Dummy', slug='dummy',
-        date_from=now(), plugins='pretix.plugins.banktransfer'
+        date_from=now(), plugins='pretix.plugins.banktransfer,pretix.plugins.paypal'
     )
     user = User.objects.create_user('dummy@dummy.dummy', 'dummy')
     t = Team.objects.create(organizer=event.organizer, can_view_orders=True, can_change_orders=True)
@@ -338,3 +338,84 @@ Buchungstag;Valuta;Buchungstext;Auftraggeber / Empfänger;Verwendungszweck;Betra
         data[inp.attrs['name']] = inp.text
     r = client.post('/control/event/dummy/dummy/banktransfer/import/', data)
     assert '/job/' in r['Location']
+
+
+@pytest.mark.django_db
+def test_pending_paypal_drop_fee(env, job):
+    fee = env[2].fees.create(
+        fee_type=OrderFee.FEE_TYPE_PAYMENT, value=Decimal('2.00')
+    )
+    env[2].total += Decimal('2.00')
+    env[2].save()
+    p = env[2].payments.create(
+        provider='paypal',
+        state=OrderPayment.PAYMENT_STATE_PENDING,
+        fee=fee,
+        amount=env[2].total
+    )
+    process_banktransfers(job, [{
+        'payer': 'Karla Kundin',
+        'reference': 'Bestellung DUMMY1234S',
+        'date': '2016-01-26',
+        'amount': '23.00'
+    }])
+    env[2].refresh_from_db()
+    assert env[2].status == Order.STATUS_PAID
+    assert env[2].fees.count() == 0
+    assert env[2].total == Decimal('23.00')
+    p.refresh_from_db()
+    assert p.state == OrderPayment.PAYMENT_STATE_CANCELED
+
+
+@pytest.mark.django_db
+def test_pending_paypal_replace_fee_included(env, job):
+    env[0].settings.set('payment_banktransfer__fee_abs', '1.00')
+    fee = env[2].fees.create(
+        fee_type=OrderFee.FEE_TYPE_PAYMENT, value=Decimal('2.00')
+    )
+    env[2].total += Decimal('2.00')
+    env[2].save()
+    env[2].payments.create(
+        provider='paypal',
+        state=OrderPayment.PAYMENT_STATE_PENDING,
+        fee=fee,
+        amount=env[2].total
+    )
+    process_banktransfers(job, [{
+        'payer': 'Karla Kundin',
+        'reference': 'Bestellung DUMMY1234S',
+        'date': '2016-01-26',
+        'amount': '24.00'
+    }])
+    env[2].refresh_from_db()
+    assert env[2].status == Order.STATUS_PAID
+    assert env[2].fees.count() == 1
+    assert env[2].fees.last().value == Decimal('1.00')
+    assert env[2].total == Decimal('24.00')
+
+
+@pytest.mark.django_db
+def test_pending_paypal_replace_fee_missing(env, job):
+    env[0].settings.set('payment_banktransfer__fee_abs', '1.00')
+    fee = env[2].fees.create(
+        fee_type=OrderFee.FEE_TYPE_PAYMENT, value=Decimal('2.00')
+    )
+    env[2].total += Decimal('2.00')
+    env[2].save()
+    env[2].payments.create(
+        provider='paypal',
+        state=OrderPayment.PAYMENT_STATE_PENDING,
+        fee=fee,
+        amount=env[2].total
+    )
+    process_banktransfers(job, [{
+        'payer': 'Karla Kundin',
+        'reference': 'Bestellung DUMMY1234S',
+        'date': '2016-01-26',
+        'amount': '23.00'
+    }])
+    env[2].refresh_from_db()
+    assert env[2].status == Order.STATUS_PENDING
+    assert env[2].fees.count() == 1
+    assert env[2].fees.last().value == Decimal('1.00')
+    assert env[2].total == Decimal('24.00')