-
Notifications
You must be signed in to change notification settings - Fork 1
COHIV-40: update forms, batch 1 (wip) #161
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
base: develop
Are you sure you want to change the base?
Conversation
75b96ae to
e1540ad
Compare
# Conflicts: # django/.gitignore # django/README.md # django/bootstrap.sh # django/bootstrap_setup.py # django/docker-compose.dev.yml # django/docker/mariadb-init/01-create-test-db.sql # django/install.py # django/install_dependencies.sh
119ee89 to
42af8f8
Compare
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.
The new views look good! But I have a few points we should look at and probably discuss.
| ) | ||
| template_name = "geno/invoice_manual.html" | ||
| error_flag = False | ||
| tabs = [ |
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 seems redundant because the tabgroup is already defined in the settings COHIVA_ADMIN_NAVIGATION > Inkasso > Rechnungen erstellen. Also below in InvoiceBatchView.
| class ContractCheckFormsView(DocumentGeneratorView): | ||
| permission_required = ("geno.canview_share", "geno.rental_contracts") | ||
| doctype = "contract_check" | ||
| class ContractCheckFormsView(CohivaAdminViewMixin, TemplateView): |
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.
Why not subclassing DocumentGeneratorView anymore? It seems that this adds back in redundant code, that has been refactored away into DocumentGeneratorView before. It would be nicer if DocumentGeneratorView could be adapted to fit this use case as well. But we could also do that in a second step.
| # django-cors-headers | ||
| astroid==3.3.11 | ||
| # via pylint | ||
| async-timeout==5.0.1 |
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.
It seems that we still need that for Python 3.11, I would keep that in.
| # via | ||
| # aiohttp | ||
| # aiosignal | ||
| greenlet==3.2.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.
It seems that we still need that for Python 3.11, I would keep that in.
| table.order_by = "-total" | ||
| RequestConfig(self.request, paginate={"per_page": 100}).configure(table) | ||
| return {"title": "Debitoren", "table": table, "invoice_table": True} | ||
|
|
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.
The new table looks nice, but we loose functionality by not using the tables app anymore: Pagination and sorting are missing. Particularly the sorting by "Saldo" (ascending and descending) was an important feature. Can't we keep django_tables2 and customize the style like described here? Then we could use that also for other tables without hard-coding it in the view.
Also the search/filter does not seem to work anymore.
| Returns a dict mapping category_id to template data. | ||
| """ | ||
| return { | ||
| 7: { # Gästezimmer (Miete Gästezimmer) |
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.
Here we should actually use category_name as key and not category_id. The category id is arbitrary and can vary by instance.
also gets i18n started
I confirm that I have read the Contributor Agreement v1.1, agree to be bound on them and confirm that my contribution is compliant.