Details

comments/admin.py
28 ↗	(On Diff #26898)	Why is it necessary to ignore the type? Accessing a private property shouldn't happen. If it is important that this can be accessed from outside, make it so that it can be without suppressing warnings. If this really is the only way, explain why in a comment.
common/admin.py
10 ↗	(On Diff #26898)	This is a strange way to construct a URL. Hard-coding `films` in there shouldn't be necessary.
common/templates/common/base.html
37–40 ↗	(On Diff #26898)	When is an underscore used for separation, and when is a dash used? I'm not up to date on the URL design rules used in this project, but for me it looks inconsistent.
common/templates/flatpages/default.html
1 ↗	(On Diff #26898)	Are references from 'common' to 'films' allowed? If this is something temporary, add a comment that explains this.
films/templates/films/about.html
18	The same string is slugified multiple times while rendering this page. Wouldn't it be better to do this on the flatpage object, so that it can be cached?
36	Similar question as above: wouldn't it be lighter to just markdownify the flatpage content once on saving and store it in the DB? That'll make it lighter to render the page. Another advantage is that such an approach makes it easier to change the MarkDown library/options/extras/etc. without having to worry about older pages breaking. By converting to HTML on save, only those pages that have the attention of a person will be updated, which means that such breakage has a chance of being noticed.
films/urls.py
31–32	A view function in the `urls.py` file? I would just put it in the views code, where it belongs.
films/views/gallery.py
140 ↗	(On Diff #26898)	Does this change have anything to do with the other changes in this patch?

Sybren A. Stüvel (sybren) added a reviewer: Sybren A. Stüvel (sybren).Jul 17 2020, 3:35 PM

Natalia Maniakowska (natka) requested review of this revision.Jul 17 2020, 4:52 PM

Natalia Maniakowska (natka) added inline comments.

comments/admin.py
28 ↗	(On Diff #26898)	This is a property added in the annotation above, in the `get_queryset` method. It is to be used only in the admin, to limit the number of database queries - I imagine that there will eventually be hundreds or thousands of comments and wanted to avoid sending a query or two for each of them. Since it's an annotation, mypy complains about Comment object not having such an attribute, and also does not recognise its type, raising two errors in this line (`attr-defined` and `no-any-return`). Do you have any suggestions on how to make mypy happy here?
common/templates/common/base.html
37–40 ↗	(On Diff #26898)	I believe Django documentation always uses dashes, so I went for this convention when working on the new apps, and later I realised that underscore was used in the already existing training app. I wanted to fix it at some point... but maybe it's a good opportunity to do it now.
films/templates/films/about.html
36	This is a very good point. I assumed that we want to store the markdown-formatted content in the database because then it will be easier to update the page, without the need to edit the html. But I could save the rendered html in a separate non-editable field and use that other field's content in the template.
films/views/gallery.py
140 ↗	(On Diff #26898)	This is just to fix an error - this function needs two arguments. I have committed a fix to develop in the meantime (but this was after I created this revision).

Extend the FlatPage model and view

Extend the FlatPage model, put all related files in films app
Override flatpage view to use the extended model, fix urls

Harbormaster completed remote builds in B9085: Diff 26929.Jul 17 2020, 7:56 PM

A small url adjustment in flatpage view

Harbormaster completed remote builds in B9108: Diff 26978.Jul 20 2020, 10:02 AM

Natalia Maniakowska (natka) marked 4 inline comments as done.Jul 20 2020, 11:31 AM

Natalia Maniakowska (natka) marked an inline comment as done.Jul 20 2020, 11:33 AM

Sybren A. Stüvel (sybren) requested changes to this revision.Jul 20 2020, 1:10 PM

Sybren A. Stüvel (sybren) added inline comments.

films/models/flatpages.py
12–14 ↗	(On Diff #26978)	Doing this in `clean()` means that this code doesn't run when you call `flatpage.save()` (docs). Is that intentional?
films/views/flatpages.py
37–46 ↗	(On Diff #26978)	This block is getting quite complex. Refactor into one or more smaller functions.
44 ↗	(On Diff #26978)	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.
45–46 ↗	(On Diff #26978)	No need to use `else` after `return`.
films/views/gallery.py
140 ↗	(On Diff #26898)	You can update patches, that way you can remove unrelated changes if you see they accidentally made it into the patch.

This revision now requires changes to proceed.Jul 20 2020, 1:10 PM

Natalia Maniakowska (natka) added inline comments.Jul 20 2020, 4:40 PM

films/models/flatpages.py
12–14 ↗	(On Diff #26978)	Hm, the docs also say that clean is a good place to automatically provide a value for a field, and we've done it this way in other models. Certainly there's no particular intention to avoid running this code on `flatpage.save()` - we just don't manually call `save()` anywhere, so I did not assume that it was a problem. Do you think it would make more sense to put this code in `save()` in this case?
films/views/flatpages.py
44 ↗	(On Diff #26978)	This is the default flatpage view implementation copied from `django.contrib.flatpages.views`... Guess I should have given it more attention than I did.

Refactor flatpage view and fix mypy errors

Harbormaster completed remote builds in B9123: Diff 27002.Jul 20 2020, 9:26 PM

Natalia Maniakowska (natka) added inline comments.Jul 21 2020, 9:01 AM

films/views/flatpages.py
44 ↗	(On Diff #26978)	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.

Natalia Maniakowska (natka) marked 2 inline comments as done.Jul 21 2020, 9:04 AM

Remove an unrelated bugfix

Harbormaster completed remote builds in B9128: Diff 27011.Jul 21 2020, 10:10 AM

Sybren A. Stüvel (sybren) requested changes to this revision.Jul 21 2020, 10:43 AM

Sybren A. Stüvel (sybren) added inline comments.

films/admin/flatpages.py
17–19 ↗	(On Diff #27011)	This doesn't seem very friendly, and I don't think this is consistent with the current implementation either. From the code I expect the film to be selected in a dropdown, and the `url` field to exclude the film slug.
films/models/flatpages.py
8 ↗	(On Diff #27011)	The name doesn't convey anything about the purpose of this class, beyond the fact that it has a few extra properties. Currently there are multiple things happening in this class: It changes the content type of the `FlatPage.content` field from HTML to MarkDown. It connects the page to a specific Film, and ensures deletion of the page on deletion of that film. This looks like it could be split up in two classes, one for each 'thing'. I think that the content type change to MarkDown could be moved into a mix-in class and placed in the `common.models` module.
films/urls.py
31	Here page URLs end with a slash, whereas in the other URL patterns this is not the case. If the trailing slash is really necessary, at least document why this is the case. If it is not really necessary, the view code can also be simplified.
films/views/flatpages.py
20 ↗	(On Diff #27011)	"overwritten" → "overridden"
22–23 ↗	(On Diff #27011)	This isn't true. It doesn't wrap the default flatpage view, it re-implements it.
41 ↗	(On Diff #27011)	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.
44 ↗	(On Diff #26978)	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.

This revision now requires changes to proceed.Jul 21 2020, 10:43 AM

This revision was not accepted when it landed; it landed in state Needs Revision.Jul 21 2020, 3:23 PM

Closed by commit rBLSdf8f77bc0bee: Use flat pages for films' "About" pages. (authored by Natalia Maniakowska (natka)). · Explain Why

This revision was automatically updated to reflect the committed changes.

Natalia Maniakowska (natka) added a commit: rBLSdf8f77bc0bee: Use flat pages for films' "About" pages..

Natalia Maniakowska (natka) added a commit: rBLS94e29c96622f: Improve flat page url handling in templates.

Natalia Maniakowska (natka) added a commit: rBLSab35c237aa2b: Re-register flatpage admin model, fix mypy issues.

Natalia Maniakowska (natka) added a commit: rBLS22d48ee01dcd: Extend the FlatPage model, put all related files in films app.

Natalia Maniakowska (natka) added a commit: rBLSc68df835dd2b: Override flatpage view to use the extended model, fix urls.

Natalia Maniakowska (natka) added a commit: rBLS21995e7fd82e: A small url adjustment in flatpage view.

Natalia Maniakowska (natka) added a commit: rBLS80b5917c50fc: Refactor flatpage view and fix mypy errors.

Thank you for the feedback Sybren. For the record, I had a chat with Natalia and we will take a simplified approach to this.
The idea is to link a project to a new "FilmFlatPage" model, inspired by what has been done so far, but to use a more explicit /films/<film_slug>/page/<page_slug> route instead.

I think that's a good idea, indeed.

Use flat pages for films' "About" pages.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 27021

common/views/flatpage.py

films/templates/films/about.html

films/templatetags/film_extras.py

films/urls.py

Use flat pages for films' "About" pages.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 27021

common/views/flatpage.py

films/templates/films/about.html

films/templatetags/film_extras.py

films/urls.py

Use flat pages for films' "About" pages.
ClosedPublic
Actions

Revision Contents
Changeset List