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

Add the ability to pay from the "settle" page #137

Closed
Natim opened this issue Feb 5, 2016 · 32 comments · Fixed by #1290
Closed

Add the ability to pay from the "settle" page #137

Natim opened this issue Feb 5, 2016 · 32 comments · Fixed by #1290

Comments

@Natim
Copy link
Member

Natim commented Feb 5, 2016

No description provided.

@almet
Copy link
Member

almet commented Feb 6, 2016

If someone is deleted and doesn't have a balance of 0, then it will only be deactivated. It means that it's not possible to add her in the next expenses, nor it's possible to delete the person.

To mark the account as settled, you would need to reactivate the person, set the balance to 0 via a transfer and then delete the person again. That might not be ideal.

Instead, we could add a link to have the person get her money back / pay and then delete it. I'm not sure if that's what's you are proposing. Another way would be to have a new state on the persons, that could be activated/deactivated/settled.

@Natim
Copy link
Member Author

Natim commented Feb 8, 2016

No that is not related to the remove button. I did it adding transfert as a new expense.
It works but it might be useful to automate it in the settle page.

Like Lea paid 50€ to Robin she still have got to pay 30 to Mathilde.

@QuentinRoy
Copy link
Member

To be honest I always found this mechanism confusing and over-complicated.

I wonder if the best solution could just to forbid the “deletion” of a member when his balance is not null.

@Natim
Copy link
Member Author

Natim commented Feb 8, 2016

This issue is not about deleting the user from the list. It is about making her pay what she needs to pay to someone and marking her account has sold.

@almet
Copy link
Member

almet commented Feb 1, 2017

Thanks for the proposal. What I understand is that you want a button, that could appear on the "settle" page which would add a new bill with the payment from one person to the other. Is that correct?

Would you like to work on an implementation for this?

@almet almet changed the title If someone leave the group before the other, we need to settle her down and mark her account as settled Add the ability to pay from the "settle" page Feb 1, 2017
@almet almet mentioned this issue Jul 6, 2018
@LeoMouyna
Copy link
Contributor

@almet, sorry I didn't find this issue before and it's exactly what I wanted. I may work on it this week-end 😉

@almet
Copy link
Member

almet commented Oct 16, 2019

Neat :-)

@almet
Copy link
Member

almet commented Oct 17, 2019

See #43 also :-)

@LeoMouyna
Copy link
Contributor

LeoMouyna commented Oct 17, 2019

Just a question before starting.
What exactly the button pay should do ?

  1. Open a bill form with pre setted value (like this you can define the amount)
  2. Send directly a request to the back-end (with fixed amount) and refresh the page

@almet
Copy link
Member

almet commented Oct 17, 2019

Interesting! I was mainly thinking about option 2, but… 1 makes sense only if you need to change the amount, which should not happen. in which case would you need to change the amount?

@almet almet mentioned this issue Oct 17, 2019
@LeoMouyna
Copy link
Contributor

For example if you only paid 10 when your debt are 20. I don't know if it's a real case I just wanted to make sure I didn't miss understand the ticket 😉

@almet
Copy link
Member

almet commented Oct 17, 2019

Option 1 seems actually better to me : it lets the user check and modify (if needed) beforehand :-)

@Natim
Copy link
Member Author

Natim commented Oct 18, 2019

Actually we can make it option 2 with a edit button in the list of things.
I guess the most important is to have a different type and not use a usually bill but a settle kind. Especially for the stats page.

@almet
Copy link
Member

almet commented Oct 18, 2019

I'm more leaning towards option 1 because it covers more use cases : when clicking the button / link, open a specific bill form with fields pre-filled, the user can then check and click « Settle this » if that works for them.

@LeoMouyna
Copy link
Contributor

@Natim I checked the #414, but I don't understand why refund shouldn't be on statistics page ?

For now I implemented option 2 because I didn't read the comments but I don't know how I can bypass a csrf_token check during form.validate().
So it's another plus point for option 1 ! 🎆

Option 1 form will only have an amount field pre set according to the settle row.
It will be a modal form with "Refund for <receiver_name>" as title.

Is that ok for you @almet ?

@almet
Copy link
Member

almet commented Oct 19, 2019

About #414, and why the refund information shouldn't be counted in the stats : refunds are a way to reimburse the people who landed the money to the group. If we count the refunds in the statistics, then it's pointless : everything will be at 0 at the end.

So, we need to distinguish the reimbursment bills from the other (classic) bills. I believe that adding a type field in the database to specify the type of bill should be enough. For now, we could have two different possible values for this new database field could be settlement and loan, (and loan be the default).

I'm pinging @eneiluj here, because this could impact the API as well. If you have any suggestions on how to do that in a different way, please shoot :-)


Now, about the CSRF token, I'm not sure why you want to bypass it? If we chosen to go for option 2, I believe that creating the bill on the server side (without going trough the form) would be the simplest way to accomplish this.

It will be a modal form with "Refund for <receiver_name>" as title.

Perfect for the name. You also need to pre-fill all the other fields, so the end-user just has to click « Okay ».

@LeoMouyna
Copy link
Contributor

LeoMouyna commented Oct 19, 2019

About csrf_token I just generated a fake form because I wanted to use form.save() method but it's useless if I just make a save query. 🤦‍♂️

Yes everything would be at 0 but I didn't get why it was a problem but if you don't want that to happen, I'll wait for eneiluj investigation before changing datamodel.
Other question about this "tag", should settlement bills appear on bill list page ?

About payment form, other fields wont be visible by user but will be filled at form init. Do you prefer have the exact same form than a classic bill with a different title ?

@almet
Copy link
Member

almet commented Oct 19, 2019

Zero stats doesn't seem useful, are they?

I think we can go for my proposal so far (no need to wait for @eneiluj feedback here, I believe we
could make adjustments afterwards if needed).

About your questions :

settlement bills should indeed appear on the bill-list page, we need to have something that identifies them as such.

For the form, I believe we should reuse the existing form, but pass a few flags to it so that some info is not displayed to the user, and that an explanation message is displayed on top of it.

If you see that this form has too many ifs, you can probably create a different one, as keeping things simple is a goal in this project.

@julien-nc
Copy link

I think I agree with @almet about reimbursement bills not counted in stats.

In Cospend and MoneyBuster, I've implemented "automatic reimbursement bills creation" and they currently mess up with stats. It's pretty basic at the moment and just creates all reimbursement bills at once, with full owed amounts. Then everybody's balance is back to zero. It looks like option 2.

Untill now, in the IHM and Cospend projects I'm participating in, we handle one-to-one reimbursement by adding a "classic" bill and naming it "reimbursement blabla". It's almost equivalent to option 1 (except there is no bill type there).

Adding a bill type seems to be the minimal change to handle that "stats" issue. This parameter could be optional in the API (default to "loan") to avoid compatibility problems with old clients versions.

@LeoMouyna
Copy link
Contributor

LeoMouyna commented Oct 20, 2019

A little update, please don't pay attention to the color.

2019-10-20 22-55-36

TODO:

  • Add related unit test
  • Remove settlement bills from statistics (but maybe not for your balance computation ?)

@almet
Copy link
Member

almet commented Oct 20, 2019

Nice :-) Yes, you need to keep them in the balance computation :-)

@LeoMouyna
Copy link
Contributor

Won't it be awkward if your balance is 0 but paid and spent are different ?

@Natim
Copy link
Member Author

Natim commented Oct 21, 2019

They are already different because you often paid more or less than you spent since you paid other shares sometimes and the other way around too.

It is intersting to see:

  • the balance (how much money is out or do you owe)
  • the total that you paid (how much money went out paying things)
  • the total that you spent (how much money the activity did cost you in the end)

When the balance is at 0 everything is settled properly.

@sim6
Copy link

sim6 commented Feb 3, 2020

Firstly, thanks for this project, it is awesome.

The @LeoMouyna partial solution looks good to me, but, please could you add who pays in the refund form title?

Maybe could be great to have also a generic refund form where we can select the payer and the receiver. This can be used to track money movements. For example when a arbitrary participant refund another participant who want to leave the project.

I'm using negative amounts for incoming bills but it could be confusing. If we would have an income bill type could be great also.

Anyway, the first feature is very important to me for long term projects.

Is the development of this feature stalled?

@LeoMouyna
Copy link
Contributor

Hi @sim6.
I probably messed up with issues number but you can see the related PR.
I don't know if this feature is still required and tbh I don't remember exactly what I did 3 months ago so I will take a look tonight and show the payer. 😸

@SiM
Copy link

SiM commented Feb 14, 2020

Hi,
I was just looking for this feature and I see that it's close to being integrated, so I just have to wait a little longer! :)

Thanks @LeoMouyna !

@tmancini85
Copy link

Hi @LeoMouyna,
first of all thanks for your work on this!

I don't know if the development is finished. Do you have any updates?

I also saw that, following @almet indications, you've called bill types classes "loan" and "settlement". Even if it's something internal to the app and final users will not see it, could it lead to some misunderstanding? Loans can also occurs between users, it's the same as offering a reimbursement (in the stats both should only change paid amounts and not spent ones). Maybe "expense" and one between "reimbursment/refund/transfer" (with "expense" the default) could it be more explicit.

Anyway, bill types are a very useful improvement since it will also possible to add incomes, as asked by @sim6. And about them, I don't know if it is better to open another issue/feature request, I saw the #414 was closed so maybe this is the right place for both reimbursements and incomes.

@LeoMouyna
Copy link
Contributor

Hello @tmancini85.

I developed this feature like 10 months ago but it wasn't as good as I wanted and we never agreed on the right solution. Maybe I will take some time to implement it again but right now I don't have enough free time to work on it.

But you are probably right, add a Bill type could be a great improvement and could help solve some other issues. Feel free to look at my dev and rework it if you can. If you have any question about its I'm available to answer it (if I can remember 😅)

@Glandos
Copy link
Member

Glandos commented Jan 14, 2021

For reference, some extensive work was done in #507 but not merged because it needed more work.

@tmancini85
Copy link

Hi @LeoMouyna and @Glandos, first of all sorry, I didn't notice #507 was closed.

Anyway, it would be cool to work on it, but unfortunately my full time non-IT job doesn't leave me enough free time to start coding again, for the moment I can only be a FLOSS passionate from the outside. Maybe one day, but surely for that time IHM will have already moved a lot forward 😉

@FlowingCloudRTL
Copy link
Contributor

Hi! I noticed that there is still no pay feature in the settle section on master. What is the current progress on this issue? If it is still in the inprogress phase I would love to contribute to the implementation.

@almet
Copy link
Member

almet commented Dec 6, 2022

Hi! I noticed that there is still no pay feature in the settle section on master. What is the current progress on this issue? If it is still in the inprogress phase I would love to contribute to the implementation.

Yes, don't hesitate to send us some code to review :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment