From 89229a3248e9eb3ca8f39298048eb5df2c26516b Mon Sep 17 00:00:00 2001 From: Changaco Date: Sat, 17 Aug 2024 12:58:40 +0200 Subject: [PATCH 1/4] refactor how notification counts are logged --- liberapay/cron.py | 3 ++- liberapay/models/participant.py | 6 ++++++ liberapay/payin/cron.py | 32 +++++++++++++++++--------------- 3 files changed, 25 insertions(+), 16 deletions(-) diff --git a/liberapay/cron.py b/liberapay/cron.py index 4b9526cfb..458f45e60 100644 --- a/liberapay/cron.py +++ b/liberapay/cron.py @@ -1,4 +1,5 @@ from collections import namedtuple +from contextvars import copy_context from datetime import timedelta import logging import threading @@ -165,7 +166,7 @@ def f(): if break_before_call(): break self.running = True - r = self.func() + r = copy_context().run(self.func) if break_after_call(): break except Exception as e: diff --git a/liberapay/models/participant.py b/liberapay/models/participant.py index 0f5732a73..45aa1208c 100644 --- a/liberapay/models/participant.py +++ b/liberapay/models/participant.py @@ -1,5 +1,6 @@ from base64 import b64decode, b64encode from collections import defaultdict +from contextvars import ContextVar from datetime import date, timedelta from decimal import Decimal from email.utils import formataddr @@ -115,6 +116,8 @@ class Participant(Model, MixinTeam): ANON = False EMAIL_VERIFICATION_TIMEOUT = EMAIL_VERIFICATION_TIMEOUT + notification_counts = ContextVar('notification_counts') + session = None def __eq__(self, other): @@ -1410,6 +1413,9 @@ def notify(self, event, force_email=False, email=True, web=True, idem_key=None, VALUES (%(p_id)s, %(event)s, %(context)s, %(web)s, %(email)s, %(email_status)s, %(idem_key)s) RETURNING id; """, locals()) + notif_counts = self.notification_counts.get(None) + if notif_counts is not None: + notif_counts[event] += 1 if not web: return n_id self.set_attributes(pending_notifs=self.pending_notifs + 1) diff --git a/liberapay/payin/cron.py b/liberapay/payin/cron.py index faceb204d..cdde5d540 100644 --- a/liberapay/payin/cron.py +++ b/liberapay/payin/cron.py @@ -1,5 +1,6 @@ from collections import defaultdict from datetime import date +from functools import wraps from operator import itemgetter from time import sleep @@ -12,6 +13,7 @@ RecipientAccountSuspended, UserDoesntAcceptTips, NextAction, ) from ..i18n.currencies import Money +from ..models.participant import Participant from ..website import website from ..utils import utcnow from ..utils.types import Object @@ -19,6 +21,18 @@ from .stripe import charge +def log_notification_counts(f): + @wraps(f) + def g(*args, **kw): + Participant.notification_counts.set(defaultdict(int)) + r = f(*args, **kw) + for k, n in sorted(Participant.notification_counts.get().items()): + logger.info("Sent %i %s notifications.", n, k) + return r + + return g + + def reschedule_renewals(): """This function looks for inconsistencies in scheduled payins. """ @@ -99,13 +113,13 @@ def reschedule_renewals(): sleep(0.1) +@log_notification_counts def send_donation_reminder_notifications(): """This function reminds donors to renew their donations. The notifications are sent two weeks before the due date. """ db = website.db - counts = defaultdict(int) rows = db.all(""" SELECT (SELECT p FROM participants p WHERE p.id = sp.payer) AS payer , json_agg((SELECT a FROM ( @@ -169,7 +183,6 @@ def send_donation_reminder_notifications(): overdue=overdue, email_unverified_address=True, ) - counts['donate_reminder'] += 1 db.run(""" UPDATE scheduled_payins SET notifs_count = notifs_count + 1 @@ -177,10 +190,9 @@ def send_donation_reminder_notifications(): WHERE payer = %s AND id IN %s """, (payer.id, tuple(sp['id'] for sp in payins))) - for k, n in sorted(counts.items()): - logger.info("Sent %i %s notifications." % (n, k)) +@log_notification_counts def send_upcoming_debit_notifications(): """This daily cron job notifies donors who are about to be debited. @@ -188,7 +200,6 @@ def send_upcoming_debit_notifications(): payment of the "month" (31 days, not the calendar month). """ db = website.db - counts = defaultdict(int) rows = db.all(""" SELECT (SELECT p FROM participants p WHERE p.id = sp.payer) AS payer , json_agg((SELECT a FROM ( @@ -249,7 +260,6 @@ def send_upcoming_debit_notifications(): else: event = 'missing_route' payer.notify(event, email_unverified_address=True, **context) - counts[event] += 1 db.run(""" UPDATE scheduled_payins SET notifs_count = notifs_count + 1 @@ -257,15 +267,13 @@ def send_upcoming_debit_notifications(): WHERE payer = %s AND id IN %s """, (payer.id, tuple(sp['id'] for sp in payins))) - for k, n in sorted(counts.items()): - logger.info("Sent %i %s notifications." % (n, k)) +@log_notification_counts def execute_scheduled_payins(): """This daily cron job initiates scheduled payments. """ db = website.db - counts = defaultdict(int) retry = False rows = db.all(""" SELECT p AS payer, json_agg(json_build_object( @@ -364,7 +372,6 @@ def unpack(): email_unverified_address=True, force_email=True, ) - counts['renewal_unauthorized'] += 1 continue if payin.status == 'failed' and route.status == 'expired': can_retry = db.one(""" @@ -385,7 +392,6 @@ def unpack(): provider='Stripe', email_unverified_address=True, ) - counts['payin_' + payin.status] += 1 elif actionable: db.run(""" UPDATE scheduled_payins @@ -406,7 +412,6 @@ def unpack(): email_unverified_address=True, force_email=True, ) - counts['renewal_actionable'] += 1 if impossible: for tr in impossible: tr['execution_date'] = execution_date @@ -416,9 +421,6 @@ def unpack(): transfers=impossible, email_unverified_address=True, ) - counts['renewal_aborted'] += 1 - for k, n in sorted(counts.items()): - logger.info("Sent %i %s notifications." % (n, k)) if retry: execute_scheduled_payins() From 333264380febef8d7d293f50280a9721c935bbc9 Mon Sep 17 00:00:00 2001 From: Changaco Date: Sun, 18 Aug 2024 11:39:45 +0200 Subject: [PATCH 2/4] refactor the sending of payment result notifications --- liberapay/models/exchange_route.py | 10 ++++++++++ liberapay/payin/common.py | 25 +++++++++++++++++++++++++ liberapay/payin/cron.py | 7 ------- liberapay/payin/stripe.py | 10 +++++++++- www/callbacks/stripe.spt | 11 +---------- 5 files changed, 45 insertions(+), 18 deletions(-) diff --git a/liberapay/models/exchange_route.py b/liberapay/models/exchange_route.py index e06d71a5c..b227d07da 100644 --- a/liberapay/models/exchange_route.py +++ b/liberapay/models/exchange_route.py @@ -384,6 +384,16 @@ def has_been_charged_successfully(self): LIMIT 1 """, (self.participant.id, self.id))) + @property + def processor_display_name(self): + match self.network.split('-', 1)[0]: + case 'paypal': + return "PayPal" + case 'stripe': + return "Stripe" + case _: + raise NotImplementedError(self.network) + @cached_property def stripe_payment_method(self): return stripe.PaymentMethod.retrieve(self.address) diff --git a/liberapay/payin/common.py b/liberapay/payin/common.py index 1eb35d88d..66d368bdd 100644 --- a/liberapay/payin/common.py +++ b/liberapay/payin/common.py @@ -884,6 +884,31 @@ def update_payin_transfer( return pt +def handle_payin_result(db, payin): + """Notify the payer of the success or failure of a charge. + """ + assert payin.status in ('failed', 'succeeded') + if payin.ctime > (utcnow() - timedelta(hours=6)) and not payin.off_session: + return + payer = db.Participant.from_id(payin.payer) + if payin.status == 'succeeded': + payer.notify( + 'payin_succeeded', + payin=payin._asdict(), + email_unverified_address=True, + idem_key=f"{payin.id}_{payin.status}", + ) + elif payin.status == 'failed': + route = db.ExchangeRoute.from_id(payer, payin.route) + payer.notify( + 'payin_failed', + payin=payin._asdict(), + provider=route.processor_display_name, + email_unverified_address=True, + idem_key=f"{payin.id}_{payin.status}", + ) + + def abort_payin(db, payin, error='aborted by payer'): """Mark a payin as cancelled. diff --git a/liberapay/payin/cron.py b/liberapay/payin/cron.py index cdde5d540..b687e8687 100644 --- a/liberapay/payin/cron.py +++ b/liberapay/payin/cron.py @@ -385,13 +385,6 @@ def unpack(): if can_retry: retry = True continue - if payin.status in ('failed', 'succeeded'): - payer.notify( - 'payin_' + payin.status, - payin=payin._asdict(), - provider='Stripe', - email_unverified_address=True, - ) elif actionable: db.run(""" UPDATE scheduled_payins diff --git a/liberapay/payin/stripe.py b/liberapay/payin/stripe.py index 0d75fd9cc..9b2758eca 100644 --- a/liberapay/payin/stripe.py +++ b/liberapay/payin/stripe.py @@ -10,7 +10,7 @@ from ..models.exchange_route import ExchangeRoute from ..website import website from .common import ( - abort_payin, adjust_payin_transfers, prepare_payin, + abort_payin, adjust_payin_transfers, handle_payin_result, prepare_payin, record_payin_refund, record_payin_transfer_reversal, resolve_tip, update_payin, update_payin_transfer, ) @@ -453,6 +453,7 @@ def settle_charge_and_transfers( refunded_amount = None if charge.amount_refunded: refunded_amount = int_to_Money(charge.amount_refunded, charge.currency) + old_status = payin.status payin = update_payin( db, payin.id, charge.id, charge.status, error, amount_settled=amount_settled, fee=fee, intent_id=intent_id, @@ -540,6 +541,9 @@ def settle_charge_and_transfers( update_donor=(update_donor and i == last), ) + if payin.status != old_status and payin.status in ('failed', 'succeeded'): + handle_payin_result(db, payin) + return payin @@ -745,6 +749,7 @@ def settle_destination_charge( if charge.amount_refunded: refunded_amount = int_to_Money(charge.amount_refunded, charge.currency) + old_status = payin.status payin = update_payin( db, payin.id, charge.id, status, error, amount_settled=amount_settled, fee=fee, intent_id=intent_id, @@ -782,6 +787,9 @@ def settle_destination_charge( reversed_amount=reversed_amount, update_donor=update_donor, ) + if payin.status != old_status and payin.status in ('failed', 'succeeded'): + handle_payin_result(db, payin) + return payin diff --git a/www/callbacks/stripe.spt b/www/callbacks/stripe.spt index 54af17d2d..399eab504 100644 --- a/www/callbacks/stripe.spt +++ b/www/callbacks/stripe.spt @@ -11,7 +11,6 @@ from liberapay.payin.stripe import ( ) from liberapay.utils import utcnow -SIX_HOURS = timedelta(hours=6) PRODUCTION = website.env.instance_type == 'production' [---] @@ -46,15 +45,7 @@ if event_object_type == 'charge': assert payin if payin.ctime > (utcnow() - timedelta(minutes=50)): raise response.error(409, "This callback is too early.") - payin = settle_charge(website.db, payin, charge) - if status in ('failed', 'succeeded') and payin.ctime < (utcnow() - SIX_HOURS): - payer = Participant.from_id(payin.payer) - payer.notify( - 'payin_' + payin.status, - payin=payin._asdict(), - provider='Stripe', - email_unverified_address=True, - ) + settle_charge(website.db, payin, charge) elif event_object_type == 'charge.dispute': dispute = event.data.object From 6b7fe05d9ab3cd8b96823a97d565ebd4bafdb619 Mon Sep 17 00:00:00 2001 From: Changaco Date: Sun, 18 Aug 2024 11:40:55 +0200 Subject: [PATCH 3/4] notify payers when PayPal payments succeed or fail --- liberapay/payin/paypal.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/liberapay/payin/paypal.py b/liberapay/payin/paypal.py index 4f7163658..40076c7b2 100644 --- a/liberapay/payin/paypal.py +++ b/liberapay/payin/paypal.py @@ -12,8 +12,8 @@ from ..i18n.currencies import Money from ..website import website from .common import ( - abort_payin, update_payin, update_payin_transfer, record_payin_refund, - record_payin_transfer_reversal, + abort_payin, handle_payin_result, update_payin, update_payin_transfer, + record_payin_refund, record_payin_transfer_reversal, ) @@ -243,6 +243,7 @@ def record_order_result(db, payin, order): ) for pu in order['purchase_units'] ) or None + old_status = payin.status payin = update_payin( db, payin.id, order['id'], status, error, refunded_amount=refunded_amount ) @@ -286,6 +287,9 @@ def record_order_result(db, payin, order): amount=net_amount, fee=pt_fee, reversed_amount=reversed_amount ) + if payin.status != old_status and payin.status in ('failed', 'succeeded'): + handle_payin_result(db, payin) + return payin From 5701f49cf7afe364499a41f7bd47820952b8f73f Mon Sep 17 00:00:00 2001 From: Changaco Date: Mon, 26 Aug 2024 12:43:51 +0200 Subject: [PATCH 4/4] always send payin success notifications Some users are confused as to why they don't receive notifications for all payins. --- emails/payin_succeeded.spt | 2 +- liberapay/constants.py | 4 ++-- liberapay/payin/common.py | 6 ++++-- liberapay/payin/stripe.py | 30 +++++++++++++++++++----------- 4 files changed, 26 insertions(+), 16 deletions(-) diff --git a/emails/payin_succeeded.spt b/emails/payin_succeeded.spt index 7779dacbf..4e88a4f0d 100644 --- a/emails/payin_succeeded.spt +++ b/emails/payin_succeeded.spt @@ -12,7 +12,7 @@ ) }}

% else

{{ _( - "The payment of {money_amount} initiated earlier today has succeeded.", + "The payment of {money_amount} initiated today has succeeded.", money_amount=payin.amount, ) if payin.ctime.date() == notification_ts.date() else _( "The payment of {money_amount} initiated on {date} has succeeded.", diff --git a/liberapay/constants.py b/liberapay/constants.py index 7a748868e..70e63a5f1 100644 --- a/liberapay/constants.py +++ b/liberapay/constants.py @@ -122,8 +122,8 @@ def generate_value(self, currency): Event('donate_reminder', 2, _("When it's time to renew my donations")), Event('pledgee_joined', 16, _("When someone I pledge to joins Liberapay")), Event('team_invite', 32, _("When someone invites me to join a team")), - Event('payin_failed', 2**11, _("When a payment I initiated fails")), - Event('payin_succeeded', 2**12, _("When a payment I initiated succeeds")), + Event('payin_failed', 2**11, _("When a payment from me to someone else fails")), + Event('payin_succeeded', 2**12, _("When a payment from me to someone else succeeds")), Event('payin_refund_initiated', 2**13, _("When money is being refunded back to me")), Event('upcoming_debit', 2**14, _("When an automatic donation renewal payment is upcoming")), Event('missing_route', 2**15, _("When I no longer have any valid payment instrument")), diff --git a/liberapay/payin/common.py b/liberapay/payin/common.py index 66d368bdd..6e9575b77 100644 --- a/liberapay/payin/common.py +++ b/liberapay/payin/common.py @@ -16,6 +16,7 @@ ) from ..i18n.currencies import Money, MoneyBasket from ..utils import group_by +from ..website import website ProtoTransfer = namedtuple( @@ -888,8 +889,6 @@ def handle_payin_result(db, payin): """Notify the payer of the success or failure of a charge. """ assert payin.status in ('failed', 'succeeded') - if payin.ctime > (utcnow() - timedelta(hours=6)) and not payin.off_session: - return payer = db.Participant.from_id(payin.payer) if payin.status == 'succeeded': payer.notify( @@ -899,6 +898,9 @@ def handle_payin_result(db, payin): idem_key=f"{payin.id}_{payin.status}", ) elif payin.status == 'failed': + if website.state.get({}).get('user') == payer: + # We're about to show the payin's result to the payer. + return route = db.ExchangeRoute.from_id(payer, payin.route) payer.notify( 'payin_failed', diff --git a/liberapay/payin/stripe.py b/liberapay/payin/stripe.py index 9b2758eca..ea941b04b 100644 --- a/liberapay/payin/stripe.py +++ b/liberapay/payin/stripe.py @@ -134,12 +134,14 @@ def charge(db, payin, payer, route, update_donor=True): if len(transfers) == 1: payin, charge = destination_charge( db, payin, payer, statement_descriptor=('Liberapay %i' % payin.id), - update_donor=update_donor, + update_donor=update_donor, handle_result=False, ) if payin.status == 'failed': payin, charge = try_other_destinations( db, payin, payer, charge, update_donor=update_donor, ) + if payin.status in ('failed', 'succeeded'): + handle_payin_result(db, payin) else: payin, charge = charge_and_transfer( db, payin, payer, statement_descriptor=('Liberapay %i' % payin.id), @@ -204,12 +206,12 @@ def try_other_destinations(db, payin, payer, charge, update_donor=True): if len(payin_transfers) == 1: payin, charge = destination_charge( db, payin, payer, statement_descriptor=('Liberapay %i' % payin.id), - update_donor=update_donor, + update_donor=update_donor, handle_result=False, ) else: payin, charge = charge_and_transfer( db, payin, payer, statement_descriptor=('Liberapay %i' % payin.id), - update_donor=update_donor, + update_donor=update_donor, handle_result=False, ) except NextAction: raise @@ -227,7 +229,7 @@ def try_other_destinations(db, payin, payer, charge, update_donor=True): def charge_and_transfer( - db, payin, payer, statement_descriptor, update_donor=True, + db, payin, payer, statement_descriptor, update_donor=True, handle_result=True, ): """Create a standalone Charge then multiple Transfers. @@ -288,12 +290,15 @@ def charge_and_transfer( intent_id = getattr(intent, 'id', None) payin = settle_charge_and_transfers( db, payin, charge, intent_id=intent_id, update_donor=update_donor, + handle_result=handle_result, ) send_payin_notification(db, payin, payer, charge, route) return payin, charge -def destination_charge(db, payin, payer, statement_descriptor, update_donor=True): +def destination_charge( + db, payin, payer, statement_descriptor, update_donor=True, handle_result=True, +): """Create a Destination Charge. Doc: https://stripe.com/docs/connect/destination-charges @@ -363,6 +368,7 @@ def destination_charge(db, payin, payer, statement_descriptor, update_donor=True intent_id = getattr(intent, 'id', None) payin = settle_destination_charge( db, payin, charge, pt, intent_id=intent_id, update_donor=update_donor, + handle_result=handle_result, ) send_payin_notification(db, payin, payer, charge, route) return payin, charge @@ -435,7 +441,7 @@ def settle_charge(db, payin, charge): def settle_charge_and_transfers( - db, payin, charge, intent_id=None, update_donor=True, + db, payin, charge, intent_id=None, update_donor=True, handle_result=True, ): """Record the result of a charge, and execute the transfers if it succeeded. """ @@ -541,8 +547,9 @@ def settle_charge_and_transfers( update_donor=(update_donor and i == last), ) - if payin.status != old_status and payin.status in ('failed', 'succeeded'): - handle_payin_result(db, payin) + if handle_result: + if payin.status != old_status and payin.status in ('failed', 'succeeded'): + handle_payin_result(db, payin) return payin @@ -729,7 +736,7 @@ def sync_transfer(db, pt, update_donor=True): def settle_destination_charge( - db, payin, charge, pt, intent_id=None, update_donor=True, + db, payin, charge, pt, intent_id=None, update_donor=True, handle_result=True, ): """Record the result of a charge, and recover the fee. """ @@ -787,8 +794,9 @@ def settle_destination_charge( reversed_amount=reversed_amount, update_donor=update_donor, ) - if payin.status != old_status and payin.status in ('failed', 'succeeded'): - handle_payin_result(db, payin) + if handle_result: + if payin.status != old_status and payin.status in ('failed', 'succeeded'): + handle_payin_result(db, payin) return payin