search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 821 forks source link

GridField support for versioned records #7032

Closed chillu closed 6 years ago

chillu commented 7 years ago

In SS4, we've created the ChangeSet API (with the "campaigns" module) for the specific purpose of giving authors more control on the publication of content. Which relies on applying Versioned to DataObjects in your project code.

At the moment, we don't have any support in core to actually manage those versioned DataObjects efficiently. While this can be added through third party modules, it's not really a long-term solution if we're serious about making all types of content managed in SilverStripe "publishable".

Since we're likely going to create a React/JSON-powered GridField in the 4.x release cycle, I'm a bit cautious expanding the GridField API surface in core significantly. So I'm wondering if we should improve the existing versioneddataobjects module instead of pulling this into core, and focus on better core extension points for now.

Potential features:

Existing modules:

Potential core improvements:

chillu commented 7 years ago

@stecman @bigSiebs @adrexia @andrewvt @micschk @lukasfrey @jebates @tylerkidd @stevie-mayhew @bendubuisson You have all been involved in GridField/Versioned-related modules - I'd like to get your feedback on what you see as the most useful actions for SilverStripe leading up to the 4.x release. We've made the Versioned system more solid through ChangeSets ("campaigns"), but versioning your DataObjects is still opt-in for 4.x. Assuming that authors generally prefer the draft/publish flow to extend to all managed objects, how can we make it easier for devs to opt-in to this model?

stecman commented 7 years ago

I'm not sure what the possible scope of changes is at this point, but for what it's worth, the VersionedDataObject extension in heyday/silverstripe-versioned-dataobjects (originally written by @camspiers) only exists to make the Versioned extension from the framework useable, since it's not out of the box. It seems like Versioned was written very specifically to provide versioning for SiteTree, rather than a generic versioning solution. I haven't dug through all of the SS4 code, but it looks like this hasn't really changed.

What I'd really like to see is the framework provide the glue that VersionedDataObject does currently; where you can add the extension to a DataObject and it just works. Basic out-of-the-box UI support for the versioning provided by the framework is great, but Versioned probably needs to be easier to drop in (and well advertised) for the UI side of it to really be useful.

tractorcow commented 7 years ago

@ingo have you seen https://github.com/silverstripe/silverstripe-versioned/blob/master/src/VersionedGridFieldItemRequest.php this was built based on the versioned data objects module, as well as some inspiration from @unclecheese's better buttons.

stecman commented 7 years ago
  • Publish/unpublish buttons on detail form
  • Cascading publish by "owner" (e.g. a page record with has_many relationships)
  • Mark "owner" record as modified if it contains unpublished dependencies (SS4 only?)

Potentially covered by these, but nested versioning is a bit of a nightmare UX-wise in the CMS. It might be worth providing an option to slave versioning in a way that's invisible to the user.

In the past I've replaced the publish/save draft actions in related versioned records with a "save" action that saves as draft. When the parent/owner is published, all of these related records are implicitly published. As far as the CMS user is aware, it's all one thing - avoiding the "pages are versioned but these things inside it are published immediately" conversation.

stevie-mayhew commented 7 years ago

I agree with @stecman here. The main work which I've done involving versioning has been publish chains for related objects and implementing custom actions for gridfields like the ones that @tractorcow linked above.

We've also attempted to have Versioned parents having versioned children objects, and then to be able to "publish" the children which allows the parent to be informed of that change while allowing the parent to "publish" all of its children on a change. It has been quite complicated in some instances to achieve this, which is a problem I believe campaigns will solve for us.

andrewvt commented 7 years ago

Our module https://github.com/bluehousegroup/silverstripe-data-object-version-viewer has https://github.com/heyday/silverstripe-versioneddataobjects as a dependency for exactly the reason @stecman mentions above (versioned dataobjects doesn't work out of the box without it). Before we discovered the silverstripe-versioneddataobjects module we were neck deep in handling stages correctly ourselves which was a handful and produced spooky behavior left as is. I strongly recommend the functionality in silverstripe-versioneddataobjects be put right in the framework so that when versioning is turned on it works reasonably out of the box.

pitchandtone commented 7 years ago

We've found that while versioning is required from a tech point of view for content blocks and similar, most clients don't actually think about sub items versions at all, so similar to @stevie-mayhew above, we've created nested publishing flows and remove versioning UI from subobjects as it's just too complex for a client to get their head around.

pitchandtone commented 7 years ago

SS4 versioning Data extension looks a lot better then v3 and the changes seem to point to versioned data objects module surplus to requirements thanks to that work.

sminnee commented 7 years ago

@pitchandtone without any special GridFields, the default behaviour in the SS4 CMS is that stage content is shown, and that as long as you set up $owns metadata, clicking publish on the page will also publish all subordinate versioned dataobjects. If I understand your user needs correctly, this should give you what you want?

If you add the page to a campaign, then all of the subordinate dataobjects that are due to be published will be listed out in the campaign, to give more detail.

sminnee commented 7 years ago

@chillu it looks like the only thing in heyday/versioneddataobjects missing from silverstripe/versioned is the VersionedModelAdmin, which seems like a bit of an orphan.

pitchandtone commented 7 years ago

@sminnee yeah that sounds good. We do have a couple of clients that require sub-object publishing, but that's the exception to the rule.

chillu commented 7 years ago

Publish/unpublish buttons on detail form Cascading publish by "owner" (e.g. a page record with has_many relationships) Inclusion of draft records in ModelAdmin search

I've confirmed that these is already in place for SS4

Publish draft/publish/modified status indication on GridField table view Publish draft/publish/modified status indication on ModelAdmin search results

Created https://github.com/silverstripe/silverstripe-versioned/issues/22 for this.

Which other items from my list in the ticket description are important? Any crucial bits missing from SS4?

we've created nested publishing flows and remove versioning UI from subobjects as it's just too complex for a client to get their head around. [...] We do have a couple of clients that require sub-object publishing, but that's the exception to the rule.

How did you deal with this in terms of unpublishing sub-objects? While cascading publish is pretty straightforward, I could see cases where an author wants to move a sub-object back to draft (take it off the published page) without unpublishing the page itself. So in this case, you wouldn't want a "publish" button on the sub-object, but still need an "unpublish" button. Or do you just use "delete/archive"? Related issue: https://github.com/silverstripe/silverstripe-versioned/issues/21

stevie-mayhew commented 7 years ago

@chillu the issue there is often that you will need to "remove" the object back to a draft version, and often then have publish the parent object so that this cascades through - especially if you have some form of caching involved.

I'm not sure of the best way to provide an "out of box" functionality for this, as its more configuration than convention in that case.

sminnee commented 7 years ago

How did you deal with this in terms of unpublishing sub-objects? While cascading publish is pretty straightforward, I could see cases where an author wants to move a sub-object back to draft (take it off the published page) without unpublishing the page itself.

I wouldn't take it as self-evident that users will need this, or at least not in all cases.

For some applications, sure, in which case you could have an option to provide publication controls on every atomic record. But I wouldn't assume that every piece of nested data needs these.

For other cases, it would probably be overkill. In these cases I would expect that deleting the record and then publishing the parent page would be a reasonably natural way of handling this.

By way of analogy, you wouldn't want separate publication controls on the Content and Title fields of a page, and some nested data types are going to be as lightweight as those in the user's mental model.

So if we were to have per-sub-record controls for publish/unpublish, I would make it an optional add-on. This might require better extension points in the GridFieldDetailForm (which would also allow for a refactor of betterbuttons)

pitchandtone commented 7 years ago

One of the assumptions we have struggled with in v3 (unsure if this has been split more in v4 yet) is the assumption that publishing and unpublishing permissions are the same.

While it's an OK default, it should be configurable as in there should be two different functions canUnpublish and canPublish with extends calls so that you can handle this discreetly. For example, workflow module disables publishing when in place, which also means people can't unpublish things without hacking around stuff.

Agree with both @chillu and @sminnee re unpublishing items, most of the time we've seen that people still think about the page and are fine with delete object, re-publish page, but there are some clients who are saavy enough to need unpublishing per sub object, however as @stevie-mayhew points out, it opens up the question of what else is affected when this happens.

I think it's safe enough to have delete object, unpublish page as the default, but I'd be keen to see what comes back from @chillu's user testing on that front.

pitchandtone commented 7 years ago

OK so related to this, I've been playing with $owns tonight, so far it seems like the recursive publish works well. So I was about to delete some extensions that we had added to elemental to control unpublishing and duplication, as I assumed $owns would be used recursively in both of those methods but that doesn't seem to be the case.

Is this by design (shall I leave my extensions in but try use $owns) or an oversight?

Next question is if $owns is used by duplicate, should the variable be moved to DataObject instead rather than Versioned and then utilised by Versioned?

pitchandtone commented 7 years ago

And on delete

sminnee commented 7 years ago

Hrm, these sound like oversights although in the case of many-many and has-one "owned" data it's not clear what the logic should be. Can you raise a ticket?

chillu commented 7 years ago

So if we were to have per-sub-record controls for publish/unpublish, I would make it an optional add-on. This might require better extension points in the GridFieldDetailForm

That's what https://github.com/silverstripe/silverstripe-versioned/issues/21 is about. Can we determine "sub-records" though? It's context specific. You might want to auto-publish a "staff member" when associated to a page, but also provide a separate "staff members" ModelAdmin where they can be individually published. At the moment Versioned->findOwners() will only determine owners for the current record, not for the type of record. And GridFieldDetailForm doesn't have a reference to the parent object to check if it owns the currently edited record. We could opt-in to individual publish in ModelAdmin, but leave it disabled in GridField's default config?

Hrm, these sound like oversights although in the case of many-many and has-one "owned" data it's not clear what the logic should be. Can you raise a ticket?

I've created https://github.com/silverstripe/silverstripe-versioned/issues/23

chillu commented 7 years ago

Also, I think a lot of the discussions around ownership are repeats from earlier discussions lead by Hamish/Damian during the RFC stage: I'm just getting a strong coffee and re-reading that RFC: https://github.com/silverstripe/silverstripe-framework/issues/4932

sminnee commented 7 years ago

You might want to auto-publish a "staff member" when associated to a page, but also provide a separate "staff members" ModelAdmin where they can be individually published.

We could opt-in to individual publish in ModelAdmin, but leave it disabled in GridField's default config?

That makes sense to me. This could be an optional VersionedModelAdmin base class or extension that you apply to the applicable model admins? I don't think it needs to automagically appear if there's a simple way to enable it.

chillu commented 7 years ago

That makes sense to me. This could be an optional VersionedModelAdmin base class or extension that you apply to the applicable model admins? I don't think it needs to automagically appear if there's a simple way to enable it.

I've updated https://github.com/silverstripe/silverstripe-versioned/issues/21, and flipped it around: VersionedGridFieldEditForm should be opt-in, not opt-out.

I've also just realised that "revert" doesn't work the way authors might expect, and we need to at least add a warning: https://github.com/silverstripe/silverstripe-versioned/issues/24. ChangeSet::revert() is proposed, but not actually implemented. ChangeSet::unpublish() isn't even proposed in the RFC, probably because of the ambiguity it creates around what's going to be unpublished?

A related comment on 4.2.2.15. DataObject::unpublish in the RFC

Note: the meaning of the term "unpublish" has been left consistent with the behaviour of "unpublish" action in 3.x. This feature could potentially be replaced with a new action, which is to "undo the last publish of this record", which would instead query the DB for live version of this record prior to the current, and revert it to that. This would require additional scoping.

chillu commented 7 years ago

Another goodie: Unpublish cascades to owners by default - https://github.com/silverstripe/silverstripe-versioned/issues/25

pitchandtone commented 7 years ago

@chillu @sminnee @tractorcow any thoughts on duplicate and delete also utilising $owns?

tractorcow commented 7 years ago

Duplicate has "duplicate with children" and "duplicate self only" UX (at least for pages). It could use owns, but it's risky if some owned object's don't easily support duplication (e.g. files).

pitchandtone commented 7 years ago

@tractorcow is this a similar distinction in that has_one are problematic?

I'd guess has_one Link or has_one Page would not normally be required either.

tractorcow commented 7 years ago

It's not so much the relationship as the datamodel. it's not always logically valid to assume an object can be duplicated, e.g. Member, File.

pitchandtone commented 7 years ago

File fits in the has_one category and Member doesn't seem like something that would ever appear in an $owns array, particularly because it isn't Versioned (although I'm arguing that perhaps $owns should be wider than Versioned).

If $owns isn't relevant (and stays Versioned specific), I'll add an equivalant for duplicate and delete via an Extension.

tractorcow commented 7 years ago

For discussion on recursive unpublish please continue the conversation over at https://github.com/silverstripe/silverstripe-versioned/issues/23