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

Monthly expenses #526

Merged
merged 1 commit into from Feb 9, 2020
Merged

Monthly expenses #526

merged 1 commit into from Feb 9, 2020

Conversation

Dutchy-
Copy link
Contributor

@Dutchy- Dutchy- commented Jan 10, 2020

When used as a monthly expenses tracker (without the requirement for settling) this tool lacks an overview. This is a WIP initial attempt to add a monthly overview of all expenses.

This is my first version:
image

I do not believe it is ready to merge yet, but I wanted to get some feedback on the location of the new table, the design, should it also contain running/yearly totals, etc.

Last but not least, is this a feature you even want? :)
I'm looking forward to your feedback.

Edit: I know I still need to use the translation functions for the strings

@Dutchy- Dutchy- changed the title WIP: Initial montly expenses WIP: Initial monthly expenses Jan 10, 2020
@Glandos
Copy link
Member

Glandos commented Jan 13, 2020

To me, monthly expenses are not a goal.

However, more statistics is a nice feature :) Your example is very basic, but remember that some people would like to gather them differently, e.g. quarter (3 months), yearly, or even daily if you are using it on holidays.
So for example, the second column is useless. The period should go in the first (e.g. 2019-01 in your first work), and a control to change the gathering period. As a start, a simple select should be fine to me.

@Dutchy-
Copy link
Contributor Author

Dutchy- commented Jan 26, 2020

I've updated it to look like this
image
I've changed the display so it can be used for different period aggregations later.

I do not have time at this moment to implement a select box for different period aggregations, would you consider merging it as is, so different periods can be added later?

@Dutchy- Dutchy- changed the title WIP: Initial monthly expenses Monthly expenses Jan 26, 2020
@Glandos
Copy link
Member

Glandos commented Jan 27, 2020

It's simple, nice, clear, and extendable. To me, it's OK to merge this.
@Natim Can you confirm? It's a small review :)

By the way, just a small question: is it OK to have a fixed width in pixels for the 1st column? I know the whole page isn't completely responsive, but it is not a good reason :)

monthly = defaultdict(lambda: defaultdict(int))

for member in self.active_members:
for bill in self.get_member_bills(member.id).all():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to rewrite this so that we do a unique SQL request rather than one per members?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably; I will change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed this - as it turned out, a query for bills by active members was simply wrong for the historical statistics. I now just use get_bills().

@almet
Copy link
Member

almet commented Jan 27, 2020

Hi, thanks for the work on this. If we are to have a « statistics » view like this one, I would rather have a graph rather than have a table like is proposed here, what do you think?

@Glandos
Copy link
Member

Glandos commented Jan 27, 2020

Both table and chart are valid to me, with different use cases. Sometimes, you want a quick look at raw numbers, or just copy/paste them. Sometimes, you need to compare months with each other, and charts are better.

However, charting usually requires external JS libraries, and it usually increases the page loading.

@Dutchy-
Copy link
Contributor Author

Dutchy- commented Jan 27, 2020

By the way, just a small question: is it OK to have a fixed width in pixels for the 1st column? I know the whole page isn't completely responsive, but it is not a good reason :)

I think it's ok but I haven't checked on a mobile device or anything. Since I have to change something anyway I will doublecheck.

Hi, thanks for the work on this. If we are to have a « statistics » view like this one, I would rather have a graph rather than have a table like is proposed here, what do you think?

In my use case I'd rather have a table. An additional chart does not hurt but I'd say that can stay for future work :)

@Dutchy-
Copy link
Contributor Author

Dutchy- commented Feb 4, 2020

I've updated the query.
Here's a screenshot of a mobile view:

image

Let me know if you have any further feedback. It's all done for me.

Copy link
Member

@almet almet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for this, I'm okay with the code, with just a few minor changes to do, and we're okay !

Sorry for the delay on all this, it should be easier for contributors to push code around here. Thanks again for you contribution.

:return: a dict of years mapping to a dict of months mapping to the amount
:rtype dict:
"""
monthly = defaultdict(lambda: defaultdict(int))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amount seems to be a float rather than an int.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really matter, since it is only used for setting the initial value, which is 0.
And, to be exact, it should be a Decimal, but that will be for the future :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are totally correct, it's better not to confuse types. Changed it to a float :)

@@ -311,8 +311,9 @@ footer .footer-left {
background: url("../images/see.png") no-repeat right;
}

#bill_table {
#bill_table, #month_table {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can improve a bit the naming of this table. what about "monthly_expenses_table" ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or monthly_stats as you named it like this elsewhere (I find the naming even better than my proposal)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

ihatemoney/web.py Show resolved Hide resolved
@Dutchy-
Copy link
Contributor Author

Dutchy- commented Feb 6, 2020

Sorry for the delay on all this, it should be easier for contributors to push code around here. Thanks again for you contribution.

I don't feel like I've had to wait much :) There were longer pauses between my own changes. You guys are doing great!

@almet
Copy link
Member

almet commented Feb 9, 2020

This looks good to me! Thanks :-)

@almet almet merged commit 02242f2 into spiral-project:master Feb 9, 2020
Jojo144 pushed a commit to Jojo144/ihatemoney that referenced this pull request Mar 21, 2020
@zorun zorun added this to the v5 milestone Jul 17, 2020
TomRoussel pushed a commit to TomRoussel/ihatemoney that referenced this pull request Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants