Added Graphs to Stats tab (For issue #136) #731

jav099 · 2021-04-04T17:00:39Z

Implemented graphs for the stats tab as per issue #136.

On the thread for the issue, it is mentioned that using a charting library that requires a license is a non-goal for the project, and the library C3.js was suggested. Thus, I started my implementation using this but ran into some limitations and noticed that C3's repo is basically dead. On the issues thread for C3, a lot of users suggested changing to Billboard.js, another open-source charting library that like C3 is built on D3. In fact Billboard.js was initially forked from C3 back in 2017 and is actively maintained, unlike C3.

Therefore, I switched to implementing the charts using Billboard and as I understand, no license is required to use it.

I implemented 4 graphs for the statistics tab:

1. Balance per member bar chart.

This is a very simple bar chart, displaying the current balance for each project member. There foreground is green/red for easier visualization on whether the balance is negative or positive.

Why this chart?
To be fully honest, this one does not show much new information. In fact this information is already presented on the left side, however, so the inclusion of this chart is purely for aesthetic reasons, as a quick glance at this chart can tell someone which member is in the negatives and by how much, relative to the other members.

Implementation Details:

Used already existing information being served to the HTML file and Billboard bar chart to generate the graph, nothing too complex.

2. Monthly Expenditure per member

This is a line graph with multiple lines, each representing the expenditure for each member per month.

Why this chart?

In a previous pull request that tried to fix this issue, it was mentioned that a line chart representing the combined monthly expenditure for the whole group was a good addition, and this is just simply an extension of that. One can quickly tell when each person has expenditure and how much. It is also important to note that Billboard's graphs allow for mouse hover over the graph, and so if the mouse hovers over a particular month the exact quantity will show up in a small pop-up box, so the user is not left trying to guess based on the Y-axis.

Implementation Details:

This graph required a modification to models.py, since this information was currently not being sent to the HTML template. In order to implement this in models.py, I added a function get_monthly_expenditure(member_id) which uses the already existing function get_member_bills to be able to generate a list of tuples of the form ([year month],[amount]) for each month with an existing expense for the provided member id. This was then simply added to the information being served in member_stats. Tests in api_test were updated accordingly. Used Billboard line chart

3. Combined Monthly Expenditure

This is another line chart but instead of breaking down monthly expenditure per member, it presents it combined.

Why this chart?
This presents a simpler and more aesthetically pleasing way to present the information that is currently being presented in table form. Again, as mentioned above, this was noted to be a good addition in a previous pull request.

Implementation Details:

Used information that was already being served to the template, and used Billboard line chart

4. Expenditure Distribution Pie Chart

Why this chart?
Easy to see which member is contributing the most, and by extension who is not contributing much at all.

Implementation Details:

Used information that was already being served to the template, and used Billboard pie chart

almet · 2021-04-07T10:24:03Z

That's really great. Thanks for the implementation and the details you provided on the issue.

I don't really think the first graph is needed as it's a duplication of information already here and easily visible, but if that's deemed useful by others here I'll gladly accept it.

almet

Hi, and thanks for the work you put in this. That's really appreciated.

I don't quite like the idea of looping in the templates to output JS statements. I feel this is a mix of concerns that should be separated.

To me, it looks like it could either be done in JavaScript entirely, or that we should at least isolate the logic. I did this before for some other projects, so I don't blame you, but I feel that we could go on a different, cleaner way here.

Hope that helps, and thanks again!

almet · 2021-04-07T10:26:38Z

ihatemoney/models.py

@@ -164,6 +165,32 @@ def monthly_stats(self):
    def uses_weights(self):
        return len([i for i in self.members if i.weight != 1]) > 0

+    def get_monthly_expenditure(self, member_id):


As a non-native english-speaker, I'm not familiar with the term "expenditures". How does it differs with "expenses"? If there is no difference, should we keep using "expenses" instead?

I am also a non-native English speaker and I just looked it up and I think although they are similar, "expenses" is probably more appropriate too. I'll change it!

almet · 2021-04-07T10:29:12Z

ihatemoney/templates/layout.html

@@ -6,6 +6,11 @@
    <meta http-equiv="content-type" content="text/html; charset=utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <link rel=stylesheet type=text/css href="{{ url_for("static", filename='css/main.css') }}">
+    <!-- Load d3.js -->
+    <script src="https://d3js.org/d3.v6.min.js" charset="utf-8"></script>


If possible, we shouldn't rely on external hosts to provide us with the javascript files. Could you please include it in this repository? Thanks!

almet · 2021-04-07T10:30:19Z

ihatemoney/templates/statistics.html

+
+
+        <!-- Required for some style in charts, feel free to move from here -->
+        <style>


Could you include this in a separate .css file rather than inline here?

almet · 2021-04-07T10:30:50Z

ihatemoney/templates/statistics.html

+        #balanceBar .fill_red { fill: red; }
+        </style>
+        <br>
+        <div id="balanceBar"></div>


As I said in a comment, I'm for removing this one as it doesn't add much information.

almet · 2021-04-07T10:31:25Z

ihatemoney/templates/statistics.html

+                balance_min_value = val;
+            }
+        {% endfor %}
+        //TODO: Remove this, its for debugging.


Please remove this :-)

almet · 2021-04-07T10:31:44Z

ihatemoney/templates/statistics.html

+        members.forEach(function(entry) {
+            console.log(entry);
+        });
+        //TODO: Remove this, its for debugging.


Ditto : we don't want to add debug statements here.

almet · 2021-04-07T10:33:48Z

ihatemoney/templates/statistics.html

+        var total_exp_per_member = {};
+        var balance_min_value = Number.POSITIVE_INFINITY;
+        var balance_max_value = Number.NEGATIVE_INFINITY;
+        {% for stat in members_stats %}


I'm not sure we actually want to loop here on the template. What do you think about passing the member_stats as json and using it on the javascript function, rather than looping on the template?

I agree with this. Honestly, I am not too familiar with JavaScript so I am unsure of how I would go ahead and implement this. Any suggestions? I'll gladly implement it.

Neat, thanks for the answer.

I can tell you the way I would do this, or at least how I would start :

Remove the JavaScript code from the template ;

Create a separate file, import it on the template with a script tag ;

Try to limit the calls on the template to function call, which could pass JSON objects coming from the template

Split the JS logic into blocks of small bits to make it easier to grasp, do the iteration this way.

Hope it makes sense / helps. Let me know otherwise

Ok, yeah this does make sense. I'm thinking about doing onLoad to call the function. But I am unsure on where I would call this. Since we want to limit the calls it probably is not a good idea to call it on each chart div so would calling it on <div class="d-flex flex-column"> be appropriate?

And more importantly, for passing the object into the function how would this be? Would it be like myFunc({{ member_stats, months}})?

Thanks!

onLoad seems fine to me, as long as it calls when all the dependencies are loaded (wasn't clear from the docs).

I think one call for each graph would be just fine, so one fonction which would call and bind all the divs. You don't really need to group them all.

For passing the args, I believe you would need to convert them to JSON first. I would use a |tojson Jinja2 filter for this, so something like

myFunc({{ member_stats | tojson }}, {{ months | tojson }})

Hello, thanks for the help!

I was able to fix that issue and it all works perfectly now.

However, a possibly small issue has popped up. In order to JSONify the data passed into the JS function, I had to change member_stats on models.py such that instead of "member" being the person object, it was just the member.name because that is all that is used on statistics.html and moreover because if I didn't, the interpreter would throw an error that a Person object can't be serialized.

The issue that stems from this is that there are 2 tests that fail when I do this because the tests are set up to expect the whole Person object, not just the name. I know for a fact that this is the reason the tests fail because when I revert it, the only test that fails is one in budget_test that fails because the interpreter complained as I mentioned above.

Specifically, the failing test cases are: test_statistics on budget_test and api_test.

How would you like to proceed? I can easily change one of the failing tests (the one on api_test) so that now it accepts on just the name, but the other test that fails throws a strange HTML/Regex error that I am unsure how to change.

Hey.

If it's not possible to serialize directly, you might want to add a serialize method which returns a python dict with serialized values, and call this in the template. If possible, I believe we shouldn't change the signature of the API unless that's really what we want. Hope it helps.

Actually, there is already a way to serialize the model, see https://github.com/spiral-project/ihatemoney/blob/master/ihatemoney/models.py#L410

If this doesn't make sense, you might want to push your code to this branch so i can have a look at it :-)

Made sense. I made the changes and passed all tests. Pushed the changes to this branch

almet · 2021-04-07T10:35:29Z

ihatemoney/templates/statistics.html

+
+
+
+        <script> 


As this is quite a long script tag, I would prefer to isolate it in a separate file. It would also force us to avoid looping JavaScript statements in the template directly.

almet · 2021-04-12T06:41:12Z

That's great! I'll try to have a look tonight, cheers!

almet

Hey @jav099 ! I've took some time to look at your code and still have a few remarks. I hope it's not getting too long for you. Let me know if you've had enough.

I rebased your code on top on the latest master and force-pushed your branch, so be sure to update it locally to get my changes.
I also made some wording changes, renaming expenditures to expenses where it made sense.

Now on the code itself, I feel this populateCharts function does too much. We should ideally break it down in multiple functions which would be used each to generate a separate graph.

If you need to have some shared logic between them, for instance to prepare the data, I would advocate for the use of a separate function which would prepare the data and then pass the data to each of the "chart" functions.

Splitting the functions in many smaller ones has multiple benefits :

You can pass the IDs to bind to for each function ;
You can pass some strings for the names of the data series. It will be useful for translation purposes. Currently, all the strings are in english only and it's a problem.
You will be sure that some logic on the first graphs will not have an impact on subsequent graphs down the line.

It means that on the JS part, you would need to create a function for each graph, which would take the data, the strings and the IDs to bind to as arguments.

… And call it from the python part with the correct arguments.

jav099 · 2021-04-13T23:00:59Z

Hello!

@almet, unfortunately, exams are right around the corner for me and I don't have time right now to make the changes. Moreover, once again I have the issue where I can't even see the app since when I do make serve it says there is no such table project, even though I did not change anything since the last commit where it all worked fine.

I could potentially be able to work on these changes once summer break starts but that's some time away and it seems like they are some quick changes and maybe you want to get them out before then.

Let me know either way, and I apologize for not being able to do the requested changes right now and for making populateCharts too long.

almet · 2021-04-14T16:09:09Z

Hi @jav099, thanks for the heads-up. You don't need to apology. You did all the hard work, and eventually this will allow our users (and myself!) to have some interesting stats and graphs. So a big thank you for this.

Thanks for letting us know. Most of the time, people get off the hook and don't tell us so. I completely understand that you need to focus on your studies. I wish you the best of luck and don't hesitate to come and say hi and work some more with us if you want when you have some spare time again.

It's been a pleasure! See you ;-)

almet · 2021-09-01T18:50:39Z

Hi @jav099 if you still want to work on this don't hesitate, it's not so far from completion and nobody took the time to finish it yet :-)

almet · 2022-03-07T16:26:52Z

Hi, I'm closing this as nobody is taking over and the code is probably rotting. Feel free to reopen if you think it's the way to go.

jav099 marked this pull request as ready for review April 5, 2021 20:24

almet requested changes Apr 7, 2021

View reviewed changes

almet mentioned this pull request Apr 7, 2021

Adding Graphs to Statistics Tab (Issue #136) #698

Closed

jav099 and others added 3 commits April 12, 2021 21:26

Added modified files and bb js/css

918d4a3

Moved JS to separate file, removed bar chart

a2c3364

Wording : use 'expenses' rather than 'expenditures'

fda210e

almet force-pushed the statGraphs branch from 15e67cf to fda210e Compare April 12, 2021 19:32

almet requested changes Apr 12, 2021

View reviewed changes

Glandos added this to the v5.1 milestone Jul 11, 2021

almet marked this pull request as draft October 17, 2021 19:39

almet mentioned this pull request Oct 20, 2021

Display statistics as a graph #136

Closed

almet closed this Mar 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Graphs to Stats tab (For issue #136) #731

Added Graphs to Stats tab (For issue #136) #731

jav099 commented Apr 4, 2021 •

edited

Loading

almet commented Apr 7, 2021

almet left a comment

almet Apr 7, 2021

jav099 Apr 7, 2021

almet Apr 7, 2021

almet Apr 7, 2021

almet Apr 7, 2021

almet Apr 7, 2021

almet Apr 7, 2021

almet Apr 7, 2021

jav099 Apr 7, 2021

almet Apr 7, 2021

jav099 Apr 7, 2021

almet Apr 7, 2021 •

edited

Loading

jav099 Apr 11, 2021 •

edited

Loading

almet Apr 11, 2021

almet Apr 11, 2021

almet Apr 11, 2021

jav099 Apr 11, 2021

almet Apr 7, 2021

almet commented Apr 12, 2021

almet left a comment

jav099 commented Apr 13, 2021

almet commented Apr 14, 2021

almet commented Sep 1, 2021

almet commented Mar 7, 2022



		<!-- Required for some style in charts, feel free to move from here -->
		<style>

Added Graphs to Stats tab (For issue #136) #731

Added Graphs to Stats tab (For issue #136) #731

Conversation

jav099 commented Apr 4, 2021 • edited Loading

almet commented Apr 7, 2021

almet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

almet Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

jav099 Apr 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

almet commented Apr 12, 2021

almet left a comment

Choose a reason for hiding this comment

jav099 commented Apr 13, 2021

almet commented Apr 14, 2021

almet commented Sep 1, 2021

almet commented Mar 7, 2022

jav099 commented Apr 4, 2021 •

edited

Loading

almet Apr 7, 2021 •

edited

Loading

jav099 Apr 11, 2021 •

edited

Loading