[FEATURE] Vidage des comptes des anciens utilisateurs #633
Conversation
Utilisation d'agrégations pour limiter les impacts sur les performances de la base de données.
…ure/accounts-dump
Fix les tests unitaires Co-authored-by: thomas girod <[email protected]>
| @cached_property | ||
| def last_subscription_date(self) -> Optional[date]: | ||
| latest_subscription = ( | ||
| self.subscriptions.filter(subscription_end__lte=timezone.now()) | ||
| .order_by("-subscription_end") | ||
| .first() | ||
| ) | ||
| if latest_subscription: | ||
| return latest_subscription.subscription_end | ||
| else: | ||
| return None |
There was a problem hiding this comment.
La fonction est contre-intuitive. Le nom suggère que la dernière cotisation est renvoyée, alors qu'en fait c'est la dernière parmi celles qui sont terminées. C'est un coup à ce que quelqu'un l'utilise mal plus tard.
Aussi, pourquoi se limiter à la date de fin de la dernière cotisation ? C'est un peu une sous-utilisation du fonctionnement des propriétés en cache. Autant renvoyer la cotisation complète.
| @cached_property | |
| def last_subscription_date(self) -> Optional[date]: | |
| latest_subscription = ( | |
| self.subscriptions.filter(subscription_end__lte=timezone.now()) | |
| .order_by("-subscription_end") | |
| .first() | |
| ) | |
| if latest_subscription: | |
| return latest_subscription.subscription_end | |
| else: | |
| return None | |
| @cached_property | |
| def last_subscription(self) -> Optional["Subscription"]: | |
| return self.subscriptions.order_by("subscription_end").last() |
| def last_subscription_since(self) -> Tuple[int, int, int]: | ||
| last_date = self.last_subscription_date | ||
| if last_date is None: | ||
| return (0, 0, 0) | ||
|
|
||
| diff = date.today() - last_date | ||
| return [diff.days // 365, (diff.days % 365) // 30, (diff.days % 365) % 30] |
There was a problem hiding this comment.
Le type de retour ne dit rien sur ce que c'est et la fonction est utilisée dans un contexte où des balises de template existent déjà. Mieux vaut retirer.
| {% trans %}Not subscribed{% endtrans %} | ||
| {% if profile.was_subscribed %} | ||
| {% set duration = profile.last_subscription_since() %} | ||
| {% trans y=duration[0], m=duration[1], d=duration[2], date=profile.last_subscription_date %}Not subscribed for {{ y }} year(s) {{ m }} month(s) {{ d }} day(s) ({{ date }}){% endtrans %} |
There was a problem hiding this comment.
| {% trans y=duration[0], m=duration[1], d=duration[2], date=profile.last_subscription_date %}Not subscribed for {{ y }} year(s) {{ m }} month(s) {{ d }} day(s) ({{ date }}){% endtrans %} | |
| {% trans %}Not subscribed for : {% endtrans %} {{ profile.last_subscription|timesince }} |
Django traduit automatique ce qui est retourné par timesince
| kwargs["sum_accounts"] = [ | ||
| round(Customer.objects.aggregate(Sum("amount"))["amount__sum"], 2) | ||
| if Customer.objects.aggregate(Sum("amount"))["amount__sum"] is not None | ||
| else 0, | ||
| Customer.objects.count(), | ||
| ] |
There was a problem hiding this comment.
Il n'y a pas d'intérêt à rassembler le résultat des deux requêtes dans une liste et ça rend le code moins compréhensible, non seulement dans la vue mais aussi et aussi dans le template.
L'objet retourné par la requête est un Decimal avec une précision de deux chiffres après la virgule. Il ne devrait pas y avoir besoin d'arrondir.
La duplication de ce queryset va entraîner une double requête à la db.
| inactive_accounts = User.objects.filter( | ||
| customer__amount__gt=0, | ||
| subscriptions__subscription_end__lte=date.today() | ||
| - relativedelta(years=DELTA_DUMP["years"], months=DELTA_DUMP["months"]), | ||
| ).aggregate(Sum("customer__amount"), Count("customer")) |
There was a problem hiding this comment.
La requête ne sélectionne pas ceux qui ne sont plus cotisants depuis le temps requis, mais tous ceux qui ont eu une cotisation qui s'est terminée dans ledit intervalle de temps, même s'ils ont renouvelé leur cotisation depuis.
Pas sûr que les vieux qui continuent à cotiser quand ils viennent au FIMU soient très contents.
| kwargs["sum_inactive_accounts"] = [ | ||
| round(inactive_accounts["customer__amount__sum"], 2) | ||
| if inactive_accounts["customer__amount__sum"] is not None | ||
| else 0, | ||
| inactive_accounts["customer__count"], | ||
| ] |
There was a problem hiding this comment.
Mêmes remarques que plus haut, le type de retour pour l'argent ne devrait pas avoir besoin d'être arrondi et ça rend le code peu lisible de mettre ça dans une liste. Mieux vaut utiliser deux clefs différentes dans le dictionnaire.
No description provided.