From 58d3a7ee2c9bf63d4640167861eff4c7eaaa0184 Mon Sep 17 00:00:00 2001 From: imperosol Date: Fri, 4 Oct 2024 13:41:15 +0200 Subject: [PATCH 1/2] Optimize user account pages --- core/templates/core/user_account.jinja | 51 +++---- core/templates/core/user_account_detail.jinja | 138 ++++++++++-------- core/views/user.py | 102 +++++++------ counter/models.py | 32 +++- eboutic/models.py | 22 ++- 5 files changed, 208 insertions(+), 137 deletions(-) diff --git a/core/templates/core/user_account.jinja b/core/templates/core/user_account.jinja index c1a8b742..467f864a 100644 --- a/core/templates/core/user_account.jinja +++ b/core/templates/core/user_account.jinja @@ -1,6 +1,6 @@ {% extends "core/base.jinja" %} -{% macro monthly(obj) %} +{% macro monthly(objects) %}
@@ -11,17 +11,18 @@ - {% for array in obj %} - {% for dict in array %} - {% if dict['sum'] != 0 %} - {% set link=url('core:user_account_detail', user_id=profile.id, year=dict['date'].year, month=dict['date'].month) %} - - - - - - {% endif %} - {% endfor %} + {% for object in objects %} + {% set link=url( + 'core:user_account_detail', + user_id=profile.id, + year=object['grouped_date'].year, + month=object['grouped_date'].month + ) %} + + + + + {% endfor %}
{{ dict['date'].year }}{{ dict['date']|date("E") }}{{ dict['sum'] }} €
{{ object["grouped_date"]|date("Y") }}{{ object["grouped_date"]|date("E") }}{{ "%.2f"|format(object["total"]) }} €
@@ -37,19 +38,15 @@

{% trans %}User account{% endtrans %}

{% trans %}Amount: {% endtrans %}{{ customer.amount }} €

- {% set bought = customer.buyings.exists() %} - {% set refilled = customer.refillings.exists() %} - {% if bought or refilled %} - {% if bought %} -
{% trans %}Account purchases{% endtrans %}
- {{ monthly(buyings_month) }} - {% endif %} - {% if refilled %} -
{% trans %}Reloads{% endtrans %}
- {{ monthly(refilling_month) }} - {% endif %} + {% if buyings_month %} +
{% trans %}Account purchases{% endtrans %}
+ {{ monthly(buyings_month) }} {% endif %} - {% if customer.user.invoices.exists() %} + {% if refilling_month %} +
{% trans %}Reloads{% endtrans %}
+ {{ monthly(refilling_month) }} + {% endif %} + {% if invoices_month %}
{% trans %}Eboutic invoices{% endtrans %}
{{ monthly(invoices_month) }} {% endif %} @@ -58,7 +55,11 @@
diff --git a/core/templates/core/user_account_detail.jinja b/core/templates/core/user_account_detail.jinja index 8c9b3c3c..ccf41cfb 100644 --- a/core/templates/core/user_account_detail.jinja +++ b/core/templates/core/user_account_detail.jinja @@ -5,44 +5,49 @@ {% endblock %} {% block content %} - {% if customer %} -

{% trans %}User account{% endtrans %}

-

{% trans %}Amount: {% endtrans %}{{ customer.amount }} €

-

{% trans %}Back{% endtrans %}

- {% if customer.buyings.exists() %} -

{% trans %}Account purchases{% endtrans %}

- - +

{% trans %}User account{% endtrans %}

+

{% trans %}Amount: {% endtrans %}{{ customer.amount }} €

+

{% trans %}Back{% endtrans %}

+ {% if purchases %} +

{% trans %}Account purchases{% endtrans %}

+
+ + + + + + + + + + + + + {% for purchase in purchases %} - - - - - - - - - - - {% for i in customer.buyings.order_by('-date').all().filter( - date__year=year, date__month=month) %} - - - - - - - - - {% if i.is_owned_by(user) %} - + + + + + + + + {% if purchase.is_owned_by(user) %} + {% endif %} - {% endfor %} - + {% endfor %} +
{% trans %}Date{% endtrans %}{% trans %}Counter{% endtrans %}{% trans %}Barman{% endtrans %}{% trans %}Label{% endtrans %}{% trans %}Quantity{% endtrans %}{% trans %}Total{% endtrans %}{% trans %}Payment method{% endtrans %}
{% trans %}Date{% endtrans %}{% trans %}Counter{% endtrans %}{% trans %}Barman{% endtrans %}{% trans %}Label{% endtrans %}{% trans %}Quantity{% endtrans %}{% trans %}Total{% endtrans %}{% trans %}Payment method{% endtrans %}
{{ i.date|localtime|date(DATETIME_FORMAT) }} - {{ i.date|localtime|time(DATETIME_FORMAT) }}{{ i.counter }}{{ i.seller.get_display_name() }}{{ i.label }}{{ i.quantity }}{{ i.quantity * i.unit_price }} €{{ i.get_payment_method_display() }}{% trans %}Delete{% endtrans %} + {{ purchase.date|localtime|date(DATETIME_FORMAT) }} + - {{ purchase.date|localtime|time(DATETIME_FORMAT) }} + {{ purchase.counter }}{{ purchase.seller.get_display_name() }}{{ purchase.label }}{{ purchase.quantity }}{{ purchase.quantity * purchase.unit_price }} €{{ purchase.get_payment_method_display() }} + + {% trans %}Delete{% endtrans %} + +
{% endif %} - {% if customer.refillings.exists() %} + {% if refills %}

{% trans %}Reloads{% endtrans %}

@@ -55,22 +60,30 @@ - {% for i in customer.refillings.order_by('-date').filter( date__year=year, date__month=month) %} + {% for refill in refills %} - - - - - - {% if i.is_owned_by(user) %} - + + + + + + {% if refill.is_owned_by(user) %} + {% endif %} {% endfor %}
{{ i.date|localtime|date(DATETIME_FORMAT) }} - {{ i.date|localtime|time(DATETIME_FORMAT) }}{{ i.counter }}{{ i.operator.get_display_name() }}{{ i.amount }} €{{ i.get_payment_method_display() }}{% trans %}Delete{% endtrans %}{{ refill.date|localtime|date(DATETIME_FORMAT) }} - {{ refill.date|localtime|time(DATETIME_FORMAT) }}{{ refill.counter }} + + {{ refill.operator.get_display_name() }} + + {{ refill.amount }} €{{ refill.get_payment_method_display() }} + + {% trans %}Delete{% endtrans %} + +
{% endif %} - {% if customer.user.invoices.exists() %} + {% if invoices %}

{% trans %}Eboutic invoices{% endtrans %}

@@ -81,25 +94,24 @@ - {% for i in customer.user.invoices.order_by('-date').all().filter( - date__year=year, date__month=month) %} - - - - - - {% endfor %} - -
{{ i.date|localtime|date(DATETIME_FORMAT) }} - {{ i.date|localtime|time(DATETIME_FORMAT) }} -
    - {% for it in i.items.all() %} -
  • {{ it.quantity }} x {{ it.product_name }} - {{ it.product_unit_price }} €
    • - {% endfor %} -
- 		{{ i.get_total() }} €
-{% endif %} -{% else %} -

{% trans %}User has no account{% endtrans %}

-{% endif %} -

{% trans %}Back{% endtrans %}

+ {% for invoice in invoices %} + + + {{ invoice.date|localtime|date(DATETIME_FORMAT) }} + - {{ invoice.date|localtime|time(DATETIME_FORMAT) }} + + +
    + {% for it in invoice.items.all() %} +
  • {{ it.quantity }} x {{ it.product_name }} - {{ it.product_unit_price }} €
    • + {% endfor %} +
+ + {{ invoice.total }} € + + {% endfor %} + + + {% endif %} +

{% trans %}Back{% endtrans %}

{% endblock %} diff --git a/core/views/user.py b/core/views/user.py index 58ab9738..183768a3 100644 --- a/core/views/user.py +++ b/core/views/user.py @@ -21,7 +21,6 @@ # Place - Suite 330, Boston, MA 02111-1307, USA. # # -import logging # This file contains all the views that concern the user model from datetime import date, timedelta @@ -32,6 +31,8 @@ from django.contrib.auth import login, views from django.contrib.auth.forms import PasswordChangeForm from django.contrib.auth.mixins import LoginRequiredMixin from django.core.exceptions import PermissionDenied +from django.db.models import DateField, QuerySet +from django.db.models.functions import Trunc from django.forms import CheckboxSelectMultiple from django.forms.models import modelform_factory from django.http import Http404 @@ -68,6 +69,8 @@ from core.views.forms import ( UserProfileForm, ) from counter.forms import StudentCardForm +from counter.models import Refilling, Selling +from eboutic.models import Invoice from subscription.models import Subscription from trombi.views import UserTrombiForm @@ -615,18 +618,18 @@ class UserAccountBase(UserTabsMixin, DetailView): model = User pk_url_kwarg = "user_id" current_tab = "account" + queryset = User.objects.select_related("customer") def dispatch(self, request, *arg, **kwargs): # Manually validates the rights - res = super().dispatch(request, *arg, **kwargs) if ( - self.object == request.user + kwargs.get("user_id") == request.user.id or request.user.is_in_group(pk=settings.SITH_GROUP_ACCOUNTING_ADMIN_ID) or request.user.is_in_group( name=settings.SITH_BAR_MANAGER["unix_name"] + settings.SITH_BOARD_SUFFIX ) or request.user.is_root ): - return res + return super().dispatch(request, *arg, **kwargs) raise PermissionDenied @@ -635,45 +638,31 @@ class UserAccountView(UserAccountBase): template_name = "core/user_account.jinja" - def expense_by_month(self, obj, calc): - stats = [] - - for year in obj.datetimes("date", "year", order="DESC"): - stats.append([]) - i = 0 - for month in obj.filter(date__year=year.year).datetimes( - "date", "month", order="DESC" - ): - q = obj.filter(date__year=month.year, date__month=month.month) - stats[i].append({"sum": sum([calc(p) for p in q]), "date": month}) - i += 1 - return stats - - def invoices_calc(self, query): - t = 0 - for it in query.items.all(): - t += it.quantity * it.product_unit_price - return t + @staticmethod + def expense_by_month[T](qs: QuerySet[T]) -> QuerySet[T]: + month_trunc = Trunc("date", "month", output_field=DateField()) + return ( + qs.annotate(grouped_date=month_trunc) + .values("grouped_date") + .annotate_total() + .exclude(total=0) + .order_by("-grouped_date") + ) def get_context_data(self, **kwargs): kwargs = super().get_context_data(**kwargs) kwargs["profile"] = self.object - try: - kwargs["customer"] = self.object.customer - kwargs["buyings_month"] = self.expense_by_month( - self.object.customer.buyings, (lambda q: q.unit_price * q.quantity) - ) - kwargs["invoices_month"] = self.expense_by_month( - self.object.customer.user.invoices, self.invoices_calc - ) - kwargs["refilling_month"] = self.expense_by_month( - self.object.customer.refillings, (lambda q: q.amount) - ) - kwargs["etickets"] = self.object.customer.buyings.exclude( - product__eticket=None - ).all() - except Exception as e: - logging.error(e) + kwargs["customer"] = self.object.customer + kwargs["buyings_month"] = self.expense_by_month( + Selling.objects.filter(customer=self.object.customer) + ) + kwargs["refilling_month"] = self.expense_by_month( + Refilling.objects.filter(customer=self.object.customer) + ) + kwargs["invoices_month"] = self.expense_by_month( + Invoice.objects.filter(user=self.object) + ) + kwargs["etickets"] = self.object.customer.buyings.exclude(product__eticket=None) return kwargs @@ -682,16 +671,37 @@ class UserAccountDetailView(UserAccountBase, YearMixin, MonthMixin): template_name = "core/user_account_detail.jinja" + def get(self, request, *args, **kwargs): + if not hasattr(self.get_object(), "customer"): + raise Http404(_("This user has no account")) + return super().get(request, *args, **kwargs) + def get_context_data(self, **kwargs): kwargs = super().get_context_data(**kwargs) kwargs["profile"] = self.object - kwargs["year"] = self.get_year() - kwargs["month"] = self.get_month() - try: - kwargs["customer"] = self.object.customer - except: - pass - kwargs["tab"] = "account" + kwargs["customer"] = self.object.customer + year, month = self.get_year(), self.get_month() + filters = { + "customer": self.object.customer, + "date__year": year, + "date__month": month, + } + kwargs["purchases"] = list( + Selling.objects.filter(**filters) + .select_related("counter", "counter__club", "seller") + .order_by("-date") + ) + kwargs["refills"] = list( + Refilling.objects.filter(**filters) + .select_related("counter", "counter__club", "operator") + .order_by("-date") + ) + kwargs["invoices"] = list( + Invoice.objects.filter(user=self.object, date__year=year, date__month=month) + .annotate_total() + .prefetch_related("items") + .order_by("-date") + ) return kwargs diff --git a/counter/models.py b/counter/models.py index d99dba4b..f41f6c8c 100644 --- a/counter/models.py +++ b/counter/models.py @@ -20,7 +20,7 @@ import random import string from datetime import date, datetime, timedelta from datetime import timezone as tz -from typing import Tuple +from typing import Self, Tuple from dict2xml import dict2xml from django.conf import settings @@ -585,6 +585,23 @@ class Counter(models.Model): )["total"] +class RefillingQuerySet(models.QuerySet): + def annotate_total(self) -> Self: + """Annotate the Queryset with the total amount. + + The total is just the sum of the amounts for each row. + If no grouping is involved (like in most queries), + this is just the same as doing nothing and fetching the + `amount` attribute. + + However, it may be useful when there is a `group by` clause + in the query, or when other models are queried and having + a common interface is helpful (e.g. `Selling.objects.annotate_total()` + and `Refilling.objects.annotate_total()` will both have the `total` field). + """ + return self.annotate(total=Sum("amount")) + + class Refilling(models.Model): """Handle the refilling.""" @@ -613,6 +630,8 @@ class Refilling(models.Model): ) is_validated = models.BooleanField(_("is validated"), default=False) + objects = RefillingQuerySet.as_manager() + class Meta: verbose_name = _("refilling") @@ -657,6 +676,15 @@ class Refilling(models.Model): super().delete(*args, **kwargs) +class SellingQuerySet(models.QuerySet): + def annotate_total(self) -> Self: + """Annotate the Queryset with the total amount of the sales. + + The total is considered as the sum of (unit_price * quantity). + """ + return self.annotate(total=Sum(F("unit_price") * F("quantity"))) + + class Selling(models.Model): """Handle the sellings.""" @@ -703,6 +731,8 @@ class Selling(models.Model): ) is_validated = models.BooleanField(_("is validated"), default=False) + objects = SellingQuerySet.as_manager() + class Meta: verbose_name = _("selling") diff --git a/eboutic/models.py b/eboutic/models.py index 63a5d5e5..0b8c30e1 100644 --- a/eboutic/models.py +++ b/eboutic/models.py @@ -16,12 +16,12 @@ from __future__ import annotations import hmac from datetime import datetime -from typing import Any +from typing import Any, Self from dict2xml import dict2xml from django.conf import settings from django.db import DataError, models -from django.db.models import F, Sum +from django.db.models import F, OuterRef, Subquery, Sum from django.utils.functional import cached_property from django.utils.translation import gettext_lazy as _ @@ -160,6 +160,22 @@ class Basket(models.Model): return data +class InvoiceQueryset(models.QuerySet): + def annotate_total(self) -> Self: + """Annotate the queryset with the total amount of each invoice. + + The total amount is the sum of (product_unit_price * quantity) + for all items related to the invoice. + """ + return self.annotate( + total=Subquery( + InvoiceItem.objects.filter(invoice_id=OuterRef("pk")) + .annotate(total=Sum(F("product_unit_price") * F("quantity"))) + .values("total") + ) + ) + + class Invoice(models.Model): """Invoices are generated once the payment has been validated.""" @@ -173,6 +189,8 @@ class Invoice(models.Model): date = models.DateTimeField(_("date"), auto_now=True) validated = models.BooleanField(_("validated"), default=False) + objects = InvoiceQueryset.as_manager() + def __str__(self): return f"{self.user} - {self.get_total()} - {self.date}" From b0884c6b0432fb2ae11e24b39eb03e14154eb525 Mon Sep 17 00:00:00 2001 From: imperosol Date: Tue, 8 Oct 2024 15:30:35 +0200 Subject: [PATCH 2/2] return 404 when accessing not existing account --- core/tests/test_user.py | 18 +++++++++++++++++- core/views/user.py | 11 ++++++----- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/core/tests/test_user.py b/core/tests/test_user.py index 589755f6..9794b9f6 100644 --- a/core/tests/test_user.py +++ b/core/tests/test_user.py @@ -1,7 +1,8 @@ from datetime import timedelta +import pytest from django.core.management import call_command -from django.test import TestCase +from django.test import Client, TestCase from django.urls import reverse from django.utils.timezone import now from model_bakery import baker, seq @@ -95,3 +96,18 @@ class TestSearchUsersView(TestSearchUsers): self.client.force_login(subscriber_user.make()) response = self.client.get(reverse("core:search")) assert response.status_code == 200 + + +@pytest.mark.django_db +def test_user_account_not_found(client: Client): + client.force_login(baker.make(User, is_superuser=True)) + user = baker.make(User) + res = client.get(reverse("core:user_account", kwargs={"user_id": user.id})) + assert res.status_code == 404 + res = client.get( + reverse( + "core:user_account_detail", + kwargs={"user_id": user.id, "year": 2024, "month": 10}, + ) + ) + assert res.status_code == 404 diff --git a/core/views/user.py b/core/views/user.py index 183768a3..286a3a92 100644 --- a/core/views/user.py +++ b/core/views/user.py @@ -632,6 +632,12 @@ class UserAccountBase(UserTabsMixin, DetailView): return super().dispatch(request, *arg, **kwargs) raise PermissionDenied + def get_object(self, queryset=None): + obj = super().get_object(queryset) + if not hasattr(obj, "customer"): + raise Http404(_("User has no account")) + return obj + class UserAccountView(UserAccountBase): """Display a user's account.""" @@ -671,11 +677,6 @@ class UserAccountDetailView(UserAccountBase, YearMixin, MonthMixin): template_name = "core/user_account_detail.jinja" - def get(self, request, *args, **kwargs): - if not hasattr(self.get_object(), "customer"): - raise Http404(_("This user has no account")) - return super().get(request, *args, **kwargs) - def get_context_data(self, **kwargs): kwargs = super().get_context_data(**kwargs) kwargs["profile"] = self.object