-
Notifications
You must be signed in to change notification settings - Fork 11
Feature/new articles #29
Changes from 23 commits
92aae2c
71deb4b
0fe4625
99c9995
22b195d
dbe6a33
da9431e
601e8c4
5b912ed
2802ddc
9617316
5876315
885d284
3e241f0
df7c743
5e4c307
7c0cfd7
527e166
bf28636
f968cf5
1228c36
866e5a1
0eaad19
7a33213
318869a
2d23de9
35a98a9
ade899b
c91ae5d
f68738c
09acaf7
b21028f
1367e6d
8cd681f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,184 @@ | ||
# `Article`s and `ListingPage`s | ||
|
||
`Article`s are publishable content types that live in collections. Normally | ||
articles are mounted under a parent's URL. | ||
|
||
An example of an article is a Press Release, which would be mounted under a | ||
Press Release Listing Page. The Listing Page has the URL "/press" and thus all | ||
the Press Releases have the URL "/press/<slug>". | ||
|
||
## `ArticleBase` | ||
|
||
The `icekit.articles.models.ArticleBase` inherits `title`, `slug` and | ||
publishing fields, and each `ArticleBase` subclass implements some key | ||
functionality: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this should be This might make the transition to rST a bit easier, as a headings/refs like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Docs are updated, but didn't have time to get into the linkification. |
||
|
||
1. **Links to parents.** | ||
In order for an Article to know its URL (which is based on a parent's URL), it | ||
should define a `parent` link, which is normally a link to a `ListingPage`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sounds odd. Normally a listing page would be for a specific type of article, and would just list them all. No FK from individual article instances back to a listing page instance. The listing page might be configurable to filter the list by category. The way this is described, I'd be worried that deleting a listing page will delete all of its linked articles. And I wonder why this is necessary. Are we intending to arbitrarily link some articles to one listing page and others to a different listing page (with articles and listing page both being of the same type)? Or link articles of different types (not polymorphic child types, concrete/polymorphic-parent article models) to a single article listing page? And with a single FK from the article to the listing page, why not just make them page types and directly add them to the page tree? And this means it will only appear in one page? Unless we also have additional listing pages that behave more typically. I thought the idea was for articles/content to be completely independent of pages and the page tree, but be pulled into the site anywhere they are relevant by list pages (or other page types)? Perhaps a bit more of an explanation would help to clarify the intention and provide rationale on why it is necessary or better simply having more typical listpage/article pairs, where the article is not coupled to a listing page. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason for coupling an article to a ListingPage is so that they inherit the Page's URL and that there is only one URL per article, and that can be decided by editors. So in some sites, press releases will be at /press-releases/. In others, they'll at /media/releases/. In some cases we're intending to link some articles to one listing page and some to another. SFMOMA's watch/read/listen articles are a case in point. In other cases, we're not. e.g. all press releases belong under press. In this case, I've defined Other pages may (and do) list articles in different ways, but this arrangement is the one that defines the URL. Agreed that the on_delete behaviour needs fixing, and documentation needs clarifying, and possibly ArticleBase should become ContentCollection or something. The reason Articles are not page_types in the page tree is that a) they have different admin_listing requirements (a tree is not a useful way to view/sort Articles) b) the page tree gets very unwieldy with sometimes thousands of items to sort and filter c) we don't want Articles to be children of arbitrary pages, only a defined subset. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these objectives can be achieved without an FK to a The "one true URL" for a given press release (when there are multiple Listing pages that need to show only a subset, e.g. watch/read/listen from SFMOMA, would have a category as a field on the I agree that making articles pages in the page tree is not a good idea. But it seems this implementation is half way attempting to do that by hard coding a link from an article to a specific page instance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Press Release behaviour you describe is already implemented: https://github.com/ic-labs/icekit-press-releases/blob/master/press_releases/models.py#L76 Given the watch/read/listen from SFMOMA, how would you implement a listing page at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's wrong with the way it is done in SFMOMA? If we want multiple list pages, one for each category, all at top level URLs, just add a category field to the list page plugin and filter the objects being listed by category. If the category list pages can all be under a common URL, then include the category as a URL parameter. I think this is by far the more common case and least confusing or error prone solution to filtered list pages (vs. FKs from each object being listed to a specific list page instance). Having an FK field that is ignored (at best) or faked when not needed (which is probably most of the time) in order to support a less common / more complex case seems like a bad trade-off. And I'm still worried about cascade deletes. The other There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the parent is 'faked'. It's genuinely needed in order to figure out where the user wants the content to be mounted at, to figure out where the 'back' breadcrumb goes, etc. I'd ideally not call it 'parent' but if we do, then we get to reuse the templates for a Page. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant faked in the sense that some objects implement There are one time setup steps for the end user in both cases. Create and publish a list page. Maybe it has a category field maybe it doesn't, but this difference shouldn't be a significant hurdle for users. I'm not sure what you mean by the page needing the same slug as the category. Once there's a published list page, they just create an article and select either an appropriate category or a parent instance. I don't see a huge difference for the user. It's probably easier to select a category from a select list than search for a specific list page in the page tree. My reluctance to go ahead with this FK to a parent page right now is that I think it will be difficult to change or remove once anything is using it, and I think it's still not clear that this is definitely the right way to go. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Back to first principles. The need is to allow objects to appear at some url, such as "/watch/", where "watch" is a grouping term chosen in some way by the user. There are two candidate ways of doing this: assign a category, or assign a page. If you need a page at /watch/ that lists the items, as we often do, then it seems convenient to create such a page, and link the items to it. That's what this pattern is. If you don't need a page, you don't have to have one; you make "parent" link to a category model and use a traditional URL pattern. If you don't need a parent, then what you're doing is outside of what this code is helpful for. If you like the listing page, but don't like the way items have a parent to it, you don't need to use the parent pattern. If you need a page that filters items in a different way, then either extend/upgrade the listing page model or implement something else. No dramas. The only time we need to use this pattern is when it is architecturally sensible: when there is content to be listed, and we need one or more pages to list them under. I don't think we're painting ourselves into a corner. (I was sat with the team at SFMOMA, and I can tell you the page+category approach was opaque enough that I needed to do the setup for them every time. I myself had to sift through the code the first time to figure out what was going on. They expected to be able to see the article once they had attached the category, which didn't work, because the URL pattern for the article is defined by the Page.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This explanation is a little clearer to me. Thanks. Still, If the only reason we're using the name Could we make the implementation consistent by insisting that parent must be an FK? I can imagine that this could easily cause us problems and inefficient queries. Finally, are we doing anything to make the page selection easier? E.g. showing a model choice field that is filtered to show only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Insisting on an FK might be OK, but I'd rather not make people choose 'press page' for every press release. At the moment I think we cross that bridge when we come to it, as the migration is likely to be straightforward. FWIW, I'm implementing |
||
|
||
1. **A view.** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
If an Article is mounted under a `ListingPage` parent, the ListingPagePlugin | ||
will call `get_response(request, parent)` on the article, which should | ||
return the necessary `HttpResponse`. If the Article is a `fluent_contents` | ||
model, the view functions are implemented by `fluent_contents`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe mention that this happens via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
There is also a class `icekit.articles.models.PolymorphicArticleBase` which | ||
extends the Article class with django-polymorphic functionality, allowing | ||
you to define articles of different shapes and mount them under the same | ||
`parent` model. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't we just make this the default/only option? A polymorphic parent model can be used itself (without a child). But it gives the option of adding additional slightly different types. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No major reason. Defining the admin is a bit more unwieldy. |
||
|
||
## `ListingPage` | ||
|
||
The `icekit.articles.models.ListingPage` model is a page type that requires | ||
`get_items()` and `get_visible_items()` to be defined in subclasses. | ||
When viewed, ListingPage lists the items returned by `get_items()`. | ||
`get_visible_items()` is necessary because an editor may wish to preview | ||
unpublished items (that aren't returned by `get_items()`) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, |
||
|
||
## Admins | ||
|
||
For normal Articles, inherit from `ArticleAdminBase`. For polymorphic articles, | ||
inherit from `PolymorphicArticleParentAdmin` and `PolymorphicArticleChildAdmin` | ||
as shown in the example below. | ||
|
||
## Bare-bones example | ||
|
||
The following defines a minimal rich content Article, mounted under a | ||
minimal `ArticleCategoryPage`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using a different name/type of article in the example might avoid confusion (e.g. where every base and concrete model has some form of "article" in it). E.g. press release or essay? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
In `models.py`: | ||
|
||
from django.db import models | ||
from icekit.abstract_models import FluentFieldsMixin | ||
from icekit.articles.abstract_models import ListingPage, ArticleBase | ||
|
||
|
||
class ArticleCategoryPage(ListingPage): | ||
def get_items(self): | ||
unpublished_pk = self.get_draft().pk | ||
return Article.objects.published().filter(parent_id=unpublished_pk) | ||
|
||
def get_visible_items(self): | ||
unpublished_pk = self.get_draft().pk | ||
return Article.objects.visible().filter(parent_id=unpublished_pk) | ||
|
||
|
||
class Article(ArticleBase, FluentFieldsMixin): | ||
parent = models.ForeignKey( | ||
ArticleCategoryPage, | ||
limit_choices_to={'publishing_is_draft': True} | ||
) | ||
|
||
In `admin.py`: | ||
|
||
from django.contrib import admin | ||
from icekit.admin import FluentLayoutsMixin | ||
from icekit.articles.admin import ArticleAdminBase | ||
from .models import Article | ||
|
||
|
||
@admin.register(Article) | ||
class ArticleAdmin(ArticleAdminBase, FluentLayoutsMixin): | ||
pass | ||
|
||
In `page_type_plugins.py`: | ||
|
||
from fluent_pages.extensions import page_type_pool | ||
from icekit.page_types.layout_page.admin import LayoutPageAdmin | ||
from icekit.articles.page_type_plugins import ListingPagePlugin | ||
from .models import ArticleCategoryPage | ||
|
||
|
||
@page_type_pool.register | ||
class ArticleCategoryPagePlugin(ListingPagePlugin): | ||
model = ArticleCategoryPage | ||
|
||
|
||
## Bare-bones polymorphic example | ||
|
||
The following defines a polymorphic structure of minimal rich content Article, | ||
and a zero-content (except title) `RedirectArticle`. | ||
|
||
In `models.py`: | ||
|
||
from django.db import models | ||
from django.http import HttpResponseRedirect | ||
from fluent_pages.pagetypes.redirectnode.models import RedirectNode | ||
from icekit.abstract_models import FluentFieldsMixin | ||
from icekit.fields import ICEkitURLField | ||
from icekit.articles.abstract_models import ListingPage, PolymorphicArticleBase | ||
from django.utils.translation import ugettext_lazy as _ | ||
|
||
|
||
class ArticleCategoryPage(ListingPage): | ||
def get_items(self): | ||
unpublished_pk = self.get_draft().pk | ||
return Article.objects.published().filter(parent_id=unpublished_pk) | ||
|
||
def get_visible_items(self): | ||
unpublished_pk = self.get_draft().pk | ||
return Article.objects.visible().filter(parent_id=unpublished_pk) | ||
|
||
|
||
class Article(PolymorphicArticleBase): | ||
parent = models.ForeignKey( | ||
ArticleCategoryPage, | ||
limit_choices_to={'publishing_is_draft': True} | ||
) | ||
|
||
class LayoutArticle(Article, FluentFieldsMixin): | ||
class Meta: | ||
verbose_name = "Article" | ||
|
||
|
||
class RedirectArticle(Article): | ||
new_url = ICEkitURLField( | ||
help_text=_('The URL to redirect to.') | ||
) | ||
redirect_type = models.IntegerField( | ||
_("Redirect type"), | ||
choices=RedirectNode.REDIRECT_TYPE_CHOICES, | ||
default=302, | ||
help_text=_( | ||
"Use 'normal redirect' unless you want to transfer SEO ranking to the new page." | ||
) | ||
) | ||
|
||
def get_response(self, request, *args, **kwargs): | ||
response = HttpResponseRedirect(self.new_url) | ||
response.status_code = self.redirect_type | ||
return response | ||
|
||
class Meta: | ||
verbose_name = "Redirect" | ||
|
||
In `admin.py`: | ||
|
||
from django.contrib import admin | ||
from icekit.admin import FluentLayoutsMixin | ||
from icekit.articles.admin import PolymorphicArticleParentAdmin, \ | ||
PolymorphicArticleChildAdmin | ||
from .models import Article, LayoutArticle, RedirectArticle | ||
|
||
|
||
class ArticleChildAdmin(PolymorphicArticleChildAdmin): | ||
base_model = Article | ||
|
||
|
||
class LayoutArticleAdmin(ArticleChildAdmin, FluentLayoutsMixin): | ||
base_model=LayoutArticle | ||
|
||
|
||
class RedirectArticleAdmin(ArticleChildAdmin): | ||
base_model=RedirectArticle | ||
|
||
|
||
@admin.register(Article) | ||
class ArticleParentAdmin(PolymorphicArticleParentAdmin): | ||
base_model = Article | ||
child_models = ((LayoutArticle, LayoutArticleAdmin), | ||
(RedirectArticle, RedirectArticleAdmin),) | ||
|
||
|
||
Finally, `page_type_plugins.py` is identical to the non-polymorphic example above. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
default_app_config = '%s.apps.AppConfig' % __name__ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,12 @@ | ||
from icekit.abstract_models import FluentFieldsMixin | ||
from urlparse import urljoin | ||
|
||
from django.template.response import TemplateResponse | ||
|
||
from icekit.publishing.models import PublishingModel | ||
from django.db import models | ||
from polymorphic.models import PolymorphicModel | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this import came with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confused about why this particular case warrants specifying a version in setup.py. Surely by the same argument every requirement in setup.py should have a minimum supported version? And new installations of ICEkit won't include polymorphic 0.8, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course, if we know about any minimum version requirements they should be specified in If it's easy enough (and a moved import seems pretty easy), I vote for maintaining compatibility. I believe elsewhere in ICEkit code we do try/except this import. But given the number of ICEkit installations out there and the low probability of needing to upgrade any of them to latest ICEkit without also being able to upgrade polymorphic, I'm also fine with dropping support for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Version is pegged. |
||
|
||
# TODO: this should ideally be in icekit.abstract_models, but doing so | ||
# creates circular import errors. | ||
class PublishableFluentModel(FluentFieldsMixin, PublishingModel): | ||
class Meta: | ||
abstract = True | ||
from icekit.page_types.layout_page.abstract_models import AbstractLayoutPage | ||
from django.db import models | ||
|
||
class TitleSlugMixin(models.Model): | ||
# TODO: this should perhaps become part of a wider ICEkit mixin that covers | ||
|
@@ -22,12 +22,102 @@ def __unicode__(self): | |
return self.title | ||
|
||
|
||
class PublishableArticle(PublishableFluentModel, TitleSlugMixin): | ||
''' | ||
Basic Article type (ie that forms the basis of independent collections of | ||
publishable things). | ||
''' | ||
class ListingPage(AbstractLayoutPage): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. used former. |
||
""" | ||
A Page type that serves lists of things. Good for | ||
e.g. PressReleaseListingPage or ArticleCategoryPage. | ||
""" | ||
class Meta: | ||
abstract = True | ||
|
||
def get_items(self): | ||
""" | ||
Get the items that are listed on this page. | ||
Remember that incoming relations will be on the draft version of | ||
the page. Do something like this: | ||
|
||
unpublished_pk = self.get_draft().pk | ||
return Article.objects.published().filter(parent_id=unpublished_pk) | ||
|
||
Editors normally expect to only see published() items in a listing, not | ||
visible() items, unless clearly marked as such. | ||
|
||
:return: the items that are associated with this page | ||
""" | ||
raise NotImplementedError( | ||
"Please implement `get_items()` on your ListingPage model" | ||
) | ||
|
||
|
||
def get_visible_items(self): | ||
""" | ||
Get all items that are associated with this page and can be previewed | ||
by the user. | ||
Again, incoming relations will be on the draft version of | ||
the page. Do something like this: | ||
|
||
unpublished_pk = self.get_draft().pk | ||
return Article.objects.visible().filter(parent_id=unpublished_pk) | ||
|
||
Editors normally expect to only see published() items in a listing, not | ||
visible() items, unless clearly marked as such. | ||
|
||
:return: the items that are associated with this page | ||
""" | ||
raise NotImplementedError( | ||
"Please implement `get_visible_items()` on your ListingPage model" | ||
) | ||
|
||
class ArticleBase(PublishingModel, TitleSlugMixin): | ||
""" | ||
Articles can be mounted into a publishable listing page, | ||
which has the URL returned by `get_parent_url()`. | ||
|
||
Subclasses should define a `parent` attribute, or override either | ||
`get_parent_url()` or `get_absolute_url()`. | ||
|
||
Subclasses should also define get_response for rendering itself, or | ||
get_layout_template_name() in order to render Rich Content. | ||
""" | ||
|
||
class Meta: | ||
abstract = True | ||
unique_together = (('slug', 'publishing_is_draft', 'parent'),) | ||
|
||
def get_parent_url(self): | ||
if not hasattr(self, 'parent'): | ||
raise NotImplementedError("PublishableArticle subclasses need to implement `parent` or `get_parent_url`.") | ||
|
||
parent = self.parent.get_published() or self.parent.get_draft() | ||
return parent.get_absolute_url() | ||
|
||
def get_absolute_url(self): | ||
parent_url = self.get_parent_url() | ||
return urljoin(parent_url, self.slug) + "/" | ||
|
||
def is_suppressed_message(self): | ||
if not self.parent.has_been_published: | ||
return "This article's parent needs to be published before it " \ | ||
"can be viewed by the public" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This message should use the verbose name from the actual subclass instead of "article"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
return None | ||
|
||
def get_response(self, request, parent, *args, **kwargs): | ||
context = { | ||
'page': self, | ||
'title': self.title | ||
} | ||
try: | ||
return TemplateResponse( | ||
request, | ||
self.get_layout_template_name(), | ||
context | ||
) | ||
except AttributeError: | ||
raise AttributeError("You need to define " | ||
"`get_layout_template_name()` on your `ArticleBase` model, " | ||
"or override `get_response()`") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should use the actual subclass name instead of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
|
||
class PolymorphicArticleBase(PolymorphicModel, ArticleBase): | ||
class Meta: | ||
abstract = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
Article
s looks/reads badly in plain text and rendered markup. Could we just say "Article
andListingPage
" in the heading and "Article
is a publishable content type that lives in a collection"? Same applies everywhere in docs. We should add this to the doc about writing docs.Also, it seems that
ListingPage
is an abstract model (but doesn't have anAbstract
prefix orBase
suffix like the others), andArticle
is a concrete implementation ofArticleBase
(actually there are a few concreteArticle
models, so it's a little ambiguous).If we're just talking about the concept of articles and listing pages, let's not write them as literals? E.g. "Articles and listing pages" as the heading. "Articles are publishable content types (
ArticleBase
subclasses) that live in collections (AbstractListingPage
orListingPageBase
subclasses)." as the body?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's changed to
AbstractListingPage
andAbstractCollectedContent
, and neither requires the other.