Show warning when unpublishing an object with owners

chillu commented 7 years ago

Overview

By default, objects will not cascade an unpublish to their owners (see https://github.com/silverstripe/silverstripe-versioned/issues/25). This is based on the assumption that most owner templates gracefully handle the disappearance of a previously published owned object. Sometimes the presence of this object is required for the template to function correctly though.

Example: NewCustomerPage has_one SignupFormContentBlock. The same content block might be used in different scenarios (e.g. a footer), and an author might not be aware that it's also the main entry point for new customers. Without this form, the whole page is considered functionally broken.

Acceptance Criteria

Show a warning to authors before unpublishing objects with owner objects.
Show the warning both for the default behaviour (unpublish object only) and when a developer has opted into cascading unpublish to owner objects
The warning allows authors to clearly identify affected records (e.g. a natural language object type plus the title)
The warning allows authors to click through to the CMS view of the object if CMSEditLink() is implemented
When no owner objects are detected, the CMS author isn't bothered with an additional step during unpublish
The warning shows in the Pages UI, Assets UI and GridField detail forms (incl. ModelAdmin)

Tasks

[x] Implement unpublish warning in asset-admin for files
[x] Implement unpublish warning with versioned gridfield
[x] Update tests

Out of Scope

Show warning before unpublishing objects with owned objects (see allow recursive unpublish)

Pull Requests

/cc @clarkepaul @pitchandtone @stevie-mayhew

pitchandtone commented 7 years ago

Warning might be a bit harsh, is notification better?

sminnee commented 7 years ago

If they proceed, their site will end up with broken links / image references — this seems warning-worthy, doesn't it?

However, not all unpublishes will have that effect — if it's a has_many relationship it would just end up with 1 fewer items in the list. So we might need to distinguish the different situations.

Does it need to be a special case for the assets that are linked by inclusion in a WYSIWYG editor?

pitchandtone commented 7 years ago

Files are going to be the edge case that require a warning, mainly because of the wysiwyg.

If a content element had a link to a file then the file markup would normally have an if around it.

So yes, if we can add some special code for files because it's probably the most common event, but shouldn't be the most common design.

Sent from my iPhone

On 7/07/2017, at 1:28 PM, Sam Minnée notifications@github.com wrote:

If they proceed, their site will end up with broken links / image references — this seems warning-worthy, doesn't it?

However, not all unpublishes will have that effect — if it's a has_many relationship it would just end up with 1 fewer items in the list. So we might need to distinguish the different situations.

Does it need to be a special case for the assets that are linked by inclusion in a WYSIWYG editor?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

tractorcow commented 7 years ago

I think it's fine if this is implemented on a case-by-case basis. I.e. asset-admin has it's own warning, some versioning-enabled gridfield would have it's own, and so on. My major concern is the files section.

chillu commented 7 years ago

@newleeland Can you please create designs for this?

newleeland commented 7 years ago

As an MVP we could just use a similar message we have for deleting an item #(https://github.com/silverstripe/silverstripe-asset-admin/issues/591)

screen_shot_2017-09-20_at_5_01_53_pm

chillu commented 7 years ago

Thanks for coming up with a pragmatic solution Jared! :) I'm going to remove this from the 4.0 milestone regardless, so I think we should just go and implement the full solution at some point later. Which also means you'd need to come up with a design heh.

The challenge here will be to create something that works both in Entwine and React UIs. Ideally we can make this a fairly generic system - there's going to be lots of actions that require confirmation with detailed information, e.g. batch actions shouldn't duplicate the logic we're writing here.

tractorcow commented 6 years ago

@unclecheese can you please track down and link the designs you've followed so I can refer to them for peer review? I can't seem to track the link.

silverstripe / silverstripe-versioned