svthalia / concrexit

Thalia Website built on Django.
https://thalia.nu
Other
23 stars 11 forks source link

Overhaul Payables / miscellaneous Payables #3453

Open mark-boute opened 10 months ago

mark-boute commented 10 months ago

What?

Payment Request, or ThaliaPleasePay (TPP'tje)

Why?

Invoices are annoying, they require administrative work on both the sending and receiving end. This could be made much easier by using Thalia(Please)Pay an admin would get a nice overview of which request has been payed through a Payment for a Payable (could be any type of payment). And a payer would be able to use Thalia Pay instead of manually transferring the payment.

How?

A new concrexit app, paymentrequests is needed for this, it will depend on payments

In admin there will be an entry per TPP'tje, which contains a few display fields matching the payment, namely amount, topic, notes. And an inline row per Member that has been requested to pay, this is a Payment Request and is a Payable object. It also has a status, it can be Pending, Paid, Cancelled (essentially a removed payment request, and therefor a separate flag on the class).

The create form will have text fields for amount, topic and notes, and an inline where you can search for and select members to whom the payment request apply (this may also be a single Member). Upon saving this request will just be saved as a concept (iff at least one Member is selected). It will now also be displayed in the Admin page with a [Concept] label. Upon sending this will change to [Pending], and once there are no more Pending payments linked to the TPP the status will change to [Done].

Upon sending all Members linked to the TTP through a Payable will be notified of the request, this could be through a Push Notification and/or an e-mail.

DeD1rk commented 10 months ago

This diagram is ugly but might be informative (sadly github doesn't suport this diagram type i guess):

JobDoesburg commented 10 months ago

I do like the idea, it is nice for the bookkeeping as well! It is basically a wrapper around the kasboek functionality, which will also create external invoices for this purpose rather than only transactions (that still need to be linked to other bookings in moneybird).

IMO the PaymentRequest object should be central in the design. For example, there should be an explicit admin for PaymentRequests (not only an inline on the larger object). The M2M field is a nice way to create the same request for multiple people easily, but it should be clear that the PaymentRequest is the payable.

I would give it a different name, as "ThaliaPleasePay" suggests this only works for ThaliaPay, while it should work for other payment methods as well. Also, it is more than just a payment (request), it is a way to process non-regular revenue: the money will be pushed to moneybird as revenue, even if people decide not to actually pay it. As such, it is also important to display an overview of unpaid payment requests.

Perhaps, it would be nice to have a boolean field that labels a payment request as actual revenue or not. For example, if someone is paying €20 by card to get it back as cash, that isn't actually revenue.

@nvoers @movieminer what are your opinions

DeD1rk commented 10 months ago

PaymentRequest object should be central

+1. As payments are always to Thalia, the default changelist with all requests is probably even a more useful overview than a grouped inline. We technically could do without a batch thing at all, with only a form that bulk-creates requests, without storing a model that links them (and without the ability to bulk-edit the amount/notes/etc.).

Perhaps, it would be nice to have a boolean field that labels a payment request as actual revenue or not.

That sounds valid. Similarly, I think there may need to be a distinction of whether a person has a 'betalingsverplichting'. If at the time a request is made, it's not yet certain whether it will have to be paid sometime (I'm not sure we should allow that, but consider e.g. voluntary donations), I guess there should not be an invoice made until it actually is paid or established that it has to be paid sometime.

JobDoesburg commented 9 months ago

The more I think about this, the more we are actually refactoring towards a concrete model voor Payables...

Theoretically, if this were implemented, all payables could be changed to extend this payment request class (maybe an abstract class)

@DeD1rk what are your thoughts

DeD1rk commented 9 months ago

That's true, PaymentRequest is about as close to a concrete payable as you can get. Making a real (abstract) base model is an option of course, but I think we shouldn't. Many payables don't store the properties they implement in a field, but derive them. So such a base model wouldn't have many fields (probably only a 1to1 to a payment). So we wouldn't gain much from making the payable models inherit.

However, making the Payable objects become models might be useful, if they are polymorphic (multi-table inheritance, not an abstract base model). They could get a one-to-one to the eventregistration/order/etc, and still derive the amount from those models (or have separate fields in case of PaymentRequest). An advantage is that we get a table with an overview of all payables, but the disadvantage is synchronization: we need to create a payable with 1to1 whenever we create a registration/order/etc, which is a little bit ugly and a little bit error prone (not too bad tho), and raises questions like: what do we do for instances that are free? And it hurts performance quite a bit as we would need to join the payables tables onto registrations/orders/etc to find out whether they are paid (i.e. roughly for every query)

JobDoesburg commented 9 months ago

Yes that are exactly the arguments I was thinking about. There's one more: code quality. I don't think the way payables are implemented currently are the easiest and most understandable way of doing so. Maybe refactoring to some Python abstract base class (not an abstract model per se), rather than using this payable object registry, is easier to understand

JobDoesburg commented 9 months ago

Also see #3457