Adding bill types and automatic settling between people #1290

TomRoussel · 2024-03-02T16:28:37Z

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

Brought the FlowingCloudRTL branch up to speed with the current master
Worked in the minor suggested changes by @Glandos
Changed the "bill_type" column to use an Enum (as requested by @Glandos). This enum is also used by the bill form validator
Removed the "Transfer" bill type. Personally I don't see a use for it and it seems confusing to the user. In the original PR, there didn't seem to be any interest in keeping it around
Added an api test to check if bill_type is validated correctly

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".

… to reflect the behavior

bill attribute added

eliminating unnecessary bill type

typo fixed, test cases fixed for the current bill types

…e-button Yiqunz settle button

…-test test cases added

…ll-type-tests Added tests for new bill types

…-test test cases fixed

almet · 2024-03-15T21:34:44Z

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 · 2024-03-15T22:20:55Z

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

Alembic does not handle postgres Enums correctly, so we need to manually generate the new enum type. See sqlalchemy/alembic#278

TomRoussel · 2024-03-16T09:58:26Z

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 · 2024-03-16T11:12:40Z

Perfect, thanks :-)

almet · 2024-03-16T11:21:12Z

Thanks for the work on this 👍

almet · 2024-03-16T11:21:37Z

It was a pleasure to work with you :-)

TomRoussel · 2024-03-16T11:45:30Z

Same here!

zorun

Thanks for finishing this long-awaited feature!

I'm a bit late to the party, I have added some comments, they can be addressed in a new pull request.

By the way, black is also complaining about formatting (see the CI output)

zorun · 2024-03-19T23:08:14Z

ihatemoney/tests/budget_test.py

+                "bill_type": "Transfer",
+                "amount": "500",
+            },
+        )


This looks like a leftover of the "Transfer" type

zorun · 2024-03-19T23:09:02Z

ihatemoney/tests/budget_test.py

+                "what": "Transfer of 500 to fred",
+                "payer": members_ids[0], #zorglub
+                "payed_for": members_ids[1], #fred  
+                "bill_type": "Transfer",


zorun · 2024-03-19T23:10:03Z

ihatemoney/web.py

@@ -848,6 +850,27 @@ def settle_bill():
    return render_template("settle_bills.html", bills=bills, current_view="settle_bill")


+@main.route("/<project_id>/settle/<amount>/<int:ower_id>/<int:payer_id>")
+def settle(amount, ower_id, payer_id):
+    # FIXME: Test this bill belongs to this project !


This looks like it should really be fixed! (and a test added for good measure)

zorun · 2024-03-19T23:10:38Z

ihatemoney/tests/import_test.py

@@ -536,7 +560,8 @@ def test_export_with_currencies(self):
            },
            {
                "date": "2016-12-31",
-                "what": "fromage \xe0 raclette",
+                "what": "\xe0 raclette",
+                "bill_type": "Expense",


I take it you don't like fromage? ;)

zorun · 2024-03-19T23:11:53Z

ihatemoney/web.py

@@ -848,6 +850,27 @@ def settle_bill():
    return render_template("settle_bills.html", bills=bills, current_view="settle_bill")


+@main.route("/<project_id>/settle/<amount>/<int:ower_id>/<int:payer_id>")


As far as I can tell, there is no test for this new route. We should have one.

TomRoussel · 2024-03-20T07:58:42Z

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.

Error introduced in spiral-project#1290.

Error introduced in spiral-project#1290. Fixes spiral-project#1293.

Fixes the following flake8 issue: ihatemoney/tests/budget_test.py:880:5: F811 redefinition of unused 'test_reimbursement_bill' from line 796 ihatemoney/tests/budget_test.py:930:5: F811 redefinition of unused 'test_transfer_bill' from line 846 PR spiral-project#1290 introduced these tests twice.

Error introduced in spiral-project#1290. Fixes spiral-project#1293.

Error introduced in spiral-project#1290. Fixes spiral-project#1293. WTForms needs to be bumped to >=2.3.2 as it includes a fix to `SelectField` which is required for this change to work. See: - https://wtforms.readthedocs.io/en/3.1.x/changes/#version-2-3-2 - pallets-eco/wtforms#598

Error introduced in #1290. Fixes #1293. WTForms needs to be bumped to >=2.3.2 as it includes a fix to `SelectField` which is required for this change to work. See: - https://wtforms.readthedocs.io/en/3.1.x/changes/#version-2-3-2 - pallets-eco/wtforms#598

Ruitao Li and others added 30 commits December 10, 2022 22:14

Bill types added in Bill and Project Model, Implemented in BillForm

48d50f7

import and export bill feature updated with bill type, tests modified…

4a84ca6

… to reflect the behavior

Merge pull request spiral-project#1 from FlowingCloudRTL/rui-dev

7584360

bill attribute added

eliminating unnecessary bill type

f8d85d5

Merge pull request spiral-project#3 from FlowingCloudRTL/rui-dev

e13f915

eliminating unnecessary bill type

typo fixed, test cases fixed for the current bill types

8c3c993

Merge pull request spiral-project#4 from FlowingCloudRTL/rui-dev

eabc533

typo fixed, test cases fixed for the current bill types

button added

536a2e0

Merge branch 'master' of github.com:FlowingCloudRTL/ihatemoney

9d4873e

settle button added

7ea7447

new changes

bc6a1a9

Merge pull request spiral-project#5 from FlowingCloudRTL/yiqunz-settl…

5fc8b82

…e-button Yiqunz settle button

test cases added

1353004

Merge pull request spiral-project#6 from FlowingCloudRTL/yiqun-settle…

e22b2d8

…-test test cases added

bchen-reimbursement

40fc02b

bchen-reimbursement

938d267

tests for different bill types

29e35c3

test cases fixed

2c562ab

fixed reimbursement test case

a113bc0

Merge pull request spiral-project#8 from FlowingCloudRTL/bchen-new-bi…

d9c75db

…ll-type-tests Added tests for new bill types

Merge pull request spiral-project#7 from FlowingCloudRTL/yiqun-settle…

723d729

…-test test cases fixed

Merge branch 'master' into flowing_master

411965d

Replaced assertEqual with assert

87edf49

Fixed missing bill_type in unit tests

9694a65

Removed commented code

e0068ed

Typo fix

63a97c7

Reverted unnecessary string edit

8cef949

Changed bill_type to an Enum

b0a67a5

Added test checking correct bill_type validation

bf96013

Fixed billtype displaying in all caps

4928644

Made "Settlement" translatable

ee12618

Manually handle the new Enum creation

9eb18f2

Alembic does not handle postgres Enums correctly, so we need to manually generate the new enum type. See sqlalchemy/alembic#278

almet merged commit 720f0e5 into spiral-project:master Mar 16, 2024
15 of 17 checks passed

TomRoussel deleted the bill_types_settle branch March 16, 2024 11:45

zorun reviewed Mar 19, 2024

View reviewed changes

rriski mentioned this pull request Mar 22, 2024

Bill Type: Invalid Choice: could not coerce. #1293

Closed

rriski added a commit to rriski/ihatemoney that referenced this pull request Mar 23, 2024

fix: 'Bill Type: Invalid Choice: could not coerce.' error

3766bc5

Error introduced in spiral-project#1290.

rriski added a commit to rriski/ihatemoney that referenced this pull request Mar 23, 2024

fix: 'Bill Type: Invalid Choice: could not coerce.' error

fa1dc4b

Error introduced in spiral-project#1290. Fixes spiral-project#1293.

rriski mentioned this pull request Mar 23, 2024

fix: 'Bill Type: Invalid Choice: could not coerce' error #1294

Merged

rriski added a commit to rriski/ihatemoney that referenced this pull request Mar 23, 2024

fix: 'Bill Type: Invalid Choice: could not coerce' error

cae4445

Error introduced in spiral-project#1290. Fixes spiral-project#1293.

rriski added a commit to rriski/ihatemoney that referenced this pull request Mar 23, 2024

fix: 'Bill Type: Invalid Choice: could not coerce' error

483b94c

Error introduced in spiral-project#1290. Fixes spiral-project#1293.

rriski added a commit to rriski/ihatemoney that referenced this pull request Mar 24, 2024

fix: 'Bill Type: Invalid Choice: could not coerce' error

1f18c17

Error introduced in spiral-project#1290. Fixes spiral-project#1293.

rriski added a commit to rriski/ihatemoney that referenced this pull request Mar 26, 2024

fix: 'Bill Type: Invalid Choice: could not coerce' error

fb0111c

Error introduced in spiral-project#1290. Fixes spiral-project#1293.

rriski added a commit to rriski/ihatemoney that referenced this pull request Mar 26, 2024

fix: 'Bill Type: Invalid Choice: could not coerce' error

f7bbe83

Error introduced in spiral-project#1290. Fixes spiral-project#1293.

zorun mentioned this pull request Mar 29, 2024

Find a consistent name for the new "reimbursement" / "settlement" / "transfer" bill type #1299

Open

TomRoussel mentioned this pull request Mar 31, 2024

Requested billtype changes #1302

Merged

This was referenced Apr 12, 2024

Problem with total spend #1019

Closed

Fix the problem with total spend on statistic page #1112

Closed

Bill Types added to Bill , pay feature added for Settle page, Statistics Page adjusted to be compatible with new types #1116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding bill types and automatic settling between people #1290

Adding bill types and automatic settling between people #1290

TomRoussel commented Mar 2, 2024

almet commented Mar 15, 2024

TomRoussel commented Mar 15, 2024

TomRoussel commented Mar 16, 2024

almet commented Mar 16, 2024

almet commented Mar 16, 2024 •

edited

Loading

almet commented Mar 16, 2024

TomRoussel commented Mar 16, 2024

zorun left a comment

zorun Mar 19, 2024

zorun Mar 19, 2024

zorun Mar 19, 2024

zorun Mar 19, 2024

zorun Mar 19, 2024

TomRoussel commented Mar 20, 2024

		@@ -848,6 +850,27 @@ def settle_bill():
		return render_template("settle_bills.html", bills=bills, current_view="settle_bill")


		@main.route("/<project_id>/settle/<amount>/<int:ower_id>/<int:payer_id>")

Adding bill types and automatic settling between people #1290

Adding bill types and automatic settling between people #1290

Conversation

TomRoussel commented Mar 2, 2024

Summary

Changes since the original PR

Open questions

almet commented Mar 15, 2024

TomRoussel commented Mar 15, 2024

TomRoussel commented Mar 16, 2024

almet commented Mar 16, 2024

almet commented Mar 16, 2024 • edited Loading

almet commented Mar 16, 2024

TomRoussel commented Mar 16, 2024

zorun left a comment

Choose a reason for hiding this comment

zorun Mar 19, 2024

Choose a reason for hiding this comment

zorun Mar 19, 2024

Choose a reason for hiding this comment

zorun Mar 19, 2024

Choose a reason for hiding this comment

zorun Mar 19, 2024

Choose a reason for hiding this comment

zorun Mar 19, 2024

Choose a reason for hiding this comment

TomRoussel commented Mar 20, 2024

almet commented Mar 16, 2024 •

edited

Loading