Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overhaul Payables / miscellaneous Payables #3453

Open
mark-boute opened this issue Oct 25, 2023 · 7 comments
Open

Overhaul Payables / miscellaneous Payables #3453

mark-boute opened this issue Oct 25, 2023 · 7 comments
Assignees
Labels
app:payments Issues regarding the payments-app board Need to involve the board feature Issues regarding a complete new feature priority: low Should be dealt with when nothing else remains. request-for-comments Author wants to have other people respond for their opinion

Comments

@mark-boute
Copy link
Contributor

mark-boute commented Oct 25, 2023

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.

@mark-boute mark-boute added priority: low Should be dealt with when nothing else remains. feature Issues regarding a complete new feature request-for-comments Author wants to have other people respond for their opinion labels Oct 25, 2023
@DeD1rk
Copy link
Member

DeD1rk commented Oct 25, 2023

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

@DeD1rk DeD1rk added board Need to involve the board app:payments Issues regarding the payments-app labels Oct 25, 2023
@Scarletto Scarletto self-assigned this Oct 25, 2023
@JobDoesburg
Copy link
Contributor

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
Copy link
Member

DeD1rk commented Oct 26, 2023

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
Copy link
Contributor

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
Copy link
Member

DeD1rk commented Oct 27, 2023

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
Copy link
Contributor

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
Copy link
Contributor

Also see #3457

@JobDoesburg JobDoesburg changed the title ThaliaPleasePay Overhaul Payables / miscellaneous Payables Oct 27, 2023
@mark-boute mark-boute self-assigned this Nov 8, 2023
@mark-boute mark-boute linked a pull request Nov 9, 2023 that will close this issue
11 tasks
@mark-boute mark-boute mentioned this issue Nov 9, 2023
11 tasks
@Noah-marc Noah-marc removed their assignment Jun 11, 2024
@T8902 T8902 mentioned this issue Nov 27, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app:payments Issues regarding the payments-app board Need to involve the board feature Issues regarding a complete new feature priority: low Should be dealt with when nothing else remains. request-for-comments Author wants to have other people respond for their opinion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants