Custom 404 for Page

club/views.py

@@ -228,7 +228,8 @@ class ClubPageEditView(ClubTabsMixin, PageEditViewBase):
 
     def get_object(self):
         self.page = self.club.page
-        return self._get_revision()
+        self.page.set_lock(self.request.user)
+        return self.page.revisions.last()
 
     def get_success_url(self, **kwargs):
         return reverse_lazy("club:club_view", kwargs={"club_id": self.club.id})

core/templates/core/page/base.jinja

@@ -1,27 +1,15 @@
 {% extends "core/base.jinja" %}
 
 {% block title %}
-  {% if page %}
-    {{ page.get_display_name() }}
-  {% elif page_list %}
-    {% trans %}Page list{% endtrans %}
-  {% elif new_page %}
-    {% trans %}Create page{% endtrans %}
-  {% else %}
-    {% trans %}Not found{% endtrans %}
-  {% endif %}
+  {{ page.get_display_name() }}
 {% endblock %}
 
 {% block metatags %}
-  {% if page %}
-    <meta property="og:url" content="{{ request.build_absolute_uri(page.get_absolute_url()) }}" />
-    <meta property="og:type" content="article" />
-    <meta property="article:section" content="{% trans %}Page{% endtrans %}" />
-    <meta property="og:title" content="{{ page.get_display_name() }}" />
-    <meta property="og:image" content="{{ request.build_absolute_uri(static("core/img/logo_no_text.png")) }}" />
-  {% else %}
-    {{ super() }}
-  {% endif %}
+  <meta property="og:url" content="{{ request.build_absolute_uri(page.get_absolute_url()) }}" />
+  <meta property="og:type" content="article" />
+  <meta property="article:section" content="{% trans %}Page{% endtrans %}" />
+  <meta property="og:title" content="{{ page.get_display_name() }}" />
+  <meta property="og:image" content="{{ request.build_absolute_uri(static("core/img/logo_no_text.png")) }}" />
 {% endblock %}
 
 {%- macro print_page_name(page) -%}
@@ -35,30 +23,22 @@
   {{ print_page_name(page) }}
   <div class="tool_bar">
     <div class="tools">
-      {% if page %}
-        {% if page.club %}
-          <a href="{{ url('club:club_view', club_id=page.club.id) }}">{% trans %}Return to club management{% endtrans %}</a>
-        {% else %}
-          <a href="{{ url('core:page', page.get_full_name()) }}">{% trans %}View{% endtrans %}</a>
-        {% endif %}
-        <a href="{{ url('core:page_hist', page_name=page.get_full_name()) }}">{% trans %}History{% endtrans %}</a>
-        {% if can_edit(page, user) %}
-          <a href="{{ url('core:page_edit', page_name=page.get_full_name()) }}">{% trans %}Edit{% endtrans %}</a>
-        {% endif %}
-        {% if can_edit_prop(page, user) and not page.is_club_page %}
-          <a href="{{ url('core:page_prop', page_name=page.get_full_name()) }}">{% trans %}Prop{% endtrans %}</a>
-        {% endif %}
+      {% if page.club %}
+        <a href="{{ url('club:club_view', club_id=page.club.id) }}">{% trans %}Return to club management{% endtrans %}</a>
+      {% else %}
+        <a href="{{ url('core:page', page.get_full_name()) }}">{% trans %}View{% endtrans %}</a>
+      {% endif %}
+      <a href="{{ url('core:page_hist', page_name=page.get_full_name()) }}">{% trans %}History{% endtrans %}</a>
+      {% if can_edit(page, user) %}
+        <a href="{{ url('core:page_edit', page_name=page.get_full_name()) }}">{% trans %}Edit{% endtrans %}</a>
+      {% endif %}
+      {% if can_edit_prop(page, user) and not page.is_club_page %}
+        <a href="{{ url('core:page_prop', page_name=page.get_full_name()) }}">{% trans %}Prop{% endtrans %}</a>
       {% endif %}
     </div>
   </div>
   <hr>
 
-  {% if page %}
-    {% block page %}
-    {% endblock %}
-  {% else %}
-    <h2>{% trans %}Page does not exist{% endtrans %}</h2>
-    <p><a href="{{ url('core:page_new') }}?page={{ request.resolver_match.kwargs['page_name'] }}">
-      {% trans %}Create it?{% endtrans %}</a></p>
-  {% endif %}
+  {% block page %}
+  {% endblock %}
 {% endblock %}

core/templates/core/page/detail.jinja

@@ -1,16 +1,16 @@
 {% extends "core/page/base.jinja" %}
 
 {% block page %}
-  {% if rev %}
-    <h4>{% trans rev_id=rev.revision %}This may not be the last update, you are seeing revision {{ rev_id }}!{% endtrans %}</h4>
-    <h3>{{ rev.title }}</h3>
-    <div class="page_content">{{ rev.content|markdown }}</div>
-  {% else %}
-    {% if page.revisions.last() %}
-      <h3>{{ page.revisions.last().title }}</h3>
-      <div class="page_content">{{ page.revisions.last().content|markdown }}</div>
-    {% endif %}
+  {% if revision and revision.id != last_revision.id %}
+    <h4>
+      {% trans trimmed rev_id=revision.revision %}
+        This may not be the last update, you are seeing revision {{ rev_id }}!
+      {% endtrans %}
+    </h4>
   {% endif %}
+  {% set current_revision = revision or last_revision %}
+  <h3>{{ current_revision.title }}</h3>
+  <div class="page_content">{{ current_revision.content|markdown }}</div>
 {% endblock %}
 
 

core/templates/core/page/not_found.jinja

@@ -0,0 +1,12 @@
+{% extends "core/base.jinja" %}
+
+{% block content %}
+  <h2>{% trans %}Page does not exist{% endtrans %}</h2>
+  <p>
+    {# This template is rendered when a PageNotFound error is raised,
+       so the `exception` context variable should always have a page_name attribute #}
+    <a href="{{ url('core:page_new') }}?page={{ exception.page_name }}">
+      {% trans %}Create it?{% endtrans %}
+    </a>
+  </p>
+{% endblock %}

core/templates/core/page/prop.jinja

@@ -1,18 +1,13 @@
 {% extends "core/page/base.jinja" %}
 
-{% block content %}
-  {% if page %}
-    {{ super() }}
-  {% endif %}
+{% block page %}
   <h2>{% trans %}Page properties{% endtrans %}</h2>
   <form action="" method="post">
     {% csrf_token %}
     {{ form.as_p() }}
     <p><input type="submit" value="{% trans %}Save{% endtrans %}" /></p>
   </form>
-  {% if page %}
-    <a href="{{ url('core:page_delete', page_id=page.id)}}">{% trans %}Delete{% endtrans %}</a>
-  {% endif %}
+  <a href="{{ url('core:page_delete', page_id=page.id)}}">{% trans %}Delete{% endtrans %}</a>
 {% endblock %}
 
 

core/tests/test_core.py

@@ -319,9 +319,8 @@ class TestPageHandling(TestCase):
     def test_access_page_not_found(self):
         """Should not display a page correctly."""
         response = self.client.get(reverse("core:page", kwargs={"page_name": "swagg"}))
-        assert response.status_code == 200
-        html = response.text
-        self.assertIn('<a href="/page/create/?page=swagg">', html)
+        assert response.status_code == 404
+        assert '<a href="/page/create/?page=swagg">' in response.text
 
     def test_create_page_markdown_safe(self):
         """Should format the markdown and escape html correctly."""

core/tests/test_page.py

@@ -4,11 +4,11 @@ from django.contrib.auth.models import Permission
 from django.test import Client
 from django.urls import reverse
 from model_bakery import baker
-from pytest_django.asserts import assertRedirects
+from pytest_django.asserts import assertHTMLEqual, assertRedirects
 
 from core.baker_recipes import board_user, subscriber_user
-from core.models import AnonymousUser, Page, User
-from sith.settings import SITH_GROUP_OLD_SUBSCRIBERS_ID, SITH_GROUP_SUBSCRIBERS_ID
+from core.markdown import markdown
+from core.models import AnonymousUser, Page, PageRev, User
 
 
 @pytest.mark.django_db
@@ -29,15 +29,63 @@ def test_edit_page(client: Client):
     assert revision.content == "Hello World"
 
 
+
+@pytest.mark.django_db
+def test_page_revision(client: Client):
+    page = baker.prepare(Page)
+    page.save(force_lock=True)
+    page.view_groups.add(settings.SITH_GROUP_SUBSCRIBERS_ID)
+    revisions = baker.make(
+        PageRev, page=page, _quantity=3, content=iter(["foo", "bar", "baz"])
+    )
+    client.force_login(subscriber_user.make())
+    url = reverse(
+        "core:page_rev",
+        kwargs={"page_name": page._full_name, "rev": revisions[1].id},
+    )
+    res = client.get(url)
+    assert res.status_code == 200
+    soup = BeautifulSoup(res.text, "lxml")
+    detail_html = soup.find(class_="markdown")
+    assertHTMLEqual(detail_html.decode_contents(), markdown(revisions[1].content))
+
+
+@pytest.mark.django_db
+def test_page_club_redirection(client: Client):
+    club = baker.make(Club)
+    url = reverse("core:page", kwargs={"page_name": club.page._full_name})
+    res = client.get(url)
+    redirection_url = reverse("club:club_view", kwargs={"club_id": club.id})
+    assertRedirects(res, redirection_url)
+
+
+@pytest.mark.django_db
+def test_page_revision_club_redirection(client: Client):
+    client.force_login(subscriber_user.make())
+    club = baker.make(Club)
+    revisions = baker.make(
+        PageRev, page=club.page, _quantity=3, content=iter(["foo", "bar", "baz"])
+    )
+    url = reverse(
+        "core:page_rev",
+        kwargs={"page_name": club.page._full_name, "rev": revisions[1].id},
+    )
+    res = client.get(url)
+    redirection_url = reverse(
+        "club:club_view_rev", kwargs={"club_id": club.id, "rev_id": revisions[1].id}
+    )
+    assertRedirects(res, redirection_url)
+
+
 @pytest.mark.django_db
 def test_viewable_by():
     # remove existing pages to prevent side effect
     Page.objects.all().delete()
     view_groups = [
         [settings.SITH_GROUP_PUBLIC_ID],
-        [settings.SITH_GROUP_PUBLIC_ID, SITH_GROUP_SUBSCRIBERS_ID],
-        [SITH_GROUP_SUBSCRIBERS_ID],
-        [SITH_GROUP_SUBSCRIBERS_ID, SITH_GROUP_OLD_SUBSCRIBERS_ID],
+        [settings.SITH_GROUP_PUBLIC_ID, settings.SITH_GROUP_SUBSCRIBERS_ID],
+        [settings.SITH_GROUP_SUBSCRIBERS_ID],
+        [settings.SITH_GROUP_SUBSCRIBERS_ID, settings.SITH_GROUP_OLD_SUBSCRIBERS_ID],
         [],
     ]
     pages = baker.make(Page, _quantity=len(view_groups), _bulk_create=True)

core/views/__init__.py

@@ -21,10 +21,10 @@
 # Place - Suite 330, Boston, MA 02111-1307, USA.
 #
 #
-
 from django.http import (
+    Http404,
+    HttpRequest,
     HttpResponseForbidden,
-    HttpResponseNotFound,
     HttpResponseServerError,
 )
 from django.shortcuts import render
@@ -33,17 +33,20 @@ from django.views.generic.edit import FormView
 from sentry_sdk import last_event_id
 
 from core.views.forms import LoginForm
+from core.views.page import PageNotFound
 
 
-def forbidden(request, exception):
+def forbidden(request: HttpRequest, exception):
     context = {"next": request.path, "form": LoginForm()}
     return HttpResponseForbidden(render(request, "core/403.jinja", context=context))
 
 
-def not_found(request, exception):
-    return HttpResponseNotFound(
-        render(request, "core/404.jinja", context={"exception": exception})
-    )
+def not_found(request: HttpRequest, exception: Http404):
+    if isinstance(exception, PageNotFound):
+        template_name = "core/page/not_found.jinja"
+    else:
+        template_name = "core/404.jinja"
+    return render(request, template_name, context={"exception": exception}, status=404)
 
 
 def internal_servor_error(request):

core/views/page.py

@@ -30,17 +30,24 @@ from core.auth.mixins import (
     CanEditPropMixin,
     CanViewMixin,
 )
-from core.models import LockError, Page, PageRev
+from core.models import Page, PageRev
 from core.views.forms import PageForm, PagePropForm
 from core.views.widgets.markdown import MarkdownInput
 
 
-class CanEditPagePropMixin(CanEditPropMixin):
-    def dispatch(self, request, *args, **kwargs):
-        res = super().dispatch(request, *args, **kwargs)
-        if self.object.is_club_page:
-            raise Http404
-        return res
+class PageNotFound(Http404):
+    """Http404 Exception, but specifically for when the not found object is a Page."""
+
+    def __init__(self, page_name: str):
+        self.page_name = page_name
+
+
+def get_page_or_404(full_name: str) -> Page:
+    """Like Django's get_object_or_404, but for Page, and with a custom 404 exception."""
+    page = Page.objects.filter(_full_name=full_name).first()
+    if not page:
+        raise PageNotFound(full_name)
+    return page
 
 
 class PageListView(ListView):
@@ -64,80 +71,57 @@ class PageListView(ListView):
         )
 
 
-class PageView(CanViewMixin, DetailView):
+class BasePageDetailView(CanViewMixin, DetailView):
     model = Page
-    template_name = "core/page/detail.jinja"
-
-    def dispatch(self, request, *args, **kwargs):
-        res = super().dispatch(request, *args, **kwargs)
-        if self.object and self.object.need_club_redirection:
-            return redirect("club:club_view", club_id=self.object.club.id)
-        return res
-
-    def get_object(self):
-        self.page = Page.get_page_by_full_name(self.kwargs["page_name"])
-        return self.page
-
-    def get_context_data(self, **kwargs):
-        context = super().get_context_data(**kwargs)
-        if "page" not in context:
-            context["new_page"] = self.kwargs["page_name"]
-        return context
-
-
-class PageHistView(CanViewMixin, DetailView):
-    model = Page
-    template_name = "core/page/history.jinja"
-    slug_field = "_full_name"
     slug_url_kwarg = "page_name"
     _cached_object: Page | None = None
 
     def dispatch(self, request, *args, **kwargs):
         page = self.get_object()
         if page.need_club_redirection:
-            return redirect("club:club_hist", club_id=page.club.id)
+            return redirect("club:club_view", club_id=page.club.id)
         return super().dispatch(request, *args, **kwargs)
 
     def get_object(self, *args, **kwargs):
         if not self._cached_object:
-            self._cached_object = super().get_object()
+            full_name = self.kwargs.get(self.slug_url_kwarg)
+            self._cached_object = get_page_or_404(full_name)
         return self._cached_object
 
+    def get_context_data(self, **kwargs):
+        return super().get_context_data(**kwargs) | {
+            "last_revision": self.object.revisions.last()
+        }
 
-class PageRevView(CanViewMixin, DetailView):
-    model = Page
+
+class PageView(BasePageDetailView):
+    template_name = "core/page/detail.jinja"
+
+
+class PageHistView(BasePageDetailView):
+    template_name = "core/page/history.jinja"
+
+
+class PageRevView(BasePageDetailView):
     template_name = "core/page/detail.jinja"
 
     def dispatch(self, request, *args, **kwargs):
-        res = super().dispatch(request, *args, **kwargs)
-        self.object = self.get_object()
-
-        if self.object is None:
-            return redirect("core:page_create", page_name=self.kwargs["page_name"])
-
-        if self.object.need_club_redirection:
+        page = self.get_object()
+        if page.need_club_redirection:
             return redirect(
-                "club:club_view_rev", club_id=self.object.club.id, rev_id=kwargs["rev"]
+                "club:club_view_rev", club_id=page.club.id, rev_id=kwargs["rev"]
             )
-        return res
-
-    def get_object(self, *args, **kwargs):
-        self.page = Page.get_page_by_full_name(self.kwargs["page_name"])
-        return self.page
+        self.revision = get_object_or_404(page.revisions, id=self.kwargs["rev"])
+        return super().dispatch(request, *args, **kwargs)
 
     def get_context_data(self, **kwargs):
-        context = super().get_context_data(**kwargs)
-        if not self.page:
-            return context | {"new_page": self.kwargs["page_name"]}
-        context["page"] = self.page
-        context["rev"] = self.page.revisions.filter(id=self.kwargs["rev"]).first()
-        return context
+        return super().get_context_data(**kwargs) | {"revision": self.revision}
 
 
 class PageCreateView(PermissionRequiredMixin, CreateView):
     model = Page
     form_class = PageForm
-    template_name = "core/page/prop.jinja"
+    template_name = "core/create.jinja"
     permission_required = "core.add_page"
 
     def get_initial(self):
@@ -152,30 +136,28 @@ class PageCreateView(PermissionRequiredMixin, CreateView):
         init["name"] = page_name[-1]
         return init
 
-    def get_context_data(self, **kwargs):
-        context = super().get_context_data(**kwargs)
-        context["new_page"] = True
-        return context
-
     def form_valid(self, form):
         form.instance.set_lock(self.request.user)
         ret = super().form_valid(form)
         return ret
 
 
+class CanEditPagePropMixin(CanEditPropMixin):
+    def dispatch(self, request, *args, **kwargs):
+        res = super().dispatch(request, *args, **kwargs)
+        if self.object.is_club_page:
+            raise Http404
+        return res
+
+
 class PagePropView(CanEditPagePropMixin, UpdateView):
     model = Page
     form_class = PagePropForm
     template_name = "core/page/prop.jinja"
-    slug_field = "_full_name"
-    slug_url_kwarg = "page_name"
 
     def get_object(self, queryset=None):
-        self.page = super().get_object()
-        try:
-            self.page.set_lock_recursive(self.request.user)
-        except LockError as e:
-            raise e
+        self.page = get_page_or_404(full_name=self.kwargs["page_name"])
+        self.page.set_lock_recursive(self.request.user)
         return self.page
 
 
@@ -187,22 +169,12 @@ class PageEditViewBase(CanEditMixin, UpdateView):
     template_name = "core/page/edit.jinja"
 
     def get_object(self, *args, **kwargs):
-        self.page = Page.get_page_by_full_name(self.kwargs["page_name"])
-        return self._get_revision()
-
-    def _get_revision(self):
-        if self.page is None:
-            return None
+        self.page = get_page_or_404(full_name=self.kwargs["page_name"])
         self.page.set_lock(self.request.user)
         return self.page.revisions.last()
 
     def get_context_data(self, **kwargs):
-        context = super().get_context_data(**kwargs)
-        if self.page is not None:
-            context["page"] = self.page
-        else:
-            context["new_page"] = self.kwargs["page_name"]
-        return context
+        return super().get_context_data(**kwargs) | {"page": self.page}
 
     def form_valid(self, form):
         # TODO : factor that, but first make some tests
@@ -216,16 +188,14 @@ class PageEditViewBase(CanEditMixin, UpdateView):
 
 class PageEditView(PageEditViewBase):
     def dispatch(self, request, *args, **kwargs):
-        res = super().dispatch(request, *args, **kwargs)
+        self.object = self.get_object()
         if self.object and self.object.page.need_club_redirection:
             return redirect("club:club_edit_page", club_id=self.object.page.club.id)
-        return res
+        return super().dispatch(request, *args, **kwargs)
 
 
 class PageDeleteView(CanEditPagePropMixin, DeleteView):
     model = Page
     template_name = "core/delete_confirm.jinja"
     pk_url_kwarg = "page_id"
-
-    def get_success_url(self, **kwargs):
-        return reverse_lazy("core:page_list")
+    success_url = reverse_lazy("core:page_list")