Fixed-date-debit setting for sepa mandates

[bockstaller]

Hi,

I am currently in the situation that I have an upcoming event with ~1200 participants coming up next year around easter, for which we want to start the registration in the coming months.
I'm in the lucky situation that I am not dependent on the cash flow from the participants and would like to collect the direct debits later in the year, to avoid refunding everything if I can avoid it.

The plugin currently only supports payments executed x days after a transaction. I need all debits for an event to be executed on a common day.

I could implement a hacky exporter for myself, but I think this might be a useful feature for the upstream plugin.

Draft:

I have implemented a first mockup of the settings screen to discuss the functionality:

Settings:

Relative date (default):
The same behavior as before. The user has to export the XML files regularly.

Fixed date:
Direct debits are due on a specified date for all sales.

There are now three different settings:

1. Payment Execution Date: The day the payment is due.
2. Customer Notification Days: The number of days before an upcoming debit execution, when a heads-up email is sent out to all customers using this payment method.
3. Export Notification Days: The number of days before an upcoming debit execution, when the organizers get a notification that they have to export the XML files. Recipients are either all administrators of an organization or a supplied (list of) email addresses. Additionally, a required action is issued.

Number 2 and 3 could be moved to the email settings (see Open point emails). But I think they are important enough to be a part of the payment settings and warrant timeline entries.

Fixed date payments are excluded from the event and organizer XML exports until after the export notification date.

Open Point: Emails

I think the texts of these two emails should be configurable, preferably in the emails section of the events settings.

If I am not mistaken, it is currently not possible to inject new e-mail-templates as a plugin, given that this is a fixed list of templates. It might be cool to add a hook for plugins to supply email templates.

Otherwise, I could add these settings directly into the payment provider's settings.

Next steps:

I'd like to start implementing this in the next month or so.
Therefore my questions are:

• Is this a desirable feature for pretix-sepadebit?
• If this is the case, is my draft on the right track?
• Are there any sharp corners I should be careful about?

[raphaelm]

First of all, thank you for the detailed proposal and settings draft. This looks really well thought-out!

Is this a desirable feature for pretix-sepadebit?

I'm leaning towards yes.

I'm a little bit worried about introducing a significant amount of complexity (the emailing stuff) we need to maintain for a feature that will be used only a handful of times ever. If we expect this to be something not to be used in the long run, I'd be in favor of a simpler solution (i.e. "just use a calender reminder and the sendmail plugin to send the pre-notification manually"), but I think there's potential for this beyond the current situation e.g. for events with a crowd-funding-like approach that will only be confirmed once enough people committed to buying a ticket.

@pc-coholic any thoughts?

If this is the case, is my draft on the right track?
Are there any sharp corners I should be careful about?

• How do you plan to deal with someone ordering a ticket after the due date (or before the due date but after the pre-notification date)? You can't use the configured due date, since it's in the past, and you don't have any information to compute a different one. A possible solution could be thinking about this in a different way: Instead of "relative" vs "absolute", you could leave the existing setting "pre-notification days" as-is, and just add a new setting called "earliest due date". This will also make the configuration form much simpler.
• The other tricky part will be the pre-notification email, since you'll need to track who has already received that and deal with edge cases like orders coming in after the pre-notification date, etc.
• I'm not yet sure on the correct design for the notification sent to the organizer. First, I see no reason for this to be specific to the fixed-date scenario: In the default configuration it could also be useful to get an email "you have 5 debits waiting to be transfered to the bank". Second, I don't think there's any need to make the text of that email configurable. Third, I'm not sure if it's better to have a list of email addresses to be notified inside the payment provider or to handle it through the existing notification framework, like "there's been an external refund" notifications.

As a side node, in an event series one could want to specify the due date in relation to the event start instead of a fixed date… but I think that'd be taking it too far and the fixed date is good enough for a start ;)

If I am not mistaken, it is currently not possible to inject new e-mail-templates as a plugin, given that this is a fixed list of templates. It might be cool to add a hook for plugins to supply email templates.
Otherwise, I could add these settings directly into the payment provider's settings.

Correct, there is currently no way to extend the default email settings and the correct way would be to add it in the payment provider's settings or some extra settings page.

Additionally, a required action is issued.

The "required action" mechanism is unofficially deprecated (i.e. it is not used in any plugin we know of any more). We've just never deleted it to avoid breaking compatibility. We should probably officially deprecate and delete it for good.

[bockstaller]

Thank you very much for your feedback!
Adding a simple "Earliest due date"-setting simplifies the configuration and business logic a lot.

Now orders can be in one of three different groups:

1. The order was placed before cutoff = earliest_due_date - pre_notification_days and therefore warrants a pre_notification-email. The plugin will send the email during the next runperiodic-call after the cutoff.
2. The order was placed after the cutoff. That is the same case as currently implemented. The organizer has to ensure that the pre_notification_days-value is chosen in a way to comply with regulations. The plugin doesn't have to send a pre-notification email.
3. Bad luck. We are currently seconds before the cutoff time. The customer is now filling out the mandate and is shown the earliest_due_date as the due_date. The cutoff time arrives, and the runperiodic task had run before they created their OrderPayment. From the customers' frame of reference, they have issued a 1st case order. For the plugin, it is a 2nd case order.

Handling the 3rd case is a bit tricky. But I think we can ignore it. We aren't reducing the pre-notification time for the customer, possibly violating regulations, only extending the configured pre_notification_days timespan by a few moments.

I will start by adding the earliest_due_date-functionality and visit the pre-notification and organizer-notification after that.

[raphaelm]

Good catch! if we keep the current text "we will debit you on X or shortly after, I also think it would be fine

[bockstaller]

I am pondering about the export logic. After todays commits the xml export view will export all outstanding orders. The organizers bank probably won't accept this.

We will need an additional export window setting to know which payment due dates should be exported.
Checking this could be done:

1. during export time in this loop by comparing the due_date against the setting value. This will work fine for smaller events but iterating over all unexported orders for the num_new context variable isn't realy neat.
2. by storing the due date in a model. Either by adding a new one or by extending SepaExportOrder and creating these instances during the payment process. A new model might be wasteful but doesn't change the complete lifecycle and query logic for the existing one.

I'm currently preffering the less disruptive new model approach.

[bockstaller]

Currently, the following parts of the payment lifecycle are implemented:

1. Creating a new order with/without an earliest due date
2. The export of all orders which are in their respective pre-notification period

The refunds are a bit problematic because they assume that every Sepa-OrderPayment is directly handed off to the bank and marked as paid. Canceling an order marks it instantly as overpaid.

I would suggest reassigning payment states for all new orders:
Pending: The checkout process was successful, but the payment isn't exported yet.
Confirmed: The payment has been exported.
The OrderPayment Pending state is only used to indicate an Error, but cannot be reached directly (or at least I haven't found a place where the sepadebit OrderPayment is marked as pending).

Making this change only for new orders should keep the impact as small as possible. Nevertheless, this is quite a significant behavior change.
Any thoughts on this topic @raphaelm ?

[raphaelm]

I'm currently preffering the less disruptive new model approach.

Yeah, probably the safer approach :(

The refunds are a bit problematic because they assume that every Sepa-OrderPayment is directly handed off to the bank and marked as paid. Canceling an order marks it instantly as overpaid.

I think we already have the logic that refunding any non-exported payment just removes it from the export instead of creating an outgoing transfer. Or did I understand you wrong?

I would suggest reassigning payment states for all new orders:
Pending: The checkout process was successful, but the payment isn't exported yet.
Confirmed: The payment has been exported.
The OrderPayment Pending state is only used to indicate an Error, but cannot be reached directly (or at least I haven't found a place where the sepadebit OrderPayment is marked as pending).

This would be a very breaking change that I think we'll unfortunately be unable to make. We have this plugin often used in a scenario where you place an order for an event happening today (think going to a public swimming pool today), and the export might happen days later. With that change, the ticket would still be marked as unpaid and I would not get a ticket or be able to enter the event. (Yep, it's unclean that we consider it "paid" even though no money changed hands… but that's the same at export time, we'd really need to wait for the money to arrive to not have this problem).

[bockstaller]

I think we already have the logic that refunding any non-exported payment just removes it from the export instead of creating an outgoing transfer. Or did I understand you wrong?

Sorry, I have to admit that I was confused by the refund process. Refunding an unexported payment removes it from the export. This makes my Pending/Confirmed suggestion superfluous.

[bockstaller]

@raphaelm I think I am done with implementing the fixed-date-debit functionality. The exception beeing the organizer notification feature, which is a seperate project for a separate PR.

It's my first time really diving into pretix and working with this many moving parts in one go. So, there might be some unusual solutions, which I'll address if pointed out.

[bockstaller]

I'm not sure if this is usual, I guess this slipped through.
@raphaelm do you have any thoughts?

[raphaelm]

I'm also not sure if it is usual, but it definitely shouldn't be. Thanks for pushing it up in the inbox again. I'll try to look at this tomorrow!

[raphaelm]

First, I'm again sorry it took me so long to look into this in detail.

Second, this is really good work ✨ The code has a high quality and you even added a set of tests. Very nice!

I have a few remarks inline, apart from minimal language things they mostly center about three topics:

• E-mail placeholders: We have a system in place to handle email placeholders consistently and we should use it here. This would mean that users can use all the regular placeholders like url or event in the email texts, and that help texts etc. are auto-generated. The email system has a flexible concept of "context", allowing you to add your own placeholders in a way that they only show up for this specific email text and not for any others. To do this, you'd invent a new context piece with a unique name, such as sepadebit_payment and create new placeholders that depend on it (see docs). Then, you can just pass sepadebit_payment to get_email_context and it will automatically resolve your placeholders. I know this all isn't perfectly documented since it hasn't been used much externally, so please just ask if you have any questions!

• Cronjob efficiency: The periodic task you added has a for loop over all events in the database (tens of thousands on pretix Hosted), and then build a long Q-query, which could lead to megabyte-long SQL queries. Even if we limit the query down just to events which use the new feature, it will still waste a lot of resources on checking events which happened five years a go. A much more efficient solution could be just saving the pre-notification time as a DateTimeField on the SepaDueDate model. This way, you could just do a system-wide query and get all SepaDueDate models with unsent reminders and a reminder date in the past.

• Export logic: If I'm not mistaken, this fundamentally changes the logic in the export (see inline comment), which could be an unwanted change for everyone using this plugin already. Am I correct? Do you have any ideas on how to deal with this?

[bockstaller]

Great feedback, thank you very much.
Especially with the email-stuff, should've asked earlier instead of biting through

[bockstaller]

I've implemented the suggested changes and marked your reviews as resolved as I've completed them.

Unfortunately including your cc to @julia-luna regarding the copy editing , which is probably still necessary. At least I am quite untalented in micro copy.

[raphaelm]

This is getting really good! I took another read and still found some stuff (sorry), but it's getting smaller and smaller ✨

[bockstaller]

Et voilà - all done. Back to you

[bockstaller]

Timing wise, I was forced to install dfb729e on my pretix instance.
So far so good and we will see if any problems arise. Fortunately I expect slow signups, so further changes shouldn't be to difficult

[raphaelm]

I pushed a few micro-changes. I then noticed that one of your tests fails for me (even without my changes), can you reproduce that and look into it?

[bockstaller]

It was a small indentation error I must have made after running the tests. It is fixed now.

[raphaelm]

Finally merged, sorry for the long wait!

I also pushed the strings to the translation platform:
https://translate.pretix.eu/projects/pretix/pretix-plugin-sepadebit/

[bockstaller]

Great!
No worries, the change set was stable, so I installed directly from the branch.

Thank you very much for reviewing my first non-trivial contribution 😊