From 9e0cb7647b191fc066e5554911106346cbde7e52 Mon Sep 17 00:00:00 2001 From: Thomas Girod Date: Sun, 6 Apr 2025 13:37:03 +0200 Subject: [PATCH] fix counter stats page access --- .../migrations/0031_alter_counter_options.py | 19 ++ counter/models.py | 21 ++- counter/tests/test_counter.py | 170 +++++++----------- counter/views/admin.py | 17 +- 4 files changed, 100 insertions(+), 127 deletions(-) create mode 100644 counter/migrations/0031_alter_counter_options.py diff --git a/counter/migrations/0031_alter_counter_options.py b/counter/migrations/0031_alter_counter_options.py new file mode 100644 index 00000000..c6d68529 --- /dev/null +++ b/counter/migrations/0031_alter_counter_options.py @@ -0,0 +1,19 @@ +# Generated by Django 4.2.20 on 2025-04-06 11:29 + +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("counter", "0030_returnableproduct_returnableproductbalance_and_more") + ] + + operations = [ + migrations.AlterModelOptions( + name="counter", + options={ + "permissions": [("view_counter_stats", "Can view counter stats")], + "verbose_name": "counter", + }, + ), + ] diff --git a/counter/models.py b/counter/models.py index ee6088d9..8581b19d 100644 --- a/counter/models.py +++ b/counter/models.py @@ -526,6 +526,7 @@ class Counter(models.Model): class Meta: verbose_name = _("counter") + permissions = [("view_counter_stats", "Can view counter stats")] def __str__(self): return self.name @@ -598,13 +599,12 @@ class Counter(models.Model): - the promo of the barman - the total number of office hours the barman did attend """ + name_expr = Concat(F("user__first_name"), Value(" "), F("user__last_name")) return ( self.permanencies.exclude(end=None) .annotate( - name=Concat(F("user__first_name"), Value(" "), F("user__last_name")) + name=name_expr, nickname=F("user__nick_name"), promo=F("user__promo") ) - .annotate(nickname=F("user__nick_name")) - .annotate(promo=F("user__promo")) .values("user", "name", "nickname", "promo") .annotate(perm_sum=Sum(F("end") - F("start"))) .exclude(perm_sum=None) @@ -628,18 +628,17 @@ class Counter(models.Model): since = get_start_of_semester() if isinstance(since, date): since = datetime(since.year, since.month, since.day, tzinfo=tz.utc) + name_expr = Concat( + F("customer__user__first_name"), Value(" "), F("customer__user__last_name") + ) return ( self.sellings.filter(date__gte=since) .annotate( - name=Concat( - F("customer__user__first_name"), - Value(" "), - F("customer__user__last_name"), - ) + name=name_expr, + nickname=F("customer__user__nick_name"), + promo=F("customer__user__promo"), + user=F("customer__user"), ) - .annotate(nickname=F("customer__user__nick_name")) - .annotate(promo=F("customer__user__promo")) - .annotate(user=F("customer__user")) .values("user", "promo", "name", "nickname") .annotate( selling_sum=Sum( diff --git a/counter/tests/test_counter.py b/counter/tests/test_counter.py index f0773897..e3b885b7 100644 --- a/counter/tests/test_counter.py +++ b/counter/tests/test_counter.py @@ -18,7 +18,7 @@ from decimal import Decimal import pytest from django.conf import settings -from django.contrib.auth.models import make_password +from django.contrib.auth.models import Permission, make_password from django.core.cache import cache from django.http import HttpResponse from django.shortcuts import resolve_url @@ -28,9 +28,10 @@ from django.utils import timezone from django.utils.timezone import localdate, now from freezegun import freeze_time from model_bakery import baker +from model_bakery.recipe import Recipe from pytest_django.asserts import assertRedirects -from club.models import Club, Membership +from club.models import Membership from core.baker_recipes import board_user, subscriber_user, very_old_subscriber_user from core.models import BanGroup, User from counter.baker_recipes import product_recipe, sale_recipe @@ -572,121 +573,86 @@ class TestCounterClick(TestFullClickBase): class TestCounterStats(TestCase): @classmethod def setUpTestData(cls): - cls.counter = Counter.objects.get(id=2) - cls.krophil = User.objects.get(username="krophil") - cls.skia = User.objects.get(username="skia") - cls.sli = User.objects.get(username="sli") - cls.root = User.objects.get(username="root") - cls.subscriber = User.objects.get(username="subscriber") - cls.old_subscriber = User.objects.get(username="old_subscriber") - cls.counter.sellers.add(cls.sli, cls.root, cls.skia, cls.krophil) - - barbar = Product.objects.get(code="BARB") - - # remove everything to make sure the fixtures bring no side effect - Permanency.objects.all().delete() - Selling.objects.all().delete() - - now = timezone.now() - # total of sli : 5 hours - Permanency.objects.create( - user=cls.sli, start=now, end=now + timedelta(hours=1), counter=cls.counter - ) - Permanency.objects.create( - user=cls.sli, - start=now + timedelta(hours=4), - end=now + timedelta(hours=6), - counter=cls.counter, - ) - Permanency.objects.create( - user=cls.sli, - start=now + timedelta(hours=7), - end=now + timedelta(hours=9), - counter=cls.counter, + cls.users = subscriber_user.make(_quantity=4) + product = product_recipe.make(selling_price=1) + cls.counter = baker.make( + Counter, type=["BAR"], sellers=cls.users[:4], products=[product] ) - # total of skia : 16 days, 2 hours, 35 minutes and 54 seconds - Permanency.objects.create( - user=cls.skia, start=now, end=now + timedelta(hours=1), counter=cls.counter - ) - Permanency.objects.create( - user=cls.skia, - start=now + timedelta(days=4, hours=1), - end=now + timedelta(days=20, hours=2, minutes=35, seconds=54), - counter=cls.counter, - ) + _now = timezone.now() + permanence_recipe = Recipe(Permanency, counter=cls.counter) + perms = [ + *[ # total of user 0 : 5 hours + permanence_recipe.prepare(user=cls.users[0], start=start, end=end) + for start, end in [ + (_now, _now + timedelta(hours=1)), + (_now + timedelta(hours=4), _now + timedelta(hours=6)), + (_now + timedelta(hours=7), _now + timedelta(hours=9)), + ] + ], + *[ # total of user 1 : 16 days, 2 hours, 35 minutes and 54 seconds + permanence_recipe.prepare(user=cls.users[1], start=start, end=end) + for start, end in [ + (_now, _now + timedelta(hours=1)), + ( + _now + timedelta(days=4, hours=1), + _now + timedelta(days=20, hours=2, minutes=35, seconds=54), + ), + ] + ], + *[ # total of user 2 : 2 hour + 20 hours (but the 20 hours were on last year) + permanence_recipe.prepare(user=cls.users[2], start=start, end=end) + for start, end in [ + (_now + timedelta(days=5), _now + timedelta(days=5, hours=1)), + (_now - timedelta(days=300, hours=20), _now - timedelta(days=300)), + ] + ], + ] + # user 3 has 0 hours of permanence + Permanency.objects.bulk_create(perms) - # total of root : 1 hour + 20 hours (but the 20 hours were on last year) - Permanency.objects.create( - user=cls.root, - start=now + timedelta(days=5), - end=now + timedelta(days=5, hours=1), - counter=cls.counter, - ) - Permanency.objects.create( - user=cls.root, - start=now - timedelta(days=300, hours=20), - end=now - timedelta(days=300), - counter=cls.counter, - ) - - # total of krophil : 0 hour - s = Selling( - label=barbar.name, - product=barbar, - club=baker.make(Club), + _sale_recipe = Recipe( + Selling, + club=cls.counter.club, counter=cls.counter, + product=product, unit_price=2, - seller=cls.skia, ) + sales = [ + *_sale_recipe.prepare( + quantity=100, customer=cls.users[0].customer, _quantity=10 + ), # 2000 € + *_sale_recipe.prepare( + quantity=100, customer=cls.users[1].customer, _quantity=5 + ), # 1000 € + _sale_recipe.prepare(quantity=1, customer=cls.users[2].customer), # 2€ + _sale_recipe.prepare(quantity=50, customer=cls.users[3].customer), # 100€ + ] + Selling.objects.bulk_create(sales) - krophil_customer = Customer.get_or_create(cls.krophil)[0] - sli_customer = Customer.get_or_create(cls.sli)[0] - skia_customer = Customer.get_or_create(cls.skia)[0] - root_customer = Customer.get_or_create(cls.root)[0] - - # moderate drinker. Total : 100 € - s.quantity = 50 - s.customer = krophil_customer - s.save(allow_negative=True) - - # Sli is a drunkard. Total : 2000 € - s.quantity = 100 - s.customer = sli_customer - for _ in range(10): - # little trick to make sure the instance is duplicated in db - s.pk = None - s.save(allow_negative=True) # save ten different sales - - # Skia is a heavy drinker too. Total : 1000 € - s.customer = skia_customer - for _ in range(5): - s.pk = None - s.save(allow_negative=True) - - # Root is quite an abstemious one. Total : 2 € - s.pk = None - s.quantity = 1 - s.customer = root_customer - s.save(allow_negative=True) - - def test_not_authenticated_user_fail(self): - # Test with not login user - response = self.client.get(reverse("counter:stats", args=[self.counter.id])) - assert response.status_code == 403 + def test_not_authenticated_access_fail(self): + url = reverse("counter:stats", args=[self.counter.id]) + response = self.client.get(url) + assertRedirects(response, reverse("core:login") + f"?next={url}") def test_unauthorized_user_fails(self): - self.client.force_login(User.objects.get(username="public")) + self.client.force_login(baker.make(User)) response = self.client.get(reverse("counter:stats", args=[self.counter.id])) assert response.status_code == 403 + def test_authorized_user_ok(self): + perm = Permission.objects.get(codename="view_counter_stats") + self.client.force_login(baker.make(User, user_permissions=[perm])) + response = self.client.get(reverse("counter:stats", args=[self.counter.id])) + assert response.status_code == 200 + def test_get_total_sales(self): """Test the result of the Counter.get_total_sales() method.""" assert self.counter.get_total_sales() == 3102 def test_top_barmen(self): """Test the result of Counter.get_top_barmen() is correct.""" - users = [self.skia, self.root, self.sli] + users = [self.users[1], self.users[2], self.users[0]] perm_times = [ timedelta(days=16, hours=2, minutes=35, seconds=54), timedelta(hours=21), @@ -700,12 +666,12 @@ class TestCounterStats(TestCase): "nickname": user.nick_name, "perm_sum": perm_time, } - for user, perm_time in zip(users, perm_times, strict=False) + for user, perm_time in zip(users, perm_times, strict=True) ] def test_top_customer(self): """Test the result of Counter.get_top_customers() is correct.""" - users = [self.sli, self.skia, self.krophil, self.root] + users = [self.users[0], self.users[1], self.users[3], self.users[2]] sale_amounts = [2000, 1000, 100, 2] assert list(self.counter.get_top_customers()) == [ { @@ -715,7 +681,7 @@ class TestCounterStats(TestCase): "nickname": user.nick_name, "selling_sum": sale_amount, } - for user, sale_amount in zip(users, sale_amounts, strict=False) + for user, sale_amount in zip(users, sale_amounts, strict=True) ] diff --git a/counter/views/admin.py b/counter/views/admin.py index f7d4a66b..ddd7a40e 100644 --- a/counter/views/admin.py +++ b/counter/views/admin.py @@ -27,7 +27,7 @@ from django.utils.translation import gettext as _ from django.views.generic import DetailView, ListView, TemplateView from django.views.generic.edit import CreateView, DeleteView, FormView, UpdateView -from core.auth.mixins import CanEditMixin, CanViewMixin +from core.auth.mixins import CanViewMixin from core.utils import get_semester_code, get_start_of_semester from counter.forms import ( CloseCustomerAccountForm, @@ -274,12 +274,13 @@ class SellingDeleteView(DeleteView): raise PermissionDenied -class CounterStatView(DetailView, CounterAdminMixin): +class CounterStatView(PermissionRequiredMixin, DetailView): """Show the bar stats.""" model = Counter pk_url_kwarg = "counter_id" template_name = "counter/stats.jinja" + permission_required = "counter.view_counter_stats" def get_context_data(self, **kwargs): """Add stats to the context.""" @@ -301,18 +302,6 @@ class CounterStatView(DetailView, CounterAdminMixin): ) return kwargs - def dispatch(self, request, *args, **kwargs): - try: - return super().dispatch(request, *args, **kwargs) - except PermissionDenied: - if ( - request.user.is_root - or request.user.is_board_member - or self.get_object().is_owned_by(request.user) - ): - return super(CanEditMixin, self).dispatch(request, *args, **kwargs) - raise PermissionDenied - class CounterRefillingListView(CounterAdminTabsMixin, CounterAdminMixin, ListView): """List of refillings on a counter."""