search

silverstripe / silverstripe-cms

Silverstripe CMS - this is a module for Silverstripe Framework rather than a standalone app. Use https://github.com/silverstripe/silverstripe-installer/ to set this up.
http://silverstripe.org/
BSD 3-Clause "New" or "Revised" License
516 stars 333 forks source link

Unpublishing a Page does not show in history. #2448

Closed wilr closed 4 years ago

wilr commented 5 years ago

Steps to reproduce in 4.4

  1. Create a page
  2. Publish page
  3. Unpublish the page

When viewing the History tab you can only see 'Published' and 'Saved' no where is it clear that a page has been 'Unpublished' and when.

Before I submit a PR - is this intended behaviour? Was there a reason that this is not available?

Screen Shot 2019-06-13 at 4 32 52 PM

Pull Requests

wilr commented 5 years ago

Right so I've found a few reasons why. It looks like this was a toggle in 3 which is no longer available. In my mind unpublished records (not archived) should still appear. Or should we have this toggle ala 4?

  1. First step is Versioned::augmentSQLVersioned does not include unpublished records so would need to be updated. Line 611. 
-$query->addWhere(["\"{$baseTable}_Versions\".\"WasDeleted\"" => 0]);

+$query->addWhere(
            "\"{$baseTable}_Versions\".\"WasDeleted\" = 0 OR (".
                "\"{$baseTable}_Versions\".\"WasDeleted\" = 1 AND ".
                "\"{$baseTable}_Versions\".\"WasDraft\" = 0".
            ")"
        );

  1. DataObjectScaffolderExtension needs to pass the WasDraft option.

  2. Then all the UI needs to be updated (HistoryViewerVersionState) from versioned-admin

newleeland commented 5 years ago

It would definitely be good to have Unpublished records in history. I cannot think of any UX reason why we wouldn't want it in History.

chillu commented 5 years ago

Yeah I'd say that's unintended behaviour. Unpublishing should create a new version to record this activity, and hence should show as a new entry there.

Changing the behaviour of Versioned::augmentSQLVersioned() could be considered an API change though. You could imagine devs building logic on versioned behaviour, e.g. a task which deletes older versions that now behaves differently. Since a lot of this kicks in behind the scenes, it's really hard to tell how those APIs are used. Maybe we can modify the behaviour of a specific Versioned::augmentSQLVersioned<variation>() variation and limit the impact?

brynwhyman commented 5 years ago

There's an existing issue capturing designs and further detail of implementing a toggle here: https://github.com/silverstripe/silverstripe-versioned/issues/105

chillu commented 5 years ago

@wilr We might leave the fix of this to implemeting the https://github.com/silverstripe/silverstripe-versioned-snapshots module. You can review the epic here: https://github.com/silverstripe/silverstripe-versioned/issues/68 (make sure you have the Zenhub browser extension installed to see the cards linked to it). @newleeland will update the designs on there soon

newleeland commented 5 years ago

It was decided that this work would be undertaken as a part of experience improvements work. Separate to the snapshots.

In addition Saved, Published we should also log records after a page has been:

sunnysideup commented 4 years ago

F

emteknetnz commented 4 years ago

Travis kitchen-sink is green with unmerged PR's https://travis-ci.org/github/creative-commoners/cwp-recipe-kitchen-sink/builds/717113255

emteknetnz commented 4 years ago

Linked PR's have all been merged