From 5381aba9224f98016a45276e2a18490d7dd5cf01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20P=C3=A9pin?= Date: Sun, 5 Jan 2020 16:34:10 +0100 Subject: [PATCH] Remove the nb_{yes,no} fields These fields duplicate information that is already present in the Answer table. I guess they exist(ed) as an optimisation to avoid recomputing the number of positive and negative answers per proposition each time the list page is loaded. Given the small number of propositions we have in practice and the extra housekeeping required to keep these fields up to date, I consider simplicity matters more here. --- .../0005_remove_nb_yes_no_fields.py | 24 ++++++++++++++ propositions/models.py | 7 +---- propositions/tests.py | 12 +++---- propositions/urls.py | 4 +-- propositions/views.py | 31 +++++++------------ 5 files changed, 45 insertions(+), 33 deletions(-) create mode 100644 propositions/migrations/0005_remove_nb_yes_no_fields.py diff --git a/propositions/migrations/0005_remove_nb_yes_no_fields.py b/propositions/migrations/0005_remove_nb_yes_no_fields.py new file mode 100644 index 0000000..4f09c1e --- /dev/null +++ b/propositions/migrations/0005_remove_nb_yes_no_fields.py @@ -0,0 +1,24 @@ +# Generated by Django 2.2.9 on 2020-01-05 15:26 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ("propositions", "0004_prop_renaming_and_cleaning"), + ] + + operations = [ + migrations.RemoveField(model_name="proposition", name="nb_no",), + migrations.RemoveField(model_name="proposition", name="nb_yes",), + migrations.AlterField( + model_name="answer", + name="proposition", + field=models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="propositions.Proposition", + ), + ), + ] diff --git a/propositions/models.py b/propositions/models.py index 7f4eda0..0ff1f74 100644 --- a/propositions/models.py +++ b/propositions/models.py @@ -12,8 +12,6 @@ class Proposition(models.Model): ErnestoUser, on_delete=models.CASCADE, verbose_name="Proposé par" ) link = models.URLField(blank=True) - nb_yes = models.IntegerField(default=0, verbose_name="nombre de réponses oui") - nb_no = models.IntegerField(default=0, verbose_name="nombre de réponses non") def __str__(self): return self.name @@ -27,10 +25,7 @@ class Answer(models.Model): YES = "oui" NO = "non" - REP_CHOICES = [ - (YES, "Oui"), - (NO, "Non") - ] + REP_CHOICES = [(YES, "Oui"), (NO, "Non")] proposition = models.ForeignKey(Proposition, on_delete=models.CASCADE) user = models.ForeignKey(User, on_delete=models.CASCADE) diff --git a/propositions/tests.py b/propositions/tests.py index e78a053..1d53b69 100644 --- a/propositions/tests.py +++ b/propositions/tests.py @@ -44,8 +44,6 @@ class PropositionCreateTest(TestCase): self.assertEqual(proposition.artist, "bar") self.assertEqual(proposition.link, "http://example.com") self.assertEqual(proposition.user, user.profile) - self.assertEqual(proposition.nb_yes, 0) - self.assertEqual(proposition.nb_no, 0) class PropositionListTest(TestCase): @@ -99,11 +97,13 @@ class ReponseTest(TestCase): # Oui client.get(reverse("propositions:oui", args=(self.prop.id,))) self.prop.refresh_from_db() - self.assertEqual(self.prop.nb_yes, 1) - self.assertEqual(self.prop.nb_no, 0) + self.assertEqual( + list(self.prop.answer_set.values_list("answer", flat=True)), [Answer.YES], + ) # Oui client.get(reverse("propositions:non", args=(self.prop.id,))) self.prop.refresh_from_db() - self.assertEqual(self.prop.nb_yes, 0) - self.assertEqual(self.prop.nb_no, 1) + self.assertEqual( + list(self.prop.answer_set.values_list("answer", flat=True)), [Answer.NO] + ) diff --git a/propositions/urls.py b/propositions/urls.py index f08c0c3..efa172e 100644 --- a/propositions/urls.py +++ b/propositions/urls.py @@ -8,7 +8,7 @@ app_name = "propositions" urlpatterns = [ path("", views.PropositionList.as_view(), name="list"), path("new", views.PropositionCreate.as_view(), name="create"), - path("/oui", views.answer, {"rep": "oui"}, name=Answer.YES), - path("/non", views.answer, {"rep": "non"}, name=Answer.NO), + path("/oui", views.answer, {"ans": "oui"}, name=Answer.YES), + path("/non", views.answer, {"ans": "non"}, name=Answer.NO), path("/supprimer", PropDelete.as_view(), name="delete"), ] diff --git a/propositions/views.py b/propositions/views.py index b7e8506..fc60316 100644 --- a/propositions/views.py +++ b/propositions/views.py @@ -2,6 +2,7 @@ from django.shortcuts import redirect, get_object_or_404 from django.urls import reverse_lazy from django.contrib.auth.decorators import login_required from django.contrib.auth.mixins import LoginRequiredMixin +from django.db.models import Count, Q from django.views.generic import CreateView, DeleteView, ListView from django.utils.decorators import method_decorator from django.http import HttpResponseRedirect @@ -26,7 +27,14 @@ class PropositionList(LoginRequiredMixin, ListView): template_name = "propositions/liste.html" context_object_name = "propositions" model = Proposition - ordering = ["-nb_yes", "nb_no", "name"] + + def get_queryset(self): + return ( + Proposition.objects + .annotate(nb_yes=Count("answer", filter=Q(answer__answer=Answer.YES))) + .annotate(nb_no=Count("answer", filter=Q(answer__answer=Answer.NO))) + .order_by("-nb_yes", "nb_no", "name") + ) class PropDelete(DeleteView): @@ -50,24 +58,9 @@ class PropDelete(DeleteView): @login_required -def answer(request, id, rep): +def answer(request, id, ans): proposition = get_object_or_404(Proposition, id=id) user = request.user - - old_answer = Answer.objects.filter(proposition=proposition, user=user) - if old_answer.exists(): - old_answer = old_answer.get() - if old_answer.answer == Answer.YES: - proposition.nb_yes -= 1 - else: - proposition.nb_no -= 1 - old_answer.delete() - - Answer.objects.create(proposition=proposition, user=user, answer=rep) - if rep == Answer.YES: - proposition.nb_yes += 1 - else: - proposition.nb_no += 1 - proposition.save() - + Answer.objects.filter(proposition=proposition, user=user).delete() + Answer.objects.create(proposition=proposition, user=user, answer=ans) return redirect("propositions:list")