Differential D8325 Diff 27011 films/views/flatpages.py

Changeset View

Standalone View

films/views/flatpages.py

This file was added.

				from django.conf import settings
				from django.contrib.flatpages.views import render_flatpage
				from django.contrib.sites.models import Site
				from django.contrib.sites.shortcuts import get_current_site
				from django.core.handlers.wsgi import WSGIRequest
				from django.http.request import HttpRequest
				from django.http.response import HttpResponse, Http404, HttpResponsePermanentRedirect
				from django.shortcuts import get_object_or_404
				from django.views.decorators.http import require_safe

				from common.types import assert_cast
				from films.models import ExtendedFlatPage

				DEFAULT_TEMPLATE = 'flatpages/default.html'


				@require_safe
				def flatpage(request: HttpRequest, film_slug: str, url: str) -> HttpResponse:
				"""
				An overwritten version of the flat page view.
				sybrenUnsubmitted Not Done Inline Actions "overwritten" → "overridden" sybren: "overwritten" → "overridden"

				Wraps the default flatpage view to simplify url handling in project templates,
				and uses the :model:`films.ExtendedFlatPage` model.
				sybrenUnsubmitted Not Done Inline Actions This isn't true. It doesn't wrap the default flatpage view, it re-implements it. sybren: This isn't true. It doesn't wrap the default flatpage view, it re-implements it.

				Args
				``request``
				An HTTP request.
				``film_slug``
				A title slug, identifies the film that the flatpage belongs to.
				``url``
				A string - specifies which film page the flatpage represents (e.g. 'about/').

				Templates
				Uses the template defined by the ``template_name`` field,
				or :template:`flatpages/default.html` if ``template_name`` is not defined.

				Context
				``flatpage``
				:model:`films.ExtendedFlatPage` object
				"""
				url = f'/{film_slug}/{url}'
				sybrenUnsubmitted Not Done Inline Actions Since this is not the URL that's being requested right now, but is an encoding of the flatpage key, I think either a name change or a comment would be in order. As the code is now, the name `url` has different meanings in different parts. In the parameter, it means "url of the page within the context of the given film". After line 41 it means "unique key of the flatpage". These two should not use the same name. sybren: Since this is not the URL that's being requested right now, but is an encoding of the flatpage…

				site = assert_cast(Site, get_current_site(request))
				try:
				f = get_object_or_404(ExtendedFlatPage, url=url, sites=site.id)
				sybrenUnsubmitted Done Inline Actions Inconsistent formatting, but probably also unnecessary formatting. Why would you get an object for `url` but then redirect to `'%s/' % request.path`? `f` is also not used, so instead of getting the object from the database and then ignoring it, it's better to just do an existence check. This will also avoid nesting `Http404` exceptions. sybren: Inconsistent formatting, but probably also unnecessary formatting. Why would you get an object…
				natkaAuthorUnsubmitted Done Inline Actions This is the default flatpage view implementation copied from `django.contrib.flatpages.views`... Guess I should have given it more attention than I did. natka: This is the default flatpage view implementation copied from `django.contrib.flatpages.views`...
				sybrenUnsubmitted Not Done Inline Actions This is the default flatpage view implementation copied from `django.contrib.flatpages.views`... In such cases I would always add a comment that tells people where it came from. If it's just a one-on-one copy of proven code, it puts the reviewer's mind in a different mode. sybren: > This is the default flatpage view implementation copied from `django.contrib.flatpages.views`.
				natkaAuthorUnsubmitted Done Inline Actions As for the `url` and `request.path` distinction: `url` is not the same thing as `request.path` here. `url` is an attribute of a flatpage; it is not the full path to the resource. In our case, if the `url` is `'/coffee-run/about/'`, the `request.path` will be `'/films/coffee-run/about/'` - therefore in the HttpResponsePermanentRedirect, the `url` cannot be used. natka: As for the `url` and `request.path` distinction: `url` is not the same thing as `request.path`…
				except Http404:
				if not url.endswith('/') and settings.APPEND_SLASH:
				sybrenUnsubmitted Done Inline Actions No need to use `else` after `return`. sybren: No need to use `else` after `return`.
				sybrenUnsubmitted Not Done Inline Actions This block is getting quite complex. Refactor into one or more smaller functions. sybren: This block is getting quite complex. Refactor into one or more smaller functions.
				url += '/'
				if ExtendedFlatPage.objects.filter(url=url, sites=site.id).exists():
				return HttpResponsePermanentRedirect(f'{request.path}/')
				raise

				request = assert_cast(WSGIRequest, request)
				return render_flatpage(request, f)