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

Transaction report charts #5685

Closed
gabriele-v opened this issue Feb 18, 2023 · 17 comments
Closed

Transaction report charts #5685

gabriele-v opened this issue Feb 18, 2023 · 17 comments
Assignees
Milestone

Comments

@gabriele-v
Copy link
Contributor

gabriele-v commented Feb 18, 2023

Description of the feature

GRM reports are really good for power users, but I think that may be useful to have an option to add a charts to Transaction Report for "normal" users or even for power users like me if I don't need something heavily customized.

I'd imagine it like a 4 options:

  • No Chart
  • Bar Chart
  • Line Chart
  • Pie Chart

If "grouping" is enabled, it should consider the grouping item total, otherwise it should consider the single transaction.

Two other options of grouping should be added: group by month (it will be month+year obviously) and group by year

Use cases

  • See the line chart of specific transactions over time (IE salary)
  • See the line chart of grouped transactions over time (IE groceries or fuel grouped by month)
  • See the pie chart of grouped transaction in specific time (IE a sort of category summary but with specific filters)
@gabriele-v
Copy link
Contributor Author

@n-stein not sure about your next developments, but I think this may be really useful, actually GRM is too difficult and I had request from multiple friends for specific charts which actually are obtainable only through GRM but would be solved by the above new feature.

gabriele-v added a commit to gabriele-v/moneymanagerex that referenced this issue Jan 7, 2024
gabriele-v added a commit to gabriele-v/moneymanagerex that referenced this issue Jan 7, 2024
@whalley whalley added this to the v1.7.1 milestone Jan 7, 2024
whalley added a commit that referenced this issue Jan 7, 2024
feat(#5685): add charts to Transaction Report
@whalley
Copy link
Member

whalley commented Jan 7, 2024

Not done any testing on it yet but one observation is that maybe the graph should be at the head of the report like all other standard reports.

@gabriele-v
Copy link
Contributor Author

Yes, I've thought about it but it needs a rewrite of the code because actually group totals are calculated during the loop when generating HTML table.

@rmelillo76
Copy link

rmelillo76 commented Jan 7, 2024

Nice work @gabriele-v !

@whalley Adding this javascript to the HTML may be a quick way to handle this, as it simply moves the chart to the top of the page:
This uses jQuery since it's already available to the report, but could probably be done using plain javascript too:

$('.shadowGraph').insertAfter($('.shadowTitle'));

image

image

gabriele-v added a commit to gabriele-v/moneymanagerex that referenced this issue Jan 7, 2024
@gabriele-v
Copy link
Contributor Author

Nice trick @rmelillo76, I've sent a PR with the change

whalley added a commit that referenced this issue Jan 7, 2024
@whalley whalley closed this as completed Jan 23, 2024
gabriele-v added a commit to gabriele-v/moneymanagerex that referenced this issue Jan 28, 2024
whalley added a commit that referenced this issue Jan 29, 2024
@whalley
Copy link
Member

whalley commented Jan 29, 2024

@gabriele-v Rather than

Minimum >> TRANSID                   VALUE
Maximum >> TRANSID                   VALUE

It would probably be better to report something like:

Minimum (ID = TRANSID)               VALUE
Maximum (ID = TRANSID)               VALUE

Could also consider adding a hyperlink to the transaction as we do in the transtion report detail itself.

@gabriele-v
Copy link
Contributor Author

gabriele-v commented Jan 29, 2024

Yes, hyperlink make sense but only if it's a trasaction, so group by not enabled, otherwise the key is not TRANSID but grouping item.

Regarding the parenthesis I've tried with that but I was in doubt because if you have for example a category like "Incomes (Salary)" you have two parenthesis and that's pretty ugly, I don't think anyone will name a category/account/payee with >>, but I'm open to any suggestions.

@whalley
Copy link
Member

whalley commented Jan 29, 2024

Yes, hyperlink make sense but only if it's a trasaction, so group by not enabled, otherwise the key is not TRANSID but grouping item.

Oh, never tried it with 'Group By', I see what you mean. Leave as-is.

@rmelillo76
Copy link

Hi @gabriele-v

I started to look at this and at first glance I thought Min and Max were being reversed in the example report below, but then I realized it's because the amounts are all withdrawals so the totals are both negative . But as a user, I would look at that I spent 164 on Fuel and 20 on Emissions, so 164 should be the max and 20 the min. Should we use absolute value to calculate instead - what do you think?

image

Also, the amounts are only showing whole dollars, not the cents in both the chart and the stats. I think we should show the full amount, just like the built-in reports with charts do.

image

image

@gabriele-v
Copy link
Contributor Author

Hello @rmelillo76,
regarding the maximum/mimimum I'm not sure about using the absolute value.
It may solve your case but will introduce other issues, consider for example if values are -5, +2, +7, in that case you will have minimum +2, which is not correct. Or if you have -5, +2, +3 you will have maximum +5. We may consider using absolute values only if all values are negative, but only in that specific case.

Regarding decimals you are right, it seems a bug and I'll fix it.

@whalley
Copy link
Member

whalley commented Jan 31, 2024

We may consider using absolute values only if all values are negative, but only in that specific case

Think that could lead to confusion. The current behaviour seems clear to me.

@rmelillo76
Copy link

We may consider using absolute values only if all values are negative, but only in that specific case

Think that could lead to confusion. The current behaviour seems clear to me.

I agree now after giving it more thought. Thank you both.

gabriele-v added a commit to gabriele-v/moneymanagerex that referenced this issue Jan 31, 2024
@gabriele-v
Copy link
Contributor Author

@rmelillo76 I've sent a PR to fix decimal values, you should be able to test it soon.

@gabriele-v
Copy link
Contributor Author

FYI looks like pie graph considers only the absolute value, so if I have a 20 and -20, the pie will be 50%-50%.
Obviously it doesn't make sense to create a pie with positive and negative values, but that's how it behaves.

whalley added a commit that referenced this issue Jan 31, 2024
@whalley
Copy link
Member

whalley commented Jan 31, 2024

FYI looks like pie graph considers only the absolute value, so if I have a 20 and -20, the pie will be 50%-50%. Obviously it doesn't make sense to create a pie with positive and negative values, but that's how it behaves.

Yes, not sure what else you can do in this situation. A Pie chart is probably not the best display option, but that is up to the user to select.

@n-stein
Copy link
Contributor

n-stein commented Feb 1, 2024

There's an old C-style cast on this line that is causing the Flatpak build to fail. It should be a static_cast.

https://github.com/moneymanagerex/moneymanagerex/blob/master/src%2Freports%2Ftransactions.cpp#L189

whalley added a commit to whalley/moneymanagerex that referenced this issue Feb 1, 2024
whalley added a commit that referenced this issue Feb 1, 2024
@gabriele-v
Copy link
Contributor Author

Thank you @whalley

n-stein pushed a commit to n-stein/moneymanagerex that referenced this issue Feb 3, 2024
@whalley whalley added the fixed label Feb 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants