-
Notifications
You must be signed in to change notification settings - Fork 22
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
WIP: Add Budget Support and Update Style #56
base: main
Are you sure you want to change the base?
Conversation
Add tabler.io for style support Cleanup budget page
remove use of selectize in favor of Tabler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for your contribution! I really appreciate your interest in and work on my little project.
However, I have a few concerns. Most importantly, I think that for the very least the style changes (which I do not fully understand nor see the need for) and the budget feature (which I like) should be separate PRs.
Some other points are noted as inline comments. I also noticed that the font of the transaction list seems somehow smaller. This should not be the case.
I have encountered the following error when opening a newly created budget:
127.0.0.1 - - [28/Dec/2023 21:20:50] "GET /budget/Unnamed%20Budget HTTP/1.1" 500 -
Traceback (most recent call last):
File "/home/joshua/repos/by-project/gnucash-web/.venv/lib/python3.11/site-packages/flask/app.py", line 2552, in __call__
return self.wsgi_app(environ, start_response)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/joshua/repos/by-project/gnucash-web/.venv/lib/python3.11/site-packages/flask/app.py", line 2532, in wsgi_app
response = self.handle_exception(e)
^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/joshua/repos/by-project/gnucash-web/.venv/lib/python3.11/site-packages/flask/app.py", line 2529, in wsgi_app
response = self.full_dispatch_request()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/joshua/repos/by-project/gnucash-web/.venv/lib/python3.11/site-packages/flask/app.py", line 1825, in full_dispatch_request
rv = self.handle_user_exception(e)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/joshua/repos/by-project/gnucash-web/.venv/lib/python3.11/site-packages/flask/app.py", line 1823, in full_dispatch_request
rv = self.dispatch_request()
^^^^^^^^^^^^^^^^^^^^^^^
File "/home/joshua/repos/by-project/gnucash-web/.venv/lib/python3.11/site-packages/flask/app.py", line 1799, in dispatch_request
return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/joshua/repos/by-project/gnucash-web/src/gnucash_web/auth.py", line 62, in inner
return func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^
File "/home/joshua/repos/by-project/gnucash-web/src/gnucash_web/budget.py", line 57, in show_budget
budget_start_date = budget.recurrence.recurrence_period_start
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'str' object has no attribute 'recurrence'
I'm sure this is not en extensive list. For example, I haven't yet looked at the budget implementation.
On another note, can you give me some insight into your use case? I basically only use GnuCash Web as a mobile companion to the desktop app, which is why viewing Budgets is not really important to me (especially when I can't even edit them. However, I never liked the Budget-Interface from the Desktop anyway, so I definitely thought about adding that. For what do you use the app? We can also talk about this via Mail or something else.
[tool.poetry] | ||
name = "gnucash-web" | ||
version = "0.1.0" | ||
description = "" | ||
authors = ["Joshua Bachmeier", "Jayen Ashar", "Fredrik Falk", "Trevor Reed"] | ||
readme = "README.md" | ||
|
||
[tool.poetry.dependencies] | ||
python = "^3.11" | ||
babel = "2.9.1" | ||
flask = "2.2.5" | ||
mysql = "0.0.3" | ||
mysqlclient = "2.2.0" | ||
piecash = "1.2.0" | ||
psycopg2 = "2.9.9" | ||
pycryptodome = "3.18.0" | ||
requests = "2.27.1" | ||
werkzeug = "2.3.7" | ||
pandas = "^2.1.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is poetry and what does it have to do with anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependency manager for Python (https://python-poetry.org/)
return render_template('user.j2', username=session.get('username', 'no one')) | ||
return render_template('user.html', username=session.get('username', 'no one')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least for me (Emacs) I'm pretty sure that j2 is correctly handeled as a jinja2 html template, wheras changing this to .html would result in the syntax-highlighting / indentation / etc. to be pure html and broken by the jinja tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check if there are other solutions, I have never seen a Flask project use the .j12 extension in production. VSCode handles the Jinja syntax in .HTML files really well in my experience
<head> | ||
<meta charset="utf-8"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1"> | ||
|
||
<link href="{{ url_for('static', filename='img/official/16x16/apps/gnucash-icon.png') }}" rel="icon" type="image/png"> | ||
<link href="{{ url_for('static', filename='bootstrap/css/bootstrap.min.css') }}" rel="stylesheet"> | ||
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@tabler/core@latest/dist/css/tabler.min.css"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer hosting the resources as part of the project, so a submodule setup similar to how i do it for bootstrap and selectize would be good.
This is especially problematic when not pinning a version of the resource (core@latest
) and not having a checksum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it needs done this is just a draft and I will consolidate it before publishing the PR. I wanted to get preliminary feedback.
<head> | ||
<meta charset="utf-8"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1"> | ||
|
||
<link href="{{ url_for('static', filename='img/official/16x16/apps/gnucash-icon.png') }}" rel="icon" type="image/png"> | ||
<link href="{{ url_for('static', filename='bootstrap/css/bootstrap.min.css') }}" rel="stylesheet"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused now. You removed bootstrap?
def currency(value, currency='USD', locale='en_US'): | ||
return numbers.format_currency(value, currency, locale=locale) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you not use the money
function defined above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could do that as well, I just thought the one-liner was simpler but am open to either
@@ -64,7 +64,7 @@ | |||
<div class="invalid-feedback mx-1"> | |||
Select an account to transfer money to / from | |||
</div> | |||
<script> | |||
<!-- <script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If something is not needed anymore, please don't comment it out, remove it. This is what GIT is for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thought I removed it
<li class="nav-item"> | ||
<a class="nav-link" href="{{ url_for('budget.list_budgets') }}">Budgets</a> | ||
</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somehow weird. There is no visual indication that this is the active view when the Budget view is selected. There also is no obvious way to return to the accounts view.
When we add more views to the app, the navbar will probably have to be rethought entirely, since it was only ever intended for the account view being the only one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this needs fixed too. I have been working on that just not sure of the best approach
{% if account.children %} | ||
<div id="subaccounts" class="my-3 list-group"> | ||
{# Display all subaccounts in a collapsed tree view #} | ||
<!-- All accounts accordian --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the style of the account list should be a separate PR. Is this just a refactor? The code doesn't seem simpler to me tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its was refactor to keep the style consistent between Budgets and Transactions
<link href="{{ url_for('static', filename='selectize/css/selectize.bootstrap5.css') }}" rel="stylesheet"> | ||
<!-- <link href="{{ url_for('static', filename='selectize/css/selectize.bootstrap5.css') }}" rel="stylesheet"> --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why remove selectize?! It is an integral part of the transaction creation form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Selectize was not playing nice with Dark Mode themes, from what I could tell all of the features of Selectize that were being used can be reproduced without its use.
As an afterthought, before you spend a lot of time working on those style changes: I really don't see the need for any style changes and when I ran the code in this PR, it didn't really look "better" at all and I don't understand what you intended or why this would require another dependency. So, before you invest a lot of time in a PR, we should discuss these points. |
Happy to connect on this to discuss use cases. I'll take a look at that error message, it's probably a similar error to the other one I reported where if there are empty values. We use GnuCash for a small nonprofit and use the Budgets feature pretty heavily. The styles definitely still need work, that's why I left the PR in draft tagged as a WIP. If the work doesn't end up fitting your vision for the project, that is perfectly fine! The work needs done either way to support our use case so I figured I would just contribute it back. Feel free to email me, [email protected] to discuss specifics. |
This assumes that #55 has been merged. This PR will do the following
.j2
files to.html
for IDE compatibility and readabilityTo-Do: