spiral-project / ihatemoney

A simple shared budget manager web application
https://ihatemoney.org
Other
1.2k stars 270 forks source link

Adding bill types and automatic settling between people #1290

Closed TomRoussel closed 8 months ago

TomRoussel commented 8 months ago

Summary

This PR is a continuation of #1116. The goal of this PR is to finish the work of the original PR and fix #137. The overall design changes proposed in #1116 are mostly kept, though I'll detail the changes below. This PR adds a button to the "Settle" page to automatically let users signify they have settled their debt with other people. This generates a new bill in the overview page that will bring their debt with that person to 0.

Normally this would pollute the statistics of that project, so a bill_type column is added to the database. There are 2 types, "Reimbursement" and "Expense", only the latter is used to calculate the total cost of the project.

Changes since the original PR

Open questions

With these changes, all bill creation requests to the API require that a "bill_type" is provided. This will break compatibility with apps using the API. Should we make the "bill_type" field an optional parameter? I think we can safely make the default type be "Expense".

almet commented 8 months ago

Hey, thanks for your work on this.

With these changes, all bill creation requests to the API require that a "bill_type" is provided. This will break compatibility with apps using the API. Should we make the "bill_type" field an optional parameter? I think we can safely make the default type be "Expense".

+1 for this being the default behaviour.

TomRoussel commented 8 months ago

Okay, I'll make bill_type an optional parameter to the bill adding API then and get back to you when that's ready!

TomRoussel commented 8 months ago

Done, bill_type is now "Expense" by default, and this default is now applied to all existing bills as well.

almet commented 8 months ago

Thanks for the follow up on this. On my end, it seems good. I took another glance at your changes and added a few remarks. We're almost there!

almet commented 8 months ago

Tests are currently failing on postgres, because it tries to use a unknown type:

psycopg2.errors.UndefinedObject: type "billtype" does not exist
E       LINE 1: ALTER TABLE bill ADD COLUMN bill_type billtype DEFAULT 'EXPE...

(The tests run otherwise properly when using sqlite)

TomRoussel commented 8 months ago

Oh weird, I'll have a look this weekend to see why it's doing that.

TomRoussel commented 8 months ago

Turns out alembic does not handle the generation of new enums correctly for postgres databases. See this issue.

I fixed it by manually creating the sqlalchemy enum in the upgrade step of the migration, as someone suggested in the issue I linked. In that issue someone also introduced a library that handles enum migrations in alembic automatically: alemibc-postgresql-enum, but I didn't want to add a new dependency for this small issue :).

Hopefully the tests are green now! It seemed to work on a postgres database I spun up locally.

almet commented 8 months ago

Perfect, thanks :-)

almet commented 8 months ago

Thanks for the work on this 👍

almet commented 8 months ago

It was a pleasure to work with you :-)

TomRoussel commented 8 months ago

Same here!

TomRoussel commented 8 months ago

Sure, I'll go through those comments and submit a new PR! I'll probably have time to have a look in the weekend, though no promises.

You just use python-black on the whole codebase? Then I'll make sure to run it along with the suggested changes.