Differential D8325 Diff 26978 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.shortcuts import get_current_site
				from django.http.request import HttpRequest
				from django.http.response import HttpResponse, Http404, HttpResponsePermanentRedirect
				from django.shortcuts import get_object_or_404

				from films.models import ExtendedFlatPage

				DEFAULT_TEMPLATE = 'flatpages/default.html'


				def flatpage(request: HttpRequest, film_slug: str, url: str) -> HttpResponse:
				"""
				An overwritten version of the flat page view.

				Wraps the default flatpage view to simplify url handling, and uses the
				:model:`films.ExtendedFlatPage` model.

				Args
				``request``: An HTTP request.
				sybrenUnsubmitted Not Done Inline Actions "overwritten" → "overridden" sybren: "overwritten" → "overridden"
				``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/).

				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.
				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
				"""
				if not url.startswith('/'):
				url = '/' + url
				url = f'/{film_slug}{url}'

				site_id = get_current_site(request).id
				try:
				f = get_object_or_404(ExtendedFlatPage, url=url, sites=site_id)
				except Http404:
				if not url.endswith('/') and settings.APPEND_SLASH:
				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…
				f = get_object_or_404(ExtendedFlatPage, url=url, sites=site_id)
				return HttpResponsePermanentRedirect('%s/' % request.path)
				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`…
				else:
				raise
				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.
				return render_flatpage(request, f)