Merge pull request #1264 from ae-utbm/refactor/user

Refactor some user views

com/ics_calendar.py

@@ -4,16 +4,15 @@ from dateutil.relativedelta import relativedelta
 from django.conf import settings
 from django.contrib.sites.models import Site
 from django.contrib.syndication.views import add_domain
-from django.db.models import Count, OuterRef, QuerySet, Subquery
+from django.db.models import F, QuerySet
 from django.http import HttpRequest
 from django.urls import reverse
 from django.utils import timezone
 from ical.calendar import Calendar
 from ical.calendar_stream import IcsCalendarStream
 from ical.event import Event
-from ical.types import Frequency, Recur
 
-from com.models import News, NewsDate
+from com.models import NewsDate
 from core.models import User
 
 
@@ -43,9 +42,9 @@ class IcsCalendar:
         with open(cls._INTERNAL_CALENDAR, "wb") as f:
             _ = f.write(
                 cls.ics_from_queryset(
-                    News.objects.filter(
-                        is_published=True,
-                        dates__end_date__gte=timezone.now() - relativedelta(months=6),
+                    NewsDate.objects.filter(
+                        news__is_published=True,
+                        end_date__gte=timezone.now() - (relativedelta(months=6)),
                     )
                 )
             )
@@ -54,35 +53,24 @@ class IcsCalendar:
     @classmethod
     def get_unpublished(cls, user: User) -> bytes:
         return cls.ics_from_queryset(
-            News.objects.viewable_by(user).filter(
-                is_published=False,
-                dates__end_date__gte=timezone.now() - relativedelta(months=6),
-            )
+            NewsDate.objects.viewable_by(user).filter(
+                news__is_published=False,
+                end_date__gte=timezone.now() - (relativedelta(months=6)),
+            ),
         )
 
     @classmethod
-    def ics_from_queryset(cls, queryset: QuerySet[News]) -> bytes:
+    def ics_from_queryset(cls, queryset: QuerySet[NewsDate]) -> bytes:
         calendar = Calendar()
-        date_subquery = NewsDate.objects.filter(news=OuterRef("pk")).order_by(
-            "start_date"
-        )
-        queryset = queryset.annotate(
-            start=Subquery(date_subquery.values("start_date")[:1]),
-            end=Subquery(date_subquery.values("end_date")[:1]),
-            nb_dates=Count("dates"),
-        )
-        for news in queryset:
+        for news_date in queryset.annotate(news_title=F("news__title")):
             event = Event(
-                summary=news.title,
-                description=news.summary,
-                dtstart=news.start,
-                dtend=news.end,
+                summary=news_date.news_title,
+                start=news_date.start_date,
+                end=news_date.end_date,
                 url=as_absolute_url(
-                    reverse("com:news_detail", kwargs={"news_id": news.id})
+                    reverse("com:news_detail", kwargs={"news_id": news_date.news_id})
                 ),
             )
-            if news.nb_dates > 1:
-                event.rrule = Recur(freq=Frequency.WEEKLY, count=news.nb_dates)
             calendar.events.append(event)
 
         return IcsCalendarStream.calendar_to_ics(calendar).encode("utf-8")

core/static/user/user_godfathers.scss

@@ -195,18 +195,18 @@
           }
         }
       }
+    }
 
-      &.delete {
-        margin-top: 10px;
-        display: block;
-        text-align: center;
-        color: orangered;
+    form .link-like {
+      margin-top: 10px;
+      display: block;
+      text-align: center;
+      color: orangered;
 
-        @media (max-width: 375px) {
-          position: absolute;
-          bottom: 0;
-          right: 0;
-        }
+      @media (max-width: 375px) {
+        position: absolute;
+        bottom: 0;
+        right: 0;
       }
     }
   }

core/templates/core/macros.jinja

@@ -78,12 +78,6 @@
   {% endif %}
 {% endmacro %}
 
-{% macro delete_godfather(user, profile, godfather, is_father) %}
-  {% if user == profile or user.is_root or user.is_board_member %}
-    <a class="delete" href="{{ url("core:user_godfathers_delete", user_id=profile.id, godfather_id=godfather.id, is_father=is_father) }}">{% trans %}Delete{% endtrans %}</a>
-  {% endif %}
-{% endmacro %}
-
 {% macro paginate_alpine(page, nb_pages) %}
     {# Add pagination buttons for ajax based content with alpine
 

core/templates/core/password_change.jinja

@@ -3,7 +3,7 @@
 {% block content %}
 
   {% if target %}
-    <p>{% trans user=target.get_display_name() %}Change password for {{ user }}{% endtrans %}</p>
+    <p>{% trans user=form.user.get_display_name() %}Change password for {{ user }}{% endtrans %}</p>
   {% endif %}
   <form method="post" action="">
     {% csrf_token %}

core/templates/core/user_godfathers.jinja

@@ -29,7 +29,16 @@
             <a href="{{ url('core:user_godfathers', user_id=u.id) }}" class="mini_profile_link">
               {{ u.get_mini_item() | safe }}
             </a>
-            {{ delete_godfather(user, profile, u, True) }}
+            {% if user == profile or user.is_root or user.is_board_member %}
+              <form
+                method="post"
+                class="no-margin"
+                action="{{ url("core:user_godfathers_delete", user_id=profile.id, godfather_id=u.id, is_father=True) }}"
+              >
+                {% csrf_token %}
+                <input type="submit" class="link-like" value="{% trans %}Delete{% endtrans %}">
+              </form>
+            {% endif %}
           </li>
         {% endfor %}
       </ul>
@@ -46,7 +55,16 @@
             <a href="{{ url('core:user_godfathers', user_id=u.id) }}" class="mini_profile_link">
               {{ u.get_mini_item()|safe }}
             </a>
-            {{ delete_godfather(user, profile, u, False) }}
+            {% if user == profile or user.is_root or user.is_board_member %}
+              <form
+                method="post"
+                class="no-margin"
+                action="{{ url("core:user_godfathers_delete", user_id=profile.id, godfather_id=u.id, is_father=False) }}"
+              >
+                {% csrf_token %}
+                <input type="submit" class="link-like link-red" value="{% trans %}Delete{% endtrans %}">
+              </form>
+            {% endif %}
           </li>
         {% endfor %}
       </ul>

core/tests/test_user.py

@@ -427,6 +427,19 @@ class TestUserQuerySetViewableBy:
         assert not viewable.exists()
 
 
+@pytest.mark.django_db
+def test_user_preferences(client: Client):
+    user = subscriber_user.make()
+    client.force_login(user)
+    url = reverse("core:user_prefs", kwargs={"user_id": user.id})
+    response = client.get(url)
+    assert response.status_code == 200
+    response = client.post(url, {"notify_on_click": "true"})
+    assertRedirects(response, url)
+    user.preferences.refresh_from_db()
+    assert user.preferences.notify_on_click is True
+
+
 @pytest.mark.django_db
 def test_user_stats(client: Client):
     user = subscriber_user.make()
@@ -450,3 +463,68 @@ def test_user_stats(client: Client):
     client.force_login(user)
     response = client.get(reverse("core:user_stats", kwargs={"user_id": user.id}))
     assert response.status_code == 200
+
+
+@pytest.mark.django_db
+class TestChangeUserPassword:
+    def test_as_root(self, client: Client, admin_user: User):
+        client.force_login(admin_user)
+        user = subscriber_user.make()
+        url = reverse("core:password_root_change", kwargs={"user_id": user.id})
+        response = client.get(url)
+        assert response.status_code == 200
+        response = client.post(
+            url, {"new_password1": "poutou", "new_password2": "poutou"}
+        )
+        assertRedirects(response, reverse("core:password_change_done"))
+        user.refresh_from_db()
+        assert user.check_password("poutou") is True
+
+
+@pytest.mark.django_db
+class TestUserGodfather:
+    @pytest.mark.parametrize("godfather", [True, False])
+    def test_add_family(self, client: Client, godfather):
+        user = subscriber_user.make()
+        other_user = subscriber_user.make()
+        client.force_login(user)
+        url = reverse("core:user_godfathers", kwargs={"user_id": user.id})
+        response = client.get(url)
+        assert response.status_code == 200
+        response = client.post(
+            url,
+            {"type": "godfather" if godfather else "godchild", "user": other_user.id},
+        )
+        assertRedirects(response, url)
+        if godfather:
+            assert user.godfathers.contains(other_user)
+        else:
+            assert user.godchildren.contains(other_user)
+
+    def test_tree(self, client: Client):
+        user = subscriber_user.make()
+        client.force_login(user)
+        response = client.get(
+            reverse("core:user_godfathers_tree", kwargs={"user_id": user.id})
+        )
+        assert response.status_code == 200
+
+    def test_remove_family(self, client: Client):
+        user = subscriber_user.make()
+        other_user = subscriber_user.make()
+        user.godfathers.add(other_user)
+        client.force_login(user)
+        response = client.post(
+            reverse(
+                "core:user_godfathers_delete",
+                kwargs={
+                    "user_id": user.id,
+                    "godfather_id": other_user.id,
+                    "is_father": True,
+                },
+            )
+        )
+        assertRedirects(
+            response, reverse("core:user_godfathers", kwargs={"user_id": user.id})
+        )
+        assert not user.godfathers.contains(other_user)

core/urls.py

@@ -54,6 +54,7 @@ from core.views import (
     PagePropView,
     PageRevView,
     PageView,
+    PasswordRootChangeView,
     SearchView,
     SithLoginView,
     SithPasswordChangeDoneView,
@@ -80,7 +81,6 @@ from core.views import (
     delete_user_godfather,
     logout,
     notification,
-    password_root_change,
     send_file,
 )
 
@@ -100,7 +100,7 @@ urlpatterns = [
     path("password_change/", SithPasswordChangeView.as_view(), name="password_change"),
     path(
         "password_change/<int:user_id>/",
-        password_root_change,
+        PasswordRootChangeView.as_view(),
         name="password_root_change",
     ),
     path(

core/views/forms.py

@@ -303,7 +303,6 @@ class UserGodfathersForm(forms.Form):
     )
     user = forms.ModelChoiceField(
         label=_("Select user"),
-        help_text=None,
         required=True,
         widget=AutoCompleteSelectUser,
         queryset=User.objects.all(),
@@ -315,8 +314,6 @@ class UserGodfathersForm(forms.Form):
 
     def clean_user(self):
         other_user = self.cleaned_data.get("user")
-        if not other_user:
-            raise ValidationError(_("This user does not exist"))
         if other_user == self.target_user:
             raise ValidationError(_("You cannot be related to yourself"))
         return other_user

core/views/user.py

@@ -29,8 +29,9 @@ from operator import itemgetter
 from smtplib import SMTPException
 
 from django.contrib.auth import login, views
-from django.contrib.auth.forms import PasswordChangeForm
-from django.contrib.auth.mixins import LoginRequiredMixin
+from django.contrib.auth.decorators import login_required
+from django.contrib.auth.forms import PasswordChangeForm, SetPasswordForm
+from django.contrib.auth.mixins import LoginRequiredMixin, UserPassesTestMixin
 from django.core.exceptions import PermissionDenied
 from django.db.models import DateField, F, QuerySet, Sum
 from django.db.models.functions import Trunc
@@ -38,11 +39,11 @@ from django.forms.models import modelform_factory
 from django.http import Http404
 from django.shortcuts import get_object_or_404, redirect
 from django.template.loader import render_to_string
-from django.template.response import TemplateResponse
 from django.urls import reverse, reverse_lazy
 from django.utils.decorators import method_decorator
 from django.utils.safestring import SafeString
 from django.utils.translation import gettext as _
+from django.views.decorators.http import require_POST
 from django.views.generic import (
     CreateView,
     DeleteView,
@@ -98,21 +99,23 @@ def logout(request):
     return views.logout_then_login(request)
 
 
-def password_root_change(request, user_id):
+class PasswordRootChangeView(UserPassesTestMixin, FormView):
     """Allows a root user to change someone's password."""
-    if not request.user.is_root:
-        raise PermissionDenied
-    user = get_object_or_404(User, id=user_id)
-    if request.method == "POST":
-        form = views.SetPasswordForm(user=user, data=request.POST)
-        if form.is_valid():
-            form.save()
-            return redirect("core:password_change_done")
-    else:
-        form = views.SetPasswordForm(user=user)
-    return TemplateResponse(
-        request, "core/password_change.jinja", {"form": form, "target": user}
-    )
+
+    template_name = "core/password_change.jinja"
+    form_class = SetPasswordForm
+    success_url = reverse_lazy("core:password_change_done")
+
+    def test_func(self):
+        return self.request.user.is_root
+
+    def get_form_kwargs(self):
+        user = get_object_or_404(User, id=self.kwargs["user_id"])
+        return super().get_form_kwargs() | {"user": user}
+
+    def form_valid(self, form: SetPasswordForm):
+        form.save()
+        return super().form_valid(form)
 
 
 @method_decorator(check_honeypot, name="post")
@@ -287,10 +290,12 @@ class UserView(UserTabsMixin, CanViewMixin, DetailView):
         return kwargs
 
 
+@require_POST
+@login_required
 def delete_user_godfather(request, user_id, godfather_id, is_father):
     user_is_admin = request.user.is_root or request.user.is_board_member
     if user_id != request.user.id and not user_is_admin:
-        raise PermissionDenied()
+        raise PermissionDenied
     user = get_object_or_404(User, id=user_id)
     to_remove = get_object_or_404(User, id=godfather_id)
     if is_father:
@@ -417,7 +422,6 @@ class UserUpdateProfileView(UserTabsMixin, CanEditMixin, UpdateView):
     form_class = UserProfileForm
     current_tab = "edit"
     edit_once = ["profile_pict", "date_of_birth", "first_name", "last_name"]
-    board_only = []
 
     def remove_restricted_fields(self, request):
         """Removes edit_once and board_only fields."""
@@ -426,9 +430,6 @@ class UserUpdateProfileView(UserTabsMixin, CanEditMixin, UpdateView):
                 request.user.is_board_member or request.user.is_root
             ):
                 self.form.fields.pop(i, None)
-        for i in self.board_only:
-            if not (request.user.is_board_member or request.user.is_root):
-                self.form.fields.pop(i, None)
 
     def get(self, request, *args, **kwargs):
         self.object = self.get_object()
@@ -480,10 +481,10 @@ class UserPreferencesView(UserTabsMixin, UseFragmentsMixin, CanEditMixin, Update
     current_tab = "prefs"
 
     def get_form_kwargs(self):
-        kwargs = super().get_form_kwargs()
-        pref = self.object.preferences
-        kwargs.update({"instance": pref})
-        return kwargs
+        return super().get_form_kwargs() | {"instance": self.object.preferences}
+
+    def get_success_url(self):
+        return self.request.path
 
     def get_fragment_context_data(self) -> dict[str, SafeString]:
         # Avoid cyclic import error