Fix #751 -- calculate payment fees in OrderChangeManager (#752)

* check for payment method instead of order total

* incorporate payment fee diff in totaldiff at oder change

* use fee from model and the correct order total

* add error handling

* do not change paid orders

* OrderChangedManager can only be committed once

* remove prints of stripe secrets

* add tests

* an OrderChangeManager must not be committed multiple times
* A pending free order stays pending after being changed

* comments on paid_to_free logic
This commit is contained in:
Felix Rindt
2018-01-22 12:53:46 +01:00
committed by Raphael Michel
parent 817038563f
commit 78b31149b5
3 changed files with 56 additions and 6 deletions

View File

@@ -10,7 +10,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
from django.db.models import F, Max, Q, Sum
from django.dispatch import receiver
from django.utils.formats import date_format
from django.utils.timezone import make_aware, now
@@ -513,7 +513,7 @@ def _perform_order(event: str, payment_provider: str, position_ids: List[str],
invoice = generate_invoice(order, trigger_pdf=not event.settings.invoice_email_attachment)
# send_mail will trigger PDF generation later
if order.total == Decimal('0.00'):
if order.payment_provider == 'free':
email_template = event.settings.mail_text_order_free
log_entry = 'pretix.event.order.email.order_free'
else:
@@ -680,6 +680,7 @@ class OrderChangeManager:
self.order = order
self.user = user
self.split_order = None
self._committed = False
self._totaldiff = 0
self._quotadiff = Counter()
self._operations = []
@@ -824,7 +825,10 @@ class OrderChangeManager:
raise OrderError(self.error_messages['paid_price_change'])
def _check_paid_to_free(self):
if self.order.total == 0:
if self.order.total == 0 and (self._totaldiff < 0 or (self.split_order and self.split_order.total > 0)):
# if the order becomes free, mark it paid using the 'free' provider
# this could happen if positions have been made cheaper or removed (_totaldiff < 0)
# or positions got split off to a new order (split_order with positive total)
try:
mark_order_paid(
self.order, 'free', send_mail=False, count_waitinglist=False,
@@ -1015,6 +1019,16 @@ class OrderChangeManager:
self.order.total += sum([f.value for f in self.order.fees.all()])
self.order.save()
def _payment_fee_diff(self):
prov = self._get_payment_provider()
if self.order.status != Order.STATUS_PAID and prov:
# payment fees of paid orders do not change
old_fee = OrderFee.objects.filter(order=self.order, fee_type=OrderFee.FEE_TYPE_PAYMENT).aggregate(s=Sum('value'))['s'] or 0
new_total = sum([p.price for p in self.order.positions.all()]) + self._totaldiff
if new_total != 0:
new_fee = prov.calculate_fee(new_total)
self._totaldiff += new_fee - old_fee
def _reissue_invoice(self):
i = self.order.invoices.filter(is_cancellation=False).last()
if i and self._invoice_dirty:
@@ -1062,9 +1076,18 @@ class OrderChangeManager:
logger.exception('Order changed email could not be sent')
def commit(self):
if self._committed:
# an order change can only be committed once
raise OrderError(error_messages['internal'])
self._committed = True
if not self._operations:
# Do nothing
return
# finally, incorporate difference in payment fees
self._payment_fee_diff()
with transaction.atomic():
with self.order.event.lock():
if self.order.status not in (Order.STATUS_PENDING, Order.STATUS_PAID):
@@ -1078,6 +1101,7 @@ class OrderChangeManager:
self._reissue_invoice()
self._clear_tickets_cache()
self._check_paid_to_free()
if self.notify:
self._notify_user(self.order)
if self.split_order:

View File

@@ -204,7 +204,6 @@ class ReturnView(StripeOrderView, View):
prov = self.pprov
prov._init_api()
src = stripe.Source.retrieve(request.GET.get('source'))
print(src.client_secret, request.GET.get('client_secret'))
if src.client_secret != request.GET.get('client_secret'):
messages.error(self.request, _('Sorry, there was an error in the payment process. Please check the link '
'in your emails to continue.'))
@@ -232,7 +231,6 @@ class ReturnView(StripeOrderView, View):
return self._redirect_to_order()
def _redirect_to_order(self):
print(self.request.session.get('payment_stripe_order_secret'), self.order.secret)
if self.request.session.get('payment_stripe_order_secret') != self.order.secret:
messages.error(self.request, _('Sorry, there was an error in the payment process. Please check the link '
'in your emails to continue.'))

View File

@@ -278,6 +278,13 @@ class OrderChangeManagerTests(TestCase):
country=Country('AT')
)
def test_multiple_commits_forbidden(self):
self.ocm.change_price(self.op1, Decimal('10.00'))
self.ocm.commit()
self.ocm.change_price(self.op1, Decimal('42.00'))
with self.assertRaises(OrderError):
self.ocm.commit()
def test_change_subevent_quota_required(self):
self.event.has_subevents = True
self.event.save()
@@ -545,6 +552,21 @@ class OrderChangeManagerTests(TestCase):
assert fee.tax_rate == Decimal('19.00')
assert round_decimal(fee.value * (1 - 100 / (100 + fee.tax_rate))) == fee.tax_value
def test_pending_free_order_stays_pending(self):
self.event.settings.set('tax_rate_default', self.tr19.pk)
self.ocm.change_price(self.op1, Decimal('0.00'))
self.ocm.change_price(self.op2, Decimal('0.00'))
self.ocm.commit()
self.ocm = OrderChangeManager(self.order, None)
self.order.refresh_from_db()
assert self.order.total == Decimal('0.00')
assert self.order.status == Order.STATUS_PAID
self.order.status = Order.STATUS_PENDING
self.ocm.cancel(self.op2)
self.ocm.commit()
self.order.refresh_from_db()
assert self.order.status == Order.STATUS_PENDING
def test_require_pending(self):
self.order.status = Order.STATUS_PAID
self.order.save()
@@ -734,6 +756,7 @@ class OrderChangeManagerTests(TestCase):
ia = self._enable_reverse_charge()
self.ocm.recalculate_taxes()
self.ocm.commit()
self.ocm = OrderChangeManager(self.order, None)
ops = list(self.order.positions.all())
for op in ops:
assert op.price == Decimal('21.50')
@@ -791,6 +814,7 @@ class OrderChangeManagerTests(TestCase):
prov.settings.set('_fee_reverse_calc', False)
self.ocm.change_price(self.op1, Decimal('23.00'))
self.ocm.commit()
self.ocm = OrderChangeManager(self.order, None)
self.order.refresh_from_db()
assert self.order.total == Decimal('47.92')
fee = self.order.fees.get(fee_type=OrderFee.FEE_TYPE_PAYMENT)
@@ -879,6 +903,7 @@ class OrderChangeManagerTests(TestCase):
prov.settings.set('_fee_reverse_calc', False)
self.ocm.recalculate_taxes()
self.ocm.commit()
self.ocm = OrderChangeManager(self.order, None)
self.order.refresh_from_db()
# Check if reverse charge is active
@@ -911,7 +936,6 @@ class OrderChangeManagerTests(TestCase):
# New order
assert self.op2.order != self.order
o2 = self.op2.order
print([p.price for p in o2.positions.all()], [p.value for p in o2.fees.all()])
assert o2.total == Decimal('21.93')
fee = o2.fees.get(fee_type=OrderFee.FEE_TYPE_PAYMENT)
assert fee.value == Decimal('0.43')
@@ -971,6 +995,7 @@ class OrderChangeManagerTests(TestCase):
prov.settings.set('_fee_reverse_calc', False)
self.ocm.change_price(self.op1, Decimal('23.00'))
self.ocm.commit()
self.ocm = OrderChangeManager(self.order, None)
self.order.refresh_from_db()
assert self.order.total == Decimal('47.92')
fee = self.order.fees.get(fee_type=OrderFee.FEE_TYPE_PAYMENT)
@@ -1019,6 +1044,7 @@ class OrderChangeManagerTests(TestCase):
self.event.settings.invoice_include_free = False
self.ocm.change_price(self.op2, Decimal('0.00'))
self.ocm.commit()
self.ocm = OrderChangeManager(self.order, None)
self.op2.refresh_from_db()
self.ocm._invoice_dirty = False
@@ -1038,6 +1064,7 @@ class OrderChangeManagerTests(TestCase):
def test_split_to_original_free(self):
self.ocm.change_price(self.op2, Decimal('0.00'))
self.ocm.commit()
self.ocm = OrderChangeManager(self.order, None)
self.op2.refresh_from_db()
self.ocm.split(self.op1)
@@ -1054,6 +1081,7 @@ class OrderChangeManagerTests(TestCase):
def test_split_to_new_free(self):
self.ocm.change_price(self.op2, Decimal('0.00'))
self.ocm.commit()
self.ocm = OrderChangeManager(self.order, None)
self.op2.refresh_from_db()
self.ocm.split(self.op2)