Merge pull request #859 from ae-utbm/account-pages

Optimize user account pages

core/templates/core/user_account.jinja

@@ -1,6 +1,6 @@
 {% extends "core/base.jinja" %}
 
-{% macro monthly(obj) %}
+{% macro monthly(objects) %}
   <div>
     <table>
       <thead>
@@ -11,17 +11,18 @@
         </tr>
       </thead>
       <tbody>
-        {% 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) %}
+        {% 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
+          ) %}
           <tr>
-                <td><a href="{{ link }}">{{ dict['date'].year }}</a></td>
-                <td><a href="{{ link }}">{{ dict['date']|date("E") }}</a></td>
-                <td><a href="{{ link }}">{{ dict['sum'] }} €</a></td>
+            <td><a href="{{ link }}">{{ object["grouped_date"]|date("Y") }}</a></td>
+            <td><a href="{{ link }}">{{ object["grouped_date"]|date("E") }}</a></td>
+            <td><a href="{{ link }}">{{ "%.2f"|format(object["total"]) }} €</a></td>
           </tr>
-            {% endif %}
-          {% endfor %}
         {% endfor %}
       </tbody>
     </table>
@@ -37,19 +38,15 @@
     <h3>{% trans %}User account{% endtrans %}</h3>
     <p>{% trans %}Amount: {% endtrans %}{{ customer.amount }} €</p>
     <div id="drop">
-      {% set bought = customer.buyings.exists() %}
-      {% set refilled = customer.refillings.exists() %}
-      {% if bought or refilled %}
-        {% if bought %}
+      {% if buyings_month %}
         <h5>{% trans %}Account purchases{% endtrans %}</h5>
         {{ monthly(buyings_month) }}
       {% endif %}
-        {% if refilled %}
+      {% if refilling_month %}
         <h5>{% trans %}Reloads{% endtrans %}</h5>
         {{ monthly(refilling_month) }}
       {% endif %}
-      {% endif %}
-      {% if customer.user.invoices.exists() %}
+      {% if invoices_month %}
         <h5>{% trans %}Eboutic invoices{% endtrans %}</h5>
         {{ monthly(invoices_month) }}
       {% endif %}
@@ -58,7 +55,11 @@
         <div>
           <ul>
             {% for s in etickets %}
-              <li><a href="{{ url('counter:eticket_pdf', selling_id=s.id) }}">{{ s.quantity }} x {{ s.product.eticket }}</a></li>
+              <li>
+                <a href="{{ url('counter:eticket_pdf', selling_id=s.id) }}">
+                  {{ s.quantity }} x {{ s.product.eticket }}
+                </a>
+              </li>
             {% endfor %}
           </ul>
         </div>

core/templates/core/user_account_detail.jinja

@@ -5,11 +5,10 @@
 {% endblock %}
 
 {% block content %}
-  {% if customer %}
   <h3>{% trans %}User account{% endtrans %}</h3>
   <p>{% trans %}Amount: {% endtrans %}{{ customer.amount }} €</p>
   <p><a href="{{ url('core:user_account', user_id=profile.id) }}">{% trans %}Back{% endtrans %}</a></p>
-    {% if customer.buyings.exists() %}
+  {% if purchases %}
     <h4>{% trans %}Account purchases{% endtrans %}</h4>
     <table>
       <thead>
@@ -24,25 +23,31 @@
         </tr>
       </thead>
       <tbody>
-          {% for i in customer.buyings.order_by('-date').all().filter(
-          date__year=year, date__month=month) %}
+        {% for purchase in purchases %}
           <tr>
-            <td>{{ i.date|localtime|date(DATETIME_FORMAT) }} - {{ i.date|localtime|time(DATETIME_FORMAT) }}</td>
-            <td>{{ i.counter }}</td>
-            <td><a href="{{ i.seller.get_absolute_url() }}">{{ i.seller.get_display_name() }}</a></td>
-            <td>{{ i.label }}</td>
-            <td>{{ i.quantity }}</td>
-            <td>{{ i.quantity * i.unit_price }} €</td>
-            <td>{{ i.get_payment_method_display() }}</td>
-            {% if i.is_owned_by(user) %}
-              <td><a href="{{ url('counter:selling_delete', selling_id=i.id) }}">{% trans %}Delete{% endtrans %}</a></td>
+            <td>
+              {{ purchase.date|localtime|date(DATETIME_FORMAT) }}
+              - {{ purchase.date|localtime|time(DATETIME_FORMAT) }}
+            </td>
+            <td>{{ purchase.counter }}</td>
+            <td><a href="{{ purchase.seller.get_absolute_url() }}">{{ purchase.seller.get_display_name() }}</a></td>
+            <td>{{ purchase.label }}</td>
+            <td>{{ purchase.quantity }}</td>
+            <td>{{ purchase.quantity * purchase.unit_price }} €</td>
+            <td>{{ purchase.get_payment_method_display() }}</td>
+            {% if purchase.is_owned_by(user) %}
+              <td>
+                <a href="{{ url('counter:selling_delete', selling_id=purchase.id) }}">
+                  {% trans %}Delete{% endtrans %}
+                </a>
+              </td>
             {% endif %}
           </tr>
         {% endfor %}
       </tbody>
     </table>
   {% endif %}
-  {% if customer.refillings.exists() %}
+  {% if refills %}
     <h4>{% trans %}Reloads{% endtrans %}</h4>
     <table>
       <thead>
@@ -55,22 +60,30 @@
         </tr>
       </thead>
       <tbody>
-        {% for i in customer.refillings.order_by('-date').filter( date__year=year, date__month=month) %}
+        {% for refill in refills %}
           <tr>
-            <td>{{ i.date|localtime|date(DATETIME_FORMAT) }} - {{ i.date|localtime|time(DATETIME_FORMAT) }}</td>
-            <td>{{ i.counter }}</td>
-            <td><a href="{{ i.operator.get_absolute_url() }}">{{ i.operator.get_display_name() }}</a></td>
-            <td>{{ i.amount }} €</td>
-            <td>{{ i.get_payment_method_display() }}</td>
-            {% if i.is_owned_by(user) %}
-              <td><a href="{{ url('counter:refilling_delete', refilling_id=i.id) }}">{% trans %}Delete{% endtrans %}</a></td>
+            <td>{{ refill.date|localtime|date(DATETIME_FORMAT) }} - {{ refill.date|localtime|time(DATETIME_FORMAT) }}</td>
+            <td>{{ refill.counter }}</td>
+            <td>
+              <a href="{{ refill.operator.get_absolute_url() }}">
+                {{ refill.operator.get_display_name() }}
+              </a>
+            </td>
+            <td>{{ refill.amount }} €</td>
+            <td>{{ refill.get_payment_method_display() }}</td>
+            {% if refill.is_owned_by(user) %}
+              <td>
+                <a href="{{ url('counter:refilling_delete', refilling_id=refill.id) }}">
+                  {% trans %}Delete{% endtrans %}
+                </a>
+              </td>
             {% endif %}
           </tr>
         {% endfor %}
       </tbody>
     </table>
   {% endif %}
-  {% if customer.user.invoices.exists() %}
+  {% if invoices %}
     <h4>{% trans %}Eboutic invoices{% endtrans %}</h4>
     <table>
       <thead>
@@ -81,25 +94,24 @@
         </tr>
       </thead>
       <tbody>
-        {% for i in customer.user.invoices.order_by('-date').all().filter(
-        date__year=year, date__month=month) %}
+        {% for invoice in invoices %}
           <tr>
-          <td>{{ i.date|localtime|date(DATETIME_FORMAT) }} - {{ i.date|localtime|time(DATETIME_FORMAT) }}</td>
+            <td>
+              {{ invoice.date|localtime|date(DATETIME_FORMAT) }}
+              - {{ invoice.date|localtime|time(DATETIME_FORMAT) }}
+            </td>
             <td>
               <ul>
-              {% for it in i.items.all() %}
+                {% for it in invoice.items.all() %}
                   <li>{{ it.quantity }} x {{ it.product_name }} - {{ it.product_unit_price }} €</li>
                 {% endfor %}
               </ul>
             </td>
-          <td>{{ i.get_total() }} €</td>
+            <td>{{ invoice.total }} €</td>
           </tr>
         {% endfor %}
       </tbody>
     </table>
-{% endif %}
-{% else %}
-  <p>{% trans %}User has no account{% endtrans %}</p>
-{% endif %}
-<p><a href="{{ url('core:user_account', user_id=profile.id) }}">{% trans %}Back{% endtrans %}</a></p>
+  {% endif %}
+  <p><a href="{{ url('core:user_account', user_id=profile.id) }}">{% trans %}Back{% endtrans %}</a></p>
 {% endblock %}

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

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,65 +618,57 @@ 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
 
+    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."""
 
     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
+            Selling.objects.filter(customer=self.object.customer)
         )
         kwargs["refilling_month"] = self.expense_by_month(
-                self.object.customer.refillings, (lambda q: q.amount)
+            Refilling.objects.filter(customer=self.object.customer)
         )
-            kwargs["etickets"] = self.object.customer.buyings.exclude(
-                product__eticket=None
-            ).all()
-        except Exception as e:
-            logging.error(e)
+        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
 
 
@@ -685,13 +680,29 @@ class UserAccountDetailView(UserAccountBase, YearMixin, MonthMixin):
     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"
+        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
 
 

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")
 

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}"