silverstripe / silverstripe-lumberjack

Keep in control of large SiteTrees by using GridField to manage pages.
BSD 2-Clause "Simplified" License
33 stars 35 forks source link

Incorrect "Published" status in GridField when using Fluent #75

Open JorisDebonnet opened 6 years ago

JorisDebonnet commented 6 years ago

When using the Blog module together with Fluent, any BlogPosts created in one locale will show up in the GridField as "Published" in any other locale, too. That status is not correct though, as is confirmed by going to the BlogPost's details (where everything works fine).

I'm not entirely sure whether this is something that should be fixed in Fluent or in Lumberjack. I guess it may need a 'speciale case' code block in one of both. In any case, those posts are not considered "Published" in other locales (..until they are), so I believe they should show up in the GridField as "Draft".

(I have 2.0.0-alpha1 installed of this module, and fluent 4.0.0)

JorisDebonnet commented 6 years ago

Now that I think about it, this might be a Blog-specific thing after all, since it also has "Scheduled to publish" content in there. So perhaps the issue should be created over there instead? (I actually already did, but then saw you already reacted here)

robbieaverill commented 6 years ago

Hrmmm. Can you maybe provide some more specific steps to reproduce this?

I've done this:

For me the states in the lumberjack GridField are the same in both locales. Per your bug report I'd expect them all to be reported as "Published", but I'm not seeing that.

Side note: there are a couple of instances of incorrect Versioned API usage in blog and lumberjack that are preventing the MODIFIED badge from being displayed. PRs to fix at https://github.com/silverstripe/silverstripe-lumberjack/pull/80 and https://github.com/silverstripe/silverstripe-blog/pull/530

JorisDebonnet commented 6 years ago

Sure, I made a few screenshots. As in your case, I have three blog posts and two of them are published. I created the 'base' version of the Blog and the BlogPosts in Dutch. image

Then I switched to English. This immediately shows an issue because you get the exact same overview as above. The reason this is an issue is because those two "published" posts are not, in fact, published, due to the pages not 'existing' yet in this locale.

After translating the first post to English, I get this overview: image

There is no visual difference between BlogPost 1 and 2, but 1 is present in the English locale, while 2 is not.

So that is the issue. Perhaps this should actually be tested on a Lumberjack instance without Blog, to see how it behaves then, as Blog makes it extra confusing because of its PublishDate. (that's why I also created the issue over there)

In total, I think we have three different published states here:

In any case, what I would expect in the BlogPosts overview inside Blog is that I can see which articles do not exist yet in this locale. Perhaps it can be handled in Lumberjack by greying them out or something, after all, non-translated Pages appear greyed out in the SiteTree-overview too. And then Blog might have to do something to make the "Published on ..." less confusing because it may not be published in all locales.

NightJar commented 6 years ago

Hi @JorisDebonnet that's helpful, thanks. Are you able to check the database to see which Locales it lists Nederlandse titel 2 as, in the SiteTree/BlogPost _Localised table? Translatable works by copying the state of (the translatable fields for) a page, so it's possible that the EN content is still Dutch because it hasn't been altered since being copied to the English locale.

Another thing to check is that the EN Localse doesn't have a fallback to NL?

JorisDebonnet commented 6 years ago

Hm, the database confuses me, because contrary to what I expected, all three BlogPosts are also present in the English locale in _Localised. I'm not sure when or how that happened.

So I went ahead and created a fourth BlogPost on the Dutch one and check what happens in the DB, but this time it does stay isolated on the Dutch locale, and so the explanation in my previous post holds for that one. I might have done something afterwards with those first three...

Here's what I see on the English Blog now: image

While this shows the _Localised database for all these BlogPosts: image

So the fourth one exists only in the Dutch locale, but still shows up visually on the English blogpost-list "as if it were published there".

robbieaverill commented 6 years ago

Ah yes, ok - I see the issue now. Thanks for the follow up information!

NightJar commented 6 years ago

Neat, minimal steps to reproduce: 1) create silverstripe/installer 2) require silverstripe/recipe-blog 3) Add 2 locales. 4) publish a blog page in default locale. 5) add a blog post in default locale. 6) copy and publish blog (not post) in second locale. 7) observe that blog posts from default locale are shown (correct) as published (incorrect) in the CMS for this second locale.

image

Things do at least show correctly on the front-end: image

Looking into this now.

Editing actual post page shows as expected (it is just a Page after all): image

NightJar commented 6 years ago

LumberJack's GridFieldSiteTreeState::getColumnContent looks specifically at Versioned::isPublished, where as FluentSiteTreeExtension::updateSavePublishActions is what modifies the default behaviour elsewhere in the CMS. I can't imagine any module that inspects published state not running into this issue also.

The ideal in this case would be to have some kind of extension point in the Versioned... extension 🙄 But it seems ugly. I'll have a think about how best to override isPublished with isPublishedInLocale (and similar friends).

robbieaverill commented 6 years ago

Maybe isPublished needs an updateIsPublished extension point inside it for fluent to return isPublishedInLocale

NightJar commented 6 years ago

One can beat Versioned to it by introducing another extension for Fluent that applies before Versioned (where FluentVersionedExtension must be applied after). This allows us to add isPublished via CustomMethods before Versioned can, and direct the execution path down isPublishedInLocale instead.

The trick comes in attempting to think about what other checks (e.g. isModifiedOnDraft) might be in place - and the third state... No longer is it just DRAFT or LIVE, but also NOT IN LOCALE as well. this would hold true no matter the method used to modify the published status.

All of this doesn't stop e.g. LumberJack simply checking and returning a binary state of LIVE or DRAFT only - at least not without introducing some coupling :/ (or some serious refactorying and probably API changes).

So this is quickly exploding and is worthy of more thought. I'm curious to get your thoughts on this too @tractorcow

robbieaverill commented 6 years ago

Was just doing some testing on how Fluent modifies the page tree and noticed that it's not completely accurate either at the moment: https://github.com/tractorcow/silverstripe-fluent/issues/434