Feature/new articles

[cogat]

Extends ICEkit with an authors app, for describing content authors. The Authors app uses the updated Article+ListingPage pattern, which is also used in latest press-releases and elsewhere.

[coveralls]

Coverage increased (+0.9%) to 74.031% when pulling 0eaad19 on feature/new_articles into d6643de on develop.

[mrmachine]

I've made a few minor suggestions about naming and structure, but the biggest issue that stands out for me is the concept of relating detail article instances to a listing page instance.

I've described my thoughts about this in more detail in line comments, but broadly, I think it should work in a more typical fashion where article detail models are their own thing, and listing pages pull in article detail instances as required, without needing an explicit FK to the list page.

If this really won't work, I think we need to better document this behaviour as it was a bit confusing to piece together when I started reading the docs, and provide some rationale about why it is essential and better than a typical list page which is just told to list a queryset of objects.

[mrmachine]

I think Articles looks/reads badly in plain text and rendered markup. Could we just say "Article and ListingPage" 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 an Abstract prefix or Base suffix like the others), and Article is a concrete implementation of ArticleBase (actually there are a few concrete Article 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 or ListingPageBase subclasses)." as the body?

[cogat]

It's changed to AbstractListingPage and AbstractCollectedContent, and neither requires the other.

[mrmachine]

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.

[cogat]

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 @property parent to return the first draft press release listing page.

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.

[mrmachine]

I think these objectives can be achieved without an FK to a ListingPage (or in fact any UrlNode/Page) instance.

The "one true URL" for a given press release (when there are multiple PressReleaseListingPage objects added to a page tree -- which is probably uncommon) would be the first (or a specifically chosen) PressReleaseListingPage instance as returned by app_reverse(multiple=True). Usually there will be only one. But if there are many, we just need to pick one to be the main/primary one.

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 PressReleaseListingPage that filters the PressRelease queryset being listed, or have the category as a URL parameter (e.g. /press/<category>/<slug>) to provide multiple list pages filtered by category.

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.

[cogat]

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 /watch/ that lists a subset, and to serve that subset at /watch/<slug> ? It seems you're either hardcoding the URL or using a database link. My approach allows the latter, without precluding the former.

[mrmachine]

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 on_delete behaviours (protect, set null) might not be much better either.

[cogat]

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.

[mrmachine]

I meant faked in the sense that some objects implement parent as a property that fetches an object to act as parent via some other mechanism (e.g. just picking the first one it finds) instead of having a parent FK.

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.

[cogat]

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.)

[mrmachine]

This explanation is a little clearer to me. Thanks.

Still, parent is a very generic name to begin with and is also overloaded and has different implementations. If we're suggesting that parent might be an FK to UrlNode or to something else like Category (for a more typical implementation), we should say that in the docs. It sounded to me like it would always be a page (UrlNode).

If the only reason we're using the name parent is to re-use templates, maybe it's worth changing the name and using different templates?

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 FooListingPage instances? I can imagine that a popup version of the entire page tree change list page would not be very friendly to use, and that allowing any type of UrlNode instance to be linked as the parent for an article could be problematic.

[cogat]

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 parent like this:
parent = models.ForeignKey(
"ArticleCategoryPage",
limit_choices_to={'publishing_is_draft': True},
on_delete=models.PROTECT,
)

[mrmachine]

This should be 2. (even if Markdown allows us to be lazy and renders it as an ordered list)?

[cogat]

Done

[mrmachine]

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.

[cogat]

No major reason. Defining the admin is a bit more unwieldy.

[mrmachine]

Again, icekit.articles.abstract_models.ListingPage. And should get_items() be named get_published_items()?

[mrmachine]

no_history is always empty?

[cogat]

Dunno. Removed.

[mrmachine]

We should include the WxH in the alias name, especially when it's not namespaced to a particular app or model. We don't know that every author_portrait on every site or app will need to be 360x640.

[cogat]

When we receive a design, the author_portrait will normally always be the same size throughout designs, and I bet normally not 360x640. If it needs to be a different size than 360x640 for a given project then we only override the alias in the project settings and the change would be reflected in all templates and apps for that project.

If we put WxH in the alias name, then that name would become obsolete in every project, and we'd need to define a new alias and update/override all templates to use a new alias name.

[mrmachine]

But this is a global alias. It's not namespaced to the author app (which would render author templates). This means no other app or non-authors-app-template can use this alias. If this is an alias provided and used by the authors app, in authors app templates, we can use whatever general name we like, but namespace it to the authors plugin/app.

[cogat]

But the intent is that alias should be used throughout a site, wherever we want an author_portrait.

[cogat]

I'm trying to avoid the situation we had in SFMOMA where if a design introduces or changes an image spec, we have to manually identify all the places where that image is used. Global image specs are attractive.

[cogat]

So we want the alias which is currently called 'author_portrait' to be available to all apps and templates within a project. ie we wanted to use the 'author_portrait' alias from within any template in the project, and we'd expect it to be the same size.

Namespacing an alias makes it only available to the app that namespaces it, which means we couldn't use an author_image anywhere else ie in a listing of authors that a project needs. Easy thumbs doesn't fallback to used a namespaced alias if no global alias is defined. So I can't see a way round starting with a globally namespaced definition, at least without patching easy_thumbnails.

(If another app supplied a template that had a thumbnail that used the same alias, and we wanted it to be a different size, then we would then define a namespaced alias in settings, or override the template.)

Which leaves us with the issue of whether 'author_portrait' is likely to unintentionally clash with another app's alias, and whether encoding w x h in the alias name is a good solution for avoiding clashes.

My feeling is 'author_portrait' is already unlikely to clash with anything else we install. If it does, and we need two different 'author_portrait's for two different purposes, we can define a namespaced alias for the one that isn't ICEkit. However, I'm happy amend it to 'icekit_authors_portrait_large', to encode both the defining app and the purpose, to eliminate the possibility of clashes for practical purposes.

Encoding wxh in the alias name is never a good idea, as the definition of the alias will usually have a different wxh from ICEkit's default, and we'd either need to live with a misleading alias name, or change all the templates to use a non-misleading name.

[mrmachine]

Fair enough, if we want to use the alias in templates that exist outside the authors app, then we need a global alias. And you're probably right that authors_portrait probably won't clash (or the wrong size). But it still feels like chance rather than good practice.

Previously (SFMOMA) I believe we didn't use aliases at all, which is probably what made it hard to find and replace all occurrences when designs changed. We only started using aliases when we started resolving scaling issues (offline compression, generating thumbnails at upload time, etc.)

So just by using any alias at all (even one that includes WxD), I think we are already much better off in terms of updating templates if/when the dimensions change.

I don't think it's a bad idea to include the WxD in the alias name specifically because:

1. we used to embed WxD in the template anyway, before we used aliases;
2. it avoids the issue where any other template (from another app or our own apps) needs to know beforehand the dimensions of the alias and that it is correct for the template;
3. it's probably easier to find and replace authors_portrait_WxD than just authors_portrait;
4. it's one clear convention to follow -- we don't find ourselves in a situation where actually someone does want to display a differently sized version of a portrait and then has to decide on a name like authors_portrait_small (and then if there's another template that needs authors_portrait_xsmall, and then maybe one of the templates changes and small is now smaller than xsmall).

I think it's clearer to include WxD to disambiguate the size requirement than words that describe the template/name/purpose/size. The name component of the alias should still be as descriptive as possible to avoid conflicts, but it should define the image source (not where it is used). We shouldn't care where/how/why it is used. Just what it is and what size it is.

Also, note that with offline generation of thumbnails, global thumbnails will be generated for all global aliases for all images that are uploaded. So we will have image fields on models completely unrelated to author portraits, having an "authors_portrait" thumbnail generated. The app/model namespace helps us only generate those thumbnails we need for the authors portrait image when it is uploaded.

[cogat]

Still disagree on WxH. Our maintenance burden will increase if we have to make new templates and diverging settings files for every variation in image size from project to project. I'm out of time to discuss it so am going to press ahead with omitting it. I think easy_thumbnails may need to change how it resolves aliases before we're both happy.

[mrmachine]

This brings up another potential issue. Because we are storing all images in a single image library app/model, this might not play nice with easy-thumbnail generation for aliases (which is essential for remote storage backends). Even namespaced aliases would all be for the same image app/model, and so all images in the image library will have thumbnails generated at all sizes even though they are completely unrelated things and not all represented at all sizes listed by aliases in the site. This could be a bunch of unnecessary image processing and storage at best and might require a change to easy-thumbnails as you say, or our image library (I have some ideas on this already, but will save them for a separate issue).

On WxH in the alias name, either way there is a maintenance burden as we inevitably try to marry up templates/settings/intent with new designs. I can imagine an author portrait could easily be shown at least 4 different sizes: author list page, author detail page, author section of essay/article page (content plugin), admin change list, admin change form preview, even if we intend for it to be the same size across the site in our default templates. If we try to ascribe semantic names to all of these intents/purposes, it will become difficult, especially if different projects/templates use different terminology.

Also, what happens when two of these semantically named aliases (referenced in templates) actually are the same dimensions? We either have two semantically different templates/uses sharing the same allegedly semantic alias name (which no longer gives an indication on where/how it is used) or we have two different aliases with the same options, effectively double-processing the same source image to create the same thumbnail with a different name.

Using {source}-{opts} as a convention for alias names is like using unique filenames derived from a hash of file content. Forever cachable (I think if you alter the options for a semantically named alias, you will get a new thumbnail image with the same filename). Doesn't matter which or how many or where templates use it, the same thumbnail is rendered and only processed once for a given source and set of options (much like when not using aliases at all). One template is easily updated without affecting the others (you don't need to investigate which other templates use the given alias before you decide whether to change the alias name or just change the alias options in settings and have the change apply everywhere.)

[cogat]

I'm still unconvinced and still out of time to discuss in this PR.

[mrmachine]

It might be worth putting the Author model in here, and the corresponding list page in icekit.authors.page_types.list_page (with an app_label of icekitauthors_pagetypes_listpage or something). Currently, installing icekit.authors will always register the listing page.

Or better yet, since we are providing an authors list page and a separate (non-page-type) but related model to store the author details, we should move the whole authors app to icekit.page_types.author. Articles is a better candidate for a top level icekit package because it's only providing abstract functionality, not a concrete implementation. The authors app is a concrete implementation of the article pattern and provides/relies on a page type as a critical component.

[cogat]

Agreed on option 2.

[cogat]

Option 2 is done.

[mrmachine]

The naming and return value of this method is confusing. is_foo() sounds like it should return a boolean. And it does, unless it's a string (which also evaluates truthy). This should be a property suppressed_message or a method get_suppressed_message() which always returns a string (an empty one when it's not suppressed, which evaluates falsey).

[cogat]

Fixed.

[mrmachine]

Test model implementations (and their migrations) and tests should all go in the app package being tested.

[cogat]

In the interests of time, I moved the tests, but not the models.

[coveralls]

Coverage increased (+0.7%) to 74.473% when pulling 8cd681f on feature/new_articles into 765cbbd on develop.

[coveralls]

Coverage increased (+0.7%) to 74.473% when pulling 8cd681f on feature/new_articles into 765cbbd on develop.

[jmurty]

Hi @cogat @mrmachine I haven't been following this work closely, but I am also uncomfortable with the tight coupling via FK relationships of listable articles to some owner/parent page.

I think listable items should be kept as simple as possible, and any smarts we need to enforce a single definitive URL path should live completely, or almost completely, outside the listable items themselves.

For the time being the cleanest way of doing this might be to ditch the FK relationship and have Article.get_absolute_path() call a central utility method of some kind to fetch its own path. Instead of needing to know about its own authoritative URL mount point, the item would defer this decision to the method. This central mechanism could be stupid-simple for now, something like (pseudocode):

def get_listable_item_path(item):
    if isinstance(item, Event):
        return '/'.urljoin(app_url("AuthoritativeEventsListingPage"), item.slug)
   elif OtherCases:
        blah

I can see the logic in this central method getting more complex over time to handle cases like items without slugs (e.g. should a non-page Event have a routable URL) and to make the definitive mount point configurable (without needing to re-assign a bunch of FKs). But for now simple and lightweight is better.