Closed bockstaller closed 3 years ago
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?
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.
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:
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
. 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.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.
Good catch! if we keep the current text "we will debit you on X or shortly after, I also think it would be fine
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:
num_new
context variable isn't realy neat. 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.
Currently, the following parts of the payment lifecycle are implemented:
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 ?
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).
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.
@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.
I'm not sure if this is usual, I guess this slipped through. @raphaelm do you have any thoughts?
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!
Great feedback, thank you very much. Especially with the email-stuff, should've asked earlier instead of biting through
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.
Et voilà - all done. Back to you
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
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?
It was a small indentation error I must have made after running the tests. It is fixed now.
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/
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 😊
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:
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:
pretix-sepadebit
?